Patchwork [5,of,6] filemerge: add config knob to check capabilities of internal merge tools

login
register
mail settings
Submitter Katsunori FUJIWARA
Date Aug. 15, 2018, 6:35 p.m.
Message ID <813cc2cf9c0cae192858.1534358137@blacknile>
Download mbox | patch
Permalink /patch/33747/
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 1534339490 -32400
#      Wed Aug 15 22:24:50 2018 +0900
# Node ID 813cc2cf9c0cae1928584518afb4558c443b77b7
# Parent  48c9d0cb2afe7692324e1fa7872bc577db6496af
# Available At https://bitbucket.org/foozy/mercurial-wip
#              hg pull https://bitbucket.org/foozy/mercurial-wip -r 813cc2cf9c0c
# EXP-Topic filemerge-refactor
filemerge: add config knob to check capabilities of internal merge tools

For historical reason, Mercurial assumes capabilities of internal
merge tools as below while examining rules to decide merge tool,
regardless of actual capabilities of them.

    =============== ====== ========
    specified via   binary symlinks
    =============== ====== ========
    --tool          o      o
    HGMERGE         o      o
    merge-patterns  o (*)  x (*)
    ui.merge        x (*)  x (*)
    =============== ====== ========

This causes:

  - unintentional internal merge tool is chosen for binary files via
    merge-patterns section of configuration file

  - explicit configuration of internal merge tool for symlinks is
    ignored unintentionally

But on the other hand, simple "check capability strictly" might break
backward compatibility (e.g. existing merge automations), because it
changes the result of merge tool selection.

Therefore, this patch adds config knob "merge.strict-capability-check"
to control whether capabilities of internal merge tools should be
checked strictly or not.

If this configuration is true, capabilities of internal merge tools
are checked strictly in (*) cases above.
Yuya Nishihara - Aug. 16, 2018, 10:03 a.m.
On Thu, 16 Aug 2018 03:35:37 +0900, FUJIWARA Katsunori wrote:
> +``strict-capability-check``
> +   Whether capabilities of internal merge tools are checked strictly
> +   or not, while examining rules to decide merge tool to be used.
> +   (default: False)
> +
>  ``merge-patterns``
>  ------------------
>  
> diff --git a/mercurial/help/merge-tools.txt b/mercurial/help/merge-tools.txt
> --- a/mercurial/help/merge-tools.txt
> +++ b/mercurial/help/merge-tools.txt
> @@ -80,10 +80,14 @@ step specified via   binary symlink
>  ==== =============== ====== =======
>  1.   --tool          o      o
>  2.   HGMERGE         o      o
> -3.   merge-patterns  o      x
> -4.   ui.merge        x      x
> +3.   merge-patterns  o (*)  x (*)
> +4.   ui.merge        x (*)  x (*)
>  ==== =============== ====== =======
>  
> +If ``merge.strict-capability-check`` configuration is true, Mercurial
> +checks capabilities of internal merge tools strictly in (*) cases
> +above. It is false by default for backward compatibility.

IIUC, strict-capability-check is also applied to external tools while picking
tool for a binary file.
Katsunori FUJIWARA - Aug. 16, 2018, 2:21 p.m.
At Thu, 16 Aug 2018 19:03:30 +0900,
Yuya Nishihara wrote:
> 
> On Thu, 16 Aug 2018 03:35:37 +0900, FUJIWARA Katsunori wrote:
> > +``strict-capability-check``
> > +   Whether capabilities of internal merge tools are checked strictly
> > +   or not, while examining rules to decide merge tool to be used.
> > +   (default: False)
> > +
> >  ``merge-patterns``
> >  ------------------
> >  
> > diff --git a/mercurial/help/merge-tools.txt b/mercurial/help/merge-tools.txt
> > --- a/mercurial/help/merge-tools.txt
> > +++ b/mercurial/help/merge-tools.txt
> > @@ -80,10 +80,14 @@ step specified via   binary symlink
> >  ==== =============== ====== =======
> >  1.   --tool          o      o
> >  2.   HGMERGE         o      o
> > -3.   merge-patterns  o      x
> > -4.   ui.merge        x      x
> > +3.   merge-patterns  o (*)  x (*)
> > +4.   ui.merge        x (*)  x (*)
> >  ==== =============== ====== =======
> >  
> > +If ``merge.strict-capability-check`` configuration is true, Mercurial
> > +checks capabilities of internal merge tools strictly in (*) cases
> > +above. It is false by default for backward compatibility.
> 
> IIUC, strict-capability-check is also applied to external tools while picking
> tool for a binary file.

You are right. I'll revise that.

Patch

diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -771,6 +771,9 @@  coreconfigitem('merge', 'on-failure',
 coreconfigitem('merge', 'preferancestor',
         default=lambda: ['*'],
 )
+coreconfigitem('merge', 'strict-capability-check',
+    default=False,
+)
 coreconfigitem('merge-tools', '.*',
     default=None,
     generic=True,
diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -137,6 +137,8 @@  def findexternaltool(ui, tool):
     return procutil.findexe(util.expandpath(exe))
 
 def _picktool(repo, ui, path, binary, symlink, changedelete):
+    strictcheck = ui.configbool('merge', 'strict-capability-check')
+
     def hascapability(tool, capability, strict=False):
         if strict and tool in internals:
             if internals[tool].capabilities.get(capability):
@@ -155,9 +157,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 hascapability(tool, "symlink"):
+        elif symlink and not hascapability(tool, "symlink", strictcheck):
             ui.warn(_("tool %s can't handle symlinks\n") % tmsg)
-        elif binary and not hascapability(tool, "binary"):
+        elif binary and not hascapability(tool, "binary", strictcheck):
             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
@@ -192,9 +194,13 @@  def _picktool(repo, ui, path, binary, sy
             return (hgmerge, hgmerge)
 
     # then patterns
+
+    # whether binary capability should be checked strictly
+    binarycap = binary and strictcheck
+
     for pat, tool in ui.configitems("merge-patterns"):
         mf = match.match(repo.root, '', [pat])
-        if mf(path) and check(tool, pat, symlink, False, changedelete):
+        if mf(path) and check(tool, pat, symlink, binarycap, changedelete):
             if binary and not hascapability(tool, "binary", strict=True):
                 ui.warn(_("warning: check merge-patterns configurations,"
                           " if %r for binary file %r is unintentional\n"
diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -1347,6 +1347,11 @@  This section specifies behavior during m
    halted, the repository is left in a normal ``unresolved`` merge state.
    (default: ``continue``)
 
+``strict-capability-check``
+   Whether capabilities of internal merge tools are checked strictly
+   or not, while examining rules to decide merge tool to be used.
+   (default: False)
+
 ``merge-patterns``
 ------------------
 
diff --git a/mercurial/help/merge-tools.txt b/mercurial/help/merge-tools.txt
--- a/mercurial/help/merge-tools.txt
+++ b/mercurial/help/merge-tools.txt
@@ -80,10 +80,14 @@  step specified via   binary symlink
 ==== =============== ====== =======
 1.   --tool          o      o
 2.   HGMERGE         o      o
-3.   merge-patterns  o      x
-4.   ui.merge        x      x
+3.   merge-patterns  o (*)  x (*)
+4.   ui.merge        x (*)  x (*)
 ==== =============== ====== =======
 
+If ``merge.strict-capability-check`` configuration is true, Mercurial
+checks capabilities of internal merge tools strictly in (*) cases
+above. It is false by default for backward compatibility.
+
 .. note::
 
    After selecting a merge program, Mercurial will by default attempt
diff --git a/tests/test-help.t b/tests/test-help.t
--- a/tests/test-help.t
+++ b/tests/test-help.t
@@ -1913,8 +1913,12 @@  Test dynamic list of merge tools only sh
       ----------------------------------
       1.   --tool         o      o
       2.   HGMERGE        o      o
-      3.   merge-patterns o      x
-      4.   ui.merge       x      x
+      3.   merge-patterns o (*)  x (*)
+      4.   ui.merge       x (*)  x (*)
+  
+      If "merge.strict-capability-check" configuration is true, Mercurial checks
+      capabilities of internal merge tools strictly in (*) cases above. It is
+      false by default for backward compatibility.
   
       Note:
          After selecting a merge program, Mercurial will by default attempt to
diff --git a/tests/test-merge-tools.t b/tests/test-merge-tools.t
--- a/tests/test-merge-tools.t
+++ b/tests/test-merge-tools.t
@@ -1840,6 +1840,51 @@  checked strictly.
   [1]
   $ hg merge --abort -q
 
+(for ui.merge, ignored unintentionally)
+
+  $ hg merge 9 \
+  > --config ui.merge=:other
+  tool :other (for pattern b) can't handle binary
+  tool true can't handle binary
+  tool false can't handle binary
+  no tool found to merge b
+  keep (l)ocal [working copy], take (o)ther [merge rev], or leave (u)nresolved for b? u
+  0 files updated, 0 files merged, 0 files removed, 1 files unresolved
+  use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon
+  [1]
+  $ hg merge --abort -q
+
+With merge.strict-capability-check=true, binary files capability of
+internal merge tools is checked strictly.
+
+  $ f --hexdump b
+  b:
+  0000: 03 02 01 00                                     |....|
+
+(for merge-patterns)
+
+  $ hg merge 9 --config merge.strict-capability-check=true \
+  > --config merge-patterns.b=:merge-other \
+  > --config merge-patterns.re:[a-z]=:other
+  tool :merge-other (for pattern b) can't handle binary
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ f --hexdump b
+  b:
+  0000: 00 01 02 03                                     |....|
+  $ hg merge --abort -q
+
+(for ui.merge)
+
+  $ hg merge 9 --config merge.strict-capability-check=true \
+  > --config ui.merge=:other
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ f --hexdump b
+  b:
+  0000: 00 01 02 03                                     |....|
+  $ hg merge --abort -q
+
 Check that debugpicktool examines which merge tool is chosen for
 specified file as expected
 
@@ -1883,6 +1928,36 @@  specified file as expected
   $ hg debugpickmergetool -r 6d00b3726f6e
   f = :prompt
 
+(by default, it is assumed that no internal merge tools has symlinks
+capability)
+
+  $ hg debugpickmergetool \
+  > -r 6d00b3726f6e \
+  > --config merge-patterns.f=:merge-other \
+  > --config merge-patterns.re:[f]=:merge-local \
+  > --config merge-patterns.re:[a-z]=:other
+  f = :prompt
+
+  $ hg debugpickmergetool \
+  > -r 6d00b3726f6e \
+  > --config ui.merge=:other
+  f = :prompt
+
+(with strict-capability-check=true, actual symlink capabilities are
+checked striclty)
+
+  $ hg debugpickmergetool --config merge.strict-capability-check=true \
+  > -r 6d00b3726f6e \
+  > --config merge-patterns.f=:merge-other \
+  > --config merge-patterns.re:[f]=:merge-local \
+  > --config merge-patterns.re:[a-z]=:other
+  f = :other
+
+  $ hg debugpickmergetool --config merge.strict-capability-check=true \
+  > -r 6d00b3726f6e \
+  > --config ui.merge=:other
+  f = :other
+
 #endif
 
 (--verbose shows some configurations)