Patchwork [4,of,7,mergedriver] filemerge._picktool: only pick from nomerge tools for change/delete conflicts

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 19, 2015, 9:33 p.m.
Message ID <6100ebf277120db2ea3e.1447968836@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11534/
State Accepted
Delegated to: Martin von Zweigbergk
Headers show

Comments

Siddharth Agarwal - Nov. 19, 2015, 9:33 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1447652415 28800
#      Sun Nov 15 21:40:15 2015 -0800
# Node ID 6100ebf277120db2ea3efb7fb7e7deb0c2585a60
# Parent  696cd54b8958729b6b1763b19a159c3c97b7b72b
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r 6100ebf27712
filemerge._picktool: only pick from nomerge tools for change/delete conflicts

For --tool or HGMERGE, we could have either:
(a) proceeded with the particular tool, then failed the merge.
(b) chosen to prompt regardless.

We're explicitly choosing (b) here, because it's effectively what we've been
doing so far and helps maintain an easier-to-use interface.

However, in future patches we're going to change the default selection from
'pick changed version' to 'leave unresolved'. That fixes most of the brokenness
involved with choice (b).
Martin von Zweigbergk - Nov. 19, 2015, 10:43 p.m.
On Thu, Nov 19, 2015 at 1:36 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1447652415 28800
> #      Sun Nov 15 21:40:15 2015 -0800
> # Node ID 6100ebf277120db2ea3efb7fb7e7deb0c2585a60
> # Parent  696cd54b8958729b6b1763b19a159c3c97b7b72b
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 6100ebf27712
> filemerge._picktool: only pick from nomerge tools for change/delete
> conflicts
>
> For --tool or HGMERGE, we could have either:
> (a) proceeded with the particular tool, then failed the merge.
> (b) chosen to prompt regardless.
>
> We're explicitly choosing (b) here, because it's effectively what we've
> been
> doing so far and helps maintain an easier-to-use interface.
>
> However, in future patches we're going to change the default selection from
> 'pick changed version' to 'leave unresolved'. That fixes most of the
> brokenness
> involved with choice (b).
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -120,8 +120,11 @@ def findexternaltool(ui, tool):
>      exe = _toolstr(ui, tool, "executable", tool)
>      return util.findexe(util.expandpath(exe))
>
> -def _picktool(repo, ui, path, binary, symlink):
> -    def check(tool, pat, symlink, binary):
> +def _picktool(repo, ui, path, binary, symlink, changedelete):
> +    def supportscd(tool):
> +        return tool in internals and internals[tool].mergetype == nomerge
> +
> +    def check(tool, pat, symlink, binary, changedelete):
>          tmsg = tool
>          if pat:
>              tmsg += " specified for " + pat
> @@ -134,6 +137,10 @@ def _picktool(repo, ui, path, binary, sy
>              ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
>          elif binary and not _toolbool(ui, tool, "binary"):
>              ui.warn(_("tool %s can't handle binary\n") % tmsg)
> +        elif changedelete and not supportscd(tool):
> +            # the nomerge tools are the only tools that support
> change/delete
> +            # conflicts
> +            pass
>          elif not util.gui() and _toolbool(ui, tool, "gui"):
>              ui.warn(_("tool %s requires a GUI\n") % tmsg)
>          else:
> @@ -145,21 +152,27 @@ def _picktool(repo, ui, path, binary, sy
>      force = ui.config('ui', 'forcemerge')
>      if force:
>          toolpath = _findtool(ui, force)
> -        if toolpath:
> -            return (force, util.shellquote(toolpath))
> +        if not (changedelete and not supportscd(toolpath)):
> +            if toolpath:
> +                return (force, util.shellquote(toolpath))
> +            else:
> +                # mimic HGMERGE if given tool not found
> +                return (force, force)
>          else:
> -            # mimic HGMERGE if given tool not found
> -            return (force, force)
> +            return ":prompt", None
>
>      # HGMERGE takes next precedence
>      hgmerge = os.environ.get("HGMERGE")
>      if hgmerge:
> -        return (hgmerge, hgmerge)
> +        if not (changedelete and not supportscd(hgmerge)):
>

Not this patch's fault, but would it make sense to use the same name for
the tool path in all three cases instead of "tool", "toolpath" and
"hgmerge"?


> +            return (hgmerge, hgmerge)
> +        else:
> +            return ":prompt", None
>
>      # then patterns
>      for pat, tool in ui.configitems("merge-patterns"):
>          mf = match.match(repo.root, '', [pat])
> -        if mf(path) and check(tool, pat, symlink, False):
> +        if mf(path) and check(tool, pat, symlink, False, changedelete):
>              toolpath = _findtool(ui, tool)
>              return (tool, util.shellquote(toolpath))
>
> @@ -176,17 +189,19 @@ def _picktool(repo, ui, path, binary, sy
>      tools = sorted([(-p, t) for t, p in tools.items() if t not in
> disabled])
>      uimerge = ui.config("ui", "merge")
>      if uimerge:
> -        if uimerge not in names:
> +        # external tools defined in uimerge won't be able to handle
> +        # change/delete conflicts
> +        if uimerge not in names and not changedelete:
>              return (uimerge, uimerge)
>          tools.insert(0, (None, uimerge)) # highest priority
>      tools.append((None, "hgmerge")) # the old default, if found
>      for p, t in tools:
> -        if check(t, None, symlink, binary):
> +        if check(t, None, symlink, binary, changedelete):
>              toolpath = _findtool(ui, t)
>              return (t, util.shellquote(toolpath))
>
>      # internal merge or prompt as last resort
> -    if symlink or binary:
> +    if symlink or binary or changedelete:
>          return ":prompt", None
>      return ":merge", None
>
> @@ -535,7 +550,8 @@ def _filemerge(premerge, repo, mynode, o
>      fd = fcd.path()
>      binary = fcd.isbinary() or fco.isbinary() or fca.isbinary()
>      symlink = 'l' in fcd.flags() + fco.flags()
> -    tool, toolpath = _picktool(repo, ui, fd, binary, symlink)
> +    changedelete = fcd.isabsent() or fco.isabsent()
> +    tool, toolpath = _picktool(repo, ui, fd, binary, symlink,
> changedelete)
>      if tool in internals and tool.startswith('internal:'):
>          # normalize to new-style names (':merge' etc)
>          tool = tool[len('internal'):]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 19, 2015, 10:45 p.m.
On 11/19/15 14:43, Martin von Zweigbergk wrote:
> Not this patch's fault, but would it make sense to use the same name 
> for the tool path in all three cases instead of "tool", "toolpath" and 
> "hgmerge"?

Yeah, probably, as a followup.
Martin von Zweigbergk - Nov. 20, 2015, 12:10 a.m.
On Thu, Nov 19, 2015 at 1:36 PM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1447652415 28800
> #      Sun Nov 15 21:40:15 2015 -0800
> # Node ID 6100ebf277120db2ea3efb7fb7e7deb0c2585a60
> # Parent  696cd54b8958729b6b1763b19a159c3c97b7b72b
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 6100ebf27712
> filemerge._picktool: only pick from nomerge tools for change/delete
> conflicts
>
> For --tool or HGMERGE, we could have either:
> (a) proceeded with the particular tool, then failed the merge.
> (b) chosen to prompt regardless.
>
> We're explicitly choosing (b) here, because it's effectively what we've
> been
> doing so far and helps maintain an easier-to-use interface.
>
> However, in future patches we're going to change the default selection from
> 'pick changed version' to 'leave unresolved'. That fixes most of the
> brokenness
> involved with choice (b).
>
> diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -120,8 +120,11 @@ def findexternaltool(ui, tool):
>      exe = _toolstr(ui, tool, "executable", tool)
>      return util.findexe(util.expandpath(exe))
>
> -def _picktool(repo, ui, path, binary, symlink):
> -    def check(tool, pat, symlink, binary):
> +def _picktool(repo, ui, path, binary, symlink, changedelete):
> +    def supportscd(tool):
> +        return tool in internals and internals[tool].mergetype == nomerge
> +
> +    def check(tool, pat, symlink, binary, changedelete):
>          tmsg = tool
>          if pat:
>              tmsg += " specified for " + pat
> @@ -134,6 +137,10 @@ def _picktool(repo, ui, path, binary, sy
>              ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
>          elif binary and not _toolbool(ui, tool, "binary"):
>              ui.warn(_("tool %s can't handle binary\n") % tmsg)
> +        elif changedelete and not supportscd(tool):
> +            # the nomerge tools are the only tools that support
> change/delete
> +            # conflicts
> +            pass
>          elif not util.gui() and _toolbool(ui, tool, "gui"):
>              ui.warn(_("tool %s requires a GUI\n") % tmsg)
>          else:
> @@ -145,21 +152,27 @@ def _picktool(repo, ui, path, binary, sy
>      force = ui.config('ui', 'forcemerge')
>      if force:
>          toolpath = _findtool(ui, force)
> -        if toolpath:
> -            return (force, util.shellquote(toolpath))
> +        if not (changedelete and not supportscd(toolpath)):
>

Unless you oppose it, I'll invert this if/else to avoid the negation here
in flight. Same below. It seems easier without the negation at this point,
but I don't know what the future looks like.


> +            if toolpath:
> +                return (force, util.shellquote(toolpath))
> +            else:
> +                # mimic HGMERGE if given tool not found
> +                return (force, force)
>          else:
> -            # mimic HGMERGE if given tool not found
> -            return (force, force)
> +            return ":prompt", None
>
>      # HGMERGE takes next precedence
>      hgmerge = os.environ.get("HGMERGE")
>      if hgmerge:
> -        return (hgmerge, hgmerge)
> +        if not (changedelete and not supportscd(hgmerge)):
> +            return (hgmerge, hgmerge)
> +        else:
> +            return ":prompt", None
>
>      # then patterns
>      for pat, tool in ui.configitems("merge-patterns"):
>          mf = match.match(repo.root, '', [pat])
> -        if mf(path) and check(tool, pat, symlink, False):
> +        if mf(path) and check(tool, pat, symlink, False, changedelete):
>              toolpath = _findtool(ui, tool)
>              return (tool, util.shellquote(toolpath))
>
> @@ -176,17 +189,19 @@ def _picktool(repo, ui, path, binary, sy
>      tools = sorted([(-p, t) for t, p in tools.items() if t not in
> disabled])
>      uimerge = ui.config("ui", "merge")
>      if uimerge:
> -        if uimerge not in names:
> +        # external tools defined in uimerge won't be able to handle
> +        # change/delete conflicts
> +        if uimerge not in names and not changedelete:
>              return (uimerge, uimerge)
>          tools.insert(0, (None, uimerge)) # highest priority
>      tools.append((None, "hgmerge")) # the old default, if found
>      for p, t in tools:
> -        if check(t, None, symlink, binary):
> +        if check(t, None, symlink, binary, changedelete):
>              toolpath = _findtool(ui, t)
>              return (t, util.shellquote(toolpath))
>
>      # internal merge or prompt as last resort
> -    if symlink or binary:
> +    if symlink or binary or changedelete:
>          return ":prompt", None
>      return ":merge", None
>
> @@ -535,7 +550,8 @@ def _filemerge(premerge, repo, mynode, o
>      fd = fcd.path()
>      binary = fcd.isbinary() or fco.isbinary() or fca.isbinary()
>      symlink = 'l' in fcd.flags() + fco.flags()
> -    tool, toolpath = _picktool(repo, ui, fd, binary, symlink)
> +    changedelete = fcd.isabsent() or fco.isabsent()
> +    tool, toolpath = _picktool(repo, ui, fd, binary, symlink,
> changedelete)
>      if tool in internals and tool.startswith('internal:'):
>          # normalize to new-style names (':merge' etc)
>          tool = tool[len('internal'):]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 20, 2015, 1:04 a.m.
On 11/19/15 16:10, Martin von Zweigbergk wrote:
> Unless you oppose it, I'll invert this if/else to avoid the negation 
> here in flight. Same below. It seems easier without the negation at 
> this point, but I don't know what the future looks like.

No further changes are planned here.

The idea behind the negation there was to put the overwhelmingly common 
case first, that's all. I don't care too much though.

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -120,8 +120,11 @@  def findexternaltool(ui, tool):
     exe = _toolstr(ui, tool, "executable", tool)
     return util.findexe(util.expandpath(exe))
 
-def _picktool(repo, ui, path, binary, symlink):
-    def check(tool, pat, symlink, binary):
+def _picktool(repo, ui, path, binary, symlink, changedelete):
+    def supportscd(tool):
+        return tool in internals and internals[tool].mergetype == nomerge
+
+    def check(tool, pat, symlink, binary, changedelete):
         tmsg = tool
         if pat:
             tmsg += " specified for " + pat
@@ -134,6 +137,10 @@  def _picktool(repo, ui, path, binary, sy
             ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
         elif binary and not _toolbool(ui, tool, "binary"):
             ui.warn(_("tool %s can't handle binary\n") % tmsg)
+        elif changedelete and not supportscd(tool):
+            # the nomerge tools are the only tools that support change/delete
+            # conflicts
+            pass
         elif not util.gui() and _toolbool(ui, tool, "gui"):
             ui.warn(_("tool %s requires a GUI\n") % tmsg)
         else:
@@ -145,21 +152,27 @@  def _picktool(repo, ui, path, binary, sy
     force = ui.config('ui', 'forcemerge')
     if force:
         toolpath = _findtool(ui, force)
-        if toolpath:
-            return (force, util.shellquote(toolpath))
+        if not (changedelete and not supportscd(toolpath)):
+            if toolpath:
+                return (force, util.shellquote(toolpath))
+            else:
+                # mimic HGMERGE if given tool not found
+                return (force, force)
         else:
-            # mimic HGMERGE if given tool not found
-            return (force, force)
+            return ":prompt", None
 
     # HGMERGE takes next precedence
     hgmerge = os.environ.get("HGMERGE")
     if hgmerge:
-        return (hgmerge, hgmerge)
+        if not (changedelete and not supportscd(hgmerge)):
+            return (hgmerge, hgmerge)
+        else:
+            return ":prompt", None
 
     # then patterns
     for pat, tool in ui.configitems("merge-patterns"):
         mf = match.match(repo.root, '', [pat])
-        if mf(path) and check(tool, pat, symlink, False):
+        if mf(path) and check(tool, pat, symlink, False, changedelete):
             toolpath = _findtool(ui, tool)
             return (tool, util.shellquote(toolpath))
 
@@ -176,17 +189,19 @@  def _picktool(repo, ui, path, binary, sy
     tools = sorted([(-p, t) for t, p in tools.items() if t not in disabled])
     uimerge = ui.config("ui", "merge")
     if uimerge:
-        if uimerge not in names:
+        # external tools defined in uimerge won't be able to handle
+        # change/delete conflicts
+        if uimerge not in names and not changedelete:
             return (uimerge, uimerge)
         tools.insert(0, (None, uimerge)) # highest priority
     tools.append((None, "hgmerge")) # the old default, if found
     for p, t in tools:
-        if check(t, None, symlink, binary):
+        if check(t, None, symlink, binary, changedelete):
             toolpath = _findtool(ui, t)
             return (t, util.shellquote(toolpath))
 
     # internal merge or prompt as last resort
-    if symlink or binary:
+    if symlink or binary or changedelete:
         return ":prompt", None
     return ":merge", None
 
@@ -535,7 +550,8 @@  def _filemerge(premerge, repo, mynode, o
     fd = fcd.path()
     binary = fcd.isbinary() or fco.isbinary() or fca.isbinary()
     symlink = 'l' in fcd.flags() + fco.flags()
-    tool, toolpath = _picktool(repo, ui, fd, binary, symlink)
+    changedelete = fcd.isabsent() or fco.isabsent()
+    tool, toolpath = _picktool(repo, ui, fd, binary, symlink, changedelete)
     if tool in internals and tool.startswith('internal:'):
         # normalize to new-style names (':merge' etc)
         tool = tool[len('internal'):]