Patchwork filemerge: support specfiying a python function to custom merge-tools

login
register
mail settings
Submitter Tom Hindle
Date May 16, 2018, 8:22 p.m.
Message ID <7e7e8dd8e70bbba7cca9.1526502133@hindlet-PC>
Download mbox | patch
Permalink /patch/31631/
State Accepted
Headers show

Comments

Tom Hindle - May 16, 2018, 8:22 p.m.
# HG changeset patch
# User hindlemail
# Date 1526501501 21600
#      Wed May 16 14:11:41 2018 -0600
# Node ID 7e7e8dd8e70bbba7cca9a5f17371697c159e6b14
# Parent  0fa050bc68cb7a3bfb67390f2a84ff1c7efa9778
filemerge: support specfiying a python function to custom merge-tools

Eliminates the need to specify a python executable, which may not exist on
system. Additionally launching script inprocess aids portablity on systems that
can't execute python via the shell.
example usage "merge-tools.myTool.executable=python:c:\myTool.py:mergefn"
where myTool.py contains a function:
"def mergefn(ui, repo, args, **kwargs):"
where args is list of args passed to merge tool.
(by default, expanded:  $local $base $other)

invoking the specified python function was done by exposing and invoking
(hook._pythonhook -> hook.pythonhook)
Yuya Nishihara - May 17, 2018, 1:14 p.m.
On Wed, 16 May 2018 14:22:13 -0600, tom_hindle@sil.org wrote:
> # HG changeset patch
> # User hindlemail

Can you suggest a proper "user" name?
test-check-commit.t complains that "username is not an email address."

> # Date 1526501501 21600
> #      Wed May 16 14:11:41 2018 -0600
> # Node ID 7e7e8dd8e70bbba7cca9a5f17371697c159e6b14
> # Parent  0fa050bc68cb7a3bfb67390f2a84ff1c7efa9778
> filemerge: support specfiying a python function to custom merge-tools

Queued, thanks. I'll push this when all blockers are gone.

> @@ -551,12 +559,36 @@
>          args = util.interpolate(
>              br'\$', replace, args,
>              lambda s: procutil.shellquote(util.localpath(s)))
> -        cmd = toolpath + ' ' + args
>          if _toolbool(ui, tool, "gui"):
>              repo.ui.status(_('running merge tool %s for file %s\n') %
>                             (tool, fcd.path()))
> -        repo.ui.debug('launching merge tool: %s\n' % cmd)
> -        r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool')
> +        if scriptfn is None:
> +            cmd = toolpath + ' ' + args
> +            repo.ui.debug('launching merge tool: %s\n' % cmd)
> +            r = ui.system(cmd, cwd=repo.root, environ=env,
> +                          blockedtag='mergetool')
> +        else:
> +            repo.ui.debug('launching python merge script: %s:%s\n' %
> +                          (toolpath, scriptfn))
> +            r = 0
> +            try:
> +                # avoid cycle cmdutil->merge->filemerge->extensions->cmdutil
> +                from . import extensions
> +                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)

Nit: 'hgmerge.%s' should be a module name, not a function name. Perhaps
'hgmerge.%s' % tool is more correct.

Can you send a follow-up patch once this patch gets pushed?

> +            except Exception:
> +                raise error.Abort(_("loading python merge script failed: %s") %
> +                         toolpath)
> +            mergefn = getattr(mod, scriptfn, None)
> +            if mergefn is None:
> +                raise error.Abort(_("%s does not have function: %s") %
> +                                  (toolpath, scriptfn))
> +            argslist = procutil.shellsplit(args)

Strictly speaking, shellsplit() should be avoided as possible. It's a
best-effort function mainly for debugging aids.

As I said before, an alternative approach is to pass parameters as key-value
dict, and ban the use of .args template.

> +            # avoid cycle cmdutil->merge->filemerge->hook->extensions->cmdutil
> +            from . import hook
> +            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
> +                                          mergefn, {'args' : argslist}, True)
> +            if raised:
> +                r = 1

When throw=True, hook.pythonhook() would raise exception instead of returning
"raised" value. If throw were False, int(ret) should be copied to r. Which one
did you expect?

