This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7827

rebase: don't use rebased node as dirstate p2 (BC)
ClosedPublic

Authored by martinvonz on Jan 11 2020, 1:47 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHG9c9cfecd4600: rebase: don't use rebased node as dirstate p2 (BC)
Summary

When rebasing a node, we currently use the rebased node as p2 in the
dirstate until just before we commit it (we then change to the desired
parents). This p2 is visible to the user when the rebase gets
interrupted because of merge conflicts. That can be useful to the user
as a reminder of which commit is currently being rebased, but I
believe it's incorrect for a few reasons:

  • I think the dirstate parents should be the ones that will be set when the commit is created.
  • I think having two parents means that you're merging those two commits, but when rebasing, you're generally grafting, not merging.
  • When rebasing a merge commit, we should use the two desired parents as dirstate parents (and we clearly can't have the rebased node as a third dirstate parent).
  • hg graft (and hg update --merge) sets only one parent and hg rebase should be consistent with that.

I realize that this is a somewhat large user-visible change, but I
think it's worth it because it will simplify things quite a bit.

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

martinvonz created this revision.Jan 11 2020, 1:47 AM
martinvonz updated this revision to Diff 19345.Jan 16 2020, 3:18 AM
martinvonz updated this revision to Diff 19541.Jan 23 2020, 4:34 PM
martinvonz updated this revision to Diff 19705.Jan 29 2020, 8:01 PM
martinvonz updated this revision to Diff 19861.Feb 3 2020, 1:08 PM
martinvonz updated this revision to Diff 19871.Feb 4 2020, 12:13 PM
martinvonz updated this revision to Diff 20097.Feb 10 2020, 7:22 PM
durin42 accepted this revision.Feb 10 2020, 9:16 PM
This revision is now accepted and ready to land.Feb 10 2020, 9:16 PM
martinvonz added a commit: rHG9c9cfecd4600: rebase: don't use rebased node as dirstate p2 (BC).Feb 10 2020, 9:24 PM
Closed by commit rHG9c9cfecd4600: rebase: don't use rebased node as dirstate p2 (BC) (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
martinvonz added a comment.Apr 1 2020, 2:08 PM

Heads up that we ran into a fun bug that was probably caused by this patch. I think what happened was this:

  1. User ran with an older hg, which sets two dirstate parents while rebasing
  2. User ran into a merge conflict and was dropped out to their shell
  3. User upgraded hg to the new version (with this patch)
  4. User ran hg rebase --continue with the new version, which respects the dirstate parents and thus creates a merge commit (!)

Revision Contents
Changeset List

Diff 20137

hgext/rebase.py

self.resume = False self.resume = False
else: else:
overrides = {(b'ui', b'forcemerge'): opts.get(b'tool', b'')} overrides = {(b'ui', b'forcemerge'): opts.get(b'tool', b'')}
with ui.configoverride(overrides, b'rebase'): with ui.configoverride(overrides, b'rebase'):
stats = rebasenode( stats = rebasenode(
repo, repo,
rev, rev,
p1, p1,
p2,
base, base,
self.collapsef, self.collapsef,
dest, dest,
wctx=self.wctx, wctx=self.wctx,
) )
if stats.unresolvedcount > 0: if stats.unresolvedcount > 0:
if self.inmemory: if self.inmemory:
raise error.InMemoryMergeConflictsError() raise error.InMemoryMergeConflictsError()
else: else:
raise error.InterventionRequired( raise error.InterventionRequired(
_( _(
b'unresolved conflicts (see hg ' b'unresolved conflicts (see hg '
b'resolve, then hg rebase --continue)' b'resolve, then hg rebase --continue)'
) )
) )
if not self.collapsef: if not self.collapsef:
merging = p2 != nullrev merging = p2 != nullrev
editform = cmdutil.mergeeditform(merging, b'rebase') editform = cmdutil.mergeeditform(merging, b'rebase')
editor = cmdutil.getcommiteditor( editor = cmdutil.getcommiteditor(
editform=editform, **pycompat.strkwargs(opts) editform=editform, **pycompat.strkwargs(opts)
) )
newnode = self._concludenode(rev, p1, p2, editor) newnode = self._concludenode(rev, p1, p2, editor)
else: else:
# Skip commit if we are collapsing # Skip commit if we are collapsing
if self.inmemory:
self.wctx.setbase(repo[p1])
else:
repo.setparents(repo[p1].node())
newnode = None newnode = None
# Update the state # Update the state
if newnode is not None: if newnode is not None:
self.state[rev] = repo[newnode].rev() self.state[rev] = repo[newnode].rev()
ui.debug(b'rebased as %s\n' % short(newnode)) ui.debug(b'rebased as %s\n' % short(newnode))
else: else:
if not self.collapsef: if not self.collapsef:
ui.warn( ui.warn(
newnode = repo.commit( newnode = repo.commit(
text=commitmsg, user=user, date=date, extra=extra, editor=editor text=commitmsg, user=user, date=date, extra=extra, editor=editor
) )
repo.dirstate.setbranch(repo[newnode].branch()) repo.dirstate.setbranch(repo[newnode].branch())
return newnode return newnode
def rebasenode(repo, rev, p1, base, collapse, dest, wctx): def rebasenode(repo, rev, p1, p2, base, collapse, dest, wctx):
"""Rebase a single revision rev on top of p1 using base as merge ancestor""" """Rebase a single revision rev on top of p1 using base as merge ancestor"""
# Merge phase # Merge phase
# Update to destination and merge it with local # Update to destination and merge it with local
p1ctx = repo[p1] p1ctx = repo[p1]
if wctx.isinmemory(): if wctx.isinmemory():
wctx.setbase(p1ctx) wctx.setbase(p1ctx)
else: else:
if repo[b'.'].rev() != p1: if repo[b'.'].rev() != p1:
rev, rev,
branchmerge=True, branchmerge=True,
force=True, force=True,
ancestor=base, ancestor=base,
mergeancestor=mergeancestor, mergeancestor=mergeancestor,
labels=[b'dest', b'source'], labels=[b'dest', b'source'],
wc=wctx, wc=wctx,
) )
wctx.setparents(p1ctx.node(), repo[p2].node())
if collapse: if collapse:
copies.graftcopies(wctx, ctx, repo[dest]) copies.graftcopies(wctx, ctx, repo[dest])
else: else:
# If we're not using --collapse, we need to # If we're not using --collapse, we need to
# duplicate copies between the revision we're # duplicate copies between the revision we're
# rebasing and its first parent. # rebasing and its first parent.
copies.graftcopies(wctx, ctx, ctx.p1()) copies.graftcopies(wctx, ctx, ctx.p1())
return stats return stats

relnotes/next

== New Experimental Features == == New Experimental Features ==
== Bug Fixes == == Bug Fixes ==
== Backwards Compatibility Changes == == Backwards Compatibility Changes ==
* When `hg rebase` pauses for merge conflict resolution, the working
copy will no longer have the rebased node as a second parent. You
can use the new `conflictparents()` revset for finding the other
parent during a conflict.
== Internal API Changes == == Internal API Changes ==
* The deprecated `ui.progress()` has now been deleted. Please use * The deprecated `ui.progress()` has now been deleted. Please use
`ui.makeprogress()` instead. `ui.makeprogress()` instead.
* `hg.merge()` has lost its `abort` argument. Please call * `hg.merge()` has lost its `abort` argument. Please call
`hg.abortmerge()` directly instead. `hg.abortmerge()` directly instead.
* The `*others` argument of `cmdutil.check_incompatible_arguments()` * The `*others` argument of `cmdutil.check_incompatible_arguments()`
changed from being varargs argument to being a single collection. changed from being varargs argument to being a single collection.

tests/test-rebase-abort.t

note: not rebasing 3:a6484957d6b9 "B bis", its destination already has all its changes note: not rebasing 3:a6484957d6b9 "B bis", its destination already has all its changes
rebasing 4:145842775fec "C1" (tip) rebasing 4:145842775fec "C1" (tip)
merging c merging c
warning: conflicts while merging c! (edit, then use 'hg resolve --mark') warning: conflicts while merging c! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ hg tglog $ hg tglog
@ 4:draft 'C1' % 4:draft 'C1'
| |
o 3:draft 'B bis' o 3:draft 'B bis'
| |
| @ 2:secret 'C' | @ 2:secret 'C'
| | | |
| o 1:public 'B' | o 1:public 'B'
|/ |/
o 0:public 'A' o 0:public 'A'

tests/test-rebase-collapse.t

warning: conflicts while merging A! (edit, then use 'hg resolve --mark') warning: conflicts while merging A! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ hg tglog $ hg tglog
o 3: 63668d570d21 'C' o 3: 63668d570d21 'C'
| |
| @ 2: 82b8abf9c185 'D' | @ 2: 82b8abf9c185 'D'
| | | |
@ | 1: f899f3910ce7 'B' % | 1: f899f3910ce7 'B'
|/ |/
o 0: 4a2df7238c3b 'A' o 0: 4a2df7238c3b 'A'
$ cat A $ cat A
<<<<<<< dest: 82b8abf9c185 D - test: D <<<<<<< dest: 82b8abf9c185 D - test: D
D D
||||||| base ||||||| base
A A
======= =======
B B
>>>>>>> source: f899f3910ce7 B - test: B >>>>>>> source: f899f3910ce7 B - test: B
$ echo BC > A $ echo BC > A
$ hg resolve -m $ hg resolve -m
(no more unresolved files) (no more unresolved files)
continue: hg rebase --continue continue: hg rebase --continue
$ hg rebase --continue $ hg rebase --continue
rebasing 1:f899f3910ce7 "B" (B) rebasing 1:f899f3910ce7 "B" (B)
rebasing 3:63668d570d21 "C" (C tip) rebasing 3:63668d570d21 "C" (C tip)
merging A merging A
warning: conflicts while merging A! (edit, then use 'hg resolve --mark') warning: conflicts while merging A! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ hg tglog $ hg tglog
@ 3: 63668d570d21 'C' % 3: 63668d570d21 'C'
| |
| @ 2: 82b8abf9c185 'D' | @ 2: 82b8abf9c185 'D'
| | | |
o | 1: f899f3910ce7 'B' o | 1: f899f3910ce7 'B'
|/ |/
o 0: 4a2df7238c3b 'A' o 0: 4a2df7238c3b 'A'
$ cat A $ cat A

tests/test-rebase-conflicts.t

rebasing 2:dc0947a82db8 "C" (C) rebasing 2:dc0947a82db8 "C" (C)
rebasing 3:e7b3f00ed42e "D" (D) rebasing 3:e7b3f00ed42e "D" (D)
rebasing 4:03ca77807e91 "E" (E) rebasing 4:03ca77807e91 "E" (E)
rebasing 5:9a6b91dc2044 "F" (F tip) rebasing 5:9a6b91dc2044 "F" (F tip)
merging conflict merging conflict
warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark') warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
The current parents are not 7 and 8 even though that's what we're merging
$ hg tglog $ hg tglog
@ 8:draft 'E' @ 8:draft 'E'
| |
| o 7:draft 'D' | @ 7:draft 'D'
|/ |/
o 6:draft 'C' o 6:draft 'C'
| |
| @ 5:draft 'F' | % 5:draft 'F'
| |\ | |\
| | o 4:draft 'E' | | o 4:draft 'E'
| | | | | |
| o | 3:draft 'D' | o | 3:draft 'D'
| |/ | |/
| o 2:draft 'C' | o 2:draft 'C'
| | | |
o | 1:draft 'B' o | 1:draft 'B'

tests/test-rebase-interruptions.t

rebasing 2:965c486023db "C" rebasing 2:965c486023db "C"
M A M A
rebasing 6:a0b2430ebfb8 "F" (tip) rebasing 6:a0b2430ebfb8 "F" (tip)
abort: precommit hook exited with status 1 abort: precommit hook exited with status 1
[255] [255]
$ hg tglogp $ hg tglogp
@ 7: 401ccec5e39f secret 'C' @ 7: 401ccec5e39f secret 'C'
| |
| @ 6: a0b2430ebfb8 secret 'F' | o 6: a0b2430ebfb8 secret 'F'
| | | |
o | 5: 45396c49d53b public 'B' o | 5: 45396c49d53b public 'B'
| | | |
o | 4: ae36e8e3dfd7 public 'E' o | 4: ae36e8e3dfd7 public 'E'
| | | |
o | 3: 46b37eabc604 public 'D' o | 3: 46b37eabc604 public 'D'
| | | |
| o 2: 965c486023db secret 'C' | o 2: 965c486023db secret 'C'
rebasing 6:a0b2430ebfb8 "F" (tip) rebasing 6:a0b2430ebfb8 "F" (tip)
transaction abort! transaction abort!
rollback completed rollback completed
abort: pretxncommit hook exited with status 1 abort: pretxncommit hook exited with status 1
[255] [255]
$ hg tglogp $ hg tglogp
@ 7: 401ccec5e39f secret 'C' @ 7: 401ccec5e39f secret 'C'
| |
| @ 6: a0b2430ebfb8 secret 'F' | o 6: a0b2430ebfb8 secret 'F'
| | | |
o | 5: 45396c49d53b public 'B' o | 5: 45396c49d53b public 'B'
| | | |
o | 4: ae36e8e3dfd7 public 'E' o | 4: ae36e8e3dfd7 public 'E'
| | | |
o | 3: 46b37eabc604 public 'D' o | 3: 46b37eabc604 public 'D'
| | | |
| o 2: 965c486023db secret 'C' | o 2: 965c486023db secret 'C'
rebasing 6:a0b2430ebfb8 "F" (tip) rebasing 6:a0b2430ebfb8 "F" (tip)
transaction abort! transaction abort!
rollback completed rollback completed
abort: pretxnclose hook exited with status 1 abort: pretxnclose hook exited with status 1
[255] [255]
$ hg tglogp $ hg tglogp
@ 7: 401ccec5e39f secret 'C' @ 7: 401ccec5e39f secret 'C'
| |
| @ 6: a0b2430ebfb8 secret 'F' | o 6: a0b2430ebfb8 secret 'F'
| | | |
o | 5: 45396c49d53b public 'B' o | 5: 45396c49d53b public 'B'
| | | |
o | 4: ae36e8e3dfd7 public 'E' o | 4: ae36e8e3dfd7 public 'E'
| | | |
o | 3: 46b37eabc604 public 'D' o | 3: 46b37eabc604 public 'D'
| | | |
| o 2: 965c486023db secret 'C' | o 2: 965c486023db secret 'C'

tests/test-rebase-obsolete.t

unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ cp -R . $TESTTMP/hidden-state2 $ cp -R . $TESTTMP/hidden-state2
$ hg log -G $ hg log -G
@ 2:b18e25de2cf5 D @ 2:b18e25de2cf5 D
| |
| @ 1:2ec65233581b B (pruned using prune)
|/
o 0:426bada5c675 A o 0:426bada5c675 A
$ hg summary $ hg summary
parent: 2:b18e25de2cf5 tip parent: 2:b18e25de2cf5 tip
D D
parent: 1:2ec65233581b (obsolete)
B
branch: default branch: default
commit: 2 modified, 1 unknown, 1 unresolved (merge) commit: 1 modified, 1 added, 1 unknown, 1 unresolved
update: (current) update: (current)
phases: 3 draft phases: 2 draft
rebase: 0 rebased, 2 remaining (rebase --continue) rebase: 0 rebased, 2 remaining (rebase --continue)
$ hg rebase --abort $ hg rebase --abort
rebase aborted rebase aborted
Also test --continue for the above case Also test --continue for the above case
$ cd $TESTTMP/hidden-state2 $ cd $TESTTMP/hidden-state2

tests/test-rebase-parameters.t

$ hg rebase -s 2 -d 1 --tool internal:fail $ hg rebase -s 2 -d 1 --tool internal:fail
rebasing 2:e4e3f3546619 "c2b" (tip) rebasing 2:e4e3f3546619 "c2b" (tip)
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ hg summary $ hg summary
parent: 1:56daeba07f4b parent: 1:56daeba07f4b
c2 c2
parent: 2:e4e3f3546619 tip
c2b
branch: default branch: default
commit: 1 modified, 1 unresolved (merge) commit: 1 unresolved (clean)
update: (current) update: 1 new changesets, 2 branch heads (merge)
phases: 3 draft phases: 3 draft
rebase: 0 rebased, 1 remaining (rebase --continue) rebase: 0 rebased, 1 remaining (rebase --continue)
$ hg resolve -l $ hg resolve -l
U c2 U c2
$ hg resolve -m c2 $ hg resolve -m c2
(no more unresolved files) (no more unresolved files)

tests/test-rebase-transaction.t

warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark') warning: conflicts while merging conflict! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue) unresolved conflicts (see hg resolve, then hg rebase --continue)
[1] [1]
$ hg tglog $ hg tglog
o 5: D o 5: D
| |
| @ 4: Z | @ 4: Z
| | | |
@ | 3: C % | 3: C
| | | |
| o 2: Y | o 2: Y
| | | |
o | 1: B o | 1: B
|/ |/
o 0: A o 0: A
$ hg st $ hg st
M C
M conflict M conflict
A B A B
A C
? conflict.orig ? conflict.orig
$ echo resolved > conflict $ echo resolved > conflict
$ hg resolve -m $ hg resolve -m
(no more unresolved files) (no more unresolved files)
continue: hg rebase --continue continue: hg rebase --continue
$ hg continue $ hg continue
already rebased 1:112478962961 "B" (B) as 79bc8f4973ce already rebased 1:112478962961 "B" (B) as 79bc8f4973ce
rebasing 3:c26739dbe603 "C" (C) rebasing 3:c26739dbe603 "C" (C)