Merging empty files is not very interesting or realistic.
- Group Reviewers
- rHGdeeb215be337: tests: update test-copies-chain-merge.t to not use empty files
(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.
Is it *more* unexplained now than before? The comment on line 745 says "The current code arbitrarily pick one side". That still seems valid.
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.
D9195 was sent after this patch, so I have little sympathy for that reason.
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?
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.