This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8237

copies-tests: update the analysis of the BD/DB cases
ClosedPublic

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

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGb8882f5d7cb4: copies-tests: update the analysis of the BD/DB cases
Summary

After thinking more about it, the current change in output between the two
merges is not due to an ambiguity, it is introduce by a bug in merge/commit
that record a wrong file history.

Fixing the bug should fix the output. In the meantime, we have a wrong behavior.

In practice the bug is not in the merge code (who does the right things), but in
the commit code (who get confused when recording the new file revision).

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

marmoute created this revision.Mar 5 2020, 1:14 PM
martinvonz requested changes to this revision.Mar 6 2020, 3:05 AM

I'm sending a bunch of comments on D8078. I'm hoping some of them can incorporated in this series.

This revision now requires changes to proceed.Mar 6 2020, 3:05 AM
martinvonz added inline comments.Mar 6 2020, 11:07 AM
tests/test-copies-chain-merge.t
297–298

"*one* of"

Also, shouldn't this exactly duplicate the comment on lines 456-457?

521

why change this?

marmoute added inline comments.Mar 6 2020, 11:20 AM
tests/test-copies-chain-merge.t
521

I wanted to highlight the expected change when the fix arrive. We can drop it if this confuse you.

martinvonz added inline comments.Mar 6 2020, 11:28 AM
tests/test-copies-chain-merge.t
521

Yes, I find it confusing (especially with the "This output is correct" comment above), so please remove.

marmoute updated this revision to Diff 20588.Mar 6 2020, 6:51 PM

series update after Martin feedback

marmoute added a commit: rHGb8882f5d7cb4: copies-tests: update the analysis of the BD/DB cases.Mar 6 2020, 7:30 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGb8882f5d7cb4: copies-tests: update the analysis of the BD/DB cases (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mtests/test-copies-chain-merge.t (26 lines)

Diff 20599

tests/test-copies-chain-merge.t

| |
o 1 i-1: a -move-> c] o 1 i-1: a -move-> c]
| |
o 0 i-0 initial commit: a b] o 0 i-0 initial commit: a b]
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 file
Note: Note:
| In this case, the merge get conflicting information since on one side we have | In this case, the merge get conflicting information since on one side we have
| a "brand new" d. and one the other one we have "d renamed from c (itself | a "brand new" d. and one the other one we have "d renamed from c (itself
| renamed from c)". | renamed from c)".
| |
| The current code arbitrarily pick one side | The current code arbitrarily pick one side
o 2 i-2: c -move-> d] o 2 i-2: c -move-> d]
| |
o 1 i-1: a -move-> c] o 1 i-1: a -move-> c]
| |
o 0 i-0 initial commit: a b] o 0 i-0 initial commit: a b]
Note: Note:
| In this case, the merge get conflicting information since each side have a | In this case, one of the merge wrongly record a merge while there is none.
| different way to reach 'f'. | This lead to bad copy tracing information to be dug up.
martinvonzUnsubmitted
Not Done

"*one* of"

Also, shouldn't this exactly duplicate the comment on lines 456-457?

martinvonz: "*one* of" Also, shouldn't this exactly duplicate the comment on lines 456-457?
final summary final summary
$ hg update null --quiet $ hg update null --quiet
$ hg log -G $ hg log -G
o 20 mEAm-0 simple merge - the other way] o 20 mEAm-0 simple merge - the other way]
|\ |\
+---o 19 mAEm-0 simple merge - one way] +---o 19 mAEm-0 simple merge - one 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: Note:
| In this case, the merge get conflicting information since on one side we have | In this case, one of the merge wrongly record a merge while there is none.
| a "brand new" d. and one the other one we have "d renamed from c (itself | This lead to bad copy tracing information to be dug up.
| renamed from c)".
|
| The current code arbitrarily pick one side
$ 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 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 recorded copy is different depending of where we started the merge from since The bugs makes recorded copy is different depending of where we started the merge from since
$ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d' $ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d'
0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 644 d 0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 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
This second b004912a8510032a0350a74daa2803dadfb00e12 seems wrong. We should record the merge 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 15 0bb5445dc4d0 01c2f5eabdc4 b004912a8510
(This `hg log` 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 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 2 i-2: c -move-> d]
| |
o 1 i-1: a -move-> c] o 1 i-1: a -move-> c]
| |
o 0 i-0 initial commit: a b] o 0 i-0 initial commit: a b]
(That output seems wrong, if we had opportunity to record the merge, we should This `hg log` output is correct
probably have recorded the merge).
$ 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 a
martinvonzUnsubmitted
Not Done

why change this?

martinvonz: why change this?
marmouteAuthorUnsubmitted
Done

I wanted to highlight the expected change when the fix arrive. We can drop it if this confuse you.

marmoute: I wanted to highlight the expected change when the fix arrive. We can drop it if this confuse…
martinvonzUnsubmitted
Not Done

Yes, I find it confusing (especially with the "This output is correct" comment above), so please remove.

martinvonz: Yes, I find it confusing (especially with the "This output is correct" comment above), so…
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 re-adding the file afterward Comparing with a merge re-adding the file afterward
--------------------------------------------------- ---------------------------------------------------