Page MenuHomePhabricator

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

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.

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

Gentle ping for review.

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
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 :-)