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

From: Torsten Bögershausen <[email-masked]>
Date: Thu, 03 Jan 2013 00:14:32 +0100

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