Patchwork [1,of,3] copies: separate moves via directory renames from explicit copies

login
register
mail settings
Submitter Siddharth Agarwal
Date Dec. 21, 2012, 9:08 p.m.
Message ID <078b88a00bff61b44f10.1356124101@sid0x220>
Download mbox | patch
Permalink /patch/262/
State Accepted
Commit 6c35b53cd28be0bfa8dd7cb2c54dec62ba73bafb
Headers show

Comments

Siddharth Agarwal - Dec. 21, 2012, 9:08 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0 at fb.com>
# Date 1356044912 28800
# Node ID 078b88a00bff61b44f10d71881a95324bf787925
# Parent  d00f0bfe007da572898cb37305a70ec31ee1ce8c
copies: separate moves via directory renames from explicit copies

Currently the "copy" dict contains both explicit copies/moves made by a
context and pending moves that need to happen because the other context moved
the directory the file was in. For explicit copies, the dict stores a
destination to source map, while for pending moves via directory renames, it
stores a source to destination map. The merge code uses this fact in a non-
obvious way to differentiate between these two cases.

We make this explicit by storing these pending moves in a separate dict. The
dict still has a source to destination map, but that is called out in the
docstring.
Siddharth Agarwal - Dec. 21, 2012, 11:04 p.m.
On 12/21/2012 01:08 PM, Siddharth Agarwal wrote:
> diff --git a/mercurial/copies.py b/mercurial/copies.py
> --- a/mercurial/copies.py
> +++ b/mercurial/copies.py
> @@ -170,11 +170,17 @@ def mergecopies(repo, c1, c2, ca):
>       Find moves and copies between context c1 and c2 that are relevant
>       for merging.
>   
> -    Returns two dicts, "copy" and "diverge".
> +    Returns four dicts: "copy", "moveviadirmove", "diverge", and
> +    "renamedelete".
>

BTW I know the name "moveviadirmove" is not a very good one, but I 
couldn't think of a better one. Suggestions welcome.
Pierre-Yves David - Dec. 22, 2012, 12:27 a.m.
On 21 d?c. 2012, at 22:08, Siddharth Agarwal wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0 at fb.com>
> # Date 1356044912 28800
> # Node ID 078b88a00bff61b44f10d71881a95324bf787925
> # Parent  d00f0bfe007da572898cb37305a70ec31ee1ce8c
> copies: separate moves via directory renames from explicit copies
> 
> Currently the "copy" dict contains both explicit copies/moves made by a
> context and pending moves that need to happen because the other context moved
> the directory the file was in. For explicit copies, the dict stores a
> destination to source map, while for pending moves via directory renames, it
> stores a source to destination map. The merge code uses this fact in a non-
> obvious way to differentiate between these two cases.
> 
> We make this explicit by storing these pending moves in a separate dict. The
> dict still has a source to destination map, but that is called out in the
> docstring.
> 
> diff --git a/mercurial/copies.py b/mercurial/copies.py
> --- a/mercurial/copies.py
> +++ b/mercurial/copies.py
> @@ -170,11 +170,17 @@ def mergecopies(repo, c1, c2, ca):
>     Find moves and copies between context c1 and c2 that are relevant
>     for merging.
> 
> -    Returns two dicts, "copy" and "diverge".
> +    Returns four dicts: "copy", "moveviadirmove", "diverge", and
> +    "renamedelete".

I would use movewithdit instead of moveviadirmove

