Re: [PATCH] git-mergetool--lib.sh: fix mergetool.<tool>.* configurations ignored for known tools

From: Sylvain Rabot <sylvain@abstraction.fr>
Date: Jun 11, 2010, 10:06 UTC
Message-ID: <AANLkTim3UWlP7N2ucI3ncN2jzc4lSEyiJcQhYNElQVXl@mail.gmail.com>
In-Reply-To: <4C0FEA7B.9030409@hashpling.org>

On Wed, Jun 9, 2010 at 21:24, Charles Bailey <charles@hashpling.org> wrote:

Show 20 quoted lines

> On 05/06/2010 03:31, Sylvain Rabot wrote:
>>
>> At this time when you define merge.tool with a known tool,
>> such as meld, p4merge, diffuse ... etc, mergetool.<tool>.*
>> configurations are ignored and git mergetool will use its
>> own templates.
>>
>> This patch adds a detection for mergetool.<tool>.cmd configuration
>> in the run_merge_tool function. If the configuration is set, it will
>> try to run the tool with mergetool.<tool>.path if its set. It also
>> consider the mergetool.<tool>.trustExitCode configuration.
>>
>> Signed-off-by: Sylvain Rabot<sylvain@abstraction.fr>
>> ---
>>  git-mergetool--lib.sh |   60
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 60 insertions(+), 0 deletions(-)
>>
>
> First, my apologies for only having had the time to skim this so far.

No worries ;)

Show 5 quoted lines

>
> Can I just ask some basic questions about the purpose of this patch. Is it
> the intention that if mergetool.<tool>.cmd is set then you want to mergetool
> to behave 'as if' the merge tool wasn't a 'known' tool and just performed
> the "*)" case ?

yes

Show 5 quoted lines

>
> If so, it seems like a lot of extra boiler-plate and error handling that
> doesn't exist in the normal "*)" case. Should we have have this in the "*)"
> case as well? If so, we should look to rework it so that we can re-use the
> code rather than duplicating it.

I did not modify the "*)" being afraid to break it all, but that would be the right thing to do.

>
> From a user perspective, if they want to run a "known" tool but in a way
> that is different from the default behaviour can't they just give it a
> different name, e.g. merge.tool=my_kdiff3 , mergetool.my_kdiff3.cmd=... ?

That's the workaround, yes, but, if you are the user and you are not aware of this bahavior, you will do exactly like a did, i.e., lose your time to torture your git configuration because it is not working the way you was expecting it would.

>
> Thanks,

Thanks for your time.

Show 6 quoted lines

>
> Charles.
>
> --
> Almost dormant mergetool maintainer.
>

-- 
Sylvain