Patchwork D1047: tweakdefaults: add restack command

login
register
mail settings
Submitter phabricator
Date Oct. 13, 2017, 6:02 a.m.
Message ID <differential-rev-PHID-DREV-ixkiktk5g6t6wosm4m7p-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/24811/
State Superseded
Headers show

Comments

phabricator - Oct. 13, 2017, 6:02 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The restack command uses a predefined rebase revset that moves orphaned
  changesets to places so they are no longer orphaned.
  
  This is a highly wanted feature.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

AFFECTED FILES
  mercurial/ui.py
  tests/test-tweakdefaults-restack.t

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Oct. 13, 2017, 8:45 a.m.
dlax added a comment.


  As an end user, I'd probably not use this; it's too much magic.

INLINE COMMENTS

> ui.py:56
> +# Destination revset explanation:
> +# - max(roots(ALLSRC) & ::SRC)^
> +#   The obsoleted changeset that the SRC stack is based on.

Maybe also explain what `ALLSRC` and `SRC` are.
Also, it's not clear from reading this which "orphaned changesets" would be rebased. All of them?

> ui.py:86
> +[experimental]
> +rebase.multidest = 1
>  """

Do we want to activate an experimental option in tweakdefaults?

> test-tweakdefaults-restack.t:28
> +  > EOS
> +  $ hg restack
> +  rebasing 6:799bf3a4122a "B3" (B3)

It's not obvious where wordking directory is, so maybe issue a `hg log -r .`

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Oct. 13, 2017, 5:41 p.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1047#17465, @dlax wrote:
  
  > As an end user, I'd probably not use this; it's too much magic.
  
  
  `restack` is used pretty frequently by internal users. I think it could be hard for some users (ex. designers) to understand the DAG, or know what to do after `hg amend` in the middle of a stack. So this command is pretty handy.
  
  I also think it's at least less magic than `evolve` since the it's just a plain rebase with a fancy revset (displayed when you run `--help`). Users understanding revsets could follow its logic and understand what it's doing.

INLINE COMMENTS

> dlax wrote in ui.py:56
> Maybe also explain what `ALLSRC` and `SRC` are.
> Also, it's not clear from reading this which "orphaned changesets" would be rebased. All of them?

This will be explained in rebase command help text (by the time experimental flag gets removed).

> dlax wrote in ui.py:86
> Do we want to activate an experimental option in tweakdefaults?

I think it's fine since this is invisible to end-user. I also plan to remove the experimental flag soon(tm).

> dlax wrote in test-tweakdefaults-restack.t:28
> It's not obvious where wordking directory is, so maybe issue a `hg log -r .`

Working directory is `null`. Since `rebase` is expected to take care of working directory movement and that is covered by rebase tests, I think it's okay to not care about working directory here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: dlax, mercurial-devel
phabricator - Oct. 13, 2017, 6:16 p.m.
lothiraldan added a comment.


  I'm +1 for having stabilization command in core that handles 80% of the use-case which is mid-stack amend.
  
  But instead of having an alias, no matter how awesome it is that we can implement such command using only revsets, could we have it instead as a command? So we could add documentation and a proper help. I'm not sure that all end-users will be able to understand the revsets and I don't think the tweakdefaults comments will be available to them.
  
  I think that there were some discussions about some problems with pruned changeset, is there a scenario where restack fails or misbehave with pruned changesets?
  
  Also as a user, I would like to restack only my current stack (no matter how it is defined) if I have other orphans changesets (like if parts of my changesets have been queued), I don't want them to be rebased while I'm working on something else.
  
  With all these considerations, I would prefer to have restack as an experimental extension and not activated by default. We highlight it during 4.4 announcement so people could test with their own workflow and gives feedback before activating it by default.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 6:26 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D1047#17639, @lothiraldan wrote:
  
  > I'm +1 for having stabilization command in core that handles 80% of the use-case which is mid-stack amend.
  >
  > But instead of having an alias, no matter how awesome it is that we can implement such command using only revsets, could we have it instead as a command? So we could add documentation and a proper help. I'm not sure that all end-users will be able to understand the revsets and I don't think the tweakdefaults comments will be available to them.
  >
  > I think that there were some discussions about some problems with pruned changeset, is there a scenario where restack fails or misbehave with pruned changesets?
  >
  > Also as a user, I would like to restack only my current stack (no matter how it is defined) if I have other orphans changesets (like if parts of my changesets have been queued), I don't want them to be rebased while I'm working on something else.
  >
  > With all these considerations, I would prefer to have restack as an experimental extension and not activated by default. We highlight it during 4.4 announcement so people could test with their own workflow and gives feedback before activating it by default.
  
  
  I agree with all the above.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 7:08 p.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1047#17639, @lothiraldan wrote:
  
  > But instead of having an alias, no matter how awesome it is that we can implement such command using only revsets, could we have it instead as a command? So we could add documentation and a proper help. I'm not sure that all end-users will be able to understand the revsets and I don't think the tweakdefaults comments will be available to them.
  
  
  I found the revset helpful for understanding what it does. I understand non-power users may feel differently.
  
  > I think that there were some discussions about some problems with pruned changeset, is there a scenario where restack fails or misbehave with pruned changesets?
  
  If you read the comment or the test, it's already handled by using `first(A+B)`.
  
  > Also as a user, I would like to restack only my current stack (no matter how it is defined) if I have other orphans changesets (like if parts of my changesets have been queued), I don't want them to be rebased while I'm working on something else.
  
  That's doable by changing `-r`.
  
  > With all these considerations, I would prefer to have restack as an experimental extension and not activated by default. We highlight it during 4.4 announcement so people could test with their own workflow and gives feedback before activating it by default.
  
  That sounds okay. I'll move the code to `hgext`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 7:48 p.m.
durin42 added a comment.


  Counterproposal:
  
  `hg rebase --auto <REVSET>`
  
  or
  
  `hg rebase --auto -r <REVSET>`
  
  rather than introducing a new concept of a stack. I know Facebook has this concept internally, but for core hg we don't have the notion of a stack outside `mq` and `show`: the former is heavily deprecated, and the latter is experimental and still subject to change. Also, I feel like 'automatic rebase' is a pretty good description of what's actually happening to resolve the trivial orphans-only trouble cases. Thoughts?
  
  I'd be happy to spend a bit of time writing up a commit for rebase--auto if people want to play with it.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 8:43 p.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1047#17692, @durin42 wrote:
  
  > Counterproposal:
  >
  > `hg rebase --auto <REVSET>`
  >
  > or
  >
  > `hg rebase --auto -r <REVSET>`
  >
  > rather than introducing a new concept of a stack. I know Facebook has this concept internally, but for core hg we don't have the notion of a stack outside `mq` and `show`: the former is heavily deprecated, and the latter is experimental and still subject to change. Also, I feel like 'automatic rebase' is a pretty good description of what's actually happening to resolve the trivial orphans-only trouble cases. Thoughts?
  >
  > I'd be happy to spend a bit of time writing up a commit for rebase--auto if people want to play with it.
  
  
  I'm unconvinced that `--auto` should be bound to the concept of stabilization. I think it should be made more flexible. ex. some users may ask "why --auto does not rebase my commits to master"?
  
  Currently, rebase has a non-configurable `_destrebase` revset that is used when `-d` is not specified. It's basically useless. And we even have `commands.rebase.requiredest` to disable it.
  
  I think we can have proper ways to make it possible that the "default" behavior is useful and does the "--auto" thing, while still being flexible.
  
  What I'm thinking about is a config section like:
  
    [revsetalias:rebase.dest] # revsetalias that is only used in rebase destination scope
    auto = if(SRC is orphan and obsoleted,first(max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::)+max(::((roots(ALLSRC) & ::SRC)^)-obsolete()))))
    # missing bit: express "ifempty(x, trueset, falseset)" in revset language
  
  Benefit:
  
  1. Users can run `hg rebase -r 'draft()' -d auto`, which seems to be convenient enough.
  2. Sysadmins can override the config so it also works for rebasing to `master`. More flexible.
  
  In addition, we can have something like:
  
    [commands] # ignored by HGPLAIN
    rebase.defaultsrcdest = "not public()", auto # used when both src and dest are not provided
    rebase.defaultdest = auto # used when src is provided, but not dest
    rebase.defaultsrc = ("not public()" & ::.)+(.::)  # used when src is not provided, but dest is provided
  
  Then users can just run `hg rebase` without arguments.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 8:59 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1047#17714, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D1047#17692, @durin42 wrote:
  >
  > > Counterproposal:
  > >
  > > `hg rebase --auto <REVSET>`
  > >
  > > or
  > >
  > > `hg rebase --auto -r <REVSET>`
  > >
  > > rather than introducing a new concept of a stack. I know Facebook has this concept internally, but for core hg we don't have the notion of a stack outside `mq` and `show`: the former is heavily deprecated, and the latter is experimental and still subject to change. Also, I feel like 'automatic rebase' is a pretty good description of what's actually happening to resolve the trivial orphans-only trouble cases. Thoughts?
  > >
  > > I'd be happy to spend a bit of time writing up a commit for rebase--auto if people want to play with it.
  >
  >
  > I'm unconvinced that `--auto` should be bound to the concept of stabilization. I think it should be made more flexible. ex. some users may ask "why --auto does not rebase my commits to master"?
  
  
  That's a good point. In a perfect world you'd do something like "rebase to my successor, unless my successor is an ancestor of the otherwise-default-destination, in which case rebase to there."
  
  > Currently, rebase has a non-configurable `_destrebase` revset that is used when `-d` is not specified. It's basically useless. And we even have `commands.rebase.requiredest` to disable it.
  > 
  > I think we can have proper ways to make it possible that the "default" behavior is useful and does the "--auto" thing, while still being flexible.
  > 
  > What I'm thinking about is a config section like:
  > 
  >   [revsetalias:rebase.dest] # revsetalias that is only used in rebase destination scope
  >   auto = if(SRC is orphan and obsoleted,first(max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::)+max(::((roots(ALLSRC) & ::SRC)^)-obsolete()))))
  >   # missing bit: express "ifempty(x, trueset, falseset)" in revset language
  
  I like where your head is at, but I think I'd probably do `[command] rebase.dest.auto` to be in line with our current direction on commands.
  
  The only concern I have is that this is pretty complicated to express in a revset, but if we can figure it out that's almost certainly the way to go...
  
  > Benefit:
  > 
  > 1. Users can run `hg rebase -r 'draft()' -d auto`, which seems to be convenient enough.
  > 2. Sysadmins can override the config so it also works for rebasing to `master`. More flexible.
  > 
  >   In addition, we can have something like:
  > 
  >   [commands] # ignored by HGPLAIN rebase.defaultsrcdest = "not public()", auto # used when both src and dest are not provided rebase.defaultdest = auto # used when src is provided, but not dest rebase.defaultsrc = ("not public()" & ::.)+(.::)  # used when src is not provided, but dest is provided
  > 
  >   Then users can just run `hg rebase` without arguments.
  
  Overall, I like the (configurable) `rebase --dest auto`, and I think that configurable destination shorthand(s) is meritorious even if it doesn't obviate an eventual `hg restack` or `hg stabilize` (though I think it does, at least for the basic case of having some non-divergent orphans, at least well enough we could start enabling more of the obsolete/rebase/histedit/amend stuff out of the box.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 9:41 p.m.
quark added a comment.


  In https://phab.mercurial-scm.org/D1047#17715, @durin42 wrote:
  
  > The only concern I have is that this is pretty complicated to express in a revset, but if we can figure it out that's almost certainly the way to go...
  
  
  If this is the only concern, how about defining a `_deststabilize(src, allsrc)` revset in `rebase`? (since we already have `_destrebase` for now) Then the user-facing revset could be shorter. This solves extra problems, like we can do extra work to handle the divergence case, and do some caching to improve performance on huge repos in the revset implementation.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 13, 2017, 9:46 p.m.
quark added a comment.


  The reason I perfer a revset alias is it's more flexible: people can define `auto2` so `-d auto2` just works. Or use `auto` in a more complex way: `first(auto|myauto)` (if auto cannot find the destination, use `myauto`).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 4:14 a.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1047#17725, @quark wrote:
  
  > The reason I perfer a revset alias is it's more flexible: people can define `auto2` so `-d auto2` just works. Or use `auto` in a more complex way: `first(auto|myauto)` (if auto cannot find the destination, use `myauto`).
  
  
  Yeah, that makes sense to me.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Oct. 14, 2017, 4:15 a.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D1047#17724, @quark wrote:
  
  > In https://phab.mercurial-scm.org/D1047#17715, @durin42 wrote:
  >
  > > The only concern I have is that this is pretty complicated to express in a revset, but if we can figure it out that's almost certainly the way to go...
  >
  >
  > If this is the only concern, how about defining a `_deststabilize(src, allsrc)` revset in `rebase`? (since we already have `_destrebase` for now) Then the user-facing revset could be shorter and do not require PhD-level knowledge to write. This solves extra problems, like we can do extra work to handle the divergence case, and do some caching to improve performance on huge repos in the revset implementation.
  
  
  Seems fine to me, maybe write up a brief proposal so people don't have to read the whole comment thread here?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Nov. 9, 2017, 8:46 p.m.
quark abandoned this revision.
quark added a comment.


  This does not belong to tweakdefaults, which is for BC purpose.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: durin42, martinvonz, lothiraldan, dlax, mercurial-devel
phabricator - Nov. 12, 2017, 6:18 p.m.
indygreg added a comment.


  I'm a bit late to the party. But here are my thoughts.
  
  While I think there should be some "auto stabilize" behavior on `hg rebase`, I also believe there should be a top-level command or alias for recovering orphans. If nothing else, it is because it is a common operation. What that's named, I'm not sure. But the implementation of `hg restack` in this patch does implement that UI primitive.
  
  Whatever we do, the output of commands that create orphans and would benefit from hints to this command. Furthermore, it would be ideal if the terminology agreed. Right now, `hg amend` will print e.g. "2 new orphan changesets." That's it. What verb you can use to bring these "orphans" back into "stable" history, I don't know. "restack" is not exactly intuitive. Neither is "rebase." I think we should come up with a better term than "orphan" - one that has a physical world relationship to whatever we call "restack." I think concepts like "detach" and "stabilize" are more appropriate.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1047

To: quark, #hg-reviewers
Cc: indygreg, durin42, martinvonz, lothiraldan, dlax, mercurial-devel

Patch

diff --git a/tests/test-tweakdefaults-restack.t b/tests/test-tweakdefaults-restack.t
new file mode 100644
--- /dev/null
+++ b/tests/test-tweakdefaults-restack.t
@@ -0,0 +1,54 @@ 
+  $ cat >> $HGRCPATH<<EOF
+  > [ui]
+  > tweakdefaults=1
+  > [extensions]
+  > drawdag=$TESTDIR/drawdag.py
+  > [experimental]
+  > stabilization=createmarkers allowunstable
+  > EOF
+
+  $ hg init repo1
+  $ cd repo1
+  $ hg debugdrawdag <<'EOS'
+  >   G1  F2   # rebase: F1 -> F2
+  >   |    |
+  >  F1    H   # prune: H
+  >   |    |
+  >   D    E
+  >   |    |
+  >  B1 B2 B3  # amend: B1 -> B2 -> B3
+  >   \ | /
+  >    \|/
+  >     A
+  > EOS
+  $ hg restack
+  rebasing 8:70cffc4fc494 "F2" (F2)
+  rebasing 4:68fe7abd1dcc "D" (D)
+  rebasing 9:7c43db0f1f66 "G1" (G1 tip)
+  $ hg log -G -T '{desc}' -r 'sort(all(), topo)'
+  o  G1
+  |
+  o  D
+  |
+  o  F2
+  |
+  | x  F2
+  | |
+  | x  H
+  |/
+  o  E
+  |
+  o  B3
+  |
+  | x  B2
+  |/
+  | x  G1
+  | |
+  | x  F1
+  | |
+  | x  D
+  | |
+  | x  B1
+  |/
+  o  A
+  
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -50,6 +50,27 @@ 
 # The rollback command is dangerous. As a rule, don't use it.
 rollback = False
 
+[alias]
+# Move orphaned changesets to better places.
+# Destination revset explanation:
+# - max(roots(ALLSRC) & ::SRC)^
+#   The obsoleted changeset that the SRC stack is based on.
+# - successors(...)-obsolete()
+#   A non-obsoleted successor. Or an empty set (no successor).
+# - max(...::)
+#   Get the top of the stack, so history is more linear.
+#   This is the rebase destination for SRC when there is a successor.
+# - max(::((roots(ALLSRC) & ::SRC)^)-obsolete())
+#   The first non-obsoleted changeset before the obsoleted changeset that the
+#   SRC stack is based on.
+#   This is the rebase destination for SRC when there is no successor.
+# - first(A+B)
+#   Pick B if A is empty.
+restack = rebase
+ -r 'orphan()-obsolete()'
+ -d 'first(max((successors(max(roots(ALLSRC) & ::SRC)^)-obsolete())::)+
+           max(::((roots(ALLSRC) & ::SRC)^)-obsolete()))'
+
 [commands]
 # Make `hg status` emit cwd-relative paths by default.
 status.relative = yes
@@ -60,6 +81,9 @@ 
 
 [diff]
 git = 1
+
+[experimental]
+rebase.multidest = 1
 """
 
 samplehgrcs = {