This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9172

tests: update test-copies-chain-merge.t to not use empty files
ClosedPublic

Authored by martinvonz on Oct 8 2020, 12:17 PM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHGdeeb215be337: tests: update test-copies-chain-merge.t to not use empty files
Summary

Merging empty files is not very interesting or realistic.

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.Oct 8 2020, 12:17 PM
martinvonz updated this revision to Diff 23122.Oct 9 2020, 2:28 AM
pulkit accepted this revision.Oct 9 2020, 5:03 AM
This revision is now accepted and ready to land.Oct 9 2020, 5:03 AM
marmoute added a subscriber: marmoute.Oct 13 2020, 6:50 AM

cc @pulkit

tests/test-copies-chain-merge.t
780–788

(rewritting this since phab apparently lost the initial writting)

This is unexpected and unexplained output changes. So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.

This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

martinvonz added inline comments.Oct 13 2020, 12:01 PM
tests/test-copies-chain-merge.t
780–788

This is unexpected and unexplained output changes.

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.
This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

The only thing that changes is the filelog case. That's not too surprising given that the filelog changes with this patch, and it's not something you've changed recently, so I'm not worried that you've broken it. Also, even if it's testing a slightly different case, the filelog-centric copy tracing has been there forever and has its own testing, so I'm also not concerned that we've lost a very important test case for it.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

D9195 was sent after this patch, so I have little sympathy for that reason.

marmoute added inline comments.Oct 13 2020, 3:12 PM
tests/test-copies-chain-merge.t
780–788

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

My general expectation is for change affecting tests to explain why, which was missing here. You explanation of "this is an arbitrary pick seems fine.

However the main problem remains, why are we doing this change at all?

  • Either we expect that having content in the file will have absolutely no effect or anything. Then making this change is purely gratuitous.
  • Or we do expect this to matters, in which case we should introduce new test cases instead of removing the olds one in favor of the new ones.

So, as I see it, we currently have either a problematic change in the current test coverage or a gratuitous changes that get in a the way of more important, actual, semantic fixes. These actual semantic fixes that have been made as part of an active, long running series of work on this topic. The creation of this patch not being coordinated with the author of that active series of work. does not help.

Revision Contents
Changeset List

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

Diff 23180

tests/test-copies-chain-merge.t

#endif #endif
$ hg init repo-chain $ hg init repo-chain
$ cd repo-chain $ cd repo-chain
Add some linear rename initialy Add some linear rename initialy
$ touch a b h $ echo a > a
$ echo b > b
$ echo h > h
$ hg ci -Am 'i-0 initial commit: a b h' $ hg ci -Am 'i-0 initial commit: a b h'
adding a adding a
adding b adding b
adding h adding h
$ hg mv a c $ hg mv a c
$ hg ci -Am 'i-1: a -move-> c' $ hg ci -Am 'i-1: a -move-> c'
$ hg mv c d $ hg mv c d
$ hg ci -Am 'i-2: c -move-> d' $ hg ci -Am 'i-2: c -move-> d'
$ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d' $ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644 d'
b004912a8510032a0350a74daa2803dadfb00e12 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
$ 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 169be882533bc917905d46c0c951aa9a1e288dcf 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 169be882533b 000000000000 000000000000
1 8 b004912a8510 000000000000 000000000000 1 8 b004912a8510 000000000000 000000000000
Log output should not include a merge commit as it did not happen Log output should not include a merge commit as it did not happen
$ hg log -Gfr 'desc("mBDm-0")' d $ hg log -Gfr 'desc("mBDm-0")' d
o 8 d-2 re-add d o 8 d-2 re-add d
| |
~ ~
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)
$ hg up 'desc("a-2")' $ hg up 'desc("a-2")'
2 files updated, 0 files merged, 1 files removed, 0 files unresolved 2 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg merge 'desc("e-2")' $ hg merge 'desc("e-2")' --tool :union
merging f
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 'mAEm-0 simple merge - one way' $ hg ci -m 'mAEm-0 simple merge - one way'
$ hg up 'desc("e-2")' $ hg up 'desc("e-2")'
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("a-2")' $ hg merge 'desc("a-2")' --tool :union
merging f
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 'mEAm-0 simple merge - the other way' $ hg ci -m 'mEAm-0 simple merge - the other way'
created new head created new head
$ hg log -G --rev '::(desc("mAEm")+desc("mEAm"))' $ hg log -G --rev '::(desc("mAEm")+desc("mEAm"))'
@ 20 mEAm-0 simple merge - the other way @ 20 mEAm-0 simple merge - the other way
|\ |\
+---o 19 mAEm-0 simple merge - one way +---o 19 mAEm-0 simple merge - one way
| |/ | |/
| o 10 e-2 g -move-> f | o 10 e-2 g -move-> f
| | | |
| o 9 e-1 b -move-> g | o 9 e-1 b -move-> g
| | | |
o | 4 a-2: e -move-> f o | 4 a-2: e -move-> f
| | | |
o | 3 a-1: d -move-> e o | 3 a-1: d -move-> e
|/ |/
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 h o 0 i-0 initial commit: a b h
$ hg manifest --debug --rev 'desc("mAEm-0")' | grep '644 f' $ hg manifest --debug --rev 'desc("mAEm-0")' | grep '644 f'
eb806e34ef6be4c264effd5933d31004ad15a793 644 f c39c6083dad048d5138618a46f123e2f397f4f18 644 f
$ hg manifest --debug --rev 'desc("mEAm-0")' | grep '644 f' $ hg manifest --debug --rev 'desc("mEAm-0")' | grep '644 f'
eb806e34ef6be4c264effd5933d31004ad15a793 644 f a9a8bc3860c9d8fa5f2f7e6ea8d40498322737fd 644 f
$ hg manifest --debug --rev 'desc("a-2")' | grep '644 f' $ hg manifest --debug --rev 'desc("a-2")' | grep '644 f'
0dd616bc7ab1a111921d95d76f69cda5c2ac539c 644 f 263ea25e220aaeb7b9bac551c702037849aa75e8 644 f
$ hg manifest --debug --rev 'desc("e-2")' | grep '644 f' $ hg manifest --debug --rev 'desc("e-2")' | grep '644 f'
6da5a2eecb9c833f830b67a4972366d49a9a142c 644 f 71b9b7e73d973572ade6dd765477fcee6890e8b1 644 f
$ hg debugindex f $ hg debugindex f
rev linkrev nodeid p1 p2 rev linkrev nodeid p1 p2
0 4 0dd616bc7ab1 000000000000 000000000000 0 4 263ea25e220a 000000000000 000000000000
1 10 6da5a2eecb9c 000000000000 000000000000 1 10 71b9b7e73d97 000000000000 000000000000
2 19 eb806e34ef6b 0dd616bc7ab1 6da5a2eecb9c 2 19 c39c6083dad0 263ea25e220a 71b9b7e73d97
3 20 a9a8bc3860c9 71b9b7e73d97 263ea25e220a
# Here the filelog based implementation is not looking at the rename # Here the filelog based implementation is not looking at the rename
# information (because the file exist on both side). However the changelog # information (because the file exist on both side). However the changelog
# based on works fine. We have different output. # based on works fine. We have different output.
$ hg status --copies --rev 'desc("a-2")' --rev 'desc("mAEm-0")' $ hg status --copies --rev 'desc("a-2")' --rev 'desc("mAEm-0")'
M f M f
b (no-filelog !) b (no-filelog !)
2 files updated, 0 files merged, 1 files removed, 0 files unresolved 2 files updated, 0 files merged, 1 files removed, 0 files unresolved
$ hg mv h i $ hg mv h i
$ 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 169be882533b 000000000000 000000000000
1 8 b004912a8510 000000000000 000000000000 1 8 b004912a8510 000000000000 000000000000
2 22 c72365ee036f 000000000000 000000000000 2 22 4a067cf8965d 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")'
1 files updated, 0 files merged, 0 files removed, 0 files unresolved 1 files updated, 0 files merged, 0 files removed, 0 files unresolved
| "a -> c -> d". and one the other one we have "h -> i -> d". | "a -> c -> d". and one the other one we have "h -> i -> d".
| |
| The current code arbitrarily pick one side | The current code arbitrarily pick one side
$ hg up 'desc("f-2")' $ hg up '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
$ hg merge 'desc("g-1")' --tool :union $ hg merge 'desc("g-1")' --tool :union
merging d merging d
0 files updated, 1 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 'mFGm-0 simple merge - one way' $ hg ci -m 'mFGm-0 simple merge - one way'
created new head created new head
$ hg up 'desc("g-1")' $ hg up 'desc("g-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("f-2")' --tool :union $ hg merge 'desc("f-2")' --tool :union
merging d merging d
0 files updated, 1 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 'mGFm-0 simple merge - the other way' $ hg ci -m 'mGFm-0 simple merge - the other way'
created new head created new head
$ hg log -G --rev '::(desc("mGFm")+desc("mFGm"))' $ hg log -G --rev '::(desc("mGFm")+desc("mFGm"))'
@ 29 mGFm-0 simple merge - the other way @ 29 mGFm-0 simple merge - the other way
|\ |\
+---o 28 mFGm-0 simple merge - one way +---o 28 mFGm-0 simple merge - one way
| |/ | |/
| o 25 g-1: update d | o 25 g-1: update d
| | | |
o | 22 f-2: rename i -> d o | 22 f-2: rename i -> d
| | | |
o | 21 f-1: rename h -> i o | 21 f-1: rename h -> i
|/ |/
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 h o 0 i-0 initial commit: a b h
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mFGm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mFGm-0")'
A d A d
h (no-filelog !) h
a (filelog !)
R a R a
R h R h
$ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGFm-0")' $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mGFm-0")'
A d A d
a a (no-filelog !)
h (filelog !)
marmouteUnsubmitted
Not Done

(rewritting this since phab apparently lost the initial writting)

This is unexpected and unexplained output changes. So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.

This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

marmoute: (rewritting this since phab apparently lost the initial writting) This is unexpected and…
martinvonzAuthorUnsubmitted
Done

This is unexpected and unexplained output changes.

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

So I strongly suspect that this patch is silently changing the test coverage. I would prefer that we add explicit test case for file being actually merged instead of changing the existing coverage.
This unexpected test change is, in my opinion, a strong enough reason to drop this patch and resume review on a new patch.

The only thing that changes is the filelog case. That's not too surprising given that the filelog changes with this patch, and it's not something you've changed recently, so I'm not worried that you've broken it. Also, even if it's testing a slightly different case, the filelog-centric copy tracing has been there forever and has its own testing, so I'm also not concerned that we've lost a very important test case for it.

In addition, this patch conflicts with a larger series I submitted that fixes two important bugs: (see D9195 and descendants). So I would much prefer to have this patch dropped for now and a more robust successors build above my bugfix series.

D9195 was sent after this patch, so I have little sympathy for that reason.

martinvonz: > This is unexpected and unexplained output changes. Is it *more* unexplained now than before?
marmouteUnsubmitted
Not Done

Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.

My general expectation is for change affecting tests to explain why, which was missing here. You explanation of "this is an arbitrary pick seems fine.

However the main problem remains, why are we doing this change at all?

  • Either we expect that having content in the file will have absolutely no effect or anything. Then making this change is purely gratuitous.
  • Or we do expect this to matters, in which case we should introduce new test cases instead of removing the olds one in favor of the new ones.

So, as I see it, we currently have either a problematic change in the current test coverage or a gratuitous changes that get in a the way of more important, actual, semantic fixes. These actual semantic fixes that have been made as part of an active, long running series of work on this topic. The creation of this patch not being coordinated with the author of that active series of work. does not help.

marmoute: > Is it *more* unexplained now than before? The comment on line 745 says "The current code…
R a R a
R h R h
$ hg status --copies --rev 'desc("f-2")' --rev 'desc("mFGm-0")' $ hg status --copies --rev 'desc("f-2")' --rev 'desc("mFGm-0")'
M d M d
$ hg status --copies --rev 'desc("f-2")' --rev 'desc("mGFm-0")' $ hg status --copies --rev 'desc("f-2")' --rev 'desc("mGFm-0")'
M d M d
$ hg status --copies --rev 'desc("f-1")' --rev 'desc("mFGm-0")' $ hg status --copies --rev 'desc("f-1")' --rev 'desc("mFGm-0")'
M d M d