This is an archive of the discontinued Mercurial Phabricator instance.

rebase: remove revprecursor and revpruned states (BC)
ClosedPublic

Authored by quark on Jul 10 2017, 3:15 PM.

Details

Summary

Those states are no longer necessary for rebase to function properly. Remove
them to make the code cleaner.

Marked as BC because in a corner case where working parent is obsoleted, and
is skipped for rebase, we no longer move working parent after rebase
completes. That is better since if working parent is obsoleted, it should be
the user moving working parent back there (after a rebase) explicitly, in
that case, we shouldn't move working parent again.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 10 2017, 3:15 PM
krbullock edited subscribers, added: mercurial-devel; removed: reviewers.Jul 10 2017, 4:12 PM
durin42 accepted this revision.Jul 14 2017, 3:17 PM
durin42 added a subscriber: durin42.

Once d21 is done, this looks fine.

This revision is now accepted and ready to land.Jul 14 2017, 3:17 PM
quark updated this revision to Diff 158.Jul 14 2017, 4:28 PM
This revision now requires review to proceed.Jul 14 2017, 4:28 PM
durin42 accepted this revision.Jul 14 2017, 5:24 PM
This revision is now accepted and ready to land.Jul 14 2017, 5:24 PM
quark abandoned this revision.Jul 18 2017, 1:41 PM
quark updated this revision to Diff 773.Aug 11 2017, 5:01 AM
This revision is now accepted and ready to land.Aug 11 2017, 5:01 AM
quark updated this revision to Diff 833.Aug 13 2017, 12:28 AM
quark updated this revision to Diff 895.Aug 14 2017, 8:10 PM
This revision was automatically updated to reflect the committed changes.
martinvonz added a subscriber: martinvonz.EditedAug 26 2017, 3:32 AM

This broke some of our internal tests. We do something like this:

Start with a patch on top of upstream @:

o A
|
@

Commit A gets sent for review and landed. The user then does "hg pull":

o A'
|
| @ A obsolete 
|/
o

What our tests (and most of our users) actually do is "hg pull --rebase". Before this patch, that would result in:

@ A'
|
o

After this patch, the update doesn't happen, so the user stays in the state in the graph above. I suppose the same would happen if there were commits on top of A. Expecting the update to happen seems reasonable to me, but I also see your point.

Here's another argument for the old behavior. Let's say there was no pull involved and we instead had this:

o D
|
o B'
|
| o C
| |
| x B
| |
| o A 
|/
o

Then "hg rebase -d D -b C". If that command was run from A or C, one would end up on A' or C'. If it was run from B, it seems reasonable that one would end up on A', as one would before this patch, I believe.

So what do you think? Should we back out this patch?

quark added a comment.EditedAug 26 2017, 9:57 PM

I think pull --rebase might want to do update tip if rebase -b . -d tip returns 1. That covers another case without obsmarker:

Before pull:

  @ B draft  
  |
  o A public

Remote:

  o C public
  |
  o B public
  |
  o A public

After pull --rebase:

  ? C public # probably wants to update here
  |
  ? B public
  |
  o A public

The second example might be treated differently, for example, originalwd could be moved back to an non-obsolete revision in rebaseset.

In D24#8494, @quark wrote:

I think:

  1. pull --rebase should do more than just rebase -b . -d tip

I think I'm okay with pull telling rebase the destination, but I don't think I would like more special handling than that.

  1. It's more consistent for whoever making working parent obsolete to move working parent

I agree in general, but when the working parent is made obsolete by an obsmarker received by pull, then it's not as clear. As you write further down, "pull" is not supposed to modify the working copy, and I think it would be very surprising if it did.

For 1, even without obsmarker but just B becomes public:

Before pull:
  @ B draft  
  |
  o A public
Remote:
  o C public
  |
  o B public
  |
  o A public
After pull:
  ? C public
  |
  ? B public
  |
  o A public

Apparently pull --rebase should move working parent to C.

I believe that doesn't currently happen, but yes, I think it would make sense.

Do you think rebase -b B -d C needs to move working parent in this case? I don't think so.

I know that doesn't currently happen, but I think it would make sense for it to do that. That would mean that "hg rebase -d X" would always make a the working directory a descendant of X. We have an internal "hg sync" alias that's currently simply "hg pull --rebase". Our users are often surprised that it doesn't update to the latest version when they're on an ancestor (as in your example). You could say that we should not have such a stupid alias for that, and I agree, but it's still perhaps a useful data point. Either way, there's BC to think about. Making "hg rebase -b B -d C" not rebase anything and just move the working copy may be a larger BC change than we're willing to do even if we did agree that it was the right behavior.

For 2, it could be tricky because pull is supposed to not touch working copy.
I'm not opposite making rebase do extra things like the working parent movement. But I'd prefer doing it in a way that does not introduce legacy states. It could be done by having an optional state[wdirrev].

Sure, I agree with that.

Selfishly, I would still back this change out so we can get a release out internally, but I'm not sure that would be in line with how generally deal with regressions. This regression is not out in any release yet, so I suspect we wouldn't back out and and instead try to fix it before the next release (if we agree that it should be fixed). So I think I'll just create an internal release with this change backed out and we can do something like your state[wddirrev] suggestion for the next release.

quark added a comment.Aug 27 2017, 5:39 AM

I have sent D527 which solves this issue (and more) without introducing special states.