Re: [PATCH] tests: turn on test-lint-shell-syntax by default

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2013, 22:38 UTC
Message-ID: <7v4nikiu81.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130113173207.GC5973@elie.Belkin>

Jonathan Nieder <jrnieder@gmail.com> writes:

Show 23 quoted lines

> Hi,
>
> Torsten Bögershausen wrote:
>
>> -	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
>> +	/^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';
>
> Hmm.  Neither the old version nor the new one matches what seem to
> be typical uses of 'which', based on a quick code search:
>
> 	if which sl >/dev/null 2>&1
> 	then
> 		sl -l
> 		...
> 	fi
>
> or
>
> 	if test -x "$(which sl 2>/dev/null)"
> 	then
> 		sl -l
> 		...
> 	fi

Yes, these two misuses are what we want it to trigger on, so the test is very easy to trigger and produce a false positive, but does not trigger on what we really want to catch.

That does not sound like a good benefit/cost ratio to me.