Patchwork [3,of,6] filemerge: add the function to examine a capability of a internal tool

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Aug. 15, 2018, 6:35 p.m.
Message ID <1d16378efeb91e435d52.1534358135@blacknile>
Download mbox | patch
Permalink /patch/33743/
State Accepted
Headers show

Comments

Katsunori FUJIWARA - Aug. 15, 2018, 6:35 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1534245351 -32400
#      Tue Aug 14 20:15:51 2018 +0900
# Node ID 1d16378efeb91e435d520b081a7a34b78d7fa7e2
# Parent  ca968875ed9869cbfa464af05c2a217219f298ed
# Available At https://bitbucket.org/foozy/mercurial-wip
#              hg pull https://bitbucket.org/foozy/mercurial-wip -r 1d16378efeb9
# EXP-Topic filemerge-refactor
filemerge: add the function to examine a capability of a internal tool

For "symlink" and "binary" capabilities, _toolbool() can not examine
these of internal merge tools strictly, because it examines only
configurations in "merge-tools" section.

Users can configure them explicitly as below for example, but this is
not ordinary usage and not convenient:

  [merge-tools]
  :other.symlink = true
  :other.binary = true

This patch adds hascapability() internal function, which can examine
actual capabilities of a internal merge tool strictly.

At this patch, hascapability() is still used with "strict=False".
Subsequent patches use it with "strict=True".
Yuya Nishihara - Aug. 16, 2018, 9:51 a.m.
On Thu, 16 Aug 2018 03:35:35 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1534245351 -32400
> #      Tue Aug 14 20:15:51 2018 +0900
> # Node ID 1d16378efeb91e435d520b081a7a34b78d7fa7e2
> # Parent  ca968875ed9869cbfa464af05c2a217219f298ed
> # Available At https://bitbucket.org/foozy/mercurial-wip
> #              hg pull https://bitbucket.org/foozy/mercurial-wip -r 1d16378efeb9
> # EXP-Topic filemerge-refactor
> filemerge: add the function to examine a capability of a internal tool

> Users can configure them explicitly as below for example, but this is
> not ordinary usage and not convenient:
> 
>   [merge-tools]
>   :other.symlink = true
>   :other.binary = true

Is this a documented feature? I suspect it would be a bug.

> --- a/mercurial/filemerge.py
> +++ b/mercurial/filemerge.py
> @@ -137,6 +137,12 @@ def findexternaltool(ui, tool):
>      return procutil.findexe(util.expandpath(exe))
>  
>  def _picktool(repo, ui, path, binary, symlink, changedelete):
> +    def hascapability(tool, capability, strict=False):
> +        if strict and tool in internals:
> +            if internals[tool].capabilities.get(capability):
> +                return True
> +        return _toolbool(ui, tool, capability)

If we want to support "merge-tools:<foo>.binary = false" for example, we'll
have to make _toolbool() tri-state.
Katsunori FUJIWARA - Aug. 16, 2018, 2:21 p.m.
At Thu, 16 Aug 2018 18:51:55 +0900,
Yuya Nishihara wrote:
> 
> On Thu, 16 Aug 2018 03:35:35 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1534245351 -32400
> > #      Tue Aug 14 20:15:51 2018 +0900
> > # Node ID 1d16378efeb91e435d520b081a7a34b78d7fa7e2
> > # Parent  ca968875ed9869cbfa464af05c2a217219f298ed
> > # Available At https://bitbucket.org/foozy/mercurial-wip
> > #              hg pull https://bitbucket.org/foozy/mercurial-wip -r 1d16378efeb9
> > # EXP-Topic filemerge-refactor
> > filemerge: add the function to examine a capability of a internal tool
> 
> > Users can configure them explicitly as below for example, but this is
> > not ordinary usage and not convenient:
> > 
> >   [merge-tools]
> >   :other.symlink = true
> >   :other.binary = true
> 
> Is this a documented feature? I suspect it would be a bug.

Not documented, but works so, because _toolbool() does not check
whether specified 'tool' is internal one or not.

I'm OK to ignore configurations in "merge-tools" for internal merge
tools, if this is out of backward compatibility scope.

Should I do so ?


> > --- a/mercurial/filemerge.py
> > +++ b/mercurial/filemerge.py
> > @@ -137,6 +137,12 @@ def findexternaltool(ui, tool):
> >      return procutil.findexe(util.expandpath(exe))
> >  
> >  def _picktool(repo, ui, path, binary, symlink, changedelete):
> > +    def hascapability(tool, capability, strict=False):
> > +        if strict and tool in internals:
> > +            if internals[tool].capabilities.get(capability):
> > +                return True
> > +        return _toolbool(ui, tool, capability)
> 
> If we want to support "merge-tools:<foo>.binary = false" for example, we'll
> have to make _toolbool() tri-state.
>
Yuya Nishihara - Aug. 17, 2018, 1:56 a.m.
On Thu, 16 Aug 2018 23:21:05 +0900, FUJIWARA Katsunori wrote:
> At Thu, 16 Aug 2018 18:51:55 +0900,
> Yuya Nishihara wrote:
> > On Thu, 16 Aug 2018 03:35:35 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1534245351 -32400
> > > #      Tue Aug 14 20:15:51 2018 +0900
> > > # Node ID 1d16378efeb91e435d520b081a7a34b78d7fa7e2
> > > # Parent  ca968875ed9869cbfa464af05c2a217219f298ed
> > > # Available At https://bitbucket.org/foozy/mercurial-wip
> > > #              hg pull https://bitbucket.org/foozy/mercurial-wip -r 1d16378efeb9
> > > # EXP-Topic filemerge-refactor
> > > filemerge: add the function to examine a capability of a internal tool
> > 
> > > Users can configure them explicitly as below for example, but this is
> > > not ordinary usage and not convenient:
> > > 
> > >   [merge-tools]
> > >   :other.symlink = true
> > >   :other.binary = true
> > 
> > Is this a documented feature? I suspect it would be a bug.
> 
> Not documented, but works so, because _toolbool() does not check
> whether specified 'tool' is internal one or not.
> 
> I'm OK to ignore configurations in "merge-tools" for internal merge
> tools, if this is out of backward compatibility scope.
> 
> Should I do so ?

Let's remove that "feature" as a bug. The doc explicitly states that
"merge-tools - this section configures external merge tool."

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -137,6 +137,12 @@  def findexternaltool(ui, tool):
     return procutil.findexe(util.expandpath(exe))
 
 def _picktool(repo, ui, path, binary, symlink, changedelete):
+    def hascapability(tool, capability, strict=False):
+        if strict and tool in internals:
+            if internals[tool].capabilities.get(capability):
+                return True
+        return _toolbool(ui, tool, capability)
+
     def supportscd(tool):
         return tool in internals and internals[tool].mergetype == nomerge
 
@@ -149,9 +155,9 @@  def _picktool(repo, ui, path, binary, sy
                 ui.warn(_("couldn't find merge tool %s\n") % tmsg)
             else: # configured but non-existing tools are more silent
                 ui.note(_("couldn't find merge tool %s\n") % tmsg)
-        elif symlink and not _toolbool(ui, tool, "symlink"):
+        elif symlink and not hascapability(tool, "symlink"):
             ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
-        elif binary and not _toolbool(ui, tool, "binary"):
+        elif binary and not hascapability(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