Page MenuHomePhabricator

[RFC] merge: stop caring about whether files are related (issue6163)
Needs ReviewPublic

Authored by martinvonz on Tue, Oct 6, 8:05 PM.

Details

Reviewers
marmoute
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.Tue, Oct 6, 8:05 PM
marmoute requested changes to this revision.Thu, Oct 8, 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.Thu, Oct 8, 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.Thu, Oct 8, 12:18 PM
martinvonz planned changes to this revision.Thu, Oct 8, 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.Thu, Oct 8, 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.Fri, Oct 9, 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.Fri, Oct 9, 3:37 AM