This is an archive of the discontinued Mercurial Phabricator instance.

Differential D5963

copies: handle a case when both merging csets are not descendant of merge base
ClosedPublic

Authored by khanchi97 on Feb 14 2019, 9:15 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG7694b685bb10: copies: handle a case when both merging csets are not descendant of merge base
Summary

This patch fix the behaviour of fullcopytracing algorithm in the case
when both the merging csets are not the descendant of merge base.
Although it seems to be the rare case when both the csets are not
descendant of merge base. But it can be seen in most of cases of
content-divergence in evolve extension, where merge base is the common
predecessor.
Previous patch added a test where this algorithm can fail to continue
because of an assumption that only one of the two csets can be dirty.
This patch fix that error.

For refrence I suggest you to look into the previous discussion held
on a patch sent by Pulkit: https://phab.mercurial-scm.org/D3896

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

khanchi97 created this revision.Feb 14 2019, 9:15 AM
khanchi97 added inline comments.Feb 14 2019, 9:29 AM
mercurial/copies.py
655

I am not sure if combining of partial copies in this case is correct or not. But I did combine these to do the similar behaviour as we are doing at other places. I don't have much knowledge about partial copies :/

khanchi97 edited the summary of this revision. (Show Details)Feb 20 2019, 12:43 PM
khanchi97 retitled this revision from copies: handle the case when both merging csets are not descendant of merge base to copies: handle a case when both merging csets are not descendant of merge base.
khanchi97 added a comment.Feb 24 2019, 11:44 PM

Ping for review.

khanchi97 updated this revision to Diff 14241.Feb 25 2019, 2:33 PM
khanchi97 updated this revision to Diff 14310.Mar 3 2019, 1:50 AM
khanchi97 updated this revision to Diff 14348.Mar 4 2019, 2:52 PM
khanchi97 added a comment.Mar 9 2019, 12:00 AM

Gentle ping for review.

martinvonz added a subscriber: martinvonz.Mar 10 2019, 1:10 PM
In D5963#88980, @khanchi97 wrote:

Gentle ping for review.

FYI, I finally have some time to review this. I recently wrote a changeset-centric version of pathcopies(). I was going to do the same with mergecopies(), but I had not gotten around to it yet, so I'm not very familiar with that code yet. I spent around two hours trying to understand the code yesterday. I'll spend some more tonight. Hopefully I'll be able to say if I agree with the patch after that :)

khanchi97 updated this revision to Diff 14457.Mar 11 2019, 5:49 AM
martinvonz added inline comments.Mar 11 2019, 11:34 AM
mercurial/copies.py
655

I am also still not sure, even after spending several hours on this code :( But I am also not sure it's incorrect, and I don't want to hold up this patch more than I already have, so I'll queue this. Thanks for your patience. As I said before, I'm going to write a changeset-centric version of mergecopies() pretty soon, and then I'll have to understand this better. I'll also add more tests then. (I'm not trying to say that I think your patch is broken; it's just that the entire mergecopies() is pretty complicated, so I wouldn't be surprised if there are bugs anywhere in it.)

martinvonz accepted this revision.Mar 11 2019, 11:34 AM
This revision is now accepted and ready to land.Mar 11 2019, 11:34 AM
Closed by commit rHG7694b685bb10: copies: handle a case when both merging csets are not descendant of merge base (authored by khanchi97). · Explain WhyMar 11 2019, 11:44 AM
This revision was automatically updated to reflect the committed changes.
khanchi97 added inline comments.Mar 11 2019, 12:31 PM
mercurial/copies.py
655

Yeah, I think I understand what you said. Thanks for accepting the patch :-)

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (10 lines)
Mtests/test-copies.t (10 lines)

Diff 14460

mercurial/copies.py

copy = dict(data1['copy']) copy = dict(data1['copy'])
copy.update(data2['copy']) copy.update(data2['copy'])
fullcopy = dict(data1['fullcopy']) fullcopy = dict(data1['fullcopy'])
fullcopy.update(data2['fullcopy']) fullcopy.update(data2['fullcopy'])
if dirtyc1: if dirtyc1:
_combinecopies(data2['incomplete'], data1['incomplete'], copy, diverge, _combinecopies(data2['incomplete'], data1['incomplete'], copy, diverge,
incompletediverge) incompletediverge)
else: if dirtyc2:
_combinecopies(data1['incomplete'], data2['incomplete'], copy, diverge, _combinecopies(data1['incomplete'], data2['incomplete'], copy, diverge,
incompletediverge) incompletediverge)
renamedelete = {} renamedelete = {}
renamedeleteset = set() renamedeleteset = set()
divergeset = set() divergeset = set()
for of, fl in list(diverge.items()): for of, fl in list(diverge.items()):
if len(fl) == 1 or of in c1 or of in c2: if len(fl) == 1 or of in c1 or of in c2:
'fullcopy': {}, 'fullcopy': {},
'incomplete': {}, 'incomplete': {},
'diverge': bothdiverge, 'diverge': bothdiverge,
'incompletediverge': bothincompletediverge 'incompletediverge': bothincompletediverge
} }
for f in bothnew: for f in bothnew:
_checkcopies(c1, c2, f, base, tca, dirtyc1, limit, both1) _checkcopies(c1, c2, f, base, tca, dirtyc1, limit, both1)
_checkcopies(c2, c1, f, base, tca, dirtyc2, limit, both2) _checkcopies(c2, c1, f, base, tca, dirtyc2, limit, both2)
if dirtyc1: if dirtyc1 and dirtyc2:
remainder = _combinecopies(both2['incomplete'], both1['incomplete'],
copy, bothdiverge, bothincompletediverge)
remainder1 = _combinecopies(both1['incomplete'], both2['incomplete'],
copy, bothdiverge, bothincompletediverge)
remainder.update(remainder1)
khanchi97AuthorUnsubmitted
Not Done

