On Wed, 16 May 2018 14:22:13 -0600, tom_hin...@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?
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Reply via email to