This is an archive of the discontinued Mercurial Phabricator instance.

Differential D24

rebase: remove revprecursor and revpruned states (BC)
ClosedPublic

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

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGd8d0ef5f5975: rebase: remove revprecursor and revpruned states (BC)
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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
Closed by commit rHGd8d0ef5f5975: rebase: remove revprecursor and revpruned states (BC) (authored by quark). · Explain WhyAug 16 2017, 2:07 AM
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.

martinvonz added a comment.Aug 27 2017, 3:00 AM
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.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (28 lines)
Mtests/test-rebase-obsolete.t (4 lines)

Diff 976

hgext/rebase.py

# The following constants are used throughout the rebase module. The ordering of # The following constants are used throughout the rebase module. The ordering of
# their values must be maintained. # their values must be maintained.
# Indicates that a revision needs to be rebased # Indicates that a revision needs to be rebased
revtodo = -1 revtodo = -1
nullmerge = -2 nullmerge = -2
revignored = -3 revignored = -3
# successor in rebase destination
revprecursor = -4 # legacy revstates no longer needed in current code
# plain prune (no successor) # -4: revprecursor, -5: revpruned
revpruned = -5 legacystates = {'-4', '-5'}
cmdtable = {} cmdtable = {}
command = registrar.command(cmdtable) command = registrar.command(cmdtable)
# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
# be specifying the version(s) of Mercurial they are tested with, or # be specifying the version(s) of Mercurial they are tested with, or
# leave the attribute unspecified. # leave the attribute unspecified.
testedwith = 'ships-with-hg-core' testedwith = 'ships-with-hg-core'
keepbranches = bool(int(l)) keepbranches = bool(int(l))
elif i == 6 and not (len(l) == 81 and ':' in l): elif i == 6 and not (len(l) == 81 and ':' in l):
# line 6 is a recent addition, so for backwards # line 6 is a recent addition, so for backwards
# compatibility check that the line doesn't look like the # compatibility check that the line doesn't look like the
# oldrev:newrev lines # oldrev:newrev lines
activebookmark = l activebookmark = l
else: else:
oldrev, newrev = l.split(':') oldrev, newrev = l.split(':')
if newrev in (str(nullmerge), str(revignored), if newrev in legacystates:
str(revprecursor), str(revpruned)): continue
if newrev in (str(nullmerge), str(revignored)):
state[repo[oldrev].rev()] = int(newrev) state[repo[oldrev].rev()] = int(newrev)
elif newrev == nullid: elif newrev == nullid:
state[repo[oldrev].rev()] = revtodo state[repo[oldrev].rev()] = revtodo
# Legacy compat special case # Legacy compat special case
else: else:
state[repo[oldrev].rev()] = repo[newrev].rev() state[repo[oldrev].rev()] = repo[newrev].rev()
except IOError as err: except IOError as err:
else: else:
msg = _('cannot continue inconsistent rebase') msg = _('cannot continue inconsistent rebase')
hint = _('use "hg rebase --abort" to clear broken state') hint = _('use "hg rebase --abort" to clear broken state')
raise error.Abort(msg, hint=hint) raise error.Abort(msg, hint=hint)
if isabort: if isabort:
return abort(self.repo, self.originalwd, self.dest, return abort(self.repo, self.originalwd, self.dest,
self.state, activebookmark=self.activebookmark) self.state, activebookmark=self.activebookmark)
obsrevs = (r for r, st in self.state.items() if st == revprecursor)
self._handleskippingobsolete(self.state.keys(), obsrevs, self.dest)
def _preparenewrebase(self, dest, rebaseset): def _preparenewrebase(self, dest, rebaseset):
if dest is None: if dest is None:
return _nothingtorebase() return _nothingtorebase()
allowunstable = obsolete.isenabled(self.repo, obsolete.allowunstableopt) allowunstable = obsolete.isenabled(self.repo, obsolete.allowunstableopt)
if (not (self.keepf or allowunstable) if (not (self.keepf or allowunstable)
and self.repo.revs('first(children(%ld) - %ld)', and self.repo.revs('first(children(%ld) - %ld)',
rebaseset, rebaseset)): rebaseset, rebaseset)):
'to commit\n') % (rev, ctx)) 'to commit\n') % (rev, ctx))
self.skipped.add(rev) self.skipped.add(rev)
self.state[rev] = p1 self.state[rev] = p1
ui.debug('next revision set to %s\n' % p1) ui.debug('next revision set to %s\n' % p1)
elif self.state[rev] == nullmerge: elif self.state[rev] == nullmerge:
ui.debug('ignoring null merge rebase of %s\n' % rev) ui.debug('ignoring null merge rebase of %s\n' % rev)
elif self.state[rev] == revignored: elif self.state[rev] == revignored:
ui.status(_('not rebasing ignored %s\n') % desc) ui.status(_('not rebasing ignored %s\n') % desc)
elif self.state[rev] == revprecursor:
pass
elif self.state[rev] == revpruned:
pass
else: else:
ui.status(_('already rebased %s as %s\n') % ui.status(_('already rebased %s as %s\n') %
(desc, repo[self.state[rev]])) (desc, repo[self.state[rev]]))
ui.progress(_('rebasing'), None) ui.progress(_('rebasing'), None)
ui.note(_('rebase merging completed\n')) ui.note(_('rebase merging completed\n'))
def _finishrebase(self): def _finishrebase(self):
self.state[oldrev] = newrev self.state[oldrev] = newrev
if 'qtip' in repo.tags(): if 'qtip' in repo.tags():
updatemq(repo, self.state, self.skipped, **opts) updatemq(repo, self.state, self.skipped, **opts)
# restore original working directory # restore original working directory
# (we do this before stripping) # (we do this before stripping)
newwd = self.state.get(self.originalwd, self.originalwd) newwd = self.state.get(self.originalwd, self.originalwd)
if newwd == revprecursor: if newwd < 0:
newwd = self.obsoletenotrebased[self.originalwd]
elif newwd < 0:
# original directory is a parent of rebase set root or ignored # original directory is a parent of rebase set root or ignored
newwd = self.originalwd newwd = self.originalwd
if newwd not in [c.rev() for c in repo[None].parents()]: if newwd not in [c.rev() for c in repo[None].parents()]:
ui.note(_("update back to initial working directory parent\n")) ui.note(_("update back to initial working directory parent\n"))
hg.updaterepo(repo, newwd, False) hg.updaterepo(repo, newwd, False)
if not self.keepf: if not self.keepf:
collapsedas = None collapsedas = None
for ignored in set(rebasedomain) - set(rebaseset): for ignored in set(rebasedomain) - set(rebaseset):
state[ignored] = revignored state[ignored] = revignored
unfi = repo.unfiltered() unfi = repo.unfiltered()
for r in obsoletenotrebased: for r in obsoletenotrebased:
desc = _ctxdesc(unfi[r]) desc = _ctxdesc(unfi[r])
succ = obsoletenotrebased[r] succ = obsoletenotrebased[r]
if succ is None: if succ is None:
msg = _('note: not rebasing %s, it has no successor\n') % desc msg = _('note: not rebasing %s, it has no successor\n') % desc
state[r] = revpruned del state[r]
else: else:
destctx = unfi[succ] destctx = unfi[succ]
destdesc = '%d:%s "%s"' % (destctx.rev(), destctx, destdesc = '%d:%s "%s"' % (destctx.rev(), destctx,
destctx.description().split('\n', 1)[0]) destctx.description().split('\n', 1)[0])
msg = (_('note: not rebasing %s, already in destination as %s\n') msg = (_('note: not rebasing %s, already in destination as %s\n')
% (desc, destdesc)) % (desc, destdesc))
state[r] = revprecursor del state[r]
repo.ui.status(msg) repo.ui.status(msg)
return originalwd, dest.rev(), state return originalwd, dest.rev(), state
def clearrebased(ui, repo, dest, state, skipped, collapsedas=None): def clearrebased(ui, repo, dest, state, skipped, collapsedas=None):
"""dispose of rebased revision at the end of the rebase """dispose of rebased revision at the end of the rebase
If `collapsedas` is not None, the rebase was a collapse whose result if the If `collapsedas` is not None, the rebase was a collapse whose result if the
`collapsedas` node.""" `collapsedas` node."""

tests/test-rebase-obsolete.t

$ hg log -r . # working dir is at rev 3 (successor of 2) $ hg log -r . # working dir is at rev 3 (successor of 2)
3:be1832deae9a b (no-eol) 3:be1832deae9a b (no-eol)
$ hg book -r 2 mybook --hidden # rev 2 has a bookmark on it now $ hg book -r 2 mybook --hidden # rev 2 has a bookmark on it now
$ hg up 2 && hg log -r . # working dir is at rev 2 again $ hg up 2 && hg log -r . # working dir is at rev 2 again
0 files updated, 0 files merged, 1 files removed, 0 files unresolved 0 files updated, 0 files merged, 1 files removed, 0 files unresolved
2:1e9a3c00cbe9 b (no-eol) 2:1e9a3c00cbe9 b (no-eol)
$ hg rebase -r 2 -d 3 --config experimental.stabilization.track-operation=1 $ hg rebase -r 2 -d 3 --config experimental.stabilization.track-operation=1
note: not rebasing 2:1e9a3c00cbe9 "b" (mybook), already in destination as 3:be1832deae9a "b" note: not rebasing 2:1e9a3c00cbe9 "b" (mybook), already in destination as 3:be1832deae9a "b"
Check that working directory was updated to rev 3 although rev 2 was skipped Check that working directory was not updated to rev 3 because rev 2 was skipped
during the rebase operation during the rebase operation
$ hg log -r . $ hg log -r .
3:be1832deae9a b (no-eol) 2:1e9a3c00cbe9 b (no-eol)
Check that bookmark was not moved to rev 3 if rev 2 was skipped during the Check that bookmark was not moved to rev 3 if rev 2 was skipped during the
rebase operation. This makes sense because if rev 2 has a successor, the rebase operation. This makes sense because if rev 2 has a successor, the
operation generating that successor (ex. rebase) should be responsible for operation generating that successor (ex. rebase) should be responsible for
moving bookmarks. If the bookmark is on a precursor, like rev 2, that means the moving bookmarks. If the bookmark is on a precursor, like rev 2, that means the
user manually moved it back. In that case we should not move it again. user manually moved it back. In that case we should not move it again.
$ hg bookmarks $ hg bookmarks
mybook 2:1e9a3c00cbe9 mybook 2:1e9a3c00cbe9
$ hg debugobsolete --rev tip $ hg debugobsolete --rev tip
1e9a3c00cbe90d236ac05ef61efcc5e40b7412bc be1832deae9ac531caa7438b8dcf6055a122cd8e 0 (*) {'user': 'test'} (glob) 1e9a3c00cbe90d236ac05ef61efcc5e40b7412bc be1832deae9ac531caa7438b8dcf6055a122cd8e 0 (*) {'user': 'test'} (glob)