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

[Jonathan Nieder <jrnieder@gmail.com>]

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