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

From: Torsten Bögershausen <tboegi@web.de>
Date: Jan 15, 2013, 20:12 UTC
Message-ID: <50F5B83E.9060800@web.de>
In-Reply-To: <7v4nikiu81.fsf@alter.siamese.dyndns.org>

On 13.01.13 23:38, Junio C Hamano wrote:

Show 32 quoted lines

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
>> 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.
>

Thanks for comments, I think writing a regexp for which is difficult. What do we think about something like this for fishing for which:

--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -644,6 +644,10 @@ yes () {
                :
        done
 }
+which () {
+       echo >&2 "which is not portable (please use type)"
+       exit 1
+}


This will happen in runtime, which might be good enough ?


@Matt:
>The "[^#]" appears to ensure that there's at least one character
>before the which and that it's not a pound sign.  Why is this done?
This is simply wrong.