From: Steffen Prohaska <prohaska@zib.de>
Date: Thu, 18 Oct 2007 08:40:09 GMT
Subject: Re: [PATCH 1/2 v3] mergetool: use path to mergetool in config var mergetool.<tool>.path
Message-ID: <4591BA15-EB6B-4058-A2D0-879556481E59@zib.de>
In-Reply-To: <20071018080049.GK14735@spearce.org>


On Oct 18, 2007, at 10:00 AM, Shawn O. Pearce wrote:

> Steffen Prohaska <prohaska@zib.de> wrote:
>> On Oct 18, 2007, at 7:27 AM, Shawn O. Pearce wrote:
>>> ...
>>>> +    test -n "$merge_tool" || valid_tool "$merge_tool" || {
>>>
>>> Wouldn't an empty $merge_tool string be caught above in the
>>> valid_tool function where it falls through and returns 1?
>>> So isn't test -n here redundant?
>>
>> Sharp eyes, thanks.
>>
>> Correct is
>>
>> test -z "$merge_tool" || valid_tool "$merge_tool" || {
>>
>> No merge tool or a valid merge tool is allowed at this place.
>> If the tool's already empty there's no need to tell the user
>> that it will be reset to empty.
>>
>> I'll send a v4 version.
>
> Thanks but its a little late.

Sorry.


> I'm about to push maint/master/next/pu
> and this series is in next.  While testing it I found another
> bug related to init_tool_merge_tool_path() not having $merge_tool
> initialized and thus causing it to never select a default tool if
> the user didn't have one configured.

I see. Thanks for fixing this.


> Here's what I'm actually about to push out:

One remark ...

[...]
>
>  if test -z "$merge_tool"; then
>      merge_tool=`git config merge.tool`
> -    case "$merge_tool" in
> -	kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff |  
> gvimdiff | "")
> -	    ;; # happy
> -	*)
> +    if ! valid_tool "$merge_tool"; then

I think its ok to have an empty tool here ...

>  	    echo >&2 "git config option merge.tool set to unknown tool:  
> $merge_tool"
>  	    echo >&2 "Resetting to default..."

... So we should not print this message if the tool is empty.

But an empty tool is not valid. Therefore a check if "$merge_tool"
is not empty should be added to the if.

	Steffen