Re: [PATCH] tests: turn on test-lint-shell-syntax by default
On 27.01.13 18:34, Junio C Hamano wrote: Torsten Bögershausen tbo...@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 tbo...@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
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Torsten Bögershausen tbo...@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 ' -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Junio C Hamano gits...@pobox.com writes: Torsten Bögershausen tbo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Hi, Torsten Bögershausen wrote: On 15.01.13 21:38, Junio C Hamano wrote: Torsten Bögershausen tbo...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: On 15.01.13 21:38, Junio C Hamano wrote: Torsten Bögershausen tbo...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Jonathan Nieder jrnie...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Torsten Bögershausen tbo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Junio C Hamano gits...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Torsten Bögershausen tbo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
On 15.01.13 21:38, Junio C Hamano wrote: Torsten Bögershausen tbo...@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 21 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 21; 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 21 if test $? -ne 1 then skip_all='skipping git svn tests, svn not found' test_done fi === ( p4 -h p4d -h ) /dev/null 21 || { 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
On 13.01.13 23:38, Junio C Hamano wrote: Jonathan Nieder jrnie...@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 21 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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Torsten Bögershausen tbo...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
On 12.01.13 07:00, Junio C Hamano wrote: Torsten Bögershausen tbo...@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 tbo...@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; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
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? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] tests: turn on test-lint-shell-syntax by default
Torsten Bögershausen tbo...@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 tbo...@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)) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html