> 
>     "copy" is a mapping from destination name -> source name,
>     where source is in c1 and destination is in c2 or vice-versa.
> 
> +    "moveviadirmove" is a mapping from source name -> destination name,
> +    where the file at source present in one context but not the other
> +    needs to be moved to destination by the merge process, because the
> +    other context moved the directory it is in.
> +
>     "diverge" is a mapping of source name -> list of destination names
>     for divergent renames.
> 
> @@ -183,16 +189,16 @@ def mergecopies(repo, c1, c2, ca):
>     """
>     # avoid silly behavior for update from empty dir
>     if not c1 or not c2 or c1 == c2:
> -        return {}, {}, {}
> +        return {}, {}, {}, {}
> 
>     # avoid silly behavior for parent -> working dir
>     if c2.node() is None and c1.node() == repo.dirstate.p1():
> -        return repo.dirstate.copies(), {}, {}
> +        return repo.dirstate.copies(), {}, {}, {}
> 
>     limit = _findlimit(repo, c1.rev(), c2.rev())
>     if limit is None:
>         # no common ancestor, no copies
> -        return {}, {}, {}
> +        return {}, {}, {}, {}
>     m1 = c1.manifest()
>     m2 = c2.manifest()
>     ma = ca.manifest()
> @@ -206,6 +212,7 @@ def mergecopies(repo, c1, c2, ca):
> 
>     ctx = util.lrucachefunc(makectx)
>     copy = {}
> +    moveviadirmove = {}
>     fullcopy = {}
>     diverge = {}
> 
> @@ -315,7 +322,7 @@ def mergecopies(repo, c1, c2, ca):
>     del diverge2
> 
>     if not fullcopy:
> -        return copy, diverge, renamedelete
> +        return copy, moveviadirmove, diverge, renamedelete
> 
>     repo.ui.debug("  checking for directory renames\n")
> 
> @@ -352,7 +359,7 @@ def mergecopies(repo, c1, c2, ca):
>     del d1, d2, invalid
> 
>     if not dirmove:
> -        return copy, diverge, renamedelete
> +        return copy, moveviadirmove, diverge, renamedelete
> 
>     for d in dirmove:
>         repo.ui.debug("  dir %s -> %s\n" % (d, dirmove[d]))
> @@ -365,8 +372,8 @@ def mergecopies(repo, c1, c2, ca):
>                     # new file added in a directory that was moved, move it
>                     df = dirmove[d] + f[len(d):]
>                     if df not in copy:
> -                        copy[f] = df
> -                        repo.ui.debug("  file %s -> %s\n" % (f, copy[f]))
> +                        moveviadirmove[f] = df
> +                        repo.ui.debug("  file %s -> %s\n" % (f, df))
>                     break
> 
> -    return copy, diverge, renamedelete
> +    return copy, moveviadirmove, diverge, renamedelete
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -213,14 +213,15 @@ def manifestmerge(repo, p1, p2, pa, over
>         repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m))
>         action.append((f, m) + args)
> 
> -    action, copy = [], {}
> +    action, copy, moveviadirmove = [], {}, {}
> 
>     if overwrite:
>         pa = p1
>     elif pa == p2: # backwards
>         pa = p1.p1()
>     elif pa and repo.ui.configbool("merge", "followcopies", True):
> -        copy, diverge, renamedelete = copies.mergecopies(repo, p1, p2, pa)
> +        copy, moveviadirmove, diverge, renamedelete = copies.mergecopies(
> +            repo, p1, p2, pa)

You should just cut that in two lines.

ret = copies.mergecopies(repo, p1, p2, pa)
copy, moveviadirmove, diverge, renamedelete = ret

The rest of the patch LGTM. I assume you ran a full test-suite on it.

Patch

diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -170,11 +170,17 @@  def mergecopies(repo, c1, c2, ca):
     Find moves and copies between context c1 and c2 that are relevant
     for merging.
 
-    Returns two dicts, "copy" and "diverge".
+    Returns four dicts: "copy", "moveviadirmove", "diverge", and
+    "renamedelete".
 
     "copy" is a mapping from destination name -> source name,
     where source is in c1 and destination is in c2 or vice-versa.
 
+    "moveviadirmove" is a mapping from source name -> destination name,
+    where the file at source present in one context but not the other
+    needs to be moved to destination by the merge process, because the
+    other context moved the directory it is in.
+
     "diverge" is a mapping of source name -> list of destination names
     for divergent renames.
 
@@ -183,16 +189,16 @@  def mergecopies(repo, c1, c2, ca):
     """
     # avoid silly behavior for update from empty dir
     if not c1 or not c2 or c1 == c2:
-        return {}, {}, {}
+        return {}, {}, {}, {}
 
     # avoid silly behavior for parent -> working dir
     if c2.node() is None and c1.node() == repo.dirstate.p1():
-        return repo.dirstate.copies(), {}, {}
+        return repo.dirstate.copies(), {}, {}, {}
 
     limit = _findlimit(repo, c1.rev(), c2.rev())
     if limit is None:
         # no common ancestor, no copies
-        return {}, {}, {}
+        return {}, {}, {}, {}
     m1 = c1.manifest()
     m2 = c2.manifest()
     ma = ca.manifest()
@@ -206,6 +212,7 @@  def mergecopies(repo, c1, c2, ca):
 
     ctx = util.lrucachefunc(makectx)
     copy = {}
