This is an archive of the discontinued Mercurial Phabricator instance.

tweakdefaults: add restack command
AbandonedPublic

Authored by quark on Oct 13 2017, 2:02 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 13 2017, 2:02 AM
quark updated this revision to Diff 2665.Oct 13 2017, 3:23 AM
dlax added a subscriber: dlax.Oct 13 2017, 4:45 AM

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

mercurial/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?

86

Do we want to activate an experimental option in tweakdefaults?

tests/test-tweakdefaults-restack.t
29

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

quark added a comment.EditedOct 13 2017, 1:41 PM
In 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 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.

mercurial/ui.py
56

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

86

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

tests/test-tweakdefaults-restack.t
29

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.

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'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.

quark added a comment.Oct 13 2017, 3:08 PM

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.

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.

quark added a comment.Oct 13 2017, 4:43 PM

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.

In D1047#17714, @quark 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.)

quark added a comment.EditedOct 13 2017, 5:41 PM

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.

quark added a comment.Oct 13 2017, 5:46 PM

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).

In 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.

In D1047#17724, @quark 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?

quark abandoned this revision.Nov 9 2017, 3:46 PM

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

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.