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

login
register
mail settings
Submitter Tom Hindle
Date May 9, 2018, 6:33 p.m.
Message ID <ac1af9a3417eddaee936.1525890829@hindlet-PC>
Download mbox | patch
Permalink /patch/31435/
State Accepted
Headers show

Comments

Tom Hindle - May 9, 2018, 6:33 p.m.
# HG changeset patch
# User hindlemail <tom_hindle@sil.org>
# Date 1525890682 21600
#      Wed May 09 12:31:22 2018 -0600
# Node ID ac1af9a3417eddaee93635682d3ebcdb8a929a8a
# Parent  8b86acc7aa64130f5b6fa69f5fc20ef4d0b09c42
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)

extensions and hooks were imported locally because of cycles:
cmdutil -> merge -> filemerge -> extensions -> cmdutil
cmdutil -> merge -> filemerge -> hook -> extensions -> cmdutil
Yuya Nishihara - May 10, 2018, 1:53 p.m.
On Wed, 09 May 2018 12:33:49 -0600, tom_hindle@sil.org wrote:
> # HG changeset patch
> # User hindlemail <tom_hindle@sil.org>
> # Date 1525890682 21600
> #      Wed May 09 12:31:22 2018 -0600
> # Node ID ac1af9a3417eddaee93635682d3ebcdb8a929a8a
> # Parent  8b86acc7aa64130f5b6fa69f5fc20ef4d0b09c42
> filemerge: support specfiying a python function to custom merge-tools

Thanks, this looks better than the previous version.
Can you add some tests?

(Inline comments follow.)

> 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)
> 
> extensions and hooks were imported locally because of cycles:
> cmdutil -> merge -> filemerge -> extensions -> cmdutil
> cmdutil -> merge -> filemerge -> hook -> extensions -> cmdutil

Yeah. Can you copy these to inline comments so we can spot and fix the
problem later. Perhaps we'll need to extract a utility module from hook
and extensions.

>  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 +554,35 @@
>          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:
> +                from . import extensions
> +                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)
> +            except Exception:
> +                ui.write(_("loading %s python merge script failed\n") %
> +                         toolpath)
> +                raise
> +            mergefn = getattr(mod, scriptfn, None)
> +            if mergefn is None:
> +                raise error.Abort(_("%s does not have function: %s\n") %
> +                                  (toolpath, scriptfn))
> +            argslist = procutil.shellsplit(args)
> +            from . import hook
> +            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
> +                                          mergefn, {'args' : argslist}, True)

I think it's better to pass parameters by variables (e.g. base=basepath),
instead of a packed argslist. In which case, we'll probably add a separate
function (e.g. _pymerge().)

> +    if toolpath and procutil.shellsplit(toolpath)[0].startswith('python:'):

Perhaps shellsplit() shouldn't be applied as 'python:' isn't a shell command.

> +        invalidsyntax = False
> +        if toolpath.count(':') >= 2:
> +            script, scriptfn = procutil.shellsplit(toolpath)[0][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

Good catch. Maybe we can backport this to hook.py?

> +        else:
> +            invalidsyntax = True
> +        if invalidsyntax:
> +            raise error.Abort(_("invalid 'python:' syntax: %s \n") % toolpath)
> +        toolpath = script
Tom Hindle - May 11, 2018, 6:05 p.m.
Hi Yuya,

Thanks for the review!

replying inline...

On 05/10/2018 07:53 AM, Yuya Nishihara wrote:
> On Wed, 09 May 2018 12:33:49 -0600, tom_hindle@sil.org wrote:
>> # HG changeset patch
>> # User hindlemail <tom_hindle@sil.org>
>> # Date 1525890682 21600
>> #      Wed May 09 12:31:22 2018 -0600
>> # Node ID ac1af9a3417eddaee93635682d3ebcdb8a929a8a
>> # Parent  8b86acc7aa64130f5b6fa69f5fc20ef4d0b09c42
>> filemerge: support specfiying a python function to custom merge-tools
> Thanks, this looks better than the previous version.
> Can you add some tests?
>
> (Inline comments follow.)

Done. I will resubmit patch shortly.

>
>> 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)
>>
>> extensions and hooks were imported locally because of cycles:
>> cmdutil -> merge -> filemerge -> extensions -> cmdutil
>> cmdutil -> merge -> filemerge -> hook -> extensions -> cmdutil
> Yeah. Can you copy these to inline comments so we can spot and fix the
> problem later. Perhaps we'll need to extract a utility module from hook
> and extensions.