+    moveviadirmove = {}
     fullcopy = {}
     diverge = {}
 
@@ -315,7 +322,7 @@  def mergecopies(repo, c1, c2, ca):
     del diverge2
 
     if not fullcopy:
-        return copy, diverge, renamedelete
+        return copy, moveviadirmove, diverge, renamedelete
 
     repo.ui.debug("  checking for directory renames\n")
 
@@ -352,7 +359,7 @@  def mergecopies(repo, c1, c2, ca):
     del d1, d2, invalid
 
     if not dirmove:
-        return copy, diverge, renamedelete
+        return copy, moveviadirmove, diverge, renamedelete
 
     for d in dirmove:
         repo.ui.debug("  dir %s -> %s\n" % (d, dirmove[d]))
@@ -365,8 +372,8 @@  def mergecopies(repo, c1, c2, ca):
                     # new file added in a directory that was moved, move it
                     df = dirmove[d] + f[len(d):]
                     if df not in copy:
-                        copy[f] = df
-                        repo.ui.debug("  file %s -> %s\n" % (f, copy[f]))
+                        moveviadirmove[f] = df
+                        repo.ui.debug("  file %s -> %s\n" % (f, df))
                     break
 
-    return copy, diverge, renamedelete
+    return copy, moveviadirmove, diverge, renamedelete
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -213,14 +213,15 @@  def manifestmerge(repo, p1, p2, pa, over
         repo.ui.debug(" %s: %s -> %s\n" % (f, msg, m))
         action.append((f, m) + args)
 
-    action, copy = [], {}
+    action, copy, moveviadirmove = [], {}, {}
 
     if overwrite:
         pa = p1
     elif pa == p2: # backwards
         pa = p1.p1()
     elif pa and repo.ui.configbool("merge", "followcopies", True):
-        copy, diverge, renamedelete = copies.mergecopies(repo, p1, p2, pa)
+        copy, moveviadirmove, diverge, renamedelete = copies.mergecopies(
+            repo, p1, p2, pa)
         for of, fl in diverge.iteritems():
             act("divergent renames", "dr", of, fl)
         for of, fl in renamedelete.iteritems():
@@ -233,6 +234,7 @@  def manifestmerge(repo, p1, p2, pa, over
 
     m1, m2, ma = p1.manifest(), p2.manifest(), pa.manifest()
     copied = set(copy.values())
+    copied.update(moveviadirmove.values())
 
     if '.hgsubstate' in m1:
         # check whether sub state is modified
@@ -259,14 +261,14 @@  def manifestmerge(repo, p1, p2, pa, over
                 act("versions differ", "m", f, f, f, rflags, False)
         elif f in copied: # files we'll deal with on m2 side
             pass
-        elif f in copy:
+        elif f in moveviadirmove: # directory rename
+            f2 = moveviadirmove[f]
+            act("remote renamed directory to " + f2, "d", f, None, f2,
+                m1.flags(f))
+        elif f in copy: # case 2 A,B/B/B or case 4,21 A/B/B
             f2 = copy[f]
-            if f2 not in m2: # directory rename
-                act("remote renamed directory to " + f2, "d",
-                    f, None, f2, m1.flags(f))
-            else: # case 2 A,B/B/B or case 4,21 A/B/B
-                act("local copied/moved to " + f2, "m",
-                    f, f2, f, fmerge(f, f2, f2), False)
+            act("local copied/moved to " + f2, "m", f, f2, f,
+                fmerge(f, f2, f2), False)
         elif f in ma: # clean, a different, no remote
             if n != ma[f]:
                 if repo.ui.promptchoice(
@@ -286,12 +288,13 @@  def manifestmerge(repo, p1, p2, pa, over
             continue
         if f in m1 or f in copied: # files already visited
             continue
-        if f in copy:
+        if f in moveviadirmove:
+            f2 = moveviadirmove[f]
+            act("local renamed directory to " + f2, "d", None, f, f2,
+                m2.flags(f))
+        elif f in copy:
             f2 = copy[f]
-            if f2 not in m1: # directory rename
-                act("local renamed directory to " + f2, "d",
-                    None, f, f2, m2.flags(f))
-            elif f2 in m2: # rename case 1, A/A,B/A
+            if f2 in m2: # rename case 1, A/A,B/A
                 act("remote copied to " + f, "m",
                     f2, f, f, fmerge(f2, f, f2), False)
             else: # case 3,20 A/B/A