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

[Pierre-Yves David]

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.

-- 
Pierre-Yves