{"thread":{"id":"32500","subject":"[PATCH 1/4] test: Add target test-lint-shell-syntax","startedAt":"2013-01-01T21:40:08Z","lastAt":"2013-01-08T04:11:23Z","messageCount":18,"participants":["Torsten Bögershausen","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"205787","messageId":"201301012240.10722.tboegi@web.de","threadId":"32500","inReplyTo":null,"subject":"[PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-01T21:40:08Z","receivedAt":"2013-01-01T21:40:08Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"Add the perl script \"check-non-portable-shell.pl\" to detect non-portable\nshell syntax\nMany systems use gnu tools which accept an extended syntax in shell scripts,\nwhich is not portable on all systems and causes the test suite to fail.\n\nTo prevent contributors using e.g. Linux to add non-portable test code,\n\"check-non-portable-shell.pl\" is run as part of\n\"make test\" or \"make in the t/ directory.\n\n\"echo -n\" is an example of a statement working on Linux,\nbut not on e.g. Mac OS X.\n\nBeside \"echo -n\" we check for\n\"sed -i\",\narrays in shell scripts (declare statement),\n\"which\" (use type instead),\nor \"==\" (bash style of =)\n\nSigned-off-by: Torsten Bögershausen \n---\n t/Makefile | 7 +++--\n t/check-non-portable-shell.pl | 67 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 72 insertions(+), 2 deletions(-)\n create mode 100755 t/check-non-portable-shell.pl\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 88e289f..7b0c4dc 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n \n all: $(DEFAULT_TEST_TARGET)\n \n-test: pre-clean $(TEST_LINT)\n+test: pre-clean test-lint-shell-syntax $(TEST_LINT)\n \t$(MAKE) aggregate-results-and-cleanup\n \n prove: pre-clean $(TEST_LINT)\n@@ -43,7 +43,7 @@ clean-except-prove-cache:\n clean: clean-except-prove-cache\n \t$(RM) .prove\n \n-test-lint: test-lint-duplicates test-lint-executable\n+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax\n \n test-lint-duplicates:\n \t@dups=`echo $(T) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n@@ -55,6 +55,9 @@ test-lint-executable:\n \t\ttest -z \"$$bad\" || { \\\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n+test-lint-shell-syntax:\n+\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n+\n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n \t$(MAKE) clean\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nnew file mode 100755\nindex 0000000..de62ef0\n--- /dev/null\n+++ b/t/check-non-portable-shell.pl\n@@ -0,0 +1,67 @@\n+#!/usr/bin/perl -w\n+######################################################################\n+# Test t0000..t9999.sh for non portable shell scripts #\n+# Examples are \"echo -n\" or \"sed -i\" #\n+# This script can be called with one or more filenames as parameters #\n+#\n+######################################################################\n+use strict;\n+my $exitcode=0;\n+\n+sub check_one_file($) {\n+\tmy $lineno=1;\n+\tmy $filename=shift;\n+\n+\topen(FINPUT, \"<$filename\") || die \"Couldn't open filename $filename\";\n+\tmy @fdata = ;\n+\tclose(FINPUT);\n+\n+\twhile (my $line = shift @fdata) {\n+ do {\n+ chomp $line;\n+ # sed -i\n+ if ($line =~ /^\\s*sed\\s+-i/) {\n+ printf(\"%s:%d:error: \\\"sed -i not portable\\\" %s\\n\", $filename, $lineno, $line);\n+\t\t\t\t$exitcode=1;\n+ }\n+ # echo -n\n+ if ($line =~ /^\\s*echo\\s+-n/) {\n+ printf(\"%s:%d:error: \\\"echo -n not portable\\\" %s\\n\", $filename, $lineno, $line);\n+\t\t\t\t$exitcode=1;\n+ }\n+ # arrays (declare statement)\n+ if ($line =~ /^\\s*declare\\s+/) {\n+ printf(\"%s:%d:error: \\\"arrays/declare not portable\\\" %s\\n\", $filename, $lineno, $line);\n+\t\t\t\t$exitcode=1;\n+ }\n+ # which\n+ if ($line =~ /^\\s*[^#]\\s*which\\s/) {\n+ printf(\"%s:%d:error: \\\"which is not portable (use type)\\\" %s\\n\", $filename, $lineno, $line);\n+\t\t\t\t$exitcode=1;\n+ }\n+\n+ # == (bash style comparison)\n+ if ($line =~ /test\\s+[^=]*==/) {\n+ printf(\"%s:%d:error: \\\"== is not portable (use =)\\\" %s\\n\", $filename, $lineno, $line);\n+\t\t\t\t$exitcode=1;\n+ }\n+\n+\t\t\t$lineno=$lineno+1;\n+\t\t}\n+ }\n+}\n+\n+\n+if ($#ARGV <= 0) {\n+\tprint STDERR \"$0: Check shell scripts for non portable syntax\\n\";\n+\tprint STDERR \"Example: $0 t[0-9]*.sh\\n\";\n+\n+\texit(2);\n+}\n+\n+while (@ARGV) {\n+\tmy $arg = shift @ARGV;\n+ check_one_file($arg);\n+}\n+\n+exit($exitcode);\n-- \n1.8.0.197.g5a90748\n"},{"id":"205798","messageId":"7v7gnw8slq.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"201301012240.10722.tboegi@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-01T22:07:45Z","receivedAt":"2013-01-01T22:07:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Add the perl script \"check-non-portable-shell.pl\" to detect non-portable\n> shell syntax\n> Many systems use gnu tools which accept an extended syntax in shell scripts,\n> which is not portable on all systems and causes the test suite to fail.\n>\n> To prevent contributors using e.g. Linux to add non-portable test code,\n> \"check-non-portable-shell.pl\" is run as part of\n> \"make test\" or \"make in the t/ directory.\n>\n> \"echo -n\" is an example of a statement working on Linux,\n> but not on e.g. Mac OS X.\n>\n> Beside \"echo -n\" we check for\n> \"sed -i\",\n> arrays in shell scripts (declare statement),\n> \"which\" (use type instead),\n> or \"==\" (bash style of =)\n>\n> Signed-off-by: Torsten Bögershausen \n\nWhat it checks looks like a good start, but the indentation of it\n(and the log message) seems very screwed up.\n\nI also have to wonder what's the false positive rate of this. When\nyou are preparing a new test, you would ideally want a mode that\nchecks only parts that you just added, without seeing noises from\nexisting violations and false positives from the part you did not\ntouch. Otherwise, it will be too cumbersome to run for developers,\nand the check mechanism will end up used by nobody.\n\n> +######################################################################\n> +# Test t0000..t9999.sh for non portable shell scripts #\n> +# Examples are \"echo -n\" or \"sed -i\" #\n> +# This script can be called with one or more filenames as parameters #\n> +#\n> +######################################################################\n\nJust a style thing (style requests are not optional, though ;-), but\nthese box comments are moderately annoying to read and extremely\nannoying to modify. Writing it like this:\n\n> +#\n> +# Test t0000..t9999.sh for non portable shell scripts\n> +# Examples are \"echo -n\" or \"sed -i\"\n> +# This script can be called with one or more filenames as parameters\n> +#\n\nshould be sufficiently loud.\n"},{"id":"205816","messageId":"50E37BD3.6060709@web.de","threadId":"32500","inReplyTo":"7v7gnw8slq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-02T00:14:11Z","receivedAt":"2013-01-02T00:14:11Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 01.01.13 23:07, Junio C Hamano wrote:\n[snip]\n> What it checks looks like a good start, but the indentation of it\n> (and the log message) seems very screwed up.\nOK\n> I also have to wonder what's the false positive rate of this. When\n> you are preparing a new test, you would ideally want a mode that\n> checks only parts that you just added, without seeing noises from\n> existing violations and false positives from the part you did not\n> touch. Otherwise, it will be too cumbersome to run for developers,\n> and the check mechanism will end up used by nobody.\n>\nThe script found all problems which make the testsuite (unecessary) fail on Mac OS X.\n\nThe false positive rate is currently 0% (otherwise I should not have send it to the list)\n\nThe suggestion is to run it every time the test suite is run, at the begining.\nAnd it seems to be fast enough:\n\n$ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh\nreal 0m0.263s\nuser 0m0.239s\nsys 0m0.021s\n\n/Torsten\n"},{"id":"205829","messageId":"7vk3rw5nng.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"50E37BD3.6060709@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T02:22:59Z","receivedAt":"2013-01-02T02:22:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> The suggestion is to run it every time the test suite is run, at the begining.\n> And it seems to be fast enough:\n>\n> $ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh\n> real 0m0.263s\n> user 0m0.239s\n> sys 0m0.021s\n\nHmph. OK.\n"},{"id":"205852","messageId":"20130102094635.GD9328@sigill.intra.peff.net","threadId":"32500","inReplyTo":"201301012240.10722.tboegi@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-02T09:46:36Z","receivedAt":"2013-01-02T09:46:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:\n\n> Add the perl script \"check-non-portable-shell.pl\" to detect non-portable\n> shell syntax\n\nCool. Thanks for adding more test-lint. But...\n\n> diff --git a/t/Makefile b/t/Makefile\n> index 88e289f..7b0c4dc 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n> \n> all: $(DEFAULT_TEST_TARGET)\n> \n> -test: pre-clean $(TEST_LINT)\n> +test: pre-clean test-lint-shell-syntax $(TEST_LINT)\n> \t$(MAKE) aggregate-results-and-cleanup\n\nI do not think it should be a hard-coded dependency of \"test\", as then\nthere is no way to turn it off. It would make more sense to me to set a\ndefault TEST_LINT that includes it, but could be overridden by the user.\n\n> prove: pre-clean $(TEST_LINT)\n> @@ -43,7 +43,7 @@ clean-except-prove-cache:\n> clean: clean-except-prove-cache\n> \t$(RM) .prove\n> \n> -test-lint: test-lint-duplicates test-lint-executable\n> +test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax\n\nThis, however, is right. The point of \"test-lint\" is \"use all the lint\",\nso adding it here makes sense (and anyone who has set \"TEST_LINT =\ntest-lint\" will get the new check).\n\n> +test-lint-shell-syntax:\n> +\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n\nThis is wrong if $(PERL_PATH) contains spaces, no? Doing \"$(PERL_PATH)\"\nis also wrong, because the expansion happens in 'make', and a\n$(PERL_PATH) with double-quotes would fool the shell. Since we export\n$PERL_PATH, I think doing:\n\n \"$$PERL_PATH\"\" check-non-portable-shell.pl $(T)\n\nwould be sufficient.\n\n> --- /dev/null\n> +++ b/t/check-non-portable-shell.pl\n> @@ -0,0 +1,67 @@\n> +#!/usr/bin/perl -w\n\nThis \"-w\" is ignored, since we execute by using $PERL_PATH. Maybe \"use\nwarnings\" instead?\n\n> +sub check_one_file($) {\n\nPerl subroutine prototypes are generally frowned on unless there is a\ngood reason to use them.\n\n> +\tmy $lineno=1;\n\nPerl keeps track of this for you in the \"$.\" variable.\n\n> +\tmy $filename=shift;\n\nAnd if you use the automagic \"<>\" handle, this is already in $ARGV (but\nnote that you need to do a little bit of magic to make that work with\n$.; see the entry for \"$.\" in perlvar, and \"eof\" in perlfunc).\n\n> +\topen(FINPUT, \"<$filename\") || die \"Couldn't open filename $filename\";\n> +\tmy @fdata = ;\n> +\tclose(FINPUT);\n> +\n> +\twhile (my $line = shift @fdata) {\n\nNot that our test scripts are so huge they won't fit into memory, but it\nis generally good practice to loop on the handle rather than reading all\nof the lines into an array.\n\n> + do {\n\nWhat's this do block for?\n\n> + # sed -i\n> + if ($line =~ /^\\s*sed\\s+-i/) {\n> + printf(\"%s:%d:error: \\\"sed -i not portable\\\" %s\\n\", $filename, $lineno, $line);\n> +\t\t\t\t$exitcode=1;\n> + }\n\nThese would be a lot more readable if the printf was pulled out into a\nhelper function. And you can avoid the escaped quotes by using perl's qq\noperator. E.g., qq(this string has \"quotes\" in it).\n\nAlso, putting a space before the \"error:\" matches what gcc outputs,\nwhich some editors (e.g., vim) can recognize and let the user jump\nstraight to the error.\n\n> +while (@ARGV) {\n> +\tmy $arg = shift @ARGV;\n> + check_one_file($arg);\n> +}\n\nYou can replace this with the magic <> filehandle.\n\n\nSo taking all of that, a more idiomatic perl script would look something\nlike:\n\n my $exit_code;\n sub err {\n my $msg = shift;\n print \"$ARGV:$.: error: $msg: $_\\n\";\n $exit_code = 1;\n }\n\n while (<>) {\n chomp;\n if (/^\\s*sed\\s+-i/) {\n err('sed -i is not portable');\n }\n # ...and so on\n\n # this resets our $. for each file\n close ARGV if eof;\n }\n exit $exit_code;\n\nI'd personally probably write the conditions like:\n\n /^\\s*sed\\s+-i/ and err('sed -i is not portable');\n\nto make the structure of the program (i.e., a list of conditions to\ncomplain about) clear, but I know not everybody agrees with such a terse\nstyle.\n\n-Peff\n"},{"id":"205859","messageId":"7vmwwr4kiq.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"20130102094635.GD9328@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-02T16:28:13Z","receivedAt":"2013-01-02T16:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So taking all of that, a more idiomatic perl script would look something\n> like:\n>\n> my $exit_code;\n> sub err {\n> my $msg = shift;\n> print \"$ARGV:$.: error: $msg: $_\\n\";\n> $exit_code = 1;\n> }\n>\n> while (<>) {\n> chomp;\n> if (/^\\s*sed\\s+-i/) {\n> err('sed -i is not portable');\n> }\n> # ...and so on\n>\n> # this resets our $. for each file\n> close ARGV if eof;\n> }\n> exit $exit_code;\n>\n> I'd personally probably write the conditions like:\n>\n> /^\\s*sed\\s+-i/ and err('sed -i is not portable');\n>\n> to make the structure of the program (i.e., a list of conditions to\n> complain about) clear, but I know not everybody agrees with such a terse\n> style.\n\nThanks for a nicely-done review.\n"},{"id":"205895","messageId":"50E4BF58.4090808@web.de","threadId":"32500","inReplyTo":"20130102094635.GD9328@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-02T23:14:32Z","receivedAt":"2013-01-02T23:14:32Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 02.01.13 10:46, Jeff King wrote:> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:\n> \n>> Add the perl script \"check-non-portable-shell.pl\" to detect non-portable\n>> shell syntax\n> \n> Cool. Thanks for adding more test-lint. But...\n> \n>> diff --git a/t/Makefile b/t/Makefile\n>> index 88e289f..7b0c4dc 100644\n>> --- a/t/Makefile\n>> +++ b/t/Makefile\n>> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n>> \n>> all: $(DEFAULT_TEST_TARGET)\n>> \n>> -test: pre-clean $(TEST_LINT)\n>> +test: pre-clean test-lint-shell-syntax $(TEST_LINT)\n>> \t$(MAKE) aggregate-results-and-cleanup\n> \n> I do not think it should be a hard-coded dependency of \"test\", as then\n> there is no way to turn it off. It would make more sense to me to set a\n> default TEST_LINT that includes it, but could be overridden by the user.\n> \n>> prove: pre-clean $(TEST_LINT)\n>> @@ -43,7 +43,7 @@ clean-except-prove-cache:\n>> clean: clean-except-prove-cache\n>> \t$(RM) .prove\n>> \n>> -test-lint: test-lint-duplicates test-lint-executable\n>> +test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax\n> \n> This, however, is right. The point of \"test-lint\" is \"use all the lint\",\n> so adding it here makes sense (and anyone who has set \"TEST_LINT =\n> test-lint\" will get the new check).\n> \n>> +test-lint-shell-syntax:\n>> +\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n> \n> This is wrong if $(PERL_PATH) contains spaces, no? Doing \"$(PERL_PATH)\"\n> is also wrong, because the expansion happens in 'make', and a\n> $(PERL_PATH) with double-quotes would fool the shell. Since we export\n> $PERL_PATH, I think doing:\n> \n> \"$$PERL_PATH\"\" check-non-portable-shell.pl $(T)\nThanks, but: \n- The double \"\" after PERL_PATH makes the string un-terminated.\n\n- Using \"$$PERL_PATH\" expands from make into \"$PERL_PATH\" on the command line\n\n- If the Makefile looks like this:\nPERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n[snip]\n$(PERL_PATH) check-non-portable-shell.pl $(T)\nThe command line will look like this:\n\"/Users/tb/projects/git/tb/pe rl\" check-non-portable-shell.pl t0000-basic.sh ...\n\nSo I think that PERL_PATH should be quoted when it is defined in the Makefile.\n\n[snip]\nPeff, many thanks. Please see V2 patch comming soon\n"},{"id":"205898","messageId":"20130102232239.GA27952@sigill.intra.peff.net","threadId":"32500","inReplyTo":"50E4BF58.4090808@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-02T23:22:39Z","receivedAt":"2013-01-02T23:22:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:\n\n> > This is wrong if $(PERL_PATH) contains spaces, no? Doing \"$(PERL_PATH)\"\n> > is also wrong, because the expansion happens in 'make', and a\n> > $(PERL_PATH) with double-quotes would fool the shell. Since we export\n> > $PERL_PATH, I think doing:\n> > \n> > \"$$PERL_PATH\"\" check-non-portable-shell.pl $(T)\n> Thanks, but: \n> - The double \"\" after PERL_PATH makes the string un-terminated.\n\nYeah, sorry, typo on my part.\n\n> - Using \"$$PERL_PATH\" expands from make into \"$PERL_PATH\" on the command line\n\nRight. That's what I intended.\n\n> - If the Makefile looks like this:\n> PERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n> [snip]\n> $(PERL_PATH) check-non-portable-shell.pl $(T)\n> The command line will look like this:\n> \"/Users/tb/projects/git/tb/pe rl\" check-non-portable-shell.pl t0000-basic.sh ...\n> \n> So I think that PERL_PATH should be quoted when it is defined in the Makefile.\n\nDoes a $PERL_PATH with quotes actually work?\n\nUsually in our runtime environment, commands that are handed to git are\nassumed to be passed directly to the shell, and you need to quote. E.g.,\nsetting diff.external to:\n\n [diff]\n external = \"foo --bar\"\n\nwill let the shell split the argument out; if you have a space, you\nwould want to set it like:\n\n [diff]\n external = \"'command with space'\"\n\nThis is the most flexible way to do it.\n\nHowever, for Makefile variables, I think we do not (and cannot) follow\nthe same rule. Notice that all of the uses of $PERL_PATH in the test\nsuite enclose it in quotes. Having extra quotes would break those\ninvocations. And the value of $PERL_PATH will be put on the #!-line,\nwhich cannot not be quoted.\n\n-Peff\n"},{"id":"205900","messageId":"50E4C9B5.8070308@web.de","threadId":"32500","inReplyTo":"20130102232239.GA27952@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-02T23:58:45Z","receivedAt":"2013-01-02T23:58:45Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03.01.13 00:22, Jeff King wrote:\n> On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:\n> \n>>> This is wrong if $(PERL_PATH) contains spaces, no? Doing \"$(PERL_PATH)\"\n>>> is also wrong, because the expansion happens in 'make', and a\n>>> $(PERL_PATH) with double-quotes would fool the shell. Since we export\n>>> $PERL_PATH, I think doing:\n>>>\n>>> \"$$PERL_PATH\"\" check-non-portable-shell.pl $(T)\n>> Thanks, but: \n>> - The double \"\" after PERL_PATH makes the string un-terminated.\n> \n> Yeah, sorry, typo on my part.\n> \n>> - Using \"$$PERL_PATH\" expands from make into \"$PERL_PATH\" on the command line\n> \n> Right. That's what I intended.\n> \n>> - If the Makefile looks like this:\n>> PERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n>> [snip]\n>> $(PERL_PATH) check-non-portable-shell.pl $(T)\n>> The command line will look like this:\n>> \"/Users/tb/projects/git/tb/pe rl\" check-non-portable-shell.pl t0000-basic.sh ...\n>>\n>> So I think that PERL_PATH should be quoted when it is defined in the Makefile.\n> \n> Does a $PERL_PATH with quotes actually work?\nAt least on my system the following combination works:\n\ngit diff\ndiff --git a/t/Makefile b/t/Makefile\nindex f8f8c54..391a5ca 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -8,7 +8,7 @@\n \n #GIT_TEST_OPTS = --verbose --debug\n SHELL_PATH ?= $(SHELL)\n-PERL_PATH ?= /usr/bin/perl\n+PERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\n\n~/projects/git/tb/t> ls -l \"/Users/tb/projects/git/tb/pe rl\"\nlrwxr-xr-x 1 tb staff 13 Jan 3 00:33 /Users/tb/projects/git/tb/pe rl -> /usr/bin/perl\n\n====================================================\nAnd this works as well, is that what you intended?\nNote: \"single Dollar\"\n====================================================\n git diff\ndiff --git a/t/Makefile b/t/Makefile\nindex f8f8c54..f624f95 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -8,7 +8,7 @@\n \n #GIT_TEST_OPTS = --verbose --debug\n SHELL_PATH ?= $(SHELL)\n-PERL_PATH ?= /usr/bin/perl\n+PERL_PATH = /Users/tb/projects/git/tb/pe rl\n TAR ?= $(TAR)\n RM ?= rm -f\n PROVE ?= prove\n@@ -57,7 +57,7 @@ test-lint-executable:\n echo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n test-lint-shell-syntax:\n- $(PERL_PATH) check-non-portable-shell.pl $(T)\n+ \"$(PERL_PATH)\" check-non-portable-shell.pl $(T)\n \n\n\n\n> Usually in our runtime environment, commands that are handed to git are\n> assumed to be passed directly to the shell, and you need to quote. E.g.,\n> setting diff.external to:\n> \n> [diff]\n> external = \"foo --bar\"\n> \n> will let the shell split the argument out; if you have a space, you\n> would want to set it like:\n> \n> [diff]\n> external = \"'command with space'\"\n> \n> This is the most flexible way to do it.\n> \n> However, for Makefile variables, I think we do not (and cannot) follow\n> the same rule. Notice that all of the uses of $PERL_PATH in the test\n> suite enclose it in quotes. Having extra quotes would break those\n> invocations. And the value of $PERL_PATH will be put on the #!-line,\n> which cannot not be quoted.\n> \n> -Peff\nI followed these lines as an example:\n\ntest-results/git-smoke.tar.gz: test-results\n\t$(PERL_PATH) ./harness \\\n\t\t--archive=\"test-results/git-smoke.tar.gz\" \\\n\t\t$(T)\n\n(and make smoke did not work, as we don't have ./harness :-(\n\nDo we need some cleanup/improvements here as well?\n\n/Torsten\n"},{"id":"205901","messageId":"7vtxqzyw0g.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"20130102094635.GD9328@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T00:01:51Z","receivedAt":"2013-01-03T00:01:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:\n>\n>> Add the perl script \"check-non-portable-shell.pl\" to detect non-portable\n>> shell syntax\n>\n> Cool. Thanks for adding more test-lint. But...\n>\n>> diff --git a/t/Makefile b/t/Makefile\n>> index 88e289f..7b0c4dc 100644\n>> --- a/t/Makefile\n>> +++ b/t/Makefile\n>> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh))\n>> \n>> all: $(DEFAULT_TEST_TARGET)\n>> \n>> -test: pre-clean $(TEST_LINT)\n>> +test: pre-clean test-lint-shell-syntax $(TEST_LINT)\n>> \t$(MAKE) aggregate-results-and-cleanup\n>\n> I do not think it should be a hard-coded dependency of \"test\", as then\n> there is no way to turn it off. It would make more sense to me to set a\n> default TEST_LINT that includes it, but could be overridden by the user.\n\nI would actually not add this to TEST_LINT by default, especially\nwhen \"duplicates\" and \"executable\" that are much simpler and less\nlikely to hit false positives are not on by default.\n\nAt least, a change to add this to TEST_LINT by default must wait to\nbe merged to any integration branch until all the fix-up topics that\nthis test triggers in the current codebase graduate to the branch.\n\n>> +test-lint-shell-syntax:\n>> +\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n>\n> This is wrong if $(PERL_PATH) contains spaces, no?\n\nYou are correct; \"harness\" thing in the same Makefile gets this\nwrong, too. I think the right invocation is:\n\n\t@'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)\n\nalthough I do not offhand know if that symbol is already exported by\nthe top-level Makefile.\n"},{"id":"205902","messageId":"7vpq1nyvp1.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"7vtxqzyw0g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T00:08:42Z","receivedAt":"2013-01-03T00:08:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I would actually not add this to TEST_LINT by default, especially\n> when \"duplicates\" and \"executable\" that are much simpler and less\n> likely to hit false positives are not on by default.\n>\n> At least, a change to add this to TEST_LINT by default must wait to\n> be merged to any integration branch until all the fix-up topics that\n> this test triggers in the current codebase graduate to the branch.\n>\n>>> +test-lint-shell-syntax:\n>>> +\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n>>\n>> This is wrong if $(PERL_PATH) contains spaces, no?\n>\n> You are correct; \"harness\" thing in the same Makefile gets this\n> wrong, too. I think the right invocation is:\n>\n> \t@'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)\n>\n> although I do not offhand know if that symbol is already exported by\n> the top-level Makefile.\n\nI'll tentatively queue this instead. The log message has also been\ncleaned up a bit.\n\n-- >8 --\nFrom: Torsten Bögershausen \nDate: Thu, 3 Jan 2013 00:20:19 +0100\nSubject: [PATCH] test: Add check-non-portable-shell.pl\n\nAdd the perl script \"check-non-portable-shell.pl\" to detect\nnon-portable shell syntax.\n\n\"echo -n\" is an example of a shell command working on Linux, but not\non Mac OS X.\n\nThese shell commands are checked and reported as error:\n\n - \"echo -n\" (printf should be used)\n - \"sed -i\" (GNUism; use a temp file instead)\n - \"declare\" (bashism, often used with arrays)\n - \"which\" (unreliable exit status and output; use type instead)\n - \"test a == b\" (bashism for \"test a = b\")\n\n\"make test-lint-shell-syntax\" can be used to run only the check.\n\nHelped-By: Jeff King \nSigned-off-by: Torsten Bögershausen \nSigned-off-by: Junio C Hamano \n---\n t/Makefile | 8 ++++++--\n t/check-non-portable-shell.pl | 27 +++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 2 deletions(-)\n create mode 100755 t/check-non-portable-shell.pl\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 3025418..a43becb 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -16,6 +16,7 @@ DEFAULT_TEST_TARGET ?= test\n \n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n+PERL_PATH_SQ = $(subst ','\\'',$(PERL_PATH))\n \n T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))\n TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))\n@@ -43,7 +44,7 @@ clean-except-prove-cache:\n clean: clean-except-prove-cache\n \t$(RM) .prove\n \n-test-lint: test-lint-duplicates test-lint-executable\n+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax\n \n test-lint-duplicates:\n \t@dups=`echo $(T) | tr ' ' '\\n' | sed 's/-.*//' | sort | uniq -d` && \\\n@@ -55,6 +56,9 @@ test-lint-executable:\n \t\ttest -z \"$$bad\" || { \\\n \t\techo >&2 \"non-executable tests:\" $$bad; exit 1; }\n \n+test-lint-shell-syntax:\n+\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\n+\n aggregate-results-and-cleanup: $(T)\n \t$(MAKE) aggregate-results\n \t$(MAKE) clean\n@@ -87,7 +91,7 @@ test-results:\n \tmkdir -p test-results\n \n test-results/git-smoke.tar.gz: test-results\n-\t$(PERL_PATH) ./harness \\\n+\t'$(PERL_PATH_SQ)' ./harness \\\n \t\t--archive=\"test-results/git-smoke.tar.gz\" \\\n \t\t$(T)\n \ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nnew file mode 100755\nindex 0000000..8b5a71d\n--- /dev/null\n+++ b/t/check-non-portable-shell.pl\n@@ -0,0 +1,27 @@\n+#!/usr/bin/perl\n+\n+# Test t0000..t9999.sh for non portable shell scripts\n+# This script can be called with one or more filenames as parameters\n+\n+use strict;\n+use warnings;\n+\n+my $exit_code=0;\n+\n+sub err {\n+\tmy $msg = shift;\n+\tprint \"$ARGV:$.: error: $msg: $_\\n\";\n+\t$exit_code = 1;\n+}\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/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+}\n+exit $exit_code;\n-- \n1.8.1.203.gc241474\n"},{"id":"205903","messageId":"7vlicbyvc2.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"50E4C9B5.8070308@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T00:16:29Z","receivedAt":"2013-01-03T00:16:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> At least on my system the following combination works:\n>\n> git diff\n> diff --git a/t/Makefile b/t/Makefile\n> index f8f8c54..391a5ca 100644\n> --- a/t/Makefile\n> +++ b/t/Makefile\n> @@ -8,7 +8,7 @@\n> \n> #GIT_TEST_OPTS = --verbose --debug\n> SHELL_PATH ?= $(SHELL)\n> -PERL_PATH ?= /usr/bin/perl\n> +PERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n\nI do not think that will fly. Having that in the main Makefile\nwhere the existing users of the symbol relies on it without any\nsurrounding quotes, e.g.\n\n$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE\n\t$(QUIET_GEN)$(RM) $@ $@+ && \\\n\tINSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \\\n\tsed -e '1{' \\\n\t -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n\t -e '\th' \\\n\t -e '\ts=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || \"'\"$$INSTLIBDIR\"'\"));=' \\\n\t -e '\tH' \\\n\t -e '\tx' \\\n\t -e '}' \\\n\t -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n\t $@.perl >$@+ && \\\n\tchmod +x $@+ && \\\n\tmv $@+ $@\n\nwhere $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH)\nwith '\\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to\nquote the value of PERL_PATH without quotes, your definition will\nlook for a relative path that is inside a directory named '\"'\n(that's a single double-quote).\n"},{"id":"205904","messageId":"50E4CF7E.9090302@web.de","threadId":"32500","inReplyTo":"7vlicbyvc2.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-03T00:23:26Z","receivedAt":"2013-01-03T00:23:26Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03.01.13 01:16, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n> \n>> At least on my system the following combination works:\n>>\n>> git diff\n>> diff --git a/t/Makefile b/t/Makefile\n>> index f8f8c54..391a5ca 100644\n>> --- a/t/Makefile\n>> +++ b/t/Makefile\n>> @@ -8,7 +8,7 @@\n>> \n>> #GIT_TEST_OPTS = --verbose --debug\n>> SHELL_PATH ?= $(SHELL)\n>> -PERL_PATH ?= /usr/bin/perl\n>> +PERL_PATH = \"/Users/tb/projects/git/tb/pe rl\"\n> \n> I do not think that will fly. Having that in the main Makefile\n> where the existing users of the symbol relies on it without any\n> surrounding quotes, e.g.\n> \n> $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE\n> \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n> \tINSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \\\n> \tsed -e '1{' \\\n> \t -e '\ts|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n> \t -e '\th' \\\n> \t -e '\ts=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || \"'\"$$INSTLIBDIR\"'\"));=' \\\n> \t -e '\tH' \\\n> \t -e '\tx' \\\n> \t -e '}' \\\n> \t -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \\\n> \t $@.perl >$@+ && \\\n> \tchmod +x $@+ && \\\n> \tmv $@+ $@\n> \n> where $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH)\n> with '\\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to\n> quote the value of PERL_PATH without quotes, your definition will\n> look for a relative path that is inside a directory named '\"'\n> (that's a single double-quote).\n\nThanks to all for the explanations, fixing up and queing.\n\nAnd good news:\npu today is \"clean\",there where no problems found:\n\ncommit d69ea46220647c048d332c471a184446cce17627\nMerge: e552539 fcf30b3\nAuthor: Junio C Hamano \nDate: Wed Jan 2 12:44:33 2013 -0800\n\n\nWhen the dust has settled, we can either enable the check always, or mention\n\"make test-lint-shell-syntax\" in the Documentation.\n"},{"id":"205907","messageId":"7vhamzyqev.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"50E4CF7E.9090302@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-03T02:02:48Z","receivedAt":"2013-01-03T02:02:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> When the dust has settled, we can either enable the check always,\n> or mention \"make test-lint-shell-syntax\" in the Documentation.\n\nIn the longer term, I'm pretty much in favor of enabling all the\nchecks that are cheap by default, as that would help people catch\neasy mistakes while preparing their patches. People do not tend to\nenable any check if it were optional.\n"},{"id":"205913","messageId":"20130103071751.GA32377@sigill.intra.peff.net","threadId":"32500","inReplyTo":"7vhamzyqev.fsf@alter.siamese.dyndns.org","subject":"[PATCH] tests: turn on test-lint by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-03T07:17:51Z","receivedAt":"2013-01-03T07:17:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 02, 2013 at 06:02:48PM -0800, Junio C Hamano wrote:\n\n> Torsten Bögershausen writes:\n> \n> > When the dust has settled, we can either enable the check always,\n> > or mention \"make test-lint-shell-syntax\" in the Documentation.\n> \n> In the longer term, I'm pretty much in favor of enabling all the\n> checks that are cheap by default, as that would help people catch\n> easy mistakes while preparing their patches. People do not tend to\n> enable any check if it were optional.\n\nThat is fine with me, and I always intended that we turn the lint on by\ndefault at some point (I'm not really sure why we didn't -- looking at\nthe list archives, I think I did not push it because it seemed like\nnobody was really that interested).\n\nCertainly the two existing checks are cheap and do not produce false\npositives, and should be safe to turn on. Like this:\n\n-- >8 --\nSubject: [PATCH] tests: turn on test-lint by default\n\nThe test Makefile knows about a few \"lint\" checks for common\nerrors. However, they are not enabled as part of \"make test\"\nby default, which means that many people do not bother\nrunning them. Since they are both quick to run and accurate\n(i.e., no false positives), there should be no harm in\nturning them on and helping submitters catch errors earlier.\n\nWe could just set:\n\n TEST_LINT = test-lint\n\nto enable all tests. But that would be unnecessarily\nannoying later on if we add slower or less accurate tests\nthat should not be part of the default. Instead, we name the\ntests individually.\n\nSigned-off-by: Jeff King \n---\n t/Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/Makefile b/t/Makefile\nindex 3025418..5c6de81 100644\n--- a/t/Makefile\n+++ b/t/Makefile\n@@ -13,6 +13,7 @@ DEFAULT_TEST_TARGET ?= test\n RM ?= rm -f\n PROVE ?= prove\n DEFAULT_TEST_TARGET ?= test\n+TEST_LINT ?= test-lint-duplicates test-lint-executable\n \n # Shell quote;\n SHELL_PATH_SQ = $(subst ','\\'',$(SHELL_PATH))\n-- \n1.8.1.rc3.4.gf3a2f57\n\n\n\n\n\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"206237","messageId":"50EB0928.3090901@web.de","threadId":"32500","inReplyTo":"7vpq1nyvp1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-07T17:43:04Z","receivedAt":"2013-01-07T17:43:04Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 03.01.13 01:08, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I would actually not add this to TEST_LINT by default, especially\n>> when \"duplicates\" and \"executable\" that are much simpler and less\n>> likely to hit false positives are not on by default.\n>>\n>> At least, a change to add this to TEST_LINT by default must wait to\n>> be merged to any integration branch until all the fix-up topics that\n>> this test triggers in the current codebase graduate to the branch.\n>>\n>>>> +test-lint-shell-syntax:\n>>>> +\t$(PERL_PATH) check-non-portable-shell.pl $(T)\n>>>\n>>> This is wrong if $(PERL_PATH) contains spaces, no?\n>>\n>> You are correct; \"harness\" thing in the same Makefile gets this\n>> wrong, too. I think the right invocation is:\n>>\n>> \t@'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)\n>>\n>> although I do not offhand know if that symbol is already exported by\n>> the top-level Makefile.\n> \n> I'll tentatively queue this instead. The log message has also been\n> cleaned up a bit.\n\nSorry for late answer, but there is a problem (both linux and Mac OS X) :-(\n$ make test-lint does not do shel syntax check, neither\n$ make test-lint-shell-syntax\n\nIn the Makefile the the line \n\t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\ndoesn't seem to anything (?)\n\nReplacing @'$(PERL_PATH_SQ)' with $(PERL_PATH) gives the following,\nexpected result: (a very long line starting like this:)\n\n$ make test-lint-shell-syntax\n/usr/bin/perl check-non-portable-shell.pl t0000-basic.sh ......\n\nconfused...\n/Torsten\n"},{"id":"206240","messageId":"7vwqvoj293.fsf@alter.siamese.dyndns.org","threadId":"32500","inReplyTo":"50EB0928.3090901@web.de","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-07T18:07:20Z","receivedAt":"2013-01-07T18:07:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Torsten Bögershausen writes:\n\n> Sorry for late answer, but there is a problem (both linux and Mac OS X) :-(\n> $ make test-lint does not do shel syntax check, neither\n> $ make test-lint-shell-syntax\n\nIn which directory?\n\n $ make -C t test-lint-shell-syntax\n ... passes silently ...\n $ ed t/t0000-basic.sh\n /test_expect_success/\n a\n which sh\n .\n w\n q\n $ make -C t test-lint-shell-syntax\n t0000-basic.sh:28: error: which is not portable (please use type): which sh\n make: *** [test-lint-shell-syntax] Error 1\n\nIf you edit out '@' (but nothing else) from this line:\n\n> \t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\n\nand run the above again, you would see that it is running this shell\ncommand:\n\n'/usr/bin/perl' check-non-portable-shell.pl t0000-basic.sh t0001-init.sh ...\n\nIf you introduce a Perl syntax error to check-non-portable-shell.pl,\nlike this, you will get:\n\n $ make -C t test-lint-shell-syntax\n syntax error at check-non-portable-shell.pl line 11, near \"whoa\n\nSo... is your shell broken? The above seems to work for dash, bash,\nksh and zsh.\n"},{"id":"206264","messageId":"50EB9C6B.6080802@web.de","threadId":"32500","inReplyTo":"7vwqvoj293.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] test: Add target test-lint-shell-syntax","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-01-08T04:11:23Z","receivedAt":"2013-01-08T04:11:23Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 07.01.13 19:07, Junio C Hamano wrote:\n> Torsten Bögershausen writes:\n> \n>> Sorry for late answer, but there is a problem (both linux and Mac OS X) :-(\n>> $ make test-lint does not do shel syntax check, neither\n>> $ make test-lint-shell-syntax\n> \n> In which directory?\n> \n> $ make -C t test-lint-shell-syntax\n> ... passes silently ...\n> $ ed t/t0000-basic.sh\n> /test_expect_success/\n> a\n> which sh\n> .\n> w\n> q\n> $ make -C t test-lint-shell-syntax\n> t0000-basic.sh:28: error: which is not portable (please use type): which sh\n> make: *** [test-lint-shell-syntax] Error 1\n> \n> If you edit out '@' (but nothing else) from this line:\n> \n>> \t@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)\n> \n> and run the above again, you would see that it is running this shell\n> command:\n> \n> '/usr/bin/perl' check-non-portable-shell.pl t0000-basic.sh t0001-init.sh ...\n> \n> If you introduce a Perl syntax error to check-non-portable-shell.pl,\n> like this, you will get:\n> \n> $ make -C t test-lint-shell-syntax\n> syntax error at check-non-portable-shell.pl line 11, near \"whoa\n> \n> So... is your shell broken? The above seems to work for dash, bash,\n> ksh and zsh.\nThanks for helping me out, and sorry for the noise.\n\nMy brain \"went off track\" while chasing a failure of t7400 on pu under Mac OS :-(\n"}]}