I am not sure if combining of partial copies in this case is correct or not. But I did combine these to do the similar behaviour as we are doing at other places. I don't have much knowledge about partial copies :/

khanchi97: I am not sure if combining of partial copies in this case is correct or not. But I did combine…
martinvonzUnsubmitted
Not Done

I am also still not sure, even after spending several hours on this code :( But I am also not sure it's incorrect, and I don't want to hold up this patch more than I already have, so I'll queue this. Thanks for your patience. As I said before, I'm going to write a changeset-centric version of mergecopies() pretty soon, and then I'll have to understand this better. I'll also add more tests then. (I'm not trying to say that I think your patch is broken; it's just that the entire mergecopies() is pretty complicated, so I wouldn't be surprised if there are bugs anywhere in it.)

martinvonz: I am also still not sure, even after spending several hours on this code :( But I am also not…
khanchi97AuthorUnsubmitted
Not Done

Yeah, I think I understand what you said. Thanks for accepting the patch :-)

khanchi97: Yeah, I think I understand what you said. Thanks for accepting the patch :-)
elif dirtyc1:
# incomplete copies may only be found on the "dirty" side for bothnew # incomplete copies may only be found on the "dirty" side for bothnew
assert not both2['incomplete'] assert not both2['incomplete']
remainder = _combinecopies({}, both1['incomplete'], copy, bothdiverge, remainder = _combinecopies({}, both1['incomplete'], copy, bothdiverge,
bothincompletediverge) bothincompletediverge)
elif dirtyc2: elif dirtyc2:
assert not both1['incomplete'] assert not both1['incomplete']
remainder = _combinecopies({}, both2['incomplete'], copy, bothdiverge, remainder = _combinecopies({}, both2['incomplete'], copy, bothdiverge,
bothincompletediverge) bothincompletediverge)

tests/test-copies.t

| b1 | b1
~ diff -r 5a4825cc2926 -r 94a2f1a0e8e2 b1 ~ diff -r 5a4825cc2926 -r 94a2f1a0e8e2 b1
--- a/b1 Thu Jan 01 00:00:00 1970 +0000 --- a/b1 Thu Jan 01 00:00:00 1970 +0000
+++ b/b1 Thu Jan 01 00:00:00 1970 +0000 +++ b/b1 Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,2 @@ @@ -1,1 +1,2 @@
b b
+baba +baba
Test which demonstrate that fullcopytracing algorithm can fail to handle a case when both the csets are dirty Test to make sure that fullcopytracing algorithm don't fail when both the merging csets are dirty
---------------------------------------------------------------------------------------------------------- (a dirty cset is one who is not the descendant of merge base)
-------------------------------------------------------------------------------------------------
$ newrepo $ newrepo
$ echo a > a $ echo a > a
$ hg add a $ hg add a
$ hg ci -m "added a" $ hg ci -m "added a"
$ echo b > b $ echo b > b
$ hg add b $ hg add b
$ hg ci -m "added b" $ hg ci -m "added b"
o 1 added b o 1 added b
| b | b
o 0 added a o 0 added a
a a
Now if we trigger a merge between cset revision 3 and 6 using base revision 4, in this case Now if we trigger a merge between cset revision 3 and 6 using base revision 4, in this case
both the merging csets will be dirty as no one is descendent of base revision: both the merging csets will be dirty as no one is descendent of base revision:
$ hg graft -r 6 --base 4 --hidden 2>&1 | grep "AssertionError" $ hg graft -r 6 --base 4 --hidden -t :other
AssertionError grafting 6:99802e4f1e46 "added willconflict and d" (tip)