Patchwork [4,of,5,V2] manifestmerge: handle abort on local unknown, remote created files

login
register
mail settings
Submitter Siddharth Agarwal
Date Feb. 8, 2013, 10:18 p.m.
Message ID <a3d0334527f026f4eb33.1360361887@sid0x220>
Download mbox | patch
Permalink /patch/843/
State Superseded
Commit 95773237df7f2d292fe690f0f0985465bf69b966
Headers show

Comments

Siddharth Agarwal - Feb. 8, 2013, 10:18 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1360357062 0
# Node ID a3d0334527f026f4eb338c10f225b09def43d911
# Parent  c163fe6e519eabe8d2bff85d9b2c7c345e38d5ba
manifestmerge: handle abort on local unknown, remote created files
Mads Kiilerich - Feb. 9, 2013, noon
On 02/08/2013 11:18 PM, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1360357062 0
> # Node ID a3d0334527f026f4eb338c10f225b09def43d911
> # Parent  c163fe6e519eabe8d2bff85d9b2c7c345e38d5ba
> manifestmerge: handle abort on local unknown, remote created files

Is this handling a new case and fixing a bug? Or is it duplicating 
existing functionality as an optimization or cleanup?

(Should be explained here ... and perhaps tested if it makes any visible 
changes.)

> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -224,7 +224,7 @@ def manifestmerge(repo, p1, p2, pa, bran
>                   m1['.hgsubstate'] += "+"
>                   break
>   
> -    prompts = []
> +    aborts, prompts = [], []
>       # Compare manifests
>       for f, n in m1.iteritems():
>           if partial and not partial(f):
> @@ -285,15 +285,40 @@ def manifestmerge(repo, p1, p2, pa, bran
>                   actions.append((f2, "m", (f, f, True),
>                                   "remote moved to " + f))
>           elif f not in ma:
> -            if (not overwrite
> -                and _checkunknownfile(repo, p1, p2, f)):
> -                actions.append((f, "m", (f, f, False),
> -                                "remote differs from untracked local"))
> +            # local unknown, remote created: the logic is described by the
> +            # following table:
> +            #
> +            # force  branchmerge  different  |  action
> +            #   n         *           n      |    get
> +            #   n         *           y      |   abort
> +            #   y         n           *      |    get
> +            #   y         y           n      |    get
> +            #   y         y           y      |   merge
> +            #

Do we have test coverage for all these cases?

> +            # Checking whether the files are different is expensive, so we
> +            # don't do that when we can avoid it.
> +            if force and not branchmerge:
> +                actions.append((f, "g", (m2.flags(f),), "remote created"))
>               else:
> -                actions.append((f, "g", (m2.flags(f),), "remote created"))
> +                different = _checkunknownfile(repo, p1, p2, f)
> +                if force and branchmerge and different:
> +                    actions.append((f, "m", (f, f, False),
> +                                    "remote differs from untracked local"))
> +                elif not force and different:
> +                    aborts.append((f, "ud"))
> +                else:
> +                    actions.append((f, "g", (m2.flags(f),), "remote created"))
>           elif n != ma[f]:
>               prompts.append((f, "dc")) # prompt deleted/changed
>   
> +    for f, m in sorted(aborts):
> +        if m == "ud":
> +            repo.ui.warn(_("%s: untracked file differs\n") % f)
> +        else: assert False, m
> +    if aborts:
> +        raise util.Abort(_("untracked files in working directory differ "
> +                           "from files in requested revision"))
> +
>       for f, m in sorted(prompts):
>           if m == "cd":
>               if repo.ui.promptchoice(

/Mads
Siddharth Agarwal - Feb. 9, 2013, 1:23 p.m.
On 02/09/2013 12:00 PM, Mads Kiilerich wrote:
> Is this handling a new case and fixing a bug? Or is it duplicating 
> existing functionality as an optimization or cleanup?

Duplicating existing functionality in a faster manner, and the next 
patch removes the original check.

> (Should be explained here ... and perhaps tested if it makes any 
> visible changes.)

No, it doesn't make any visible changes. The next changeset doesn't, either.

> Do we have test coverage for all these cases?

Yes, spread across a bunch of tests.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -224,7 +224,7 @@  def manifestmerge(repo, p1, p2, pa, bran
                 m1['.hgsubstate'] += "+"
                 break
 
-    prompts = []
+    aborts, prompts = [], []
     # Compare manifests
     for f, n in m1.iteritems():
         if partial and not partial(f):
@@ -285,15 +285,40 @@  def manifestmerge(repo, p1, p2, pa, bran
                 actions.append((f2, "m", (f, f, True),
                                 "remote moved to " + f))
         elif f not in ma:
-            if (not overwrite
-                and _checkunknownfile(repo, p1, p2, f)):
-                actions.append((f, "m", (f, f, False),
-                                "remote differs from untracked local"))
+            # local unknown, remote created: the logic is described by the
+            # following table:
+            #
+            # force  branchmerge  different  |  action
+            #   n         *           n      |    get
+            #   n         *           y      |   abort
+            #   y         n           *      |    get
+            #   y         y           n      |    get
+            #   y         y           y      |   merge
+            #
+            # Checking whether the files are different is expensive, so we
+            # don't do that when we can avoid it.
+            if force and not branchmerge:
+                actions.append((f, "g", (m2.flags(f),), "remote created"))
             else:
-                actions.append((f, "g", (m2.flags(f),), "remote created"))
+                different = _checkunknownfile(repo, p1, p2, f)
+                if force and branchmerge and different:
+                    actions.append((f, "m", (f, f, False),
+                                    "remote differs from untracked local"))
+                elif not force and different:
+                    aborts.append((f, "ud"))
+                else:
+                    actions.append((f, "g", (m2.flags(f),), "remote created"))
         elif n != ma[f]:
             prompts.append((f, "dc")) # prompt deleted/changed
 
+    for f, m in sorted(aborts):
+        if m == "ud":
+            repo.ui.warn(_("%s: untracked file differs\n") % f)
+        else: assert False, m
+    if aborts:
+        raise util.Abort(_("untracked files in working directory differ "
+                           "from files in requested revision"))
+
     for f, m in sorted(prompts):
         if m == "cd":
             if repo.ui.promptchoice(