This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9159

[RFC] merge: stop caring about whether files are related (issue6163)
Needs RevisionPublic

Authored by martinvonz on Oct 6 2020, 8:05 PM.

Details

Reviewers
marmoute
pulkit
Group Reviewers
hg-reviewers
Summary

See discussion in
https://bz.mercurial-scm.org/show_bug.cgi?id=6163. D9130 reminded me
of this idea.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

martinvonz created this revision.Oct 6 2020, 8:05 PM
marmoute requested changes to this revision.Oct 8 2020, 5:21 AM
marmoute added a subscriber: marmoute.

Regardless of the issue6163 question, simply dropping the tracking or removed files seems wrong and will introduces bug (eg: we can end up merging a "dead" file history with a fresh file history, picking the "dead" one). So we cannot do that.

minimal description of the case I am talking about below.

o → X should be copied from B, not A
| \
o |  delete X 
| |
o |  rename A to X
| |
| o rename B to X
|/
o
This revision now requires changes to proceed.Oct 8 2020, 5:21 AM
marmoute added a comment.Oct 8 2020, 5:25 AM

We would also need more code for simpler case:

4 → copy tracing from 1 to 4 should not list X←Y 
| \
3 | delete Y
|/
2 rename X → Y
|
1 …
martinvonz added a comment.Oct 8 2020, 11:15 AM
In D9159#137729, @marmoute wrote:

Regardless of the issue6163 question, simply dropping the tracking or removed files seems wrong and will introduces bug (eg: we can end up merging a "dead" file history with a fresh file history, picking the "dead" one). So we cannot do that.
minimal description of the case I am talking about below.

o → X should be copied from B, not A
| \
o |  delete X 
| |
o |  rename A to X
| |
| o rename B to X
|/
o

I added that test as a parent of this patch (D9171). It passed before and after this patch. Did I not write it down correctly?

martinvonz updated this revision to Diff 23111.Oct 8 2020, 12:18 PM
martinvonz planned changes to this revision.Oct 8 2020, 1:28 PM

I've exported the current state so you can see the differences on the entire test suite, but I plan to make it configurable so it can be rolled out slowly. Maybe I'll make a copy of test-copies-unrelated.t then, or maybe I'll add another #testcases follow nofollow to the file.

marmoute requested changes to this revision.Oct 8 2020, 5:37 PM

My previous, larger comment got lost. It contained other tests cases.

I do not have time re-type my full comment (phabricator eating comment is very upsetting). Please stop pushing this diff forward, it is broken.

martinvonz added a comment.Oct 8 2020, 6:01 PM
In D9159#137869, @marmoute wrote:

My previous, larger comment got lost. It contained other tests cases.
I do not have time re-type my full comment (phabricator eating comment is very upsetting).

Weird, never happened to me.

Please stop pushing this diff forward, it is broken.

Well, you'll have to provide at least some proof of *what* is broken then.

martinvonz updated this revision to Diff 23123.Oct 9 2020, 2:28 AM
martinvonz added a comment.Oct 9 2020, 3:35 AM

test

In D9159#137749, @martinvonz wrote:
In D9159#137729, @marmoute wrote:

Regardless of the issue6163 question, simply dropping the tracking or removed files seems wrong and will introduces bug (eg: we can end up merging a "dead" file history with a fresh file history, picking the "dead" one). So we cannot do that.
minimal description of the case I am talking about below.

o → X should be copied from B, not A
| \
o |  delete X 
| |
o |  rename A to X
| |
| o rename B to X
|/
o

I added that test as a parent of this patch (D9171). It passed before and after this patch. Did I not write it down correctly?

Hmm, I must have run tests on another version of something, because the test now fails for me with this patch, which makes sense. I think I once had a check in the code merging the copy dicts from each merge parent to make it ignore copies that had been deleted (like in this test case), but it was really slow. I took a quick look but I didn't find it and it's getting late here.

martinvonz updated this revision to Diff 23140.Oct 9 2020, 3:37 AM
pulkit requested changes to this revision.Dec 4 2020, 2:01 PM
pulkit added a subscriber: pulkit.

IIUC changes are planned on this one as the test case mentioned fails with this patch. Kindly correct me if I am wrong.

This revision now requires changes to proceed.Dec 4 2020, 2:01 PM
martinvonz added a comment.Dec 4 2020, 2:04 PM
In D9159#143133, @pulkit wrote:

IIUC changes are planned on this one as the test case mentioned fails with this patch. Kindly correct me if I am wrong.

Yes, I'll see if I can fix that without making things too slow. If I can't, this patch may still be an improvement since it fixes many of the more common cases and breaks a less common one.

marmoute added a comment.Dec 4 2020, 6:03 PM
In D9159#143135, @martinvonz wrote:
In D9159#143133, @pulkit wrote:

IIUC changes are planned on this one as the test case mentioned fails with this patch. Kindly correct me if I am wrong.

Yes, I'll see if I can fix that without making things too slow. If I can't, this patch may still be an improvement since it fixes many of the more common cases and breaks a less common one.

Please note that the changesets centric code (and tests) is still in active development on both is behavior (they are probably more bug laying around) and performance. Please refrain from sending patch in this area without coordinating with the people at Octobus doing that work. Thanks.

marmoute requested changes to this revision.Dec 4 2020, 6:04 PM

Revision Contents
Changeset List

Diff 23140

mercurial/copies.py

if childcopies: if childcopies:
newcopies = copies.copy() newcopies = copies.copy()
for dest, source in pycompat.iteritems(childcopies): for dest, source in pycompat.iteritems(childcopies):
prev = copies.get(source) prev = copies.get(source)
if prev is not None and prev[1] is not None: if prev is not None and prev[1] is not None:
source = prev[1] source = prev[1]
newcopies[dest] = (c, source) newcopies[dest] = (c, source)
assert newcopies is not copies assert newcopies is not copies
if changes is not None:
for f in changes.removed:
if f in newcopies:
if newcopies is copies:
# copy on write to avoid affecting potential other
# branches. when there are no other branches, this
# could be avoided.
newcopies = copies.copy()
newcopies[f] = (c, None)
othercopies = all_copies.get(c) othercopies = all_copies.get(c)
if othercopies is None: if othercopies is None:
all_copies[c] = newcopies all_copies[c] = newcopies
else: else:
# we are the second parent to work on c, we need to merge our # we are the second parent to work on c, we need to merge our
# work with the other. # work with the other.
# #
# In case of conflict, parent 1 take precedence over parent 2. # In case of conflict, parent 1 take precedence over parent 2.
def _revinfo_getter_extra(repo): def _revinfo_getter_extra(repo):
"""return a function that return multiple data given a <rev>"i """return a function that return multiple data given a <rev>"i
* p1: revision number of first parent * p1: revision number of first parent
* p2: revision number of first parent * p2: revision number of first parent
* p1copies: mapping of copies from p1 * p1copies: mapping of copies from p1
* p2copies: mapping of copies from p2 * p2copies: mapping of copies from p2
* removed: a list of removed files
* ismerged: a callback to know if file was merged in that revision * ismerged: a callback to know if file was merged in that revision
""" """
cl = repo.changelog cl = repo.changelog
parents = cl.parentrevs parents = cl.parentrevs
def get_ismerged(rev): def get_ismerged(rev):
ctx = repo[rev] ctx = repo[rev]
return nb_parents >= 2 return nb_parents >= 2
return ismerged return ismerged
def revinfo(rev): def revinfo(rev):
p1, p2 = parents(rev) p1, p2 = parents(rev)
ctx = repo[rev] ctx = repo[rev]
p1copies, p2copies = ctx._copies p1copies, p2copies = ctx._copies
removed = ctx.filesremoved() return p1, p2, p1copies, p2copies, get_ismerged(rev)
return p1, p2, p1copies, p2copies, removed, get_ismerged(rev)
return revinfo return revinfo
def _combine_changeset_copies_extra( def _combine_changeset_copies_extra(
revs, children, targetrev, revinfo, match, isancestor revs, children, targetrev, revinfo, match, isancestor
): ):
"""version of `_combine_changeset_copies` that works with the Google """version of `_combine_changeset_copies` that works with the Google
specific "extra" based storage for copy information""" specific "extra" based storage for copy information"""
all_copies = {} all_copies = {}
alwaysmatch = match.always() alwaysmatch = match.always()
for r in revs: for r in revs:
copies = all_copies.pop(r, None) copies = all_copies.pop(r, None)
if copies is None: if copies is None:
# this is a root # this is a root
copies = {} copies = {}
for i, c in enumerate(children[r]): for i, c in enumerate(children[r]):
p1, p2, p1copies, p2copies, removed, ismerged = revinfo(c) p1, p2, p1copies, p2copies, ismerged = revinfo(c)
if r == p1: if r == p1:
parent = 1 parent = 1
childcopies = p1copies childcopies = p1copies
else: else:
assert r == p2 assert r == p2
parent = 2 parent = 2
childcopies = p2copies childcopies = p2copies
if not alwaysmatch: if not alwaysmatch:
childcopies = { childcopies = {
dst: src for dst, src in childcopies.items() if match(dst) dst: src for dst, src in childcopies.items() if match(dst)
} }
newcopies = copies newcopies = copies
if childcopies: if childcopies:
newcopies = copies.copy() newcopies = copies.copy()
for dest, source in pycompat.iteritems(childcopies): for dest, source in pycompat.iteritems(childcopies):
prev = copies.get(source) prev = copies.get(source)
if prev is not None and prev[1] is not None: if prev is not None and prev[1] is not None:
source = prev[1] source = prev[1]
newcopies[dest] = (c, source) newcopies[dest] = (c, source)
assert newcopies is not copies assert newcopies is not copies
for f in removed:
if f in newcopies:
if newcopies is copies:
# copy on write to avoid affecting potential other
# branches. when there are no other branches, this
# could be avoided.
newcopies = copies.copy()
newcopies[f] = (c, None)
othercopies = all_copies.get(c) othercopies = all_copies.get(c)
if othercopies is None: if othercopies is None:
all_copies[c] = newcopies all_copies[c] = newcopies
else: else:
# we are the second parent to work on c, we need to merge our # we are the second parent to work on c, we need to merge our
# work with the other. # work with the other.
# #
# In case of conflict, parent 1 take precedence over parent 2. # In case of conflict, parent 1 take precedence over parent 2.
if src not in m1: if src not in m1:
# renamed on side 1, deleted on side 2 # renamed on side 1, deleted on side 2
renamedelete[src] = dsts1 renamedelete[src] = dsts1
elif src not in mb: elif src not in mb:
# Work around the "short-circuit to avoid issues with merge states" # Work around the "short-circuit to avoid issues with merge states"
# thing in pathcopies(): pathcopies(x, y) can return a copy where the # thing in pathcopies(): pathcopies(x, y) can return a copy where the
# destination doesn't exist in y. # destination doesn't exist in y.
pass pass
elif mb[src] != m2[src] and not _related(c2[src], base[src]):
return
elif mb[src] != m2[src] or mb.flags(src) != m2.flags(src): elif mb[src] != m2[src] or mb.flags(src) != m2.flags(src):
# modified on side 2 # modified on side 2
for dst in dsts1: for dst in dsts1:
copy[dst] = src copy[dst] = src
class branch_copies(object): class branch_copies(object):
"""Information about copies made on one side of a merge/graft. """Information about copies made on one side of a merge/graft.

tests/test-copies-chain-merge.t

M b M b
M d M d
$ hg status --copies --rev 'desc("i-2")' --rev 'desc("mCBm-1")' $ hg status --copies --rev 'desc("i-2")' --rev 'desc("mCBm-1")'
M b M b
M d M d
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBCm-1")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBCm-1")'
M b M b
A d A d
a (no-filelog !)
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCBm-1")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCBm-1")'
M b M b
A d A d
a (no-filelog !)
R a R a
Comparing with a merge re-adding the file afterward Comparing with a merge re-adding the file afterward
--------------------------------------------------- ---------------------------------------------------
Merge: Merge:
- one with change to an unrelated file - one with change to an unrelated file
- one deleting and recreating the change - one deleting and recreating the change
o 8 d-2 re-add d o 8 d-2 re-add d
| |
~ ~
#endif #endif
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBDm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBDm-0")'
M b M b
A d A d
a (no-filelog !)
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDBm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDBm-0")'
M b M b
A d A d
a (no-filelog !)
R a R a
Comparing with a merge with colliding rename Comparing with a merge with colliding rename
-------------------------------------------- --------------------------------------------
- the "e-" branch renaming b to f (through 'g') - the "e-" branch renaming b to f (through 'g')
- the "a-" branch renaming d to f (through e) - the "a-" branch renaming d to f (through e)
One side of the merge have a long history with rename. The other side of the One side of the merge have a long history with rename. The other side of the
merge point to a new file with a smaller history. Each side is "valid". merge point to a new file with a smaller history. Each side is "valid".
(and again the filelog based algorithm only explore one, with a pick based on (and again the filelog based algorithm only explore one, with a pick based on
revision numbers) revision numbers)
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDGm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDGm-0")'
A d A d
a (filelog !) a
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGDm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGDm-0")'
A d A d
a a
R a R a
$ hg status --copies --rev 'desc("d-2")' --rev 'desc("mDGm-0")' $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mDGm-0")'
M d M d
$ hg status --copies --rev 'desc("d-2")' --rev 'desc("mGDm-0")' $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mGDm-0")'
| |
o 0 i-0 initial commit: a b h o 0 i-0 initial commit: a b h
'a' is the copy source of 'd' 'a' is the copy source of 'd'
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCGm-0")'
A d A d
a (no-compatibility no-changeset !) a
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGCm-0")'
A d A d
a (no-compatibility no-changeset !) a
R a R a
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCGm-0")'
A d A d
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mGCm-0")'
A d A d
$ hg status --copies --rev 'desc("g-1")' --rev 'desc("mCGm-0")' $ hg status --copies --rev 'desc("g-1")' --rev 'desc("mCGm-0")'
$ hg status --copies --rev 'desc("g-1")' --rev 'desc("mGCm-0")' $ hg status --copies --rev 'desc("g-1")' --rev 'desc("mGCm-0")'
o 0 i-0 initial commit: a b h o 0 i-0 initial commit: a b h
'a' is the the copy source of 'd' 'a' is the the copy source of 'd'
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mCB-revert-m-0")'
M b M b
A d A d
a (no-compatibility no-changeset !) a
R a R a
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC-revert-m-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBC-revert-m-0")'
M b M b
A d A d
a (no-compatibility no-changeset !) a
R a R a
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mCB-revert-m-0")'
M b M b
A d A d
$ hg status --copies --rev 'desc("c-1")' --rev 'desc("mBC-revert-m-0")' $ hg status --copies --rev 'desc("c-1")' --rev 'desc("mBC-revert-m-0")'
M b M b
A d A d
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mCB-revert-m-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mCB-revert-m-0")'

tests/test-copies-unrelated.t

o 3 remove y o 3 remove y
| y | y
o 2 rename x to y o 2 rename x to y
| x y | x y
| o 1 modify x | o 1 modify x
|/ x |/ x
o 0 add x o 0 add x
x x
#if no-filelog
$ hg debugpathcopies 0 4
x -> y
$ hg graft -r 1
grafting 1:* "modify x" (glob)
merging y and x to y
#else
$ hg debugpathcopies 0 4 $ hg debugpathcopies 0 4
BROKEN: This should succeed and merge the changes from x into y BROKEN: This should succeed and merge the changes from x into y
$ hg graft -r 1 $ hg graft -r 1
grafting 1:* "modify x" (glob) grafting 1:* "modify x" (glob)
file 'x' was deleted in local [local] but was modified in other [graft]. file 'x' was deleted in local [local] but was modified in other [graft].
You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved. You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved.
What do you want to do? u What do you want to do? u
abort: unresolved conflicts, can't continue abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue') (use 'hg resolve' and 'hg graft --continue')
[1] [1]
#endif
Add x, remove it, then add it back, then rename x to y. Similar to the case Add x, remove it, then add it back, then rename x to y. Similar to the case
above, but here the break in history is before the rename. above, but here the break in history is before the rename.
$ newrepo $ newrepo
$ echo x > x $ echo x > x
$ hg ci -Aqm 'add x' $ hg ci -Aqm 'add x'
$ echo x2 > x $ echo x2 > x
$ hg ci -m 'modify x' $ hg ci -m 'modify x'
file 'x' was deleted in local [local] but was modified in other [graft]. file 'x' was deleted in local [local] but was modified in other [graft].
You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved. You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved.
What do you want to do? u What do you want to do? u
abort: unresolved conflicts, can't continue abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue') (use 'hg resolve' and 'hg graft --continue')
[1] [1]
#endif #endif
$ hg co -qC 2 $ hg co -qC 2
BROKEN: This should succeed and merge the changes from x into y
$ hg graft -r 5 $ hg graft -r 5
grafting 5:* "rename x to y"* (glob) grafting 5:* "rename x to y"* (glob)
file 'x' was deleted in other [graft] but was modified in local [local]. merging x and y to y
You can use (c)hanged version, (d)elete, or leave (u)nresolved.
What do you want to do? u
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[1]
Add x, remove it, then add it back, rename x to y from the first commit. Add x, remove it, then add it back, rename x to y from the first commit.
Similar to the case above, but here the break in history is parallel to the Similar to the case above, but here the break in history is parallel to the
rename. rename.
$ newrepo $ newrepo
$ echo x > x $ echo x > x
$ hg ci -Aqm 'add x' $ hg ci -Aqm 'add x'
$ hg rm x $ hg rm x
o 0 add x o 0 add x
x x
$ hg debugpathcopies 2 3 $ hg debugpathcopies 2 3
x -> y x -> y
BROKEN: This should merge the changes from x into y BROKEN: This should merge the changes from x into y
$ hg graft -r 2 $ hg graft -r 2
grafting 2:* "add x again with different content" (glob) grafting 2:* "add x again with different content" (glob)
$ hg co -qC 2 $ hg co -qC 2
BROKEN: This should succeed and merge the changes from x into y
$ hg graft -r 3 $ hg graft -r 3
grafting 3:* "rename x to y" (glob) grafting 3:* "rename x to y" (glob)
file 'x' was deleted in other [graft] but was modified in local [local]. merging x and y to y
You can use (c)hanged version, (d)elete, or leave (u)nresolved.
What do you want to do? u
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[1]
Add x on two branches, then rename x to y on one side. Similar to the case Add x on two branches, then rename x to y on one side. Similar to the case
above, but here the break in history is via the base commit. above, but here the break in history is via the base commit.
$ newrepo $ newrepo
$ echo a > a $ echo a > a
$ hg ci -Aqm 'base' $ hg ci -Aqm 'base'
$ echo x > x $ echo x > x
$ hg ci -Aqm 'add x' $ hg ci -Aqm 'add x'
file 'x' was deleted in local [local] but was modified in other [graft]. file 'x' was deleted in local [local] but was modified in other [graft].
You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved. You can use (c)hanged version, leave (d)eleted, or leave (u)nresolved.
What do you want to do? u What do you want to do? u
abort: unresolved conflicts, can't continue abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue') (use 'hg resolve' and 'hg graft --continue')
[1] [1]
#endif #endif
$ hg co -qC 2 $ hg co -qC 2
BROKEN: This should succeed and merge the changes from x into y
$ hg graft -r 5 $ hg graft -r 5
grafting 5:* "rename x to y"* (glob) grafting 5:* "rename x to y"* (glob)
file 'x' was deleted in other [graft] but was modified in local [local]. merging x and y to y
You can use (c)hanged version, (d)elete, or leave (u)nresolved.
What do you want to do? u
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[1]
Copies via null revision (there shouldn't be any) Copies via null revision (there shouldn't be any)
$ newrepo $ newrepo
$ echo x > x $ echo x > x
$ hg ci -Aqm 'add x' $ hg ci -Aqm 'add x'
$ hg cp x y $ hg cp x y
$ hg ci -m 'copy x to y' $ hg ci -m 'copy x to y'
$ hg co -q null $ hg co -q null

tests/test-copies.t

| | z | | z
| o 2 rename y to z | o 2 rename y to z
| | y z | | y z
o | 1 rename x to z o | 1 rename x to z
|/ x z |/ x z
o 0 add x and y o 0 add x and y
x y x y
$ hg debugpathcopies 1 4 $ hg debugpathcopies 1 4
y -> z (no-filelog !)
$ hg debugpathcopies 2 4 $ hg debugpathcopies 2 4
x -> z (no-filelog !) x -> z (no-filelog !)
$ hg debugpathcopies 0 4 $ hg debugpathcopies 0 4
y -> z (no-filelog !)
x -> z (filelog !) x -> z (filelog !)
$ hg debugpathcopies 1 5 $ hg debugpathcopies 1 5
y -> z (no-filelog !)
$ hg debugpathcopies 2 5 $ hg debugpathcopies 2 5
x -> z (no-filelog !) x -> z (no-filelog !)
$ hg debugpathcopies 0 5 $ hg debugpathcopies 0 5
x -> z x -> z
Test for a case in fullcopytracing algorithm where neither of the merging csets Test for a case in fullcopytracing algorithm where neither of the merging csets
is a descendant of the merge base. This test reflects that the algorithm is a descendant of the merge base. This test reflects that the algorithm