{"thread":{"id":"32603","subject":"[PATCH] tests: turn on test-lint-shell-syntax by default","startedAt":"2013-01-12T05:50:45Z","lastAt":"2013-02-05T21:56:24Z","messageCount":19,"participants":["Torsten Bögershausen","Junio C Hamano","Matt Kraai","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"206578","messageId":"201301120650.46479.tboegi@web.de","threadId":"32603","inReplyTo":null,"subject":"[PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-12T05:50:45Z","receivedAt":"2013-01-12T05:50:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"The test Makefile has a default set of lint tests which are run\nas part of \"make test\".\n\nThe macro TEST_LINT defaults to \"test-lint-duplicates test-lint-executable\".\n\nAdd test-lint-shell-syntax here, to detect non-portable shell syntax early.\n\nSigned-off-by: Torsten Bögershausen \n---\n t/Makefile | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 1923cc1..6fa2b80 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -13,7 +13,7 @@ TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\n DEFAULT_TEST_TARGET ?= test\n-TEST_LINT ?= test-lint-duplicates test-lint-executable\n+TEST_LINT ?= test-lint-duplicates test-lint-executable test-lint-shell-syntax\n \n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n-- \n1.8.0.197.g5a90748\n"},{"id":"206579","messageId":"7vvcb37xfe.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"201301120650.46479.tboegi@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-12T06:00:37Z","receivedAt":"2013-01-12T06:00:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> The test Makefile has a default set of lint tests which are run\n> as part of \"make test\".\n>\n> The macro TEST_LINT defaults to \"test-lint-duplicates test-lint-executable\".\n>\n> Add test-lint-shell-syntax here, to detect non-portable shell syntax early.\n>\n> Signed-off-by: Torsten Bögershausen \n> ---\n\nAs I said already, I do not want to do this yet without further\nreduction of false positives.\n\nAddition of the shell script test was a good starting point, but as\nit stands, it still is too brittle and will trigger on something\neven trivially innouous, like this:\n\n\ttest_expect_success 'no issues' '\n \tcat >test.file <<-\\EOF &&\n which is the right way?\n EOF\n '\n\n> t/Makefile | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/Makefile b/t/Makefile\n> index 1923cc1..6fa2b80 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -13,7 +13,7 @@ TAR ?= $(TAR)\n> RM ?= rm -f\n> PROVE ?= prove\n> DEFAULT_TEST_TARGET ?= test\n> -TEST_LINT ?= test-lint-duplicates test-lint-executable\n> +TEST_LINT ?= test-lint-duplicates test-lint-executable test-lint-shell-syntax\n> \n> # Shell quote;\n> SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n"},{"id":"206648","messageId":"50F28BB5.9080607@web.de","threadId":"32603","inReplyTo":"7vvcb37xfe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-13T10:25:57Z","receivedAt":"2013-01-13T10:25:57Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 12.01.13 07:00, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n> \n>> The test Makefile has a default set of lint tests which are run\n>> as part of \"make test\".\n>>\n>> The macro TEST_LINT defaults to \"test-lint-duplicates test-lint-executable\".\n>>\n>> Add test-lint-shell-syntax here, to detect non-portable shell syntax early.\n>>\n>> Signed-off-by: Torsten Bögershausen \n>> ---\n> \n> As I said already, I do not want to do this yet without further\n> reduction of false positives.\nWhich reinds me that the expression fishing for \"which\" is really poor.\n\nHow about something like the following:\n\n-- >8 --\nSubject: [PATCH] Reduce false positive in check-non-portable-shell.pl\n\ncheck-non-portable-shell.pl is using simple regular expressions to\nfind illegal shell syntax.\nImprove the expressions and reduce the chance for false positves:\n\n\"sed -i\" must be followed by 1..n whitespace and 1 non whitespace\n\"declare\" must be followed by 1..n whitespace and 1 non whitespace\n\"echo -n\" must be followed by 1..n whitespace and 1 non whitespace\n\"which\" must be followed by 1..n whitespace, a string, \"end of line\"\n\n\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex 8b5a71d..7151dd6 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -16,10 +16,10 @@ sub err {\n \n while (<>) {\n \tchomp;\n-\t/^\\s*sed\\s+-i/ and err 'sed -i is not portable';\n-\t/^\\s*echo\\s+-n/ and err 'echo -n is not portable (please use printf)';\n-\t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n-\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n+\t/^\\s*sed\\s+-i\\s+\\S/ and err 'sed -i is not portable';\n+\t/^\\s*echo\\s+-n\\s+\\S/ and err 'echo -n is not portable (please use printf)';\n+\t/^\\s*declare\\s+\\S/ and err 'arrays/declare not portable';\n+\t/^\\s*[^#]\\s*which\\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';\n \t/test\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n"},{"id":"206699","messageId":"20130113165037.GA5118@ftbfs.org","threadId":"32603","inReplyTo":"50F28BB5.9080607@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-01-13T16:50:37Z","receivedAt":"2013-01-13T16:50:37Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"On Sun, Jan 13, 2013 at 11:25:57AM +0100, Torsten Bögershausen wrote:\n> @@ -16,10 +16,10 @@ sub err {\n> \n> while (<>) {\n> \tchomp;\n> -\t/^\\s*sed\\s+-i/ and err 'sed -i is not portable';\n> -\t/^\\s*echo\\s+-n/ and err 'echo -n is not portable (please use printf)';\n> -\t/^\\s*declare\\s+/ and err 'arrays/declare not portable';\n> -\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n> +\t/^\\s*sed\\s+-i\\s+\\S/ and err 'sed -i is not portable';\n> +\t/^\\s*echo\\s+-n\\s+\\S/ and err 'echo -n is not portable (please use printf)';\n> +\t/^\\s*declare\\s+\\S/ and err 'arrays/declare not portable';\n> +\t/^\\s*[^#]\\s*which\\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';\n\nThe \"[^#]\" appears to ensure that there's at least one character\nbefore the which and that it's not a pound sign. Why is this done?\nWhy isn't it done for the other commands?\n"},{"id":"206702","messageId":"20130113173207.GC5973@elie.Belkin","threadId":"32603","inReplyTo":"50F28BB5.9080607@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-13T17:32:07Z","receivedAt":"2013-01-13T17:32:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTorsten Bögershausen wrote:\n\n> -\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n> +\t/^\\s*[^#]\\s*which\\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';\n\nHmm. Neither the old version nor the new one matches what seem to\nbe typical uses of 'which', based on a quick code search:\n\n\tif which sl >/dev/null 2>&1\n\tthen\n\t\tsl -l\n\t\t...\n\tfi\n\nor\n\n\tif test -x \"$(which sl 2>/dev/null)\"\n\tthen\n\t\tsl -l\n\t\t...\n\tfi\n"},{"id":"206715","messageId":"7v4nikiu81.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"20130113173207.GC5973@elie.Belkin","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-13T22:38:54Z","receivedAt":"2013-01-13T22:38:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Hi,\n>\n> Torsten Bögershausen wrote:\n>\n>> -\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n>> +\t/^\\s*[^#]\\s*which\\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';\n>\n> Hmm. Neither the old version nor the new one matches what seem to\n> be typical uses of 'which', based on a quick code search:\n>\n> \tif which sl >/dev/null 2>&1\n> \tthen\n> \t\tsl -l\n> \t\t...\n> \tfi\n>\n> or\n>\n> \tif test -x \"$(which sl 2>/dev/null)\"\n> \tthen\n> \t\tsl -l\n> \t\t...\n> \tfi\n\nYes, these two misuses are what we want it to trigger on, so the\ntest is very easy to trigger and produce a false positive, but does\nnot trigger on what we really want to catch.\n\nThat does not sound like a good benefit/cost ratio to me.\n"},{"id":"206970","messageId":"50F5B83E.9060800@web.de","threadId":"32603","inReplyTo":"7v4nikiu81.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-15T20:12:46Z","receivedAt":"2013-01-15T20:12:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 13.01.13 23:38, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n> \n>> Hi,\n>>\n>> Torsten Bögershausen wrote:\n>>\n>>> -\t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n>>> +\t/^\\s*[^#]\\s*which\\s+[-a-zA-Z0-9]+$/ and err 'which is not portable (please use type)';\n>>\n>> Hmm. Neither the old version nor the new one matches what seem to\n>> be typical uses of 'which', based on a quick code search:\n>>\n>> \tif which sl >/dev/null 2>&1\n>> \tthen\n>> \t\tsl -l\n>> \t\t...\n>> \tfi\n>>\n>> or\n>>\n>> \tif test -x \"$(which sl 2>/dev/null)\"\n>> \tthen\n>> \t\tsl -l\n>> \t\t...\n>> \tfi\n> \n> Yes, these two misuses are what we want it to trigger on, so the\n> test is very easy to trigger and produce a false positive, but does\n> not trigger on what we really want to catch.\n> \n> That does not sound like a good benefit/cost ratio to me.\n> \nThanks for comments, I think writing a regexp for which is difficult.\nWhat do we think about something like this for fishing for which:\n\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -644,6 +644,10 @@ yes () {\n :\n done\n }\n+which () {\n+ echo >&2 \"which is not portable (please use type)\"\n+ exit 1\n+}\n\n\nThis will happen in runtime, which might be good enough ?\n\n\n@Matt:\n>The \"[^#]\" appears to ensure that there's at least one character\n>before the which and that it's not a pound sign. Why is this done?\nThis is simply wrong.\n"},{"id":"206973","messageId":"7vk3re2ncb.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"50F5B83E.9060800@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T20:38:44Z","receivedAt":"2013-01-15T20:38:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> What do we think about something like this for fishing for which:\n>\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -644,6 +644,10 @@ yes () {\n> :\n> done\n> }\n> +which () {\n> + echo >&2 \"which is not portable (please use type)\"\n> + exit 1\n> +}\n>\n>\n> This will happen in runtime, which might be good enough ?\n\n\tif (\n\t\twhich frotz &&\n test $(frobonitz --version\" -le 2.0\n\t )\n then\n\t\ttest_set_prereq FROTZ_FROBONITZ\n\telse\n\t\techo >&2 \"suitable Frotz/Frobonitz combo not available;\"\n echo >&2 \"some tests may be skipped\"\n\tfi\n\nI somehow think this is a lost cause.\n"},{"id":"207867","messageId":"51037E5F.8090506@web.de","threadId":"32603","inReplyTo":"7vk3re2ncb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-26T06:57:35Z","receivedAt":"2013-01-26T06:57:35Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 15.01.13 21:38, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n> \n>> What do we think about something like this for fishing for which:\n>>\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -644,6 +644,10 @@ yes () {\n>> :\n>> done\n>> }\n>> +which () {\n>> + echo >&2 \"which is not portable (please use type)\"\n>> + exit 1\n>> +}\n>>\n>>\n>> This will happen in runtime, which might be good enough ?\n> \n> \tif (\n> \t\twhich frotz &&\n> test $(frobonitz --version\" -le 2.0\n> \t )\n> then\n> \t\ttest_set_prereq FROTZ_FROBONITZ\n> \telse\n> \t\techo >&2 \"suitable Frotz/Frobonitz combo not available;\"\n> echo >&2 \"some tests may be skipped\"\n> \tfi\n> \n> I somehow think this is a lost cause.\n\nI found different ways to detect if frotz is installed in the test suite:\na) use \"type\" (Should be the fastest ?)\nb) call the command directly, check the exit code\nc) \"! test_have_prereq\" (easy to understand, propably most expensive ?)\n\nDo we really need \"which\" to detect if frotz is installed?\n\n\n/Torsten\n\n\n\n=============\nif ! type cvs >/dev/null 2>&1\nthen\n\tskip_all='skipping cvsimport tests, cvs not found'\n\ttest_done\nfi\n\n===========\nif test -n \"$BASH\" && test -z \"$POSIXLY_CORRECT\"; then\n\t# we are in full-on bash mode\n\ttrue\nelif type bash >/dev/null 2>&1; then\n\t# execute in full-on bash mode\n\tunset POSIXLY_CORRECT\n\texec bash \"$0\" \"$@\"\nelse\n\techo '1..0 #SKIP skipping bash completion tests; bash not available'\n\texit 0\nfi\n===============\nsvn >/dev/null 2>&1\nif test $? -ne 1\nthen\n skip_all='skipping git svn tests, svn not found'\n test_done\nfi\n===============\n( p4 -h && p4d -h ) >/dev/null 2>&1 || {\n\tskip_all='skipping git p4 tests; no p4 or p4d'\n\ttest_done\n}\n===============\nif ! test_have_prereq PERL; then\n\tskip_all='skipping gitweb tests, perl not available'\n\ttest_done\nfi\n"},{"id":"207892","messageId":"7v1ud71uys.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"51037E5F.8090506@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-26T21:43:23Z","receivedAt":"2013-01-26T21:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Do we really need \"which\" to detect if frotz is installed?\n\nI think we all know the answer to that question is no, but why is\nthat a relevant question in the context of this discussion? One of\nus may be very confused.\n\nI thought the topic of this discussion was that, already knowing\nthat \"which\" should never be used anywhere in our scripts, you are\ntrying to devise a mechanical way to catch newcomers' attempts to\nuse it in their changes, in order to prevent patches that add use of\n\"which\" to be sent for review to waste our time. I was illustrating\nthat the approach to override \"which\" in a shell function for test\nscripts will not be a useful solution for that goal.\n"},{"id":"207938","messageId":"5104DAB2.5000606@web.de","threadId":"32603","inReplyTo":"7v1ud71uys.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-27T07:43:46Z","receivedAt":"2013-01-27T07:43:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 26.01.13 22:43, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n>> Do we really need \"which\" to detect if frotz is installed?\n> I think we all know the answer to that question is no, but why is\n> that a relevant question in the context of this discussion? One of\n> us may be very confused.\n>\n> I thought the topic of this discussion was that, already knowing\n> that \"which\" should never be used anywhere in our scripts, you are\n> trying to devise a mechanical way to catch newcomers' attempts to\n> use it in their changes, in order to prevent patches that add use of\n> \"which\" to be sent for review to waste our time. I was illustrating\n> that the approach to override \"which\" in a shell function for test\n> scripts will not be a useful solution for that goal.\nYes, the diskussion went away.\n\nI would rather see the check-non-portable-shell.pl enabled per default.\n\nIt looks as if the \"which\" command is hard to find, when we want a minimal\nrisk of false positves.\nI can see different solutions:\n\n1) We can make a much simpler expression which only catches the most\ncommon usage of which, like\n\"if whitch foo\".\n\nThis will not catch lines like\n\nif test -x \"$(which foo 2>/dev/null)\"\n\nBut I think the -x is not a useful anyway, because which should only\nlist command which have the executable bit set.\n\n2) We drop the which from check-non-portable-shell.pl\n\nI'll send a patch for 1)\n/Torsten\n"},{"id":"207943","messageId":"20130127093121.GA4228@elie.Belkin","threadId":"32603","inReplyTo":"51037E5F.8090506@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-27T09:31:21Z","receivedAt":"2013-01-27T09:31:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nTorsten Bögershausen wrote:\n> On 15.01.13 21:38, Junio C Hamano wrote:\n>> Torsten Bögershausen writes:\n\n>>> What do we think about something like this for fishing for which:\n[...]\n>>> +which () {\n>>> + echo >&2 \"which is not portable (please use type)\"\n>>> + exit 1\n>>> +}\n[...]\n>> \tif (\n>> \t\twhich frotz &&\n>> test $(frobonitz --version\" -le 2.0\n>> \t )\n\nWith the above definition of \"which\", the only sign of a mistake would\nbe some extra output to stderr (which is quelled when running tests in\nthe normal way). The \"exit\" is caught by the subshell and just makes\nthe \"if\" condition false.\n\nThat's not so terrible --- it could still dissuade new test authors\nfrom using \"which\". The downside I'd worry about is that it provides\na false sense of security despite not catching problems like\n\n\twrite_script x <<-EOF &&\n\t\t# Use \"foo\" if possible. Otherwise use \"bar\".\n\t\tif which foo && test $(foo --version) -le 2.0\n\t\tthen\n\t\t\t...\n\t\t...\n\tEOF\n\t./x\n\nThat's not a great tradeoff relative to the impact of the problem\nbeing solved.\n\nDon't get me wrong. I really do want to see more static or dynamic\nanalysis of git's shell scripts in the future. I fear that for the\ntradeoffs to make sense, though, the analysis needs to be more\nsophisticated:\n\n * A very common error in test scripts is leaving out the \"&&\"\n connecting adjacent statements, which causes early errors\n in a test assertion to be missed and tests to pass by mistake.\n Unfortunately the grammar of the dialect of shell used in tests is\n not regular enough to make this easily detectable using regexps.\n\n * Another common mistake is using \"cd\" without entering a subshell.\n Detecting this requires counting nested parentheses and noticing\n when a parenthesis is quoted.\n\n * Another common mistake is relying on the semantics of variable\n assignments in front of function calls. Detecting this requires\n recognizing which commands are function calls.\n\nIn the end the analysis that works best would probably involve a\nfull-fledged shell script parser. Something like \"sparse\", except for\nshell command language.\n\nSorry I don't have more practical advice in the short term.\n\nMy two cents,\nJonathan\n"},{"id":"207952","messageId":"5105280A.80002@web.de","threadId":"32603","inReplyTo":"20130127093121.GA4228@elie.Belkin","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-27T13:13:46Z","receivedAt":"2013-01-27T13:13:46Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 27.01.13 10:31, Jonathan Nieder wrote:\n> Hi,\n> \n> Torsten Bögershausen wrote:\n>> On 15.01.13 21:38, Junio C Hamano wrote:\n>>> Torsten Bögershausen writes:\n> \n>>>> What do we think about something like this for fishing for which:\n> [...]\n>>>> +which () {\n>>>> + echo >&2 \"which is not portable (please use type)\"\n>>>> + exit 1\n>>>> +}\n> [...]\n>>> \tif (\n>>> \t\twhich frotz &&\n>>> test $(frobonitz --version\" -le 2.0\n>>> \t )\n> \n> With the above definition of \"which\", the only sign of a mistake would\n> be some extra output to stderr (which is quelled when running tests in\n> the normal way). The \"exit\" is caught by the subshell and just makes\n> the \"if\" condition false.\n> \n> That's not so terrible --- it could still dissuade new test authors\n> from using \"which\". The downside I'd worry about is that it provides\n> a false sense of security despite not catching problems like\n> \n> \twrite_script x <<-EOF &&\n> \t\t# Use \"foo\" if possible. Otherwise use \"bar\".\n> \t\tif which foo && test $(foo --version) -le 2.0\n> \t\tthen\n> \t\t\t...\n> \t\t...\n> \tEOF\n> \t./x\n> \n> That's not a great tradeoff relative to the impact of the problem\n> being solved.\n> \n> Don't get me wrong. I really do want to see more static or dynamic\n> analysis of git's shell scripts in the future. I fear that for the\n> tradeoffs to make sense, though, the analysis needs to be more\n> sophisticated:\n> \n> * A very common error in test scripts is leaving out the \"&&\"\n> connecting adjacent statements, which causes early errors\n> in a test assertion to be missed and tests to pass by mistake.\n> Unfortunately the grammar of the dialect of shell used in tests is\n> not regular enough to make this easily detectable using regexps.\n> \n> * Another common mistake is using \"cd\" without entering a subshell.\n> Detecting this requires counting nested parentheses and noticing\n> when a parenthesis is quoted.\n> \n> * Another common mistake is relying on the semantics of variable\n> assignments in front of function calls. Detecting this requires\n> recognizing which commands are function calls.\n> \n> In the end the analysis that works best would probably involve a\n> full-fledged shell script parser. Something like \"sparse\", except for\n> shell command language.\n> \n> Sorry I don't have more practical advice in the short term.\n> \n> My two cents,\n> Jonathan\n\nJonathan,\nthanks for the review.\n\nMy ambition is to get the check-non-portable-shell.pl into a shape\nthat we can enable it by default.\n\nThis may be with or without checking for \"which\".\nAs a first step we will hopefully see less breakage for e.g. Mac OS\ncaused by \"echo -n\" or \"sed -i\" usage.\n\nOn the longer run, we may be able to have something more advanced.\n\nBack to the which:\n\nWriting a t0009-no-which.sh like this:\n--------------------\n#!/bin/sh\ntest_description='Test the which'\n\n. ./test-lib.sh\n\nwhich () {\n echo >&2 \"which is not portable (please use type)\"\n exit 1\n}\n\ntest_expect_success 'which is not portable' '\n\tif which frotz\n\tthen\n\t\tsay \"frotz does not exist\"\n\telse\n\t\tsay \"frotz does exist\"\n\tfi\n\n'\ntest_done\n--------------\nand running \"make test\" gives the following, at least in my system:\n\n[snip]\n\n*** t0009-no-which.sh ***\nFATAL: Unexpected exit with code 1\nmake[2]: *** [t0009-no-which.sh] Error 1\nmake[1]: *** [test] Error 2\nmake: *** [test] Error 2\n\n-----------------------\nrunning inside t/\n./t0009-no-which.sh --verbose\n\nInitialized empty Git repository in /Users/tb/projects/git/tb/t/trash directory.t0009-no-which/.git/\nexpecting success: \n if which frotz\n then\n say \"frotz does not exist\"\n else\n say \"frotz does exist\"\n fi\n\n\nwhich is not portable (please use type)\nFATAL: Unexpected exit with code 1\n\n/Torsten\n"},{"id":"207962","messageId":"7vham2y2bs.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"20130127093121.GA4228@elie.Belkin","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T17:15:35Z","receivedAt":"2013-01-27T17:15:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> ...\n> With the above definition of \"which\", the only sign of a mistake would\n> be some extra output to stderr (which is quelled when running tests in\n> the normal way). The \"exit\" is caught by the subshell and just makes\n> the \"if\" condition false.\n>\n> That's not so terrible --- it could still dissuade new test authors\n> from using \"which\". The downside I'd worry about is that it provides\n> a false sense of security despite not catching problems ...\n> ...\n> In the end the analysis that works best would probably involve a\n> full-fledged shell script parser. Something like \"sparse\", except for\n> shell command language.\n\nExactly.\n\nThat is why I keep saying that whole test-lint-shell-syntax should\nstay outside the default until it gets more robust by becoming a\nreasonable shell parser; it may not necessarily have to become\n\"full\" parser though.\n\nAs we discourage the use of tricky features of the language like\neval in individual test scripts to implement their own mini test\nframework, the \"something like sparse\" parser may initialy start\nsmall and still be useful; for example it can learn to exclude\nanything inside < writes:\n\n> Back to the which:\n> ...\n> and running \"make test\" gives the following, at least in my system:\n> ...\n\nI think everybody involved in this discussion already knows that;\nthe point is that it can easily give false negative, without the\nscripts working very hard to do so.\n\nIf we did not care about incurring runtime performance cost, we\ncould arrange:\n\n - the test framework to define a variable $TEST_ABORT that has a\n full path to a file that is in somewhere test authors cannot\n touch unless they really try hard to (i.e. preferrably outside\n $TRASH_DIRECTORY, as it is not uncommon for to tests to do \"rm *\"\n there). This location should be per $(basename \"$0\" .sh) to allow\n running multiple tests in paralell;\n\n - the test framework to \"rm -f $TEST_ABORT\" at the beginning of\n test_expect_success/failure;\n\n - test_expect_success/failure to check $TEST_ABORT and if it\n exists, abort the execution, showing the contents of the file as\n an error message.\n\nThen you can wrap commands whose use we want to limit, perhaps like\nthis, in the test framework:\n\n\twhich () {\n\t\tcat >\"$TEST_ABORT\" <<-\\EOF\n\t\tDo not use unportable 'which' in the test script.\n \"if type $cmd\" is a good way to see if $cmd exists.\n\t\tEOF\n\t}\n\n\tsed () {\n\t\tsaw_wantarg= must_abort=\n for arg\n do\n\t\t\tif test -n \"$saw_wantarg\"\n then\n\t\t\t\tsaw_wantarg=\n continue\n\t\t\tfi\n\t\t\tcase \"$arg\" in\n\t\t\t--)\tbreak ;; # end of options\n\t\t\t-i)\techo >\"$TEST_ABORT\" \"Do not use 'sed -i'\"\n\t\t\t\tmust_abort=yes\n\t\t\t\tbreak ;;\n -e)\tsaw_wantarg=yes ;; # skip next arg\n\t\t\t-*)\tcontinue ;; # options without arg\n\t\t\t*)\tbreak ;; # filename\n\t\t\tesac\n\t\tdone\n\t\tif test -z \"$must_abort\"\n\t\t\tsed \"$@\"\n\t\tfi\n\t}\n\nThen you can check that TEST_ABORT does not appear in test scripts\n(ensuring that they do not attempt to circumvent the mechanis) and\ncatch use of unwanted commands or unwanted extended features of\ncommands at runtime.\n\nBut this will incur runtime performace hit, so I am not sure it\nwould be worth it.\n"},{"id":"207981","messageId":"7vip6iv0eh.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"7v4ni2y1fm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-27T20:25:26Z","receivedAt":"2013-01-27T20:25:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we did not care about incurring runtime performance cost, we\n> could arrange:\n> ...\n> Then you can wrap commands whose use we want to limit, perhaps like\n> this, in the test framework:\n> ...\n> \tsed () {\n> ...\n> \t\tdone\n> \t\tif test -z \"$must_abort\"\n> \t\t\tsed \"$@\"\n> \t\tfi\n> \t}\n\nOf course, aside from missing \"then\", this needs to use the\nreal \"sed\", so this has to be\n\n\tif test -z \"$must_abort\"\n then\n\t\tcommand sed \"$@\"\n\tfi\n\nor something like that.\n\nAn approach along this line may reduce both the false negatives and\nfalse positives down to an acceptable level, but I doubt the result\nwould be efficient enough for us to tolerate the runtime penalty.\n"},{"id":"208760","messageId":"51116D3E.3070409@web.de","threadId":"32603","inReplyTo":"7v4ni2y1fm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-02-05T20:36:14Z","receivedAt":"2013-02-05T20:36:14Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 27.01.13 18:34, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n>\n>> Back to the which:\n>> ...\n>> and running \"make test\" gives the following, at least in my system:\n>> ...\n> I think everybody involved in this discussion already knows that;\n> the point is that it can easily give false negative, without the\n> scripts working very hard to do so.\n>\n> If we did not care about incurring runtime performance cost, we\n> could arrange:\n>\n> - the test framework to define a variable $TEST_ABORT that has a\n> full path to a file that is in somewhere test authors cannot\n> touch unless they really try hard to (i.e. preferrably outside\n> $TRASH_DIRECTORY, as it is not uncommon for to tests to do \"rm *\"\n> there). This location should be per $(basename \"$0\" .sh) to allow\n> running multiple tests in paralell;\n>\n> - the test framework to \"rm -f $TEST_ABORT\" at the beginning of\n> test_expect_success/failure;\n>\n> - test_expect_success/failure to check $TEST_ABORT and if it\n> exists, abort the execution, showing the contents of the file as\n> an error message.\n>\n> Then you can wrap commands whose use we want to limit, perhaps like\n> this, in the test framework:\n>\n> \twhich () {\n> \t\tcat >\"$TEST_ABORT\" <<-\\EOF\n> \t\tDo not use unportable 'which' in the test script.\n> \"if type $cmd\" is a good way to see if $cmd exists.\n> \t\tEOF\n> \t}\n>\n> \tsed () {\n> \t\tsaw_wantarg= must_abort=\n> for arg\n> do\n> \t\t\tif test -n \"$saw_wantarg\"\n> then\n> \t\t\t\tsaw_wantarg=\n> continue\n> \t\t\tfi\n> \t\t\tcase \"$arg\" in\n> \t\t\t--)\tbreak ;; # end of options\n> \t\t\t-i)\techo >\"$TEST_ABORT\" \"Do not use 'sed -i'\"\n> \t\t\t\tmust_abort=yes\n> \t\t\t\tbreak ;;\n> -e)\tsaw_wantarg=yes ;; # skip next arg\n> \t\t\t-*)\tcontinue ;; # options without arg\n> \t\t\t*)\tbreak ;; # filename\n> \t\t\tesac\n> \t\tdone\n> \t\tif test -z \"$must_abort\"\n> \t\t\tsed \"$@\"\n> \t\tfi\n> \t}\n>\n> Then you can check that TEST_ABORT does not appear in test scripts\n> (ensuring that they do not attempt to circumvent the mechanis) and\n> catch use of unwanted commands or unwanted extended features of\n> commands at runtime.\n>\n> But this will incur runtime performace hit, so I am not sure it\n> would be worth it.\nThanks for the detailed suggestion.\nInstead of using a file for putting out non portable syntax,\ncan we can use a similar logic as test_failure ?\n\nI did some benchmarking, the test suite on a Laptop is 37..38 minutes,\nincluding make clean && make both on next, pu, master or with the patch below.\nI couldn't find a measurable impact on the execution time.\nWhat do we think about a patch like this\n(Not sure if this cut-and-paste data applies, it's for review)\n\n\n[PATCH] test-lib: which, echo -n and sed -i are not portable\n\nThe posix version of sed command supports options -n -e -f\nThe gnu extension -i to edit a file in place is not\navailable on all systems.\nTo catch the usage of non-posix options with sed a\nwrapper function is added in test-lib.sh.\nThe wrapper checks that only -n -e -f are used.\nThe short form \"-ne\" for \"-n -e\" is allowed as well.\n\necho -n is not portable and not available on all systems,\nprintf can be used instead.\nAdd a wrapper to catch echo -n\n\nwhich is not portable, the output differs between different\nimplementations, and the return code may not be reliable.\n\nAdd a function test_bad_syntax_ in test-lib.sh, which increments\nthe variable test_bad_syntax and works similar to test_failure_\n\nSigned-off-by: Torsten Bögershausen \n---\n t/test-lib.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1a6c4ab..248ed34 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -266,6 +266,7 @@ else\n exec 4>/dev/null 3>/dev/null\n fi\n \n+test_bad_syntax=0\n test_failure=0\n test_count=0\n test_fixed=0\n@@ -300,6 +301,12 @@ test_ok_ () {\n say_color \"\" \"ok $test_count - $@\"\n }\n \n+test_bad_syntax_ () {\n+ test_bad_syntax=$(($test_bad_syntax + 1))\n+ say_color error \"$@\"\n+ test \"$immediate\" = \"\" || { GIT_EXIT_OK=t; exit 1; }\n+}\n+\n test_failure_ () {\n test_failure=$(($test_failure + 1))\n say_color error \"not ok $test_count - $1\"\n@@ -402,10 +409,15 @@ test_done () {\n fixed $test_fixed\n broken $test_broken\n failed $test_failure\n+ bad_syntax $test_bad_syntax\n \n EOF\n fi\n \n+ if test \"$test_bad_syntax\" != 0\n+ then\n+ say_color error \"# $test_bad_syntax non portable shell syntax\"\n+ fi\n if test \"$test_fixed\" != 0\n then\n say_color error \"# $test_fixed known breakage(s) vanished; please update test(s)\"\n@@ -645,6 +657,40 @@ yes () {\n done\n }\n \n+\n+# which is not portable\n+which () {\n+ test_bad_syntax_ \"Do not use unportable 'which' in the test script.\" \\\n+ \"'if type $1' is a good way to see if '$1' exists.\"\n+ return 1\n+}\n+\n+# catch non-portable usage of sed\n+sed () {\n+ for arg\n+ do\n+ case \"$arg\" in\n+ -[efn]) continue ;; # allowed posix options\n+ -ne) continue ;; # tolerated\n+ -*)test_bad_syntax_ \"Do not use 'sed \"$arg\"'. Valid options for 'sed' are -n -e -f\"\n+ return 1\n+ ;;\n+ *) continue ;;\n+ esac\n+ done\n+ command sed \"$@\"\n+}\n+\n+# catch non portable echo -n\n+echo () {\n+ if test \"$1\" = -n\n+ then\n+ test_bad_syntax_ \"Do not use 'echo -n'. Use printf instead\"\n+ else\n+ command echo \"$@\"\n+ fi\n+}\n+\n # Fix some commands on Windows\n case $(uname -s) in\n *MINGW*)\n-- \n1.8.1.1\n"},{"id":"208763","messageId":"7v4nhqsctb.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"51116D3E.3070409@web.de","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T20:52:48Z","receivedAt":"2013-02-05T20:52:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Thanks for the detailed suggestion.\n> Instead of using a file for putting out non portable syntax,\n> can we can use a similar logic as test_failure ?\n\nYour test_bad_syntax_ function can be called from a subshell, and\nits \"exit 1\" will not exit, no?\n\n\ttest_expect_success 'prepare a blob with incomplete line' '\n\t\t(\n\t\t\techo first line\n echo second line\n\t\t\techo -n final and incomplete line\n\t\t) >incomplete.txt\n\t'\n"},{"id":"208769","messageId":"7vwqumqvav.fsf@alter.siamese.dyndns.org","threadId":"32603","inReplyTo":"7v4nhqsctb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: turn on test-lint-shell-syntax by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-05T21:56:24Z","receivedAt":"2013-02-05T21:56:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Torsten Bögershausen writes:\n>\n>> Thanks for the detailed suggestion.\n>> Instead of using a file for putting out non portable syntax,\n>> can we can use a similar logic as test_failure ?\n>\n> Your test_bad_syntax_ function can be called from a subshell, and\n> its \"exit 1\" will not exit, no?\n\nWhat is more important is that the increment to $test_bad_syntax\ndone in that function will not be propagated up to the main process\nthat runs the test framework.\n\nOf course, that is why I mentioned communicating using the\nfilesystem.\n"}]}