This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7907

rebase: always be graft-like, not merge-like, also for merges
ClosedPublic

Authored by martinvonz on Jan 16 2020, 3:10 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG77bb38be00ea: rebase: always be graft-like, not merge-like, also for merges
Summary

Rebase works by updating to a commit and then grafting changes on
top. However, before this patch, it would actually merge in changes
instead of grafting them in in some cases. That is, it would use the
common ancestor as base instead of using one of the parents. That
seems wrong to me, so I'm changing it so defineparents() always
returns a value for base.

This fixes the bad behavior in test-rebase-newancestor.t, which was
introduced in 65f215ea3e8e (tests: add test for rebasing merges with
ancestors of the rebase destination, 2014-11-30).

The difference in test-rebase-dest.t is because the files in the tip
revision were A, D, E, F before this patch and A, D, F, G after it. I
think both files should ideally be there.

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 16 2020, 3:10 AM
martinvonz updated this revision to Diff 19377.Jan 16 2020, 1:23 PM
martinvonz added a comment.Jan 29 2020, 11:35 AM

Reviewers: This patch is independent of D7827, so please don't let that patch block this one.

martinvonz updated this revision to Diff 19699.Jan 29 2020, 7:56 PM
martinvonz updated this revision to Diff 19858.Feb 3 2020, 1:08 PM
martinvonz updated this revision to Diff 20093.Feb 10 2020, 7:22 PM
martinvonz added a commit: rHG77bb38be00ea: rebase: always be graft-like, not merge-like, also for merges.Feb 10 2020, 9:22 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG77bb38be00ea: rebase: always be graft-like, not merge-like, also for merges (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (82 lines)
Mtests/test-rebase-dest.t (2 lines)
Mtests/test-rebase-newancestor.t (49 lines)

Diff 20131

hgext/rebase.py

# at most 2 parents. # at most 2 parents.
for i, p in enumerate(oldps): for i, p in enumerate(oldps):
np = p # new parent np = p # new parent
if any(isancestor(x, dests[i]) for x in successorrevs(repo, p)): if any(isancestor(x, dests[i]) for x in successorrevs(repo, p)):
np = dests[i] np = dests[i]
elif p in state and state[p] > 0: elif p in state and state[p] > 0:
np = state[p] np = state[p]
# "bases" only record "special" merge bases that cannot be
# calculated from changelog DAG (i.e. isancestor(p, np) is False).
# For example:
#
# B' # rebase -s B -d D, when B was rebased to B'. dest for C
# | C # is B', but merge base for C is B, instead of
# D | # changelog.ancestor(C, B') == A. If changelog DAG and
# | B # "state" edges are merged (so there will be an edge from
# |/ # B to B'), the merge base is still ancestor(C, B') in
# A # the merged graph.
#
# Also see https://bz.mercurial-scm.org/show_bug.cgi?id=1950#c8
# which uses "virtual null merge" to explain this situation.
if isancestor(p, np):
bases[i] = nullrev
# If one parent becomes an ancestor of the other, drop the ancestor # If one parent becomes an ancestor of the other, drop the ancestor
for j, x in enumerate(newps[:i]): for j, x in enumerate(newps[:i]):
if x == nullrev: if x == nullrev:
continue continue
if isancestor(np, x): # CASE-1 if isancestor(np, x): # CASE-1
np = nullrev np = nullrev
elif isancestor(x, np): # CASE-2 elif isancestor(x, np): # CASE-2
newps[j] = np newps[j] = np
# | | # as merge base, the difference between D and F will include # | | # as merge base, the difference between D and F will include
# B C # C, so the rebased F will contain C surprisingly. If "E" was # B C # C, so the rebased F will contain C surprisingly. If "E" was
# |/ # chosen, the rebased F will contain B. # |/ # chosen, the rebased F will contain B.
# A Z # A Z
# #
# But our merge base candidates (D and E in above case) could still be # But our merge base candidates (D and E in above case) could still be
# better than the default (ancestor(F, Z) == null). Therefore still # better than the default (ancestor(F, Z) == null). Therefore still
# pick one (so choose p1 above). # pick one (so choose p1 above).
if sum(1 for b in set(bases) if b != nullrev) > 1: if sum(1 for b in set(bases) if b != nullrev and b not in newps) > 1:
unwanted = [None, None] # unwanted[i]: unwanted revs if choose bases[i] unwanted = [None, None] # unwanted[i]: unwanted revs if choose bases[i]
for i, base in enumerate(bases): for i, base in enumerate(bases):
if base == nullrev: if base == nullrev or base in newps:
continue continue
# Revisions in the side (not chosen as merge base) branch that # Revisions in the side (not chosen as merge base) branch that
# might contain "surprising" contents # might contain "surprising" contents
other_bases = set(bases) - {base} other_bases = set(bases) - {base}
siderevs = list( siderevs = list(
repo.revs(b'(%ld %% (%d+%d))', other_bases, base, dest) repo.revs(b'(%ld %% (%d+%d))', other_bases, base, dest)
) )
# If those revisions are covered by rebaseset, the result is good. # If those revisions are covered by rebaseset, the result is good.
# A merge in rebaseset would be considered to cover its ancestors. # A merge in rebaseset would be considered to cover its ancestors.
if siderevs: if siderevs:
rebaseset = [ rebaseset = [
r for r, d in state.items() if d > 0 and r not in obsskipped r for r, d in state.items() if d > 0 and r not in obsskipped
] ]
merges = [ merges = [
r for r in rebaseset if cl.parentrevs(r)[1] != nullrev r for r in rebaseset if cl.parentrevs(r)[1] != nullrev
] ]
unwanted[i] = list( unwanted[i] = list(
repo.revs( repo.revs(
b'%ld - (::%ld) - %ld', siderevs, merges, rebaseset b'%ld - (::%ld) - %ld', siderevs, merges, rebaseset
) )
) )
if any(revs is not None for revs in unwanted):
# Choose a merge base that has a minimal number of unwanted revs. # Choose a merge base that has a minimal number of unwanted revs.
l, i = min( l, i = min(
(len(revs), i) (len(revs), i)
for i, revs in enumerate(unwanted) for i, revs in enumerate(unwanted)
if revs is not None if revs is not None
) )
# The merge will include unwanted revisions. Abort now. Revisit this if # The merge will include unwanted revisions. Abort now. Revisit this if
# we have a more advanced merge algorithm that handles multiple bases. # we have a more advanced merge algorithm that handles multiple bases.
if l > 0: if l > 0:
unwanteddesc = _(b' or ').join( unwanteddesc = _(b' or ').join(
( (
b', '.join(b'%d:%s' % (r, repo[r]) for r in revs) b', '.join(b'%d:%s' % (r, repo[r]) for r in revs)
for revs in unwanted for revs in unwanted
if revs is not None if revs is not None
) )
) )
raise error.Abort( raise error.Abort(
_(b'rebasing %d:%s will include unwanted changes from %s') _(b'rebasing %d:%s will include unwanted changes from %s')
% (rev, repo[rev], unwanteddesc) % (rev, repo[rev], unwanteddesc)
) )
# newps[0] should match merge base if possible. Currently, if newps[i] # newps[0] should match merge base if possible. Currently, if newps[i]
# is nullrev, the only case is newps[i] and newps[j] (j < i), one is # is nullrev, the only case is newps[i] and newps[j] (j < i), one is
# the other's ancestor. In that case, it's fine to not swap newps here. # the other's ancestor. In that case, it's fine to not swap newps here.
# (see CASE-1 and CASE-2 above) # (see CASE-1 and CASE-2 above)
if i != 0: if i != 0:
if newps[i] != nullrev: if newps[i] != nullrev:
newps[0], newps[i] = newps[i], newps[0] newps[0], newps[i] = newps[i], newps[0]
bases[0], bases[i] = bases[i], bases[0] bases[0], bases[i] = bases[i], bases[0]
# "rebasenode" updates to new p1, use the corresponding merge base. # "rebasenode" updates to new p1, use the corresponding merge base.
if bases[0] != nullrev:
base = bases[0] base = bases[0]
else:
base = None
repo.ui.debug(b" future parents are %d and %d\n" % tuple(newps)) repo.ui.debug(b" future parents are %d and %d\n" % tuple(newps))
return newps[0], newps[1], base return newps[0], newps[1], base
def isagitpatch(repo, patchname): def isagitpatch(repo, patchname):
"""Return true if the given patch is in git format""" """Return true if the given patch is in git format"""

tests/test-rebase-dest.t

> | F > | F
> | > |
> B E > B E
> |\| > |\|
> A D > A D
> EOS > EOS
rebasing 3:a4256619d830 "B" (B) rebasing 3:a4256619d830 "B" (B)
rebasing 6:8e139e245220 "C" (C tip) rebasing 6:8e139e245220 "C" (C tip)
o 8: 51e2ce92e06a C o 8: d7d1169e9b1c C
|\ |\
| o 7: 2ed0c8546285 B | o 7: 2ed0c8546285 B
| |\ | |\
o | | 5: 8fdb2c1feb20 G o | | 5: 8fdb2c1feb20 G
| | | | | |
| | o 4: cd488e83d208 E | | o 4: cd488e83d208 E
| | | | | |
o | | 2: a6661b868de9 F o | | 2: a6661b868de9 F

tests/test-rebase-newancestor.t

The test case creates a dev branch that contains a couple of merges from the The test case creates a dev branch that contains a couple of merges from the
default branch. When rebasing to the default branch, these merges would be default branch. When rebasing to the default branch, these merges would be
merges with ancestors on the same branch. The merges _could_ contain some merges with ancestors on the same branch. The merges _could_ contain some
interesting conflict resolutions or additional changes in the merge commit, but interesting conflict resolutions or additional changes in the merge commit, but
that is mixed up with the actual merge stuff and there is in general no way to that is mixed up with the actual merge stuff and there is in general no way to
separate them. separate them.
Note: The dev branch contains _no_ changes to f-default. It might be unclear
how rebasing of ancestor merges should be handled, but the current behavior
with spurious prompts for conflicts in files that didn't change seems very
wrong.
$ hg init ancestor-merge $ hg init ancestor-merge
$ cd ancestor-merge $ cd ancestor-merge
$ touch f-default $ touch f-default
$ hg ci -Aqm 'default: create f-default' $ hg ci -Aqm 'default: create f-default'
$ hg branch -q dev $ hg branch -q dev
$ hg ci -qm 'dev: create branch' $ hg ci -qm 'dev: create branch'
$ hg rebase -r 'only(dev,default)' -d default --config ui.interactive=True << EOF $ hg rebase -r 'only(dev,default)' -d default --config ui.interactive=True << EOF
> c > c
> EOF > EOF
rebasing 1:1d1a643d390e "dev: create branch" rebasing 1:1d1a643d390e "dev: create branch"
note: not rebasing 1:1d1a643d390e "dev: create branch", its destination already has all its changes note: not rebasing 1:1d1a643d390e "dev: create branch", its destination already has all its changes
rebasing 2:ec2c14fb2984 "dev: f-dev stuff" rebasing 2:ec2c14fb2984 "dev: f-dev stuff"
rebasing 4:4b019212aaf6 "dev: merge default" rebasing 4:4b019212aaf6 "dev: merge default"
file 'f-default' was deleted in local [dest] but was modified in other [source]. note: not rebasing 4:4b019212aaf6 "dev: merge default", its destination already has all its changes
You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved.
What do you want to do? c
rebasing 6:010ced67e558 "dev: merge default" rebasing 6:010ced67e558 "dev: merge default"
note: not rebasing 6:010ced67e558 "dev: merge default", its destination already has all its changes
saved backup bundle to $TESTTMP/ancestor-merge/.hg/strip-backup/1d1a643d390e-4a6f6d17-rebase.hg saved backup bundle to $TESTTMP/ancestor-merge/.hg/strip-backup/1d1a643d390e-4a6f6d17-rebase.hg
$ hg tglog $ hg tglog
o 6: de147e4f69cf 'dev: merge default'
|
o 5: eda7b7f46f5d 'dev: merge default'
|
o 4: 3e075b1c0a40 'dev: f-dev stuff' o 4: 3e075b1c0a40 'dev: f-dev stuff'
| |
@ 3: e08089805d82 'default: f-other stuff' @ 3: e08089805d82 'default: f-other stuff'
| |
o 2: 462860db70a1 'default: remove f-default' o 2: 462860db70a1 'default: remove f-default'
| |
o 1: f157ecfd2b6b 'default: f-default stuff' o 1: f157ecfd2b6b 'default: f-default stuff'
| |
o 0: e90e8eb90b6f 'default: create f-default' o 0: e90e8eb90b6f 'default: create f-default'
Grafty cherry picking rebasing: Grafty cherry picking rebasing:
$ cd ../ancestor-merge-2 $ cd ../ancestor-merge-2
$ hg phase -fdr0: $ hg phase -fdr0:
$ hg rebase -r 'children(only(dev,default))' -d default --config ui.interactive=True << EOF $ hg rebase -r 'children(only(dev,default))' -d default --config ui.interactive=True << EOF
> c > c
> EOF > EOF
rebasing 2:ec2c14fb2984 "dev: f-dev stuff" rebasing 2:ec2c14fb2984 "dev: f-dev stuff"
rebasing 4:4b019212aaf6 "dev: merge default" rebasing 4:4b019212aaf6 "dev: merge default"
file 'f-default' was deleted in local [dest] but was modified in other [source]. abort: rebasing 4:4b019212aaf6 will include unwanted changes from 1:1d1a643d390e
You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved. [255]
What do you want to do? c
rebasing 6:010ced67e558 "dev: merge default"
saved backup bundle to $TESTTMP/ancestor-merge-2/.hg/strip-backup/ec2c14fb2984-827d7a44-rebase.hg
$ hg tglog
o 7: de147e4f69cf 'dev: merge default'
|
o 6: eda7b7f46f5d 'dev: merge default'
|
o 5: 3e075b1c0a40 'dev: f-dev stuff'
|
o 4: e08089805d82 'default: f-other stuff'
|
o 3: 462860db70a1 'default: remove f-default'
|
o 2: f157ecfd2b6b 'default: f-default stuff'
|
| o 1: 1d1a643d390e 'dev: create branch' dev
|/
o 0: e90e8eb90b6f 'default: create f-default'
$ cd .. $ cd ..
Test order of parents of rebased merged with un-rebased changes as p1. Test order of parents of rebased merged with un-rebased changes as p1.
$ hg init parentorder $ hg init parentorder
$ cd parentorder $ cd parentorder
$ touch f $ touch f
$ hg up -qr 2 $ hg up -qr 2
$ hg merge -qr 3 $ hg merge -qr 3
$ echo 'other change while merging future "rebase ancestors"' > other $ echo 'other change while merging future "rebase ancestors"' > other
$ hg ci -Aqm 'merge rebase ancestors' $ hg ci -Aqm 'merge rebase ancestors'
$ hg rebase -d 5 -v $ hg rebase -d 5 -v
rebasing 6:4c5f12f25ebe "merge rebase ancestors" (tip) rebasing 6:4c5f12f25ebe "merge rebase ancestors" (tip)
resolving manifests resolving manifests
removing other removing other
note: merging f9daf77ffe76+ and 4c5f12f25ebe using bids from ancestors a60552eb93fb and f59da8fc0fcf
calculating bids for ancestor a60552eb93fb
resolving manifests
calculating bids for ancestor f59da8fc0fcf
resolving manifests resolving manifests
auction for merging merge bids
other: consensus for g
end of auction
getting other getting other
committing files: committing files:
other other
committing manifest committing manifest
committing changelog committing changelog
rebase merging completed rebase merging completed
1 changesets found 1 changesets found
uncompressed size of bundle content: uncompressed size of bundle content: