This is an archive of the discontinued Mercurial Phabricator instance.

[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

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 …

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.

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

test

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

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.

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