Done.
>
>>   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 +554,35 @@
>>           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:
>> +                from . import extensions
>> +                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)
>> +            except Exception:
>> +                ui.write(_("loading %s python merge script failed\n") %
>> +                         toolpath)
>> +                raise
>> +            mergefn = getattr(mod, scriptfn, None)
>> +            if mergefn is None:
>> +                raise error.Abort(_("%s does not have function: %s\n") %
>> +                                  (toolpath, scriptfn))
>> +            argslist = procutil.shellsplit(args)
>> +            from . import hook
>> +            ret, raised = hook.pythonhook(ui, repo, "merge", toolpath,
>> +                                          mergefn, {'args' : argslist}, True)
> I think it's better to pass parameters by variables (e.g. base=basepath),
> instead of a packed argslist. In which case, we'll probably add a separate
> function (e.g. _pymerge().)
Sorry I don't understand this.
Aren't the args the merge tool / function receives entirely customizable?
eg. I do this:
--config "merge-tools.mymerge.args=the $base sat on $other $local mat"

How would we make a sensible arg name for each argument the user specifies?

>> +    if toolpath and procutil.shellsplit(toolpath)[0].startswith('python:'):
> Perhaps shellsplit() shouldn't be applied as 'python:' isn't a shell command.

_picktool returns a quoted 'toolpath' (using 
procutil.shellquote(toolpath)), so this was just to undo that.

I could do one of these:
a. leave it as it is
b. just strip off the quotes manually (but this doesn't seem a good 
idea, as I would have to deal with shell differences)
c. change _picktool to not call procutil.shellquote() on toolpath when 
it starts with :python

>
>> +        invalidsyntax = False
>> +        if toolpath.count(':') >= 2:
>> +            script, scriptfn = procutil.shellsplit(toolpath)[0][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
> Good catch. Maybe we can backport this to hook.py?
I will look at doing that in a different patch.

>
>> +        else:
>> +            invalidsyntax = True
>> +        if invalidsyntax:
>> +            raise error.Abort(_("invalid 'python:' syntax: %s \n") % toolpath)
>> +        toolpath = script
Yuya Nishihara - May 12, 2018, 2:12 a.m.
On Fri, 11 May 2018 12:05:50 -0600, Tom Hindle wrote:
> > I think it's better to pass parameters by variables (e.g. base=basepath),
> > instead of a packed argslist. In which case, we'll probably add a separate
> > function (e.g. _pymerge().)
> Sorry I don't understand this.

I meant the user merge function could take these variables as keyword
arguments just like hooks. e.g.

  def mymerge(ui, ..., *kwargs):
      kwargs['other']

> Aren't the args the merge tool / function receives entirely customizable?
> eg. I do this:
> --config "merge-tools.mymerge.args=the $base sat on $other $local mat"
> 
> How would we make a sensible arg name for each argument the user specifies?

So yeah, there are some impedance mismatch between [merge-tools] templates
designed for external commands and in-process hook-like functions. We could
make the in-process interface either:

 (a) like a command entry point, takes args built with the merge-tools.*.args
     template
 (b) a plain function, takes base, other, etc. as parameters.

(b) is more similar to the other extension APIs, but wouldn't work well with
the [merge-tools] syntax which has the .args= field. I'm not sure which is
better.

> >> +    if toolpath and procutil.shellsplit(toolpath)[0].startswith('python:'):
> > Perhaps shellsplit() shouldn't be applied as 'python:' isn't a shell command.
> 
> _picktool returns a quoted 'toolpath' (using 
> procutil.shellquote(toolpath)), so this was just to undo that.
> 
> I could do one of these:
> a. leave it as it is
> b. just strip off the quotes manually (but this doesn't seem a good 
> idea, as I would have to deal with shell differences)
> c. change _picktool to not call procutil.shellquote() on toolpath when 
> it starts with :python

Maybe (c) because _picktool() returns internal tools unquoted. The "python:"
syntax seems more like internal names than command names.

Patch

diff -r 8b86acc7aa64 -r ac1af9a3417e mercurial/filemerge.py
--- a/mercurial/filemerge.py	Sat Apr 28 23:16:41 2018 -0700
+++ b/mercurial/filemerge.py	Wed May 09 12:31:22 2018 -0600
@@ -114,6 +114,9 @@ 
 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 findexternaltool(ui, tool):
@@ -325,7 +328,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 +364,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 +433,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 +513,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 +554,35 @@ 
         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:
+                from . import extensions
+                mod = extensions.loadpath(toolpath, 'hgmerge.%s' % scriptfn)
+            except Exception:
+                ui.write(_("loading %s python merge script failed\n") %
+                         toolpath)
+                raise
+            mergefn = getattr(mod, scriptfn, None)
+            if mergefn is None:
+                raise error.Abort(_("%s does not have function: %s\n") %
+                                  (toolpath, scriptfn))
+            argslist = procutil.shellsplit(args)
+            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 +777,25 @@ 
     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 procutil.shellsplit(toolpath)[0].startswith('python:'):
+        invalidsyntax = False
+        if toolpath.count(':') >= 2:
+            script, scriptfn = procutil.shellsplit(toolpath)[0][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 \n") % 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 +816,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 8b86acc7aa64 -r ac1af9a3417e mercurial/hook.py
--- a/mercurial/hook.py	Sat Apr 28 23:16:41 2018 -0700
+++ b/mercurial/hook.py	Wed May 09 12:31:22 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)