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

On 27.01.13 10:31, Jonathan Nieder wrote:

Hi,

Torsten Bögershausen wrote:

quoted

On 15.01.13 21:38, Junio C Hamano wrote:

quoted

Torsten Bögershausen [off-list ref] writes:

quoted

quoted

quoted

What do we think about something like this for fishing for which:

[...]

quoted

quoted

quoted

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

[...]

quoted

quoted

	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