> diff -r 0fa050bc68cb -r 7e7e8dd8e70b tests/test-merge-tools.t
> --- a/tests/test-merge-tools.t	Sat May 12 00:34:01 2018 -0400
> +++ b/tests/test-merge-tools.t	Wed May 16 14:11:41 2018 -0600
> @@ -328,6 +328,183 @@
>    # hg resolve --list
>    R f
>  
> +executable set to python script that succeeds:
> +
> +  $ cat > /tmp/myworkingmerge.py <<EOF

Replaced all /tmp with "$TESTTMP/".

Can you add some tests to check that args are passed correctly?
Tom Hindle - May 17, 2018, 6:34 p.m.
On 05/17/2018 07:14 AM, Yuya Nishihara wrote:
> On Wed, 16 May 2018 14:22:13 -0600, tom_hindle@sil.org wrote:
>> # HG changeset patch
>> # User hindlemail
> Can you suggest a proper "user" name?
> test-check-commit.t complains that "username is not an email address."

oops sorry. My Username should have been set to:

hindlemail <tom_hindle@sil.org>

(But on the Linux machine, I was running tests on I had just set it to 
hindlemail)

>
>> # Date 1526501501 21600
>> #      Wed May 16 14:11:41 2018 -0600
>> # Node ID 7e7e8dd8e70bbba7cca9a5f17371697c159e6b14
>> # Parent  0fa050bc68cb7a3bfb67390f2a84ff1c7efa9778
>> filemerge: support specfiying a python function to custom merge-tools
> Queued, thanks. I'll push this when all blockers are gone.
>
>> @@ -551,12 +559,36 @@
>>           args = util.interpolate(
>>               br'\$', replace, args,
>>               lambda s: procutil.shellquote(util.localpath(s)))
>> -        cmd = toolpath + ' ' + args
>>           if _toolbool(ui, tool, "gui"):
>>               repo.ui.status(_('running merge tool %s for file %s\n') %
>>                              (tool, fcd.path()))
>> -        repo.ui.debug('launching merge tool: %s\n' % cmd)
>> -        r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool')
>> +        if scriptfn is None:
>> +            cmd = toolpath + ' ' + args
>> +            repo.ui.debug('launching merge tool: %s\n' % cmd)
>> +            r = ui.system(cmd, cwd=repo.root, environ=env,
>> +                          blockedtag='mergetool')
>> +        else:
>> +            repo.ui.debug('launching python merge script: %s:%s\n' %
>> +                          (toolpath, scriptfn))
>> +            r = 0
>> +            try:
>> +                # avoid cycle cmdutil->merge->filemerge->extensions->cmdutil
>> +                from . import extensions
>> +                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)
> Nit: 'hgmerge.%s' should be a module name, not a function name. Perhaps
> 'hgmerge.%s' % tool is more correct.
>
> Can you send a follow-up patch once this patch gets pushed?
Yes will do.

>
>> +            except Exception:
>> +                raise error.Abort(_("loading python merge script failed: %s") %
>> +                         toolpath)
>> +            mergefn = getattr(mod, scriptfn, None)
>> +            if mergefn is None:
>> +                raise error.Abort(_("%s does not have function: %s") %
>> +                                  (toolpath, scriptfn))
>> +            argslist = procutil.shellsplit(args)
> Strictly speaking, shellsplit() should be avoided as possible. It's a
> best-effort function mainly for debugging aids.
>
> As I said before, an alternative approach is to pass parameters as key-value
> dict, and ban the use of .args template.

I will look removing procutil.shellsplit.

>
>> +            # avoid cycle cmdutil->merge->filemerge->hook->extensions->cmdutil
>> +            from . import hook
>> +            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
>> +                                          mergefn, {'args' : argslist}, True)
>> +            if raised:
>> +                r = 1
> When throw=True, hook.pythonhook() would raise exception instead of returning
> "raised" value. If throw were False, int(ret) should be copied to r. Which one
> did you expect?
I changed this from False -> True, as (iirc) it gave better stack traces 
when python function had errors.
Shall I deal with this in a follow-up patch?

>
>> diff -r 0fa050bc68cb -r 7e7e8dd8e70b tests/test-merge-tools.t
>> --- a/tests/test-merge-tools.t	Sat May 12 00:34:01 2018 -0400
>> +++ b/tests/test-merge-tools.t	Wed May 16 14:11:41 2018 -0600
>> @@ -328,6 +328,183 @@
>>     # hg resolve --list
>>     R f
>>   
>> +executable set to python script that succeeds:
>> +
>> +  $ cat > /tmp/myworkingmerge.py <<EOF
> Replaced all /tmp with "$TESTTMP/".
>
> Can you add some tests to check that args are passed correctly?
>
Yes I will send in a follow-up patch.

