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
Lint Skipped
Unit
Unit Tests Skipped

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
571

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
571

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
571

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-copytrace-heuristics.t (73 lines)

Diff 14092

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-copytrace-heuristics.t

o 0 : 9092f1db7931 added a o 0 : 9092f1db7931 added a
draft draft
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 c9241b0f2d5b --hidden $ hg graft -r 6 --base c9241b0f2d5b --hidden
grafting 6:99802e4f1e46 "added willconflict and d" (tip) grafting 6:99802e4f1e46 "added willconflict and d" (tip)
** unknown exception encountered, please report by visiting merging willconflict
** https://mercurial-scm.org/wiki/BugTracker warning: conflicts while merging willconflict! (edit, then use 'hg resolve --mark')
** Python 2.7.12 (default, Nov 12 2018, 14:36:49) [GCC 5.4.0 20160609] abort: unresolved conflicts, can't continue
** Mercurial Distributed SCM (version 4.8.1+539-b6c610bf567e+20181221) (use 'hg resolve' and 'hg graft --continue')
** Extensions loaded: rebase, shelve [255]
Traceback (most recent call last):
File "/home/khanchi/hg/hg-committed/hg", line 43, in <module> $ hg diff
dispatch.run() diff -r 9a8bb7cc6814 willconflict
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 99, in run --- a/willconflict Thu Jan 01 00:00:00 1970 +0000
status = dispatch(req) +++ b/willconflict Thu Jan 01 00:00:00 1970 +0000
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 225, in dispatch @@ -1,1 +1,5 @@
ret = _runcatch(req) or 0 +<<<<<<< local: 9a8bb7cc6814 - test: added c
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 376, in _runcatch foobar
return _callcatch(ui, _runcatchfunc) +=======
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 384, in _callcatch +barfoo
return scmutil.callcatch(ui, func) +>>>>>>> graft: 99802e4f1e46 - test: added willconflict and d
File "/home/khanchi/hg/hg-committed/mercurial/scmutil.py", line 165, in callcatch
return func() $ echo barfoo > willconflict
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 367, in _runcatchfunc $ hg resolve -m
return _dispatch(req) (no more unresolved files)
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 1021, in _dispatch continue: hg graft --continue
cmdpats, cmdoptions) $ hg graft --continue
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 756, in runcommand grafting 6:99802e4f1e46 "added willconflict and d" (tip)
ret = _runcommand(ui, options, cmd, d) $ cd ..
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 1030, in _runcommand $ rm -rf repo
return cmdfunc()
File "/home/khanchi/hg/hg-committed/mercurial/dispatch.py", line 1018, in <lambda>
d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
File "/home/khanchi/hg/hg-committed/mercurial/util.py", line 1676, in check
return func(*args, **kwargs)
File "/home/khanchi/hg/hg-committed/mercurial/commands.py", line 2378, in graft
return _dograft(ui, repo, *revs, **opts)
File "/home/khanchi/hg/hg-committed/mercurial/commands.py", line 2590, in _dograft
stats = mergemod.graft(repo, ctx, base, ['local', 'graft'])
File "/home/khanchi/hg/hg-committed/mercurial/merge.py", line 2234, in graft
mergeancestor=mergeancestor, labels=labels)
File "/home/khanchi/hg/hg-committed/mercurial/merge.py", line 2075, in update
followcopies, matcher=matcher, mergeforce=mergeforce)
File "/home/khanchi/hg/hg-committed/mercurial/merge.py", line 1342, in calculateupdates
acceptremote, followcopies)
File "/home/khanchi/hg/hg-committed/mercurial/merge.py", line 1146, in manifestmerge
ret = copies.mergecopies(repo, wctx, p2, pa)
File "/home/khanchi/hg/hg-committed/mercurial/copies.py", line 416, in mergecopies
return _fullcopytracing(repo, c1, c2, base)
File "/home/khanchi/hg/hg-committed/mercurial/copies.py", line 568, in _fullcopytracing
assert not both2['incomplete']
AssertionError
[1]