[PATCH] filemerge: support specfiying a python function to custom merge-tools

Yuya Nishihara yuya at tcha.org
Thu May 10 09:53:24 EDT 2018


On Wed, 09 May 2018 12:33:49 -0600, tom_hindle at sil.org wrote:
> # HG changeset patch
> # User hindlemail <tom_hindle at 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


More information about the Mercurial-devel mailing list