Thanks!
Yuya Nishihara - May 18, 2018, 11:26 a.m.
On Thu, 17 May 2018 12:34:26 -0600, Tom Hindle wrote:
> On 05/17/2018 07:14 AM, Yuya Nishihara wrote:
> > On Wed, 16 May 2018 14:22:13 -0600, tom_hindle@sil.org wrote:
> >> # HG changeset patch
> >> # User hindlemail
> > Can you suggest a proper "user" name?
> > test-check-commit.t complains that "username is not an email address."
> 
> oops sorry. My Username should have been set to:
> 
> hindlemail <tom_hindle@sil.org>

Pushed with that, thanks.

> >> +            # avoid cycle cmdutil->merge->filemerge->hook->extensions->cmdutil
> >> +            from . import hook
> >> +            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
> >> +                                          mergefn, {'args' : argslist}, True)
> >> +            if raised:
> >> +                r = 1
> > When throw=True, hook.pythonhook() would raise exception instead of returning
> > "raised" value. If throw were False, int(ret) should be copied to r. Which one
> > did you expect?
> I changed this from False -> True, as (iirc) it gave better stack traces
> when python function had errors.

I'm not pretty sure, but raising uncaught exception might leave a
transaction interrupted. You can get a traceback with --traceback option
even if throw=False.

> Shall I deal with this in a follow-up patch?

Yeah, and please add a test.

Patch

diff -r 0fa050bc68cb -r 7e7e8dd8e70b mercurial/filemerge.py
--- a/mercurial/filemerge.py	Sat May 12 00:34:01 2018 -0400
+++ b/mercurial/filemerge.py	Wed May 16 14:11:41 2018 -0600
@@ -114,8 +114,16 @@ 
 def _findtool(ui, tool):
     if tool in internals:
         return tool
+    cmd = _toolstr(ui, tool, "executable", tool)
+    if cmd.startswith('python:'):
+        return cmd
     return findexternaltool(ui, tool)
 
+def _quotetoolpath(cmd):
+    if cmd.startswith('python:'):
+        return cmd
+    return procutil.shellquote(cmd)
+
 def findexternaltool(ui, tool):
     for kn in ("regkey", "regkeyalt"):
         k = _toolstr(ui, tool, kn)
@@ -165,7 +173,7 @@ 
             return ":prompt", None
         else:
             if toolpath:
-                return (force, procutil.shellquote(toolpath))
+                return (force, _quotetoolpath(toolpath))
             else:
                 # mimic HGMERGE if given tool not found
                 return (force, force)
@@ -183,7 +191,7 @@ 
         mf = match.match(repo.root, '', [pat])
         if mf(path) and check(tool, pat, symlink, False, changedelete):
             toolpath = _findtool(ui, tool)
-            return (tool, procutil.shellquote(toolpath))
+            return (tool, _quotetoolpath(toolpath))
 
     # then merge tools
     tools = {}
@@ -208,7 +216,7 @@ 
     for p, t in tools:
         if check(t, None, symlink, binary, changedelete):
             toolpath = _findtool(ui, t)
-            return (t, procutil.shellquote(toolpath))
+            return (t, _quotetoolpath(toolpath))
 
     # internal merge or prompt as last resort
     if symlink or binary or changedelete:
@@ -325,7 +333,7 @@ 
         return filectx
 
 def _premerge(repo, fcd, fco, fca, toolconf, files, labels=None):
-    tool, toolpath, binary, symlink = toolconf
+    tool, toolpath, binary, symlink, scriptfn = toolconf
     if symlink or fcd.isabsent() or fco.isabsent():
         return 1
     unused, unused, unused, back = files
@@ -361,7 +369,7 @@ 
     return 1 # continue merging
 
 def _mergecheck(repo, mynode, orig, fcd, fco, fca, toolconf):
-    tool, toolpath, binary, symlink = toolconf
+    tool, toolpath, binary, symlink, scriptfn = toolconf
     if symlink:
         repo.ui.warn(_('warning: internal %s cannot merge symlinks '
                        'for %s\n') % (tool, fcd.path()))
