On 02.01.13 10:46, Jeff King wrote:> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote: > >> Add the perl script "check-non-portable-shell.pl" to detect non-portable >> shell syntax > > Cool. Thanks for adding more test-lint. But... > >> diff --git a/t/Makefile b/t/Makefile >> index 88e289f..7b0c4dc 100644 >> --- a/t/Makefile >> +++ b/t/Makefile >> @@ -23,7 +23,7 @@ TGITWEB = $(sort $(wildcard t95[0-9][0-9]-*.sh)) >> >> all: $(DEFAULT_TEST_TARGET) >> >> -test: pre-clean $(TEST_LINT) >> +test: pre-clean test-lint-shell-syntax $(TEST_LINT) >> $(MAKE) aggregate-results-and-cleanup > > I do not think it should be a hard-coded dependency of "test", as then > there is no way to turn it off. It would make more sense to me to set a > default TEST_LINT that includes it, but could be overridden by the user. > >> prove: pre-clean $(TEST_LINT) >> @@ -43,7 +43,7 @@ clean-except-prove-cache: >> clean: clean-except-prove-cache >> $(RM) .prove >> >> -test-lint: test-lint-duplicates test-lint-executable >> +test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax > > This, however, is right. The point of "test-lint" is "use all the lint", > so adding it here makes sense (and anyone who has set "TEST_LINT = > test-lint" will get the new check). > >> +test-lint-shell-syntax: >> + $(PERL_PATH) check-non-portable-shell.pl $(T) > > This is wrong if $(PERL_PATH) contains spaces, no? Doing "$(PERL_PATH)" > is also wrong, because the expansion happens in 'make', and a > $(PERL_PATH) with double-quotes would fool the shell. Since we export > $PERL_PATH, I think doing: > > "$$PERL_PATH"" check-non-portable-shell.pl $(T) Thanks, but: - The double "" after PERL_PATH makes the string un-terminated. - Using "$$PERL_PATH" expands from make into "$PERL_PATH" on the command line - If the Makefile looks like this: PERL_PATH = "/Users/tb/projects/git/tb/pe rl" [snip] $(PERL_PATH) check-non-portable-shell.pl $(T) The command line will look like this: "/Users/tb/projects/git/tb/pe rl" check-non-portable-shell.pl t0000-basic.sh ... So I think that PERL_PATH should be quoted when it is defined in the Makefile. [snip] Peff, many thanks. Please see V2 patch comming soon -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html