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

From: Torsten Bögershausen <tboegi@web.de>
Date: Jan 27, 2013, 13:13 UTC
Message-ID: <5105280A.80002@web.de>
In-Reply-To: <20130127093121.GA4228@elie.Belkin>

On 27.01.13 10:31, Jonathan Nieder wrote:

Show 66 quoted lines

> Hi,
> 
> Torsten Bögershausen wrote:
>> On 15.01.13 21:38, Junio C Hamano wrote:
>>> Torsten Bögershausen <tboegi@web.de> writes:
> 
>>>> What do we think about something like this for fishing for which:
> [...]
>>>> +which () {
>>>> +       echo >&2 "which is not portable (please use type)"
>>>> +       exit 1
>>>> +}
> [...]
>>> 	if (
>>> 		which frotz &&
>>>                 test $(frobonitz --version" -le 2.0
>>> 	   )
> 
> With the above definition of "which", the only sign of a mistake would
> be some extra output to stderr (which is quelled when running tests in
> the normal way).  The "exit" is caught by the subshell and just makes
> the "if" condition false.
> 
> That's not so terrible --- it could still dissuade new test authors
> from using "which".  The downside I'd worry about is that it provides
> a false sense of security despite not catching problems like
> 
> 	write_script x <<-EOF &&
> 		# Use "foo" if possible.  Otherwise use "bar".
> 		if which foo && test $(foo --version) -le 2.0
> 		then
> 			...
> 		...
> 	EOF
> 	./x
> 
> That's not a great tradeoff relative to the impact of the problem
> being solved.
> 
> Don't get me wrong.  I really do want to see more static or dynamic
> analysis of git's shell scripts in the future.  I fear that for the
> tradeoffs to make sense, though, the analysis needs to be more
> sophisticated:
> 
>  * A very common error in test scripts is leaving out the "&&"
>    connecting adjacent statements, which causes early errors
>    in a test assertion to be missed and tests to pass by mistake.
>    Unfortunately the grammar of the dialect of shell used in tests is
>    not regular enough to make this easily detectable using regexps.
> 
>  * Another common mistake is using "cd" without entering a subshell.
>    Detecting this requires counting nested parentheses and noticing
>    when a parenthesis is quoted.
> 
>  * Another common mistake is relying on the semantics of variable
>    assignments in front of function calls.  Detecting this requires
>    recognizing which commands are function calls.
> 
> In the end the analysis that works best would probably involve a
> full-fledged shell script parser.  Something like "sparse", except for
> shell command language.
> 
> Sorry I don't have more practical advice in the short term.
> 
> My two cents,
> Jonathan

Jonathan, thanks for the review.

My ambition is to get the check-non-portable-shell.pl into a shape that we can enable it by default.

This may be with or without checking for "which". As a first step we will hopefully see less breakage for e.g. Mac OS caused by "echo -n" or "sed -i" usage.

On the longer run, we may be able to have something more advanced.

Back to the which:

Writing a t0009-no-which.sh like this: -------------------- #!/bin/sh test_description='Test the which'

. ./test-lib.sh

which () {
       echo >&2 "which is not portable (please use type)"
       exit 1
}

test_expect_success 'which is not portable' '
	if  which frotz
	then
		say "frotz does not exist"
	else
		say "frotz does exist"
	fi

' test_done -------------- and running "make test" gives the following, at least in my system:

[snip]

*** t0009-no-which.sh ***
FATAL: Unexpected exit with code 1
make[2]: *** [t0009-no-which.sh] Error 1
make[1]: *** [test] Error 2
make: *** [test] Error 2

----------------------- running inside t/ ./t0009-no-which.sh --verbose

Initialized empty Git repository in /Users/tb/projects/git/tb/t/trash directory.t0009-no-which/.git/
expecting success: 
        if  which frotz
        then
                say "frotz does not exist"
        else
                say "frotz does exist"
        fi

which is not portable (please use type)
FATAL: Unexpected exit with code 1

/Torsten