From: "Torsten Bögershausen" <tboegi@web.de>
To: Jeff King <peff@peff.net>
Cc: "Torsten Bögershausen" <tboegi@web.de>, git@vger.kernel.org
Subject: Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
Date: Thu, 03 Jan 2013 00:14:32 +0100 [thread overview]
Message-ID: <50E4BF58.4090808@web.de> (raw)
In-Reply-To: <20130102094635.GD9328@sigill.intra.peff.net>
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
next prev parent reply other threads:[~2013-01-02 23:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-01 21:40 [PATCH 1/4] test: Add target test-lint-shell-syntax Torsten Bögershausen
2013-01-01 22:07 ` Junio C Hamano
2013-01-02 0:14 ` Torsten Bögershausen
2013-01-02 2:22 ` Junio C Hamano
2013-01-02 9:46 ` Jeff King
2013-01-02 16:28 ` Junio C Hamano
2013-01-02 23:14 ` Torsten Bögershausen [this message]
2013-01-02 23:22 ` Jeff King
2013-01-02 23:58 ` Torsten Bögershausen
2013-01-03 0:16 ` Junio C Hamano
2013-01-03 0:23 ` Torsten Bögershausen
2013-01-03 2:02 ` Junio C Hamano
2013-01-03 7:17 ` [PATCH] tests: turn on test-lint by default Jeff King
2013-01-03 0:01 ` [PATCH 1/4] test: Add target test-lint-shell-syntax Junio C Hamano
2013-01-03 0:08 ` Junio C Hamano
2013-01-07 17:43 ` Torsten Bögershausen
2013-01-07 18:07 ` Junio C Hamano
2013-01-08 4:11 ` Torsten Bögershausen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50E4BF58.4090808@web.de \
--to=tboegi@web.de \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.