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

["Torsten Bögershausen" <tboegi@web.de>]

On 03.01.13 00:22, Jeff King wrote:
> On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:
> 
>>> 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.
> 
> Yeah, sorry, typo on my part.
> 
>> - Using "$$PERL_PATH" expands from make into "$PERL_PATH" on the command line
> 
> Right. That's what I intended.
> 
>> - 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.
> 
> Does a $PERL_PATH with quotes actually work?
At least on my system the following combination works:

git diff
diff --git a/t/Makefile b/t/Makefile
index f8f8c54..391a5ca 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,7 +8,7 @@
 
 #GIT_TEST_OPTS = --verbose --debug
 SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
+PERL_PATH = "/Users/tb/projects/git/tb/pe rl"
 TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove

~/projects/git/tb/t> ls -l "/Users/tb/projects/git/tb/pe rl"
lrwxr-xr-x  1 tb  staff  13 Jan  3 00:33 /Users/tb/projects/git/tb/pe rl -> /usr/bin/perl

====================================================
And this works as well, is that what you intended?
Note: "single Dollar"
====================================================
 git diff
diff --git a/t/Makefile b/t/Makefile
index f8f8c54..f624f95 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -8,7 +8,7 @@
 
 #GIT_TEST_OPTS = --verbose --debug
 SHELL_PATH ?= $(SHELL)
-PERL_PATH ?= /usr/bin/perl
+PERL_PATH = /Users/tb/projects/git/tb/pe rl
 TAR ?= $(TAR)
 RM ?= rm -f
 PROVE ?= prove
@@ -57,7 +57,7 @@ test-lint-executable:
                echo >&2 "non-executable tests:" $$bad; exit 1; }
 
 test-lint-shell-syntax:
-       $(PERL_PATH) check-non-portable-shell.pl $(T)
+       "$(PERL_PATH)" check-non-portable-shell.pl $(T)
 



> Usually in our runtime environment, commands that are handed to git are
> assumed to be passed directly to the shell, and you need to quote. E.g.,
> setting diff.external to:
> 
>   [diff]
>   external = "foo --bar"
> 
> will let the shell split the argument out; if you have a space, you
> would want to set it like:
> 
>   [diff]
>   external = "'command with space'"
> 
> This is the most flexible way to do it.
> 
> However, for Makefile variables, I think we do not (and cannot) follow
> the same rule.  Notice that all of the uses of $PERL_PATH in the test
> suite enclose it in quotes. Having extra quotes would break those
> invocations. And the value of $PERL_PATH will be put on the #!-line,
> which cannot not be quoted.
> 
> -Peff
I followed these lines as an example:

test-results/git-smoke.tar.gz: test-results
	$(PERL_PATH) ./harness \
		--archive="test-results/git-smoke.tar.gz" \
		$(T)

(and make smoke did not work, as we don't have ./harness :-(

Do we need some cleanup/improvements here as well?

/Torsten