@@ -430,7 +438,7 @@ 
     Generic driver for _imergelocal and _imergeother
     """
     assert localorother is not None
-    tool, toolpath, binary, symlink = toolconf
+    tool, toolpath, binary, symlink, scriptfn = toolconf
     r = simplemerge.simplemerge(repo.ui, fcd, fca, fco, label=labels,
                                 localorother=localorother)
     return True, r
@@ -510,7 +518,7 @@ 
                                             'external merge tools')
 
 def _xmerge(repo, mynode, orig, fcd, fco, fca, toolconf, files, labels=None):
-    tool, toolpath, binary, symlink = toolconf
+    tool, toolpath, binary, symlink, scriptfn = toolconf
     if fcd.isabsent() or fco.isabsent():
         repo.ui.warn(_('warning: %s cannot merge change/delete conflict '
                        'for %s\n') % (tool, fcd.path()))
@@ -551,12 +559,36 @@ 
         args = util.interpolate(
             br'\$', replace, args,
             lambda s: procutil.shellquote(util.localpath(s)))
-        cmd = toolpath + ' ' + args
         if _toolbool(ui, tool, "gui"):
             repo.ui.status(_('running merge tool %s for file %s\n') %
                            (tool, fcd.path()))
-        repo.ui.debug('launching merge tool: %s\n' % cmd)
-        r = ui.system(cmd, cwd=repo.root, environ=env, blockedtag='mergetool')
+        if scriptfn is None:
+            cmd = toolpath + ' ' + args
+            repo.ui.debug('launching merge tool: %s\n' % cmd)
+            r = ui.system(cmd, cwd=repo.root, environ=env,
+                          blockedtag='mergetool')
+        else:
+            repo.ui.debug('launching python merge script: %s:%s\n' %
+                          (toolpath, scriptfn))
+            r = 0
+            try:
+                # avoid cycle cmdutil->merge->filemerge->extensions->cmdutil
+                from . import extensions
+                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)
+            except Exception:
+                raise error.Abort(_("loading python merge script failed: %s") %
+                         toolpath)
+            mergefn = getattr(mod, scriptfn, None)
+            if mergefn is None:
+                raise error.Abort(_("%s does not have function: %s") %
+                                  (toolpath, scriptfn))
+            argslist = procutil.shellsplit(args)
+            # avoid cycle cmdutil->merge->filemerge->hook->extensions->cmdutil
+            from . import hook
+            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
+                                          mergefn, {'args' : argslist}, True)
+            if raised:
+                r = 1
         repo.ui.debug('merge tool returned: %d\n' % r)
         return True, r, False
 
@@ -751,9 +783,24 @@ 
     symlink = 'l' in fcd.flags() + fco.flags()
     changedelete = fcd.isabsent() or fco.isabsent()
     tool, toolpath = _picktool(repo, ui, fd, binary, symlink, changedelete)
+    scriptfn = None
     if tool in internals and tool.startswith('internal:'):
         # normalize to new-style names (':merge' etc)
         tool = tool[len('internal'):]
+    if toolpath and toolpath.startswith('python:'):
+        invalidsyntax = False
+        if toolpath.count(':') >= 2:
+            script, scriptfn = toolpath[7:].rsplit(':', 1)
+            if not scriptfn:
+                invalidsyntax = True
+            # missing :callable can lead to spliting on windows drive letter
+            if '\\' in scriptfn or '/' in scriptfn:
+                invalidsyntax = True
+        else:
+            invalidsyntax = True
+        if invalidsyntax:
+            raise error.Abort(_("invalid 'python:' syntax: %s") % toolpath)
+        toolpath = script
     ui.debug("picked tool '%s' for %s (binary %s symlink %s changedelete %s)\n"
              % (tool, fd, pycompat.bytestr(binary), pycompat.bytestr(symlink),
                     pycompat.bytestr(changedelete)))
@@ -774,7 +821,7 @@ 
         precheck = None
         isexternal = True
 
-    toolconf = tool, toolpath, binary, symlink
+    toolconf = tool, toolpath, binary, symlink, scriptfn
 
     if mergetype == nomerge:
         r, deleted = func(repo, mynode, orig, fcd, fco, fca, toolconf, labels)
diff -r 0fa050bc68cb -r 7e7e8dd8e70b mercurial/hook.py
--- a/mercurial/hook.py	Sat May 12 00:34:01 2018 -0400
+++ b/mercurial/hook.py	Wed May 16 14:11:41 2018 -0600
@@ -24,7 +24,7 @@ 
     stringutil,
 )
 
-def _pythonhook(ui, repo, htype, hname, funcname, args, throw):
+def pythonhook(ui, repo, htype, hname, funcname, args, throw):
     '''call python hook. hook is callable object, looked up as
     name in python module. if callable returns "true", hook
     fails, else passes. if hook raises exception, treated as
@@ -242,7 +242,7 @@ 
                 r = 1
                 raised = False
             elif callable(cmd):
-                r, raised = _pythonhook(ui, repo, htype, hname, cmd, args,
+                r, raised = pythonhook(ui, repo, htype, hname, cmd, args,
                                         throw)
             elif cmd.startswith('python:'):
                 if cmd.count(':') >= 2:
@@ -258,7 +258,7 @@ 
                     hookfn = getattr(mod, cmd)
                 else:
                     hookfn = cmd[7:].strip()
-                r, raised = _pythonhook(ui, repo, htype, hname, hookfn, args,
+                r, raised = pythonhook(ui, repo, htype, hname, hookfn, args,
                                         throw)
             else:
                 r = _exthook(ui, repo, htype, hname, cmd, args, throw)
diff -r 0fa050bc68cb -r 7e7e8dd8e70b tests/test-merge-tools.t
--- a/tests/test-merge-tools.t	Sat May 12 00:34:01 2018 -0400
+++ b/tests/test-merge-tools.t	Wed May 16 14:11:41 2018 -0600
@@ -328,6 +328,183 @@ 
   # hg resolve --list
   R f
 
+executable set to python script that succeeds:
+
+  $ cat > /tmp/myworkingmerge.py <<EOF
+  > def myworkingmergefn(ui, repo, args, **kwargs):
+  >     return False;
+  > EOF
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/myworkingmerge.py:myworkingmergefn
+  merging f
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  M f
+  # hg resolve --list
+  R f
+
+executable set to python script that fails:
+
+  $ cat > /tmp/mybrokenmerge.py <<EOF
+  > def mybrokenmergefn(ui, repo, args, **kwargs):
+  >     ui.write("some fail message\n")
+  >     return True;
+  > EOF
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/mybrokenmerge.py:mybrokenmergefn
+  merging f
+  some fail message
+  abort: /tmp/mybrokenmerge.py hook failed
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  ? f.orig
+  # hg resolve --list
+  U f
+
+executable set to python script that is missing function:
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/myworkingmerge.py:missingFunction
+  merging f
+  abort: /tmp/myworkingmerge.py does not have function: missingFunction
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  ? f.orig
+  # hg resolve --list
+  U f
+
+executable set to missing python script:
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/missingpythonscript.py:mergefn
+  merging f
+  abort: loading python merge script failed: /tmp/missingpythonscript.py
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  ? f.orig
+  # hg resolve --list
+  U f
+
+executable set to python script but callable function is missing:
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/myworkingmerge.py
+  abort: invalid 'python:' syntax: python:/tmp/myworkingmerge.py
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  # hg resolve --list
+  U f
+
+executable set to python script but callable function is empty string:
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/myworkingmerge.py:
+  abort: invalid 'python:' syntax: python:/tmp/myworkingmerge.py:
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  # hg resolve --list
+  U f
+
+executable set to python script but callable function is missing and path contains colon:
+
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config merge-tools.true.executable=python:/tmp/some:dir/myworkingmerge.py
+  abort: invalid 'python:' syntax: python:/tmp/some:dir/myworkingmerge.py
+  [255]
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  # hg resolve --list
+  U f
+
+executable set to python script filename that contains spaces:
+
+  $ mkdir -p '/tmp/my path'
+  $ cat > '/tmp/my path/my working merge with spaces in filename.py' <<EOF
+  > def myworkingmergefn(ui, repo, args, **kwargs):
+  >     return False;
+  > EOF
+  $ beforemerge
+  [merge-tools]
+  false.whatever=
+  true.priority=1
+  true.executable=cat
+  # hg update -C 1
+  $ hg merge -r 2 --config "merge-tools.true.executable=python:/tmp/my path/my working merge with spaces in filename.py:myworkingmergefn"
+  merging f
+  0 files updated, 1 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ aftermerge
+  # cat f
+  revision 1
+  space
+  # hg stat
+  M f
+  # hg resolve --list
+  R f
+
 #if unix-permissions
 
 environment variables in true.executable are handled: