From: Steffen Prohaska <prohaska@zib.de>
Date: Thu, 18 Oct 2007 07:52:12 GMT
Subject: Re: [PATCH 1/2 v3] mergetool: use path to mergetool in config var mergetool.<tool>.path
Message-ID: <923DDB10-C9E9-4797-9FC1-D31DEEBE75B7@zib.de>
In-Reply-To: <20071018052724.GA27813@spearce.org>


On Oct 18, 2007, at 7:27 AM, Shawn O. Pearce wrote:

> Steffen Prohaska <prohaska@zib.de> wrote:
>> This commit adds a mechanism to provide absolute paths to the
>> external programs called by 'git mergetool'.  ...
> ...
>> @@ -297,17 +297,38 @@ do
>>      shift
>>  done
>>
>> +valid_tool() {
>> +	case "$1" in
>> +		kdiff3 | tkdiff | xxdiff | meld | opendiff | emerge | vimdiff |  
>> gvimdiff)
>> +			;; # happy
>> +		*)
>> +			return 1
>> +			;;
>> +	esac
>> +}
> ...
>> +    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.

	Steffen