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

quoted text
> 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.


quoted text
> 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.


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

One remark ...

[...]
quoted text
>
>  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 ...

quoted text
>  	    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


-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html