Patchwork [2,of,7] copies: update _checkcopies to work in a rotated DAG

login
register
mail settings
Submitter Gábor Stefanik
Date Oct. 7, 2016, 12:31 p.m.
Message ID <08f3260e3764bcfbe139.1475843495@waste.org>
Download mbox | patch
Permalink /patch/16877/
State Changes Requested
Delegated to: Matt Mackall
Headers show

Comments

Gábor Stefanik - Oct. 7, 2016, 12:31 p.m.
# HG changeset patch
# User Gábor Stefanik <gabor.stefanik@nng.com>
# Date 1475588213 -7200
#      Tue Oct 04 15:36:53 2016 +0200
# Node ID 08f3260e3764bcfbe13945aa62bf079807df02fc
# Parent  5b7cd65a9be6c82b3ad14096b41db44c198a6769
copies: update _checkcopies to work in a rotated DAG

This introduces a distinction between "merge common ancestor" and
"topological common ancestor". During a regular merge, these two are
identical. Graft, however, performs a merge in a rotated DAG, where the
merge common ancestor will not be a common ancestor at all in the
original DAG.

To correctly find copies in case of a graft, we need to take both the
merge CA and the topological CA into account, and track any renames
between them in reverse.

Special care must be taken about the case where the merge CA lies not
between the topological CA and the source, but outside it (typically
between the topological CA and the destination).
This is handled using the remoteca parameter; it would be possible to
have _checkcopies itself detect this, but it's quite expensive and also
guaranteed not to change between multiple _checkcopies invocations in the
same mergecopies call, so it's better to check it there, and pass to
_checkcopies as a parameter.
timeless - Oct. 7, 2016, 3:41 p.m.
Gábor Stefanik wrote:
> copies: update _checkcopies to work in a rotated DAG

I'd argue this should be tagged (BC).
I absolutely support this change, it should be changed, it is a bug.

But the reason I'd argue that it's a change is that a repository
constructed along this path will result in a different changeset
depending on the version of hg:

> @@ -620,7 +620,7 @@
>    date:        Thu Jan 01 00:00:00 1970 +0000
>    summary:     2
>
> -  changeset:   14:f64defefacee
> +  changeset:   14:0c921c65ef1e
>    parent:      1:5d205f8b35b6
>    user:        foo
>    date:        Thu Jan 01 00:00:00 1970 +0000
Pierre-Yves David - Oct. 12, 2016, 11:13 p.m.
On 10/07/2016 05:41 PM, timeless wrote:
> Gábor Stefanik wrote:
>> copies: update _checkcopies to work in a rotated DAG
>
> I'd argue this should be tagged (BC).
> I absolutely support this change, it should be changed, it is a bug.
>
> But the reason I'd argue that it's a change is that a repository
> constructed along this path will result in a different changeset
> depending on the version of hg:

Looking more into what is happening here, I'm not sure it is worth a BC. 
This seems to be a plain bugfix for something that was just plain 
misbehaving in a conflicting way. I'm pretty sure nobody rely on this.

Cheers,

Patch

diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -345,10 +345,12 @@ 
     bothnew = sorted(addedinm1 & addedinm2)
 
     for f in u1u:
-        _checkcopies(c1, f, m1, m2, ca, limit, diverge, copy1, fullcopy1)
+        _checkcopies(c1, f, m1, m2, ca, ca, False, limit, diverge, copy1,
+                     fullcopy1)
 
     for f in u2u:
-        _checkcopies(c2, f, m2, m1, ca, limit, diverge, copy2, fullcopy2)
+        _checkcopies(c2, f, m2, m1, ca, ca, False, limit, diverge, copy2,
+                     fullcopy2)
 
     copy = dict(copy1.items() + copy2.items())
     movewithdir = dict(movewithdir1.items() + movewithdir2.items())
@@ -373,8 +375,10 @@ 
                       % "\n   ".join(bothnew))
     bothdiverge, _copy, _fullcopy = {}, {}, {}
     for f in bothnew:
-        _checkcopies(c1, f, m1, m2, ca, limit, bothdiverge, _copy, _fullcopy)
-        _checkcopies(c2, f, m2, m1, ca, limit, bothdiverge, _copy, _fullcopy)
+        _checkcopies(c1, f, m1, m2, ca, ca, False, limit, bothdiverge, _copy,
+                     _fullcopy)
+        _checkcopies(c2, f, m2, m1, ca, ca, False, limit, bothdiverge, _copy,
+                     _fullcopy)
     for of, fl in bothdiverge.items():
         if len(fl) == 2 and fl[0] == fl[1]:
             copy[fl[0]] = of # not actually divergent, just matching renames
@@ -454,7 +458,8 @@ 
 
     return copy, movewithdir, diverge, renamedelete
 
-def _checkcopies(ctx, f, m1, m2, ca, limit, diverge, copy, fullcopy):
+def _checkcopies(ctx, f, m1, m2, ca, tca, remoteca, limit, diverge, copy,
+                 fullcopy):
     """
     check possible copies of f from m1 to m2
 
@@ -462,7 +467,9 @@ 
     f = the filename to check
     m1 = the source manifest
     m2 = the destination manifest
-    ca = the changectx of the common ancestor
+    ca = the changectx of the common ancestor, overridden on graft
+    tca = topological common ancestor for graft-like scenarios
+    remoteca = True if ca is outside tca::ctx, False otherwise
     limit = the rev number to not search beyond
     diverge = record all diverges in this dict
     copy = record all non-divergent copies in this dict
@@ -475,6 +482,8 @@ 
     """
 
     ma = ca.manifest()
+    mta = tca.manifest()
+    backwards = ca != tca and not remoteca and f in ma
     getfctx = _makegetfctx(ctx)
 
     def _related(f1, f2, limit):
@@ -520,15 +529,26 @@ 
             continue
         seen.add(of)
 
-        fullcopy[f] = of # remember for dir rename detection
+        # remember for dir rename detection
+        if backwards:
+            fullcopy[of] = f # grafting backwards through renames
+        else:
+            fullcopy[f] = of
         if of not in m2:
             continue # no match, keep looking
         if m2[of] == ma.get(of):
             return # no merge needed, quit early
         c2 = getfctx(of, m2[of])
-        cr = _related(oc, c2, ca.rev())
+        cr = _related(oc, c2, tca.rev())
         if cr and (of == f or of == c2.path()): # non-divergent
-            copy[f] = of
+            if backwards:
+                copy[of] = f
+            elif of in ma:
+                copy[f] = of
+            elif remoteca: # special case: a <- b <- a -> b "ping-pong" rename
+                copy[of] = f
+                del fullcopy[f]
+                fullcopy[of] = f
             return
 
     if of in ma:
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -427,8 +427,8 @@ 
   $ hg graft 3 --log -u foo
   grafting 3:4c60f11aa304 "3"
   warning: can't find ancestor for 'c' copied from 'b'!
-  $ hg log --template '{rev} {parents} {desc}\n' -r tip
-  14 1:5d205f8b35b6  3
+  $ hg log --template '{rev}:{node|short} {parents} {desc}\n' -r tip
+  14:0c921c65ef1e 1:5d205f8b35b6  3
   (grafted from 4c60f11aa304a54ae1c199feb94e7fc771e51ed8)
 
 Resolve conflicted graft
@@ -620,7 +620,7 @@ 
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     2
   
-  changeset:   14:f64defefacee
+  changeset:   14:0c921c65ef1e
   parent:      1:5d205f8b35b6
   user:        foo
   date:        Thu Jan 01 00:00:00 1970 +0000