( )⚙ D1139 restack: use multidest rebase to implement restack

This is an archive of the discontinued Mercurial Phabricator instance.

restack: use multidest rebase to implement restack
ClosedPublic

Authored by quark on Oct 17 2017, 1:52 AM.
Tags
None
Subscribers

Details

Reviewers
durham
Group Reviewers
Restricted Project
Commits
rFBHGX3a7053b51da8: restack: use multidest rebase to implement restack
Summary

With the multidest rebase (D470), restack could be implemented as specifying
the source (orphan() - obsolete()) and a revset specifying destination for
each source revision.

This patch changes restack implementation to use that. The revset is
implemented as a private function _destrestack, like _destrebase in
rebase.py.

Most test changes are because the topo-sort in rebase.py removes some
unnecessary steps. So the resulting revision numbers are smaller.

There is one interesting test case that gets changed:

   D
   |      # amend: B1 -> B2 -> B1
B2 B1 B3  # amend: B1 -> B3
 \ | /
   A

Previously, restack will move D to be on top of B3. Now restack will not
move it because D is considered stable (not orphaned) since none of its
ancestors are obsoleted. The new behavior seems to be more desirable.

More tests are added to test restack source revisions (should be in a same
stack) and some "prune" cases.

The transaction logic is also made more robust so test-copytrace-amend.t
does not cause a broken rebase state.

Since the rebase operation runs in a same rebase state, restack can be
continued via rebase --continue correctly after resolving merge conflicts.
This is reflected in a newly added test.

Test Plan

With hg-dev, run ./script/unit.py

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Oct 17 2017, 1:52 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 17 2017, 1:52 AM
quark edited the summary of this revision. (Show Details)Oct 17 2017, 2:31 AM
quark updated this revision to Diff 2892.
quark updated this revision to Diff 2893.
quark edited the summary of this revision. (Show Details)Oct 17 2017, 1:24 PM
quark updated this revision to Diff 2932.
quark added inline comments.Oct 17 2017, 2:44 PM
hgext3rd/fbamend/revsets.py
108

This could be simplified to break.

Does this mean that the entire restack will continue after a merge conflict with hg rebase --continue? (Since the entire thing is just a single rebase call now.) If so, that's really great.

hgext3rd/fbamend/restack.py
24–25

Would it also make sense to have a revset that finds all of the draft changesets related to the current stack, and then use that here?

If I'm not mistaken, this would also allow restack to just be an alias to hg rebase with the appropriate source and destination revsets.

32–33

The current behavior of restack is to check all descendants if the working copy parent is public.

For example:

  o  7ca158  kulshrax
 /   E
|
| o  17831b  kulshrax
| |  E
| |
| x  485b07 (Amended as 7ca158)  kulshrax
|/   D
|
o  a3cc13  kulshrax
.  C
.
@  d97f1c  kulshrax
|  A
~

results in:

rebasing 7:17831bce1ba1 "E"
17831bce1ba1 -> 697867a73131 "E"
hgext3rd/fbamend/revsets.py
67

Could you be more explicit about what a "restack destination" is here?

99

Is the top successor from the split necessarily a head? (What happens if it has another, unrelated child?)

114–116

Just curious: what are your thoughts for how to resolve divergence here? The old behavior of picking the larger rev number was very much an arbitrary choice, since there didn't seem to be other good criteria for picking a divergent successor

kulshrax added a comment.EditedOct 17 2017, 4:00 PM

Also, are you going to modify hg next --rebase to use this new restack functionality? Presently it just calls _restackonce() (which itself is not ideal for that case since it rebases all of the children at each level of the stack rather than just the one on the path to the destination).

quark added a comment.EditedOct 17 2017, 4:24 PM

Does this mean that the entire restack will continue after a merge conflict with hg rebase --continue? (Since the entire thing is just a single rebase call now.) If so, that's really great.

Yes. Good point. I'll add a test.

Also, are you going to modify hg next --rebase to use this new restack functionality? Presently it just calls '_restackonce()` (which itself is not ideal for that case since it rebases all of the children at each level of the stack rather than just the one on the path to the destination).

Ideally yes if I can figure out a neat way to calculate the source revisions.

hgext3rd/fbamend/revsets.py
67

It's for the --dest flag like rebase -r REV -d _destrestack(REV).

rebase command calls it as "destination". Maybe it should be "the new base".

99

Good question!

Here we treat them the same as the "divergent" case - picking the largest , which could be wrong.

In the future if we choose to do nothing for "divergent" case, we could avoid that "wrong" behavior, although it feels not as smart as it can be.

I think split requires a lot of effort to handle perfectly. rebase.py itself does suboptimal things regarding on split as of today. Maybe it could be simplified by recording the top split commit as "primary rewrite edge" and other split commits as "secondary" at the time of split. But that is a huge change to obsstore today.

114–116

It could be doing nothing here but prompt the user to run a hg resolve-like command to resolve the divergence first. The resolution could be a merge, or pick one from candidates.

quark edited the summary of this revision. (Show Details)Oct 18 2017, 3:45 PM
quark updated this revision to Diff 2993.
durham requested changes to this revision.Oct 31 2017, 12:44 PM
durham added a subscriber: durham.

Changes requested for the lock issue. Otherwise looks good

hgext3rd/fbamend/restack.py
32–33

+1 was this an intentional behavior change? I can see it going either way

33–45

Moving the lock down to here means the revset computations above could be completely wrong now right? Heck, the working copy parent could be different at this point.

35

Should we print a "nothing to restack" message? Seems odd to be a complete no-op

This revision now requires changes to proceed.Oct 31 2017, 12:44 PM
quark added a comment.Nov 1 2017, 2:47 PM

@kulshrax I took a look at restackonce and it does not benefit from the multi-dest feature - It only has a single destination. So I'm leaving restackonce unchanged for now.

hgext3rd/fbamend/restack.py
33–45

Good catch.

35

Maybe. The old behavior is to print nothing.

quark updated this revision to Diff 3195.Nov 1 2017, 3:54 PM
durham accepted this revision.Nov 2 2017, 11:58 AM
This revision is now accepted and ready to land.Nov 2 2017, 11:58 AM
This revision was automatically updated to reflect the committed changes.