Re: [PATCH 1/4] test: Add target test-lint-shell-syntax

Jeff King [off-list ref] writes:

On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:

quoted

Add the perl script "check-non-portable-shell.pl" to detect non-portable
shell syntax

Cool. Thanks for adding more test-lint. But...

quoted

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.

I would actually not add this to TEST_LINT by default, especially
when "duplicates" and "executable" that are much simpler and less
likely to hit false positives are not on by default.

At least, a change to add this to TEST_LINT by default must wait to
be merged to any integration branch until all the fix-up topics that
this test triggers in the current codebase graduate to the branch.

quoted

+test-lint-shell-syntax:
+	$(PERL_PATH) check-non-portable-shell.pl $(T)

This is wrong if $(PERL_PATH) contains spaces, no?

You are correct; "harness" thing in the same Makefile gets this
wrong, too.  I think the right invocation is:

	@'$(PERL_PATH_SQ)' check-non-portable.shell.pl $(T)

although I do not offhand know if that symbol is already exported by
the top-level Makefile.