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

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

[Torsten Bögershausen]

The test Makefile has a default set of lint tests which are run
as part of "make test".

The macro TEST_LINT defaults to "test-lint-duplicates test-lint-executable".

Add test-lint-shell-syntax here, to detect non-portable shell syntax early.

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 1923cc1..6fa2b80 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,7 +13,7 @@ TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
-TEST_LINT ?= test-lint-duplicates test-lint-executable
+TEST_LINT ?= test-lint-duplicates test-lint-executable test-lint-shell-syntax
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-- 
1.8.0.197.g5a90748

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> The test Makefile has a default set of lint tests which are run
> as part of "make test".
>
> The macro TEST_LINT defaults to "test-lint-duplicates test-lint-executable".
>
> Add test-lint-shell-syntax here, to detect non-portable shell syntax early.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---

As I said already, I do not want to do this yet without further
reduction of false positives.

Addition of the shell script test was a good starting point, but as
it stands, it still is too brittle and will trigger on something
even trivially innouous, like this:

	test_expect_success 'no issues' '
        	cat >test.file <<-\EOF &&
                which is the right way?
                EOF
        '

>  t/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/Makefile b/t/Makefile
> index 1923cc1..6fa2b80 100644
> --- a/t/Makefile
> +++ b/t/Makefile
> @@ -13,7 +13,7 @@ TAR ?= $(TAR)
>  RM ?= rm -f
>  PROVE ?= prove
>  DEFAULT_TEST_TARGET ?= test
> -TEST_LINT ?= test-lint-duplicates test-lint-executable
> +TEST_LINT ?= test-lint-duplicates test-lint-executable test-lint-shell-syntax
>  
>  # Shell quote;
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))

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

[Torsten Bögershausen]

On 12.01.13 07:00, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> The test Makefile has a default set of lint tests which are run
>> as part of "make test".
>>
>> The macro TEST_LINT defaults to "test-lint-duplicates test-lint-executable".
>>
>> Add test-lint-shell-syntax here, to detect non-portable shell syntax early.
>>
>> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
>> ---
> 
> As I said already, I do not want to do this yet without further
> reduction of false positives.
Which reinds me that the expression fishing for "which" is really poor.

How about something like the following:

-- >8 --
Subject: [PATCH] Reduce false positive in check-non-portable-shell.pl

check-non-portable-shell.pl is using simple regular expressions to
find illegal shell syntax.
Improve the expressions and reduce the chance for false positves:

"sed -i" must be followed by 1..n whitespace and 1 non whitespace
"declare" must be followed by 1..n whitespace and 1 non whitespace
"echo -n" must be followed by 1..n whitespace and 1 non whitespace
"which" must be followed by 1..n whitespace, a string, "end of line"



diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index 8b5a71d..7151dd6 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -16,10 +16,10 @@ sub err {
 
 while (<>) {
 	chomp;
-	/^\s*sed\s+-i/ and err 'sed -i is not portable';
-	/^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
-	/^\s*declare\s+/ and err 'arrays/declare not portable';
-	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+	/^\s*sed\s+-i\s+\S/ and err 'sed -i is not portable';
+	/^\s*echo\s+-n\s+\S/ and err 'echo -n is not portable (please use printf)';
+	/^\s*declare\s+\S/ and err 'arrays/declare not portable';
+	/^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';
 	/test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
 	# this resets our $. for each file
 	close ARGV if eof;

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

[Matt Kraai]

On Sun, Jan 13, 2013 at 11:25:57AM +0100, Torsten Bögershausen wrote:
> @@ -16,10 +16,10 @@ sub err {
>  
>  while (<>) {
>  	chomp;
> -	/^\s*sed\s+-i/ and err 'sed -i is not portable';
> -	/^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
> -	/^\s*declare\s+/ and err 'arrays/declare not portable';
> -	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
> +	/^\s*sed\s+-i\s+\S/ and err 'sed -i is not portable';
> +	/^\s*echo\s+-n\s+\S/ and err 'echo -n is not portable (please use printf)';
> +	/^\s*declare\s+\S/ and err 'arrays/declare not portable';
> +	/^\s*[^#]\s*which\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';

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?
Why isn't it done for the other commands?

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

[Jonathan Nieder]

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

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

[Junio C Hamano]

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.

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

[Torsten Bögershausen]

On 13.01.13 23:38, Junio C Hamano wrote:
> 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.

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> 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 ?

	if (
		which frotz &&
                test $(frobonitz --version" -le 2.0
	   )
        then
		test_set_prereq FROTZ_FROBONITZ
	else
		echo >&2 "suitable Frotz/Frobonitz combo not available;"
                echo >&2 "some tests may be skipped"
	fi

I somehow think this is a lost cause.

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

[Torsten Bögershausen]

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:
>>
>> --- 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 ?
> 
> 	if (
> 		which frotz &&
>                 test $(frobonitz --version" -le 2.0
> 	   )
>         then
> 		test_set_prereq FROTZ_FROBONITZ
> 	else
> 		echo >&2 "suitable Frotz/Frobonitz combo not available;"
>                 echo >&2 "some tests may be skipped"
> 	fi
> 
> I somehow think this is a lost cause.

I found different ways to detect if frotz is installed in the test suite:
a) use "type"    (Should be the fastest ?)
b) call the command directly, check the exit code
c) "! test_have_prereq" (easy to understand, propably most expensive ?)

Do we really need  "which" to detect if frotz is installed?


/Torsten



=============
if ! type cvs >/dev/null 2>&1
then
	skip_all='skipping cvsimport tests, cvs not found'
	test_done
fi

===========
if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
	# we are in full-on bash mode
	true
elif type bash >/dev/null 2>&1; then
	# execute in full-on bash mode
	unset POSIXLY_CORRECT
	exec bash "$0" "$@"
else
	echo '1..0 #SKIP skipping bash completion tests; bash not available'
	exit 0
fi
===============
svn >/dev/null 2>&1
if test $? -ne 1
then
    skip_all='skipping git svn tests, svn not found'
    test_done
fi
===============
( p4 -h && p4d -h ) >/dev/null 2>&1 || {
	skip_all='skipping git p4 tests; no p4 or p4d'
	test_done
}
===============
if ! test_have_prereq PERL; then
	skip_all='skipping gitweb tests, perl not available'
	test_done
fi

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Do we really need  "which" to detect if frotz is installed?

I think we all know the answer to that question is no, but why is
that a relevant question in the context of this discussion?  One of
us may be very confused.

I thought the topic of this discussion was that, already knowing
that "which" should never be used anywhere in our scripts, you are
trying to devise a mechanical way to catch newcomers' attempts to
use it in their changes, in order to prevent patches that add use of
"which" to be sent for review to waste our time.  I was illustrating
that the approach to override "which" in a shell function for test
scripts will not be a useful solution for that goal.

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

[Torsten Bögershausen]

On 26.01.13 22:43, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Do we really need  "which" to detect if frotz is installed?
> I think we all know the answer to that question is no, but why is
> that a relevant question in the context of this discussion?  One of
> us may be very confused.
>
> I thought the topic of this discussion was that, already knowing
> that "which" should never be used anywhere in our scripts, you are
> trying to devise a mechanical way to catch newcomers' attempts to
> use it in their changes, in order to prevent patches that add use of
> "which" to be sent for review to waste our time.  I was illustrating
> that the approach to override "which" in a shell function for test
> scripts will not be a useful solution for that goal.
Yes, the diskussion went away.

I would rather see the check-non-portable-shell.pl enabled per default.

It looks as if the "which" command is hard to find, when we want a minimal
risk of false positves.
I can see different solutions:

1) We can make a much simpler expression which only catches the most
common usage of which, like
"if whitch foo".

This will not catch lines like

if test -x "$(which foo 2>/dev/null)"

But I think the -x is not a useful anyway, because which should only
list command which have the executable bit set.

2) We drop the which from check-non-portable-shell.pl

I'll send a patch for 1)
/Torsten

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

[Jonathan Nieder]

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

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

[Torsten Bögershausen]

On 27.01.13 10:31, Jonathan Nieder wrote:
> 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

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Back to the which:
> ...
> and running "make test" gives the following, at least in my system:
> ...

I think everybody involved in this discussion already knows that;
the point is that it can easily give false negative, without the
scripts working very hard to do so.

If we did not care about incurring runtime performance cost, we
could arrange:

 - the test framework to define a variable $TEST_ABORT that has a
   full path to a file that is in somewhere test authors cannot
   touch unless they really try hard to (i.e. preferrably outside
   $TRASH_DIRECTORY, as it is not uncommon for to tests to do "rm *"
   there). This location should be per $(basename "$0" .sh) to allow
   running multiple tests in paralell;

 - the test framework to "rm -f $TEST_ABORT" at the beginning of
   test_expect_success/failure;

 - test_expect_success/failure to check $TEST_ABORT and if it
   exists, abort the execution, showing the contents of the file as
   an error message.

Then you can wrap commands whose use we want to limit, perhaps like
this, in the test framework:

	which () {
		cat >"$TEST_ABORT" <<-\EOF
		Do not use unportable 'which' in the test script.
                "if type $cmd" is a good way to see if $cmd exists.
		EOF
	}

	sed () {
		saw_wantarg= must_abort=
                for arg
                do
			if test -n "$saw_wantarg"
                        then
				saw_wantarg=
                                continue
			fi
			case "$arg" in
			--)	break ;; # end of options
			-i)	echo >"$TEST_ABORT" "Do not use 'sed -i'"
				must_abort=yes
				break ;;
                        -e)	saw_wantarg=yes ;; # skip next arg
			-*)	continue ;; # options without arg
			*)	break ;; # filename
			esac
		done
		if test -z "$must_abort"
			sed "$@"
		fi
	}

Then you can check that TEST_ABORT does not appear in test scripts
(ensuring that they do not attempt to circumvent the mechanis) and
catch use of unwanted commands or unwanted extended features of
commands at runtime.

But this will incur runtime performace hit, so I am not sure it
would be worth it.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If we did not care about incurring runtime performance cost, we
> could arrange:
> ...
> Then you can wrap commands whose use we want to limit, perhaps like
> this, in the test framework:
> ...
> 	sed () {
> ...
> 		done
> 		if test -z "$must_abort"
> 			sed "$@"
> 		fi
> 	}

Of course, aside from missing "then", this needs to use the
real "sed", so this has to be

	if test -z "$must_abort"
        then
		command sed "$@"
	fi

or something like that.

An approach along this line may reduce both the false negatives and
false positives down to an acceptable level, but I doubt the result
would be efficient enough for us to tolerate the runtime penalty.

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

[Torsten Bögershausen]

On 27.01.13 18:34, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Back to the which:
>> ...
>> and running "make test" gives the following, at least in my system:
>> ...
> I think everybody involved in this discussion already knows that;
> the point is that it can easily give false negative, without the
> scripts working very hard to do so.
>
> If we did not care about incurring runtime performance cost, we
> could arrange:
>
>  - the test framework to define a variable $TEST_ABORT that has a
>    full path to a file that is in somewhere test authors cannot
>    touch unless they really try hard to (i.e. preferrably outside
>    $TRASH_DIRECTORY, as it is not uncommon for to tests to do "rm *"
>    there). This location should be per $(basename "$0" .sh) to allow
>    running multiple tests in paralell;
>
>  - the test framework to "rm -f $TEST_ABORT" at the beginning of
>    test_expect_success/failure;
>
>  - test_expect_success/failure to check $TEST_ABORT and if it
>    exists, abort the execution, showing the contents of the file as
>    an error message.
>
> Then you can wrap commands whose use we want to limit, perhaps like
> this, in the test framework:
>
> 	which () {
> 		cat >"$TEST_ABORT" <<-\EOF
> 		Do not use unportable 'which' in the test script.
>                 "if type $cmd" is a good way to see if $cmd exists.
> 		EOF
> 	}
>
> 	sed () {
> 		saw_wantarg= must_abort=
>                 for arg
>                 do
> 			if test -n "$saw_wantarg"
>                         then
> 				saw_wantarg=
>                                 continue
> 			fi
> 			case "$arg" in
> 			--)	break ;; # end of options
> 			-i)	echo >"$TEST_ABORT" "Do not use 'sed -i'"
> 				must_abort=yes
> 				break ;;
>                         -e)	saw_wantarg=yes ;; # skip next arg
> 			-*)	continue ;; # options without arg
> 			*)	break ;; # filename
> 			esac
> 		done
> 		if test -z "$must_abort"
> 			sed "$@"
> 		fi
> 	}
>
> Then you can check that TEST_ABORT does not appear in test scripts
> (ensuring that they do not attempt to circumvent the mechanis) and
> catch use of unwanted commands or unwanted extended features of
> commands at runtime.
>
> But this will incur runtime performace hit, so I am not sure it
> would be worth it.
Thanks for the detailed suggestion.
Instead of using a file for putting out non portable syntax,
can we can use a similar logic as test_failure ?

I did some benchmarking, the test suite on a Laptop is 37..38 minutes,
including make clean && make both on next, pu, master or with the patch below.
I couldn't find a measurable impact on the execution time.
What do we think about a patch like this
(Not sure if this cut-and-paste data applies, it's for review)


[PATCH] test-lib: which, echo -n and sed -i are not portable

The posix version of sed command supports options -n -e -f
The gnu extension -i to edit a file in place is not
available on all systems.
To catch the usage of non-posix options with sed a
wrapper function is added in test-lib.sh.
The wrapper checks that only -n -e -f are used.
The short form "-ne" for "-n -e" is allowed as well.

echo -n is not portable and not available on all systems,
printf can be used instead.
Add a wrapper to catch echo -n

which is not portable, the output differs between different
implementations, and the return code may not be reliable.

Add a function test_bad_syntax_ in test-lib.sh, which increments
the variable test_bad_syntax and works similar to test_failure_

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1a6c4ab..248ed34 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -266,6 +266,7 @@ else
     exec 4>/dev/null 3>/dev/null
 fi
 
+test_bad_syntax=0
 test_failure=0
 test_count=0
 test_fixed=0
@@ -300,6 +301,12 @@ test_ok_ () {
     say_color "" "ok $test_count - $@"
 }
 
+test_bad_syntax_ () {
+    test_bad_syntax=$(($test_bad_syntax + 1))
+    say_color error "$@"
+    test "$immediate" = "" || { GIT_EXIT_OK=t; exit 1; }
+}
+
 test_failure_ () {
     test_failure=$(($test_failure + 1))
     say_color error "not ok $test_count - $1"
@@ -402,10 +409,15 @@ test_done () {
         fixed $test_fixed
         broken $test_broken
         failed $test_failure
+        bad_syntax $test_bad_syntax
 
         EOF
     fi
 
+    if test "$test_bad_syntax" != 0
+    then
+        say_color error "# $test_bad_syntax non portable shell syntax"
+    fi
     if test "$test_fixed" != 0
     then
         say_color error "# $test_fixed known breakage(s) vanished; please update test(s)"
@@ -645,6 +657,40 @@ yes () {
     done
 }
 
+
+# which is not portable
+which () {
+    test_bad_syntax_ "Do not use unportable 'which' in the test script." \
+            "'if type $1' is a good way to see if '$1' exists."
+    return 1
+}
+
+# catch non-portable usage of sed
+sed () {
+    for arg
+    do
+        case "$arg" in
+    -[efn]) continue ;; # allowed posix options
+    -ne) continue ;; # tolerated
+        -*)test_bad_syntax_ "Do not use 'sed "$arg"'. Valid options for 'sed' are -n -e -f"
+            return 1
+            ;;
+        *) continue ;;
+        esac
+    done
+    command sed "$@"
+}
+
+# catch non portable echo -n
+echo () {
+    if test "$1" = -n
+    then
+        test_bad_syntax_ "Do not use 'echo -n'. Use printf instead"
+    else
+        command echo "$@"
+    fi
+}
+
 # Fix some commands on Windows
 case $(uname -s) in
 *MINGW*)
-- 
1.8.1.1

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> Thanks for the detailed suggestion.
> Instead of using a file for putting out non portable syntax,
> can we can use a similar logic as test_failure ?

Your test_bad_syntax_ function can be called from a subshell, and
its "exit 1" will not exit, no?

	test_expect_success 'prepare a blob with incomplete line' '
		(
			echo first line
                        echo second line
			echo -n final and incomplete line
		) >incomplete.txt
	'

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Torsten Bögershausen <tboegi@web.de> writes:
>
>> Thanks for the detailed suggestion.
>> Instead of using a file for putting out non portable syntax,
>> can we can use a similar logic as test_failure ?
>
> Your test_bad_syntax_ function can be called from a subshell, and
> its "exit 1" will not exit, no?

What is more important is that the increment to $test_bad_syntax
done in that function will not be propagated up to the main process
that runs the test framework.

Of course, that is why I mentioned communicating using the
filesystem.

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> ...
> 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 ...
> ...
> 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.

Exactly.

That is why I keep saying that whole test-lint-shell-syntax should
stay outside the default until it gets more robust by becoming a
reasonable shell parser; it may not necessarily have to become
"full" parser though.

As we discourage the use of tricky features of the language like
eval in individual test scripts to implement their own mini test
framework, the "something like sparse" parser may initialy start
small and still be useful; for example it can learn to exclude
anything inside <<HERE_DOCUMENT from getting inspected.