Patchwork D3896: copies: handle case when both merge cset are not descendant of merge base

login
register
mail settings
Submitter phabricator
Date July 8, 2018, 9:12 a.m.
Message ID <differential-rev-PHID-DREV-wrvrtdj4mdklvqinvwuy-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/32685/
State New
Headers show

Comments

phabricator - July 8, 2018, 9:12 a.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  There can be cases when both the changesets which we are merging are not
  descendants of the merge base. In those cases dirtyc1 and dirtyc2 both will be
  true. The existing code assumes that either of them will be true always but that
  is not a right assumption.
  
  I found this while working with content-divergence resolution. In
  content-divergence resolution, we use the common predecessor as the merge base
  for resolving content divergence and there it can be possible that the merge
  base is not the descendant of both the content-divergence changsets.
  
  The code in content-divergence which does this is at:
  https://www.mercurial-scm.org/repo/evolve/file/b81fd1487e04/hgext3rd/evolve/evolvecmd.py#l507

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3896

AFFECTED FILES
  mercurial/copies.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - July 8, 2018, 9:14 a.m.
pulkit added inline comments.

INLINE COMMENTS

> copies.py:496
>                         incompletediverge)
>      else:
>          _combinecopies(data1['incomplete'], data2['incomplete'], copy, diverge,

Since we can have both dirtyc1, and dirtyc2 true, I am not sure whether this else should be turned into it's own if statement?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3896

To: pulkit, #hg-reviewers
Cc: mercurial-devel
Yuya Nishihara - July 9, 2018, 12:20 p.m.
Can you add some tests that make both `dirtyc1` and `dirtyc2` set, and
trigger copy tracing?

> Since we can have both dirtyc1, and dirtyc2 true, I am not sure whether
> this else should be turned into it's own if statement?

No idea. I doubt if the current copy tracing can handle such cases. From
my vague memory, the current algorithm relies on the fact that the pseudo
`base` revision exists in between the `c1` and the `c2`.
phabricator - July 9, 2018, 12:23 p.m.
yuja added a comment.


  Can you add some tests that make both `dirtyc1` and `dirtyc2` set, and
  trigger copy tracing?
  
  > Since we can have both dirtyc1, and dirtyc2 true, I am not sure whether
  >  this else should be turned into it's own if statement?
  
  No idea. I doubt if the current copy tracing can handle such cases. From
  my vague memory, the current algorithm relies on the fact that the pseudo
  `base` revision exists in between the `c1` and the `c2`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3896

To: pulkit, #hg-reviewers
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -532,7 +532,10 @@ 
     for f in bothnew:
         _checkcopies(c1, c2, f, base, tca, dirtyc1, limit, both1)
         _checkcopies(c2, c1, f, base, tca, dirtyc2, limit, both2)
-    if dirtyc1:
+
+    if dirtyc1 and dirtyc2:
+        pass
+    elif dirtyc1:
         # incomplete copies may only be found on the "dirty" side for bothnew
         assert not both2['incomplete']
         remainder = _combinecopies({}, both1['incomplete'], copy, bothdiverge,