git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 3, 2013, 00:01 UTC
Message-ID
<7vtxqzyw0g.fsf@alter.siamese.dyndns.org>
In-Reply-To
<20130102094635.GD9328@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> 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.

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.

>> +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.

Previous: Jeff KingNext: Junio C Hamano
Message 14 of 18 in “test: Add target test-lint-shell-syntax”
  1. 1/4 test: Add target test-lint-shell-syntaxTorsten Bögershausen, Jan 1, 2013
  2. Junio C HamanoJan 1, 2013
  3. Torsten BögershausenJan 2, 2013
  4. Junio C HamanoJan 2, 2013
  5. Jeff KingJan 2, 2013
  6. Junio C HamanoJan 2, 2013
  7. Torsten BögershausenJan 2, 2013
  8. Jeff KingJan 2, 2013
  9. Torsten BögershausenJan 2, 2013
  10. Junio C HamanoJan 3, 2013
  11. Torsten BögershausenJan 3, 2013
  12. Junio C HamanoJan 3, 2013
  13. tests: turn on test-lint by defaultJeff King, Jan 3, 2013
  14. Junio C HamanoJan 3, 2013
  15. Junio C HamanoJan 3, 2013
  16. Torsten BögershausenJan 7, 2013
  17. Junio C HamanoJan 7, 2013
  18. Torsten BögershausenJan 8, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.