This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8243

copies: stop recording buggy file merge when new file overwrite an old one
AbandonedPublic

Authored by marmoute on Mar 5 2020, 1:14 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

The BD/DB and BF/FB cases in tests/test-copies-chain-merge.t shown an
inconsistent behavior when merging two branches where one plainly replace a file
untouched on the other side.

We do the minimal amount of work in the commit code to fix this bug. The case is
narrow enough that the performance impact does not scare me right now.

My priority is to get the behavior fixed with the minimal code changes. (Because
I need to behavior to be right to make more progress on the changeset centric
algorithm). We can iterate later on a more efficient approach (updating the
merge state to record the situation at the time it is initially detected).

The tests/test-convert-hg-source.t is affected because it also created such
graphs.

Diff Detail

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

Event Timeline

marmoute created this revision.Mar 5 2020, 1:14 PM
marmoute updated this revision to Diff 20550.Mar 6 2020, 5:21 AM
martinvonz added a subscriber: martinvonz.Mar 6 2020, 2:04 PM
martinvonz added inline comments.
mercurial/localrepo.py
2868–2874

Is this correct when grafting (not merging), such as when rebasing? Is it worth having a test case for that? Is it even possible to get here in that case? Maybe when rebasing a merge commit?

tests/test-copies-chain-merge.t
663

Does the removal of this comment mean that it's no longer correct? I don't think that's what you meant, so should the comment remain (or even not have been added until now)?

marmoute added inline comments.Mar 6 2020, 4:42 PM
tests/test-copies-chain-merge.t
663

The previous output is no longer wrong, so we no longer need to specify the next one is wrong.

martinvonz added inline comments.Mar 6 2020, 4:45 PM
tests/test-copies-chain-merge.t
663

Ah, so that comment meant to say that that "unlike the test case just before, this one is correct". That wasn't clear to me. But no need to change anything since it's going away anyway.

marmoute retitled this revision from copies: stop recoding buggy file merge when new file overwrite an old one to copies: stop recording buggy file merge when new file overwrite an old one.Mar 6 2020, 6:52 PM
marmoute updated this revision to Diff 20595.

series update after Martin feedback

marmoute added inline comments.Mar 6 2020, 7:50 PM
mercurial/localrepo.py
2868–2874

I don't know, but not existing test broke. Consolidating test for the plain merge case already took a long time. Not having any test failure from exisitng rebase/graft test seemed enough to me while making progress on the correctness of this.

marmoute updated this revision to Diff 20755.Mar 11 2020, 6:02 PM
martinvonz added a comment.Mar 11 2020, 6:06 PM

FYI, I'd like to look at this patch, but I'm busy with other things right now (switching back and forth between browser tabs to delay the interview feedback that I'm actually supposed to write), so it will take a bit longer before I'll find time for this one.

martinvonz added a comment.Mar 12 2020, 1:16 PM

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

marmoute added a comment.Mar 19 2020, 8:21 PM
In D8243#123559, @martinvonz wrote:

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

My initial motivation to rush the ugly way was "getting the behavior right to compare with the changeset centric one and being able to test performance improvement for the changeset centric one while having access to a specific repository". However, cancelling of all travel has cancelled the window to access that repo. I'll resubmit a cleaner versions soon.

since you did not commented on it, I assume the new behavior is fine by you, right?

martinvonz added a comment.Mar 19 2020, 8:31 PM
In D8243#124077, @marmoute wrote:
In D8243#123559, @martinvonz wrote:

This is pretty ugly and it doesn't seem that the next patch depends on it. You said you'll soon clean it up anyway, so I wonder if should just wait for the better solution instead. It doesn't seem like this fixes a serious bug so we have to rush it. Thoughts?

My initial motivation to rush the ugly way was "getting the behavior right to compare with the changeset centric one and being able to test performance improvement for the changeset centric one while having access to a specific repository". However, cancelling of all travel has cancelled the window to access that repo. I'll resubmit a cleaner versions soon.
since you did not commented on it, I assume the new behavior is fine by you, right?

You mean the new behavior in this patch? Yes, I think I'm fine with it. I initially wasn't sure if the old behavior should be considered a bug, but I think you're right that it should.

durin42 requested changes to this revision.Apr 1 2020, 3:01 PM
durin42 added a subscriber: durin42.

Per irc checkin, we expect changes on this before review.

This revision now requires changes to proceed.Apr 1 2020, 3:01 PM
pulkit added a subscriber: pulkit.Apr 10 2020, 7:39 AM

D8392 is an attempt to fix the same issue by storing extra information in mergestate.

marmoute added a comment.Apr 10 2020, 7:54 AM
In D8243#125293, @pulkit wrote:

D8392 is an attempt to fix the same issue by storing extra information in mergestate.

The approach (storing in the merge state) is the one I planned to implement before handling the task to Pulkit. so +1 on the approach.

(feel free to reuse that very same diff for it.)

marmoute abandoned this revision.Apr 10 2020, 1:46 PM

superseeded by D8392

Revision Contents
Changeset List

Diff 20550

mercurial/localrepo.py

fparent1, fparent2 = fparent2, nullid fparent1, fparent2 = fparent2, nullid
elif fparent2 != nullid: elif fparent2 != nullid:
# is one parent an ancestor of the other? # is one parent an ancestor of the other?
fparentancestors = flog.commonancestorsheads(fparent1, fparent2) fparentancestors = flog.commonancestorsheads(fparent1, fparent2)
if fparent1 in fparentancestors: if fparent1 in fparentancestors:
fparent1, fparent2 = fparent2, nullid fparent1, fparent2 = fparent2, nullid
elif fparent2 in fparentancestors: elif fparent2 in fparentancestors:
fparent2 = nullid fparent2 = nullid
elif not fparentancestors:
# The two file seems entirely unrelated, one might be a full
# replacement of the other.
#
# At that point, the logic get complicated enough that we reuse
# the existing one.
#
# Ideally, we would store this information in the mergestate a
# simply reuse it. It could probably simplify the surounding
# code too.
p1ctx = fctx._changectx.p1()
p2ctx = fctx._changectx.p2()
matcher = matchmod.exact([fctx.path()])
if self.ui.configlist(b'merge', b'preferancestor') == [b'*']:
cahs = self.changelog.commonancestorsheads(
p1ctx.node(), p2ctx.node()
)
pas = [self[anc] for anc in (sorted(cahs) or [nullid])]
else:
pas = [p1ctx.ancestor(p2ctx, warn=False)]
martinvonzUnsubmitted
Not Done

Is this correct when grafting (not merging), such as when rebasing? Is it worth having a test case for that? Is it even possible to get here in that case? Maybe when rebasing a merge commit?

martinvonz: Is this correct when grafting (not merging), such as when rebasing? Is it worth having a test…
marmouteAuthorUnsubmitted
Done

I don't know, but not existing test broke. Consolidating test for the plain merge case already took a long time. Not having any test failure from exisitng rebase/graft test seemed enough to me while making progress on the correctness of this.

marmoute: I don't know, but not existing test broke. Consolidating test for the plain merge case already…
actionbyfile, __, __ = mergemod.calculateupdates(
self,
p1ctx,
p2ctx,
pas,
branchmerge=True,
force=True,
acceptremote=False,
followcopies=True,
matcher=matcher,
)
if actionbyfile[fctx.path()][0] == mergemod.ACTION_GET:
fparent1, fparent2 = fparent2, nullid
# is the file changed? # is the file changed?
text = fctx.data() text = fctx.data()
if fparent2 != nullid or flog.cmp(fparent1, text) or meta: if fparent2 != nullid or flog.cmp(fparent1, text) or meta:
changelist.append(fname) changelist.append(fname)
return flog.add(text, meta, tr, linkrev, fparent1, fparent2) return flog.add(text, meta, tr, linkrev, fparent1, fparent2)
# are just the flags changed during merge? # are just the flags changed during merge?
elif fname in manifest1 and manifest1.flags(fname) != fctx.flags(): elif fname in manifest1 and manifest1.flags(fname) != fctx.flags():

tests/test-convert-hg-source.t

scanning source... scanning source...
sorting... sorting...
converting... converting...
8 add foo bar 8 add foo bar
7 change foo 7 change foo
6 make bar and baz copies of foo 6 make bar and baz copies of foo
5 merge local copy 5 merge local copy
4 merge remote copy 4 merge remote copy
3 Added tag that for changeset 88586c4e9f02 3 Added tag that for changeset 8601262d7472
2 Removed tag that 2 Removed tag that
1 Added tag this for changeset c56a7f387039 1 Added tag this for changeset 706614b458c1
0 mark baz executable 0 mark baz executable
updating bookmarks updating bookmarks
$ cd new $ cd new
$ hg out ../orig $ hg out ../orig
comparing with ../orig comparing with ../orig
searching for changes searching for changes
no changes found no changes found
[1] [1]
#if execbit #if execbit
$ hg bookmarks $ hg bookmarks
premerge1 3:973ef48a98a4 premerge1 3:973ef48a98a4
premerge2 8:91d107c423ba premerge2 8:c4968fdf2e5d
#else #else
Different hash because no x bit Different hash because no x bit
$ hg bookmarks $ hg bookmarks
premerge1 3:973ef48a98a4 premerge1 3:973ef48a98a4
premerge2 8:3537b15eaaca premerge2 8:3537b15eaaca
#endif #endif
Test that redoing a convert results in an identical graph Test that redoing a convert results in an identical graph
$ cd ../ $ cd ../
$ rm new/.hg/shamap $ rm new/.hg/shamap
$ hg convert --datesort orig new 2>&1 | grep -v 'subversion python bindings could not be loaded' $ hg convert --datesort orig new 2>&1 | grep -v 'subversion python bindings could not be loaded'
scanning source... scanning source...
sorting... sorting...
converting... converting...
8 add foo bar 8 add foo bar
7 change foo 7 change foo
6 make bar and baz copies of foo 6 make bar and baz copies of foo
5 merge local copy 5 merge local copy
4 merge remote copy 4 merge remote copy
3 Added tag that for changeset 88586c4e9f02 3 Added tag that for changeset 8601262d7472
2 Removed tag that 2 Removed tag that
1 Added tag this for changeset c56a7f387039 1 Added tag this for changeset 706614b458c1
0 mark baz executable 0 mark baz executable
updating bookmarks updating bookmarks
$ hg -R new log -G -T '{rev} {desc}' $ hg -R new log -G -T '{rev} {desc}'
o 8 mark baz executable o 8 mark baz executable
| |
o 7 Added tag this for changeset c56a7f387039 o 7 Added tag this for changeset 706614b458c1
| |
o 6 Removed tag that o 6 Removed tag that
| |
o 5 Added tag that for changeset 88586c4e9f02 o 5 Added tag that for changeset 8601262d7472
| |
o 4 merge remote copy o 4 merge remote copy
|\ |\
+---o 3 merge local copy +---o 3 merge local copy
| |/ | |/
| o 2 make bar and baz copies of foo | o 2 make bar and baz copies of foo
| | | |
o | 1 change foo o | 1 change foo

tests/test-copies-chain-merge.t

$ hg up 'desc("b-1")' $ hg up 'desc("b-1")'
2 files updated, 0 files merged, 0 files removed, 0 files unresolved 2 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("d-2")' $ hg merge 'desc("d-2")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ hg ci -m 'mBDm-0 simple merge - one way' $ hg ci -m 'mBDm-0 simple merge - one way'
$ hg up 'desc("d-2")' $ hg up 'desc("d-2")'
2 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("b-1")' $ hg merge 'desc("b-1")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ hg ci -m 'mDBm-0 simple merge - the other way' $ hg ci -m 'mDBm-0 simple merge - the other way'
created new head created new head
$ hg log -G --rev '::(desc("mDBm")+desc("mBDm"))' $ hg log -G --rev '::(desc("mDBm")+desc("mBDm"))'
@ 16 mDBm-0 simple merge - the other way] @ 16 mDBm-0 simple merge - the other way]
|\ |\
$ hg commit -m "f-1: rename h -> i" $ hg commit -m "f-1: rename h -> i"
created new head created new head
$ hg mv --force i d $ hg mv --force i d
$ hg commit -m "f-2: rename i -> d" $ hg commit -m "f-2: rename i -> d"
$ hg debugindex d $ hg debugindex d
rev linkrev nodeid p1 p2 rev linkrev nodeid p1 p2
0 2 01c2f5eabdc4 000000000000 000000000000 0 2 01c2f5eabdc4 000000000000 000000000000
1 10 b004912a8510 000000000000 000000000000 1 10 b004912a8510 000000000000 000000000000
2 15 0bb5445dc4d0 01c2f5eabdc4 b004912a8510 2 22 c72365ee036f 000000000000 000000000000
3 22 c72365ee036f 000000000000 000000000000
$ hg up 'desc("b-1")' $ hg up 'desc("b-1")'
3 files updated, 0 files merged, 0 files removed, 0 files unresolved 3 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("f-2")' $ hg merge 'desc("f-2")'
1 files updated, 0 files merged, 1 files removed, 0 files unresolved 1 files updated, 0 files merged, 1 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ hg ci -m 'mBFm-0 simple merge - one way' $ hg ci -m 'mBFm-0 simple merge - one way'
$ hg up 'desc("f-2")' $ hg up 'desc("f-2")'
2 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ hg merge 'desc("b-1")' $ hg merge 'desc("b-1")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
(branch merge, don't forget to commit) (branch merge, don't forget to commit)
$ hg ci -m 'mFBm-0 simple merge - the other way' $ hg ci -m 'mFBm-0 simple merge - the other way'
created new head created new head
$ hg log -G --rev '::(desc("mBFm")+desc("mFBm"))' $ hg log -G --rev '::(desc("mBFm")+desc("mFBm"))'
@ 24 mFBm-0 simple merge - the other way] @ 24 mFBm-0 simple merge - the other way]
|\ |\
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
Note:
| In this case, one of the merge wrongly record a merge while there is none.
| This lead to bad copy tracing information to be dug up.
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mBDm-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mBDm-0")'
M d M d
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mDBm-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mDBm-0")'
M d M d
$ hg status --copies --rev 'desc("d-2")' --rev 'desc("mBDm-0")' $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mBDm-0")'
M b M b
M d
$ hg status --copies --rev 'desc("d-2")' --rev 'desc("mDBm-0")' $ hg status --copies --rev 'desc("d-2")' --rev 'desc("mDBm-0")'
M b M b
$ hg status --copies --rev 'desc("i-2")' --rev 'desc("mBDm-0")' $ hg status --copies --rev 'desc("i-2")' --rev 'desc("mBDm-0")'
M b M b
M d M d
$ hg status --copies --rev 'desc("i-2")' --rev 'desc("mDBm-0")' $ hg status --copies --rev 'desc("i-2")' --rev 'desc("mDBm-0")'
M b M b
M d M d
The bugs makes recorded copy is different depending of where we started the merge from since
The result must be the same for both merges
$ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d' $ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d'
0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 644 d b004912a8510032a0350a74daa2803dadfb00e12 644 d
$ hg manifest --debug --rev 'desc("mDBm-0")' | grep '644 d' $ hg manifest --debug --rev 'desc("mDBm-0")' | grep '644 d'
b004912a8510032a0350a74daa2803dadfb00e12 644 d b004912a8510032a0350a74daa2803dadfb00e12 644 d
The 0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 entry is wrong, since the file was
deleted on one side (then recreate) and untouched on the other side, no "merge"
has happened. The resulting `d` file is the untouched version from branch `D`,
not a merge.
$ hg manifest --debug --rev 'desc("d-2")' | grep '644 d' $ hg manifest --debug --rev 'desc("d-2")' | grep '644 d'
b004912a8510032a0350a74daa2803dadfb00e12 644 d b004912a8510032a0350a74daa2803dadfb00e12 644 d
$ hg manifest --debug --rev 'desc("b-1")' | grep '644 d' $ hg manifest --debug --rev 'desc("b-1")' | grep '644 d'
01c2f5eabdc4ce2bdee42b5f86311955e6c8f573 644 d 01c2f5eabdc4ce2bdee42b5f86311955e6c8f573 644 d
$ hg debugindex d $ hg debugindex d
rev linkrev nodeid p1 p2 rev linkrev nodeid p1 p2
0 2 01c2f5eabdc4 000000000000 000000000000 0 2 01c2f5eabdc4 000000000000 000000000000
1 10 b004912a8510 000000000000 000000000000 1 10 b004912a8510 000000000000 000000000000
2 15 0bb5445dc4d0 01c2f5eabdc4 b004912a8510 2 22 c72365ee036f 000000000000 000000000000
3 22 c72365ee036f 000000000000 000000000000 3 25 7bded9d9da1f 01c2f5eabdc4 000000000000
4 23 863d9bc49190 01c2f5eabdc4 c72365ee036f 4 26 f04cac32d703 b004912a8510 7bded9d9da1f
5 25 7bded9d9da1f 01c2f5eabdc4 000000000000 5 27 d7a5eafb9322 7bded9d9da1f b004912a8510
6 26 f04cac32d703 b004912a8510 7bded9d9da1f 6 28 2ed7a51aed47 c72365ee036f 7bded9d9da1f
7 27 d7a5eafb9322 7bded9d9da1f b004912a8510
8 28 2ed7a51aed47 c72365ee036f 7bded9d9da1f
(That output if wrong, since no merge actually happened).
$ hg log -Gfr 'desc("mBDm-0")' d $ hg log -Gfr 'desc("mBDm-0")' d
o 15 mBDm-0 simple merge - one way] o 14 d-2 re-add d]
|\
o : 14 d-2 re-add d]
:/
o 2 i-2: c -move-> d]
|
o 1 i-1: a -move-> c]
| |
o 0 i-0 initial commit: a b] ~
This output is correct
martinvonzUnsubmitted
Not Done

Does the removal of this comment mean that it's no longer correct? I don't think that's what you meant, so should the comment remain (or even not have been added until now)?

martinvonz: Does the removal of this comment mean that it's no longer correct? I don't think that's what…
marmouteAuthorUnsubmitted
Done

The previous output is no longer wrong, so we no longer need to specify the next one is wrong.

marmoute: The previous output is no longer wrong, so we no longer need to specify the next one is wrong.
martinvonzUnsubmitted
Not Done

Ah, so that comment meant to say that that "unlike the test case just before, this one is correct". That wasn't clear to me. But no need to change anything since it's going away anyway.

martinvonz: Ah, so that comment meant to say that that "unlike the test case just before, this one is…
$ hg log -Gfr 'desc("mDBm-0")' d $ hg log -Gfr 'desc("mDBm-0")' d
o 14 d-2 re-add d] o 14 d-2 re-add d]
| |
~ ~
$ 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 (true !)
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
R a R a
Comparing with a merge with colliding rename Comparing with a merge with colliding rename
-------------------------------------------- --------------------------------------------
- one with change to an unrelated file (b) - one with change to an unrelated file (b)
- one overwriting a file (d) with a rename (from h to i to d) - one overwriting a file (d) with a rename (from h to i to d)
The overwriting should take over. However, the behavior is currently buggy The overwriting should take over. However, the behavior is currently buggy
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBFm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBFm-0")'
M b M b
A d A d
a (true !) h
h (false !)
R a R a
R h R h
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mFBm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mFBm-0")'
M b M b
A d A d
h h
R a R a
R h R h
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mBFm-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mBFm-0")'
M d M d
R h R h
$ hg status --copies --rev 'desc("f-2")' --rev 'desc("mBFm-0")' $ hg status --copies --rev 'desc("f-2")' --rev 'desc("mBFm-0")'
M b M b
M d
$ hg status --copies --rev 'desc("f-1")' --rev 'desc("mBFm-0")' $ hg status --copies --rev 'desc("f-1")' --rev 'desc("mBFm-0")'
M b M b
M d M d
R i R i
$ hg status --copies --rev 'desc("b-1")' --rev 'desc("mFBm-0")' $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mFBm-0")'
M d M d
R h R h
$ hg status --copies --rev 'desc("f-2")' --rev 'desc("mFBm-0")' $ hg status --copies --rev 'desc("f-2")' --rev 'desc("mFBm-0")'
M b M b
$ hg status --copies --rev 'desc("f-1")' --rev 'desc("mFBm-0")' $ hg status --copies --rev 'desc("f-1")' --rev 'desc("mFBm-0")'
M b M b
M d M d
R i R i
The following graphlog is wrong, the "a -> c -> d" chain was overwritten and should not appear. The following graphlog is wrong, the "a -> c -> d" chain was overwritten and should not appear.
$ hg log -Gfr 'desc("mBFm-0")' d $ hg log -Gfr 'desc("mBFm-0")' d
o 23 mBFm-0 simple merge - one way] o 22 f-2: rename i -> d]
|\
o : 22 f-2: rename i -> d]
| :
o : 21 f-1: rename h -> i]
:/
o 2 i-2: c -move-> d]
|
o 1 i-1: a -move-> c]
| |
o 21 f-1: rename h -> i]
:
o 0 i-0 initial commit: a b] o 0 i-0 initial commit: a b]
The following output is correct. The following output is correct.
$ hg log -Gfr 'desc("mFBm-0")' d $ hg log -Gfr 'desc("mFBm-0")' d
o 22 f-2: rename i -> d] o 22 f-2: rename i -> d]
| |