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

[Tom Hindle]

On 05/17/2018 07:14 AM, Yuya Nishihara wrote:
> On Wed, 16 May 2018 14:22:13 -0600, tom_hindle at 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 at 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!