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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 27, 2013, 09:31 UTC
Message-ID: <20130127093121.GA4228@elie.Belkin>
In-Reply-To: <51037E5F.8090506@web.de>

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