threads / patch / 32500

patch, 4 partstest: Add target test-lint-shell-syntax

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

## tl;dr

18 messages between Jan 1, 2013 and Jan 8, 2013. Diffs are folded; open one to read it.

replies: 17people: 3as markdown or json

Torsten Bögershausen· Jan 1, 2013, 21:40 UTC · lore

Add the perl script "check-non-portable-shell.pl" to detect non-portable shell syntax Many systems use gnu tools which accept an extended syntax in shell scripts, which is not portable on all systems and causes the test suite to fail.

To prevent contributors using e.g. Linux to add non-portable test code, "check-non-portable-shell.pl" is run as part of "make test" or "make in the t/ directory.

"echo -n" is an example of a statement working on Linux, but not on e.g. Mac OS X.

Beside "echo -n" we check for "sed -i", arrays in shell scripts (declare statement), "which" (use type instead), or "==" (bash style of =)

Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
 t/Makefile                    |  7 +++--
 t/check-non-portable-shell.pl | 67 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100755 t/check-non-portable-shell.pl
Show changes to 2 files +72 −2

t/Makefile, t/check-non-portable-shell.pl

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
 
 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
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -55,6 +55,9 @@ test-lint-executable:
 		test -z "$$bad" || { \
 		echo >&2 "non-executable tests:" $$bad; exit 1; }
 
+test-lint-shell-syntax:
+	$(PERL_PATH) check-non-portable-shell.pl $(T)
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
new file mode 100755
index 0000000..de62ef0
--- /dev/null
+++ b/t/check-non-portable-shell.pl
@@ -0,0 +1,67 @@
+#!/usr/bin/perl -w
+######################################################################
+# Test t0000..t9999.sh for non portable shell scripts                #
+# Examples are "echo -n" or "sed -i"                                 #
+# This script can be called with one or more filenames as parameters #
+#
+######################################################################
+use strict;
+my $exitcode=0;
+
+sub check_one_file($) {
+	my $lineno=1;
+	my $filename=shift;
+
+	open(FINPUT, "<$filename") || die "Couldn't open filename $filename";
+	my @fdata = <FINPUT>;
+	close(FINPUT);
+
+	while (my $line = shift @fdata) {
+    do {
+      chomp $line;
+      # sed -i
+      if ($line =~ /^\s*sed\s+-i/) {
+        printf("%s:%d:error: \"sed -i not portable\" %s\n", $filename, $lineno, $line);
+				$exitcode=1;
+      }
+      # echo -n
+      if ($line =~ /^\s*echo\s+-n/) {
+        printf("%s:%d:error: \"echo -n not portable\" %s\n", $filename, $lineno, $line);
+				$exitcode=1;
+      }
+      # arrays (declare statement)
+      if ($line =~ /^\s*declare\s+/) {
+        printf("%s:%d:error: \"arrays/declare not portable\" %s\n", $filename, $lineno, $line);
+				$exitcode=1;
+      }
+      # which
+      if ($line =~ /^\s*[^#]\s*which\s/) {
+        printf("%s:%d:error: \"which is not portable (use type)\" %s\n", $filename, $lineno, $line);
+				$exitcode=1;
+      }
+
+      # == (bash style comparison)
+      if ($line =~ /test\s+[^=]*==/) {
+        printf("%s:%d:error: \"== is not portable (use =)\" %s\n", $filename, $lineno, $line);
+				$exitcode=1;
+      }
+
+			$lineno=$lineno+1;
+		}
+  }
+}
+
+
+if ($#ARGV <= 0) {
+	print STDERR  "$0: Check shell scripts for non portable syntax\n";
+	print STDERR  "Example: $0 t[0-9]*.sh\n";
+
+	exit(2);
+}
+
+while (@ARGV) {
+	my $arg = shift @ARGV;
+  check_one_file($arg);
+}
+
+exit($exitcode);
-- 
1.8.0.197.g5a90748
Junio C Hamano· Jan 1, 2013, 22:07 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 19 quoted lines
> Add the perl script "check-non-portable-shell.pl" to detect non-portable
> shell syntax
> Many systems use gnu tools which accept an extended syntax in shell scripts,
> which is not portable on all systems and causes the test suite to fail.
>
> To prevent contributors using e.g. Linux to add non-portable test code,
> "check-non-portable-shell.pl" is run as part of
> "make test" or "make in the t/ directory.
>
> "echo -n" is an example of a statement working on Linux,
> but not on e.g. Mac OS X.
>
> Beside "echo -n" we check for
> "sed -i",
> arrays in shell scripts (declare statement),
> "which" (use type instead),
> or "==" (bash style of =)
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>

What it checks looks like a good start, but the indentation of it (and the log message) seems very screwed up.

I also have to wonder what's the false positive rate of this. When you are preparing a new test, you would ideally want a mode that checks only parts that you just added, without seeing noises from existing violations and false positives from the part you did not touch. Otherwise, it will be too cumbersome to run for developers, and the check mechanism will end up used by nobody.

Show 6 quoted lines
> +######################################################################
> +# Test t0000..t9999.sh for non portable shell scripts                #
> +# Examples are "echo -n" or "sed -i"                                 #
> +# This script can be called with one or more filenames as parameters #
> +#
> +######################################################################

Just a style thing (style requests are not optional, though ;-), but these box comments are moderately annoying to read and extremely annoying to modify. Writing it like this:

Show 5 quoted lines
> +#
> +# Test t0000..t9999.sh for non portable shell scripts
> +# Examples are "echo -n" or "sed -i"
> +# This script can be called with one or more filenames as parameters
> +#
should be sufficiently loud.
Torsten Bögershausen· Jan 2, 2013, 00:14 UTC · re: Junio C Hamano · lore

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

On 01.01.13 23:07, Junio C Hamano wrote: [snip]

> What it checks looks like a good start, but the indentation of it
> (and the log message) seems very screwed up.
OK
Show 7 quoted lines
> I also have to wonder what's the false positive rate of this.  When
> you are preparing a new test, you would ideally want a mode that
> checks only parts that you just added, without seeing noises from
> existing violations and false positives from the part you did not
> touch.  Otherwise, it will be too cumbersome to run for developers,
> and the check mechanism will end up used by nobody.
>
The script found all problems which make the testsuite (unecessary) fail on Mac OS X.
The false positive rate is currently 0% (otherwise I should not have send it to the list)

The suggestion is to run it every time the test suite is run, at the begining. And it seems to be fast enough:

$ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh real 0m0.263s user 0m0.239s sys 0m0.021s

/Torsten
Junio C Hamano· Jan 2, 2013, 02:22 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 7 quoted lines
> The suggestion is to run it every time the test suite is run, at the begining.
> And it seems to be fast enough:
>
> $ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh
> real    0m0.263s
> user    0m0.239s
> sys     0m0.021s
Hmph.  OK.
Jeff King· Jan 2, 2013, 09:46 UTC · re: Torsten Bögershausen · lore

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

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...
Show 11 quoted lines
> 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.

Show 7 quoted lines
>  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)
would be sufficient.
> --- /dev/null
> +++ b/t/check-non-portable-shell.pl
> @@ -0,0 +1,67 @@
> +#!/usr/bin/perl -w

This "-w" is ignored, since we execute by using $PERL_PATH. Maybe "use warnings" instead?

> +sub check_one_file($) {

Perl subroutine prototypes are generally frowned on unless there is a good reason to use them.

> +	my $lineno=1;
Perl keeps track of this for you in the "$." variable.
> +	my $filename=shift;

And if you use the automagic "<>" handle, this is already in $ARGV (but note that you need to do a little bit of magic to make that work with $.; see the entry for "$." in perlvar, and "eof" in perlfunc).

Show 5 quoted lines
> +	open(FINPUT, "<$filename") || die "Couldn't open filename $filename";
> +	my @fdata = <FINPUT>;
> +	close(FINPUT);
> +
> +	while (my $line = shift @fdata) {

Not that our test scripts are so huge they won't fit into memory, but it is generally good practice to loop on the handle rather than reading all of the lines into an array.

> +    do {
What's this do block for?
Show 5 quoted lines
> +      # sed -i
> +      if ($line =~ /^\s*sed\s+-i/) {
> +        printf("%s:%d:error: \"sed -i not portable\" %s\n", $filename, $lineno, $line);
> +				$exitcode=1;
> +      }

These would be a lot more readable if the printf was pulled out into a helper function. And you can avoid the escaped quotes by using perl's qq operator. E.g., qq(this string has "quotes" in it).

Also, putting a space before the "error:" matches what gcc outputs, which some editors (e.g., vim) can recognize and let the user jump straight to the error.

> +while (@ARGV) {
> +	my $arg = shift @ARGV;
> +  check_one_file($arg);
> +}
You can replace this with the magic <> filehandle.

So taking all of that, a more idiomatic perl script would look something like:

  my $exit_code;
  sub err {
    my $msg = shift;
    print "$ARGV:$.: error: $msg: $_\n";
    $exit_code = 1;
  }
  while (<>) {
    chomp;
    if (/^\s*sed\s+-i/) {
      err('sed -i is not portable');
    }
    # ...and so on
    # this resets our $. for each file
    close ARGV if eof;
  }
  exit $exit_code;
I'd personally probably write the conditions like:
  /^\s*sed\s+-i/ and err('sed -i is not portable');

to make the structure of the program (i.e., a list of conditions to complain about) clear, but I know not everybody agrees with such a terse style.

-Peff
Junio C Hamano· Jan 2, 2013, 16:28 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 29 quoted lines
> So taking all of that, a more idiomatic perl script would look something
> like:
>
>   my $exit_code;
>   sub err {
>     my $msg = shift;
>     print "$ARGV:$.: error: $msg: $_\n";
>     $exit_code = 1;
>   }
>
>   while (<>) {
>     chomp;
>     if (/^\s*sed\s+-i/) {
>       err('sed -i is not portable');
>     }
>     # ...and so on
>
>     # this resets our $. for each file
>     close ARGV if eof;
>   }
>   exit $exit_code;
>
> I'd personally probably write the conditions like:
>
>   /^\s*sed\s+-i/ and err('sed -i is not portable');
>
> to make the structure of the program (i.e., a list of conditions to
> complain about) clear, but I know not everybody agrees with such a terse
> style.
Thanks for a nicely-done review.
Torsten Bögershausen· Jan 2, 2013, 23:14 UTC · re: Jeff King · lore

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

On 02.01.13 10:46, Jeff King wrote:> On Tue, Jan 01, 2013 at 10:40:08PM +0100, Torsten Bögershausen wrote:
Show 43 quoted lines
> 
>> 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

Jeff King· Jan 2, 2013, 23:22 UTC · re: Torsten Bögershausen · lore

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

On Thu, Jan 03, 2013 at 12:14:32AM +0100, Torsten Bögershausen wrote:
Show 8 quoted lines
> > 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.
Show 8 quoted lines
> - 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?

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
Torsten Bögershausen· Jan 2, 2013, 23:58 UTC · re: Jeff King · lore

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

On 03.01.13 00:22, Jeff King wrote:
Show 27 quoted lines
> 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
Show changes to t/Makefile +3 −3
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
Junio C Hamano· Jan 3, 2013, 00:16 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
Show 13 quoted lines
> 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"

I do not think that will fly. Having that in the main Makefile where the existing users of the symbol relies on it without any surrounding quotes, e.g.

$(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
	$(QUIET_GEN)$(RM) $@ $@+ && \
	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
	sed -e '1{' \
	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
	    -e '	h' \
	    -e '	s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
	    -e '	H' \
	    -e '	x' \
	    -e '}' \
	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
	    $@.perl >$@+ && \
	chmod +x $@+ && \
	mv $@+ $@

where $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH) with '\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to quote the value of PERL_PATH without quotes, your definition will look for a relative path that is inside a directory named '"' (that's a single double-quote).

Torsten Bögershausen· Jan 3, 2013, 00:23 UTC · re: Junio C Hamano · lore

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

On 03.01.13 01:16, Junio C Hamano wrote:
Show 40 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> 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"
> 
> I do not think that will fly.  Having that in the main Makefile
> where the existing users of the symbol relies on it without any
> surrounding quotes, e.g.
> 
> $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl GIT-VERSION-FILE
> 	$(QUIET_GEN)$(RM) $@ $@+ && \
> 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
> 	sed -e '1{' \
> 	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
> 	    -e '	h' \
> 	    -e '	s=.*=use lib (split(/$(pathsep)/, $$ENV{GITPERLLIB} || "'"$$INSTLIBDIR"'"));=' \
> 	    -e '	H' \
> 	    -e '	x' \
> 	    -e '}' \
> 	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
> 	    $@.perl >$@+ && \
> 	chmod +x $@+ && \
> 	mv $@+ $@
> 
> where $(PERL_PATH_SQ) is defined to replace each ' in $(PERL_PATH)
> with '\'' so that '$(PERL_PATH_SQ)' becomes a shell-safe way to
> quote the value of PERL_PATH without quotes, your definition will
> look for a relative path that is inside a directory named '"'
> (that's a single double-quote).
Thanks to all for the explanations, fixing up and queing.

And good news: pu today is "clean",there where no problems found:

commit d69ea46220647c048d332c471a184446cce17627
Merge: e552539 fcf30b3
Author: Junio C Hamano <gitster@pobox.com>
Date:   Wed Jan 2 12:44:33 2013 -0800

When the dust has settled, we can either enable the check always, or mention "make test-lint-shell-syntax" in the Documentation.

Junio C Hamano· Jan 3, 2013, 02:02 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
> When the dust has settled, we can either enable the check always,
> or mention "make test-lint-shell-syntax" in the Documentation.

In the longer term, I'm pretty much in favor of enabling all the checks that are cheap by default, as that would help people catch easy mistakes while preparing their patches. People do not tend to enable any check if it were optional.

Jeff King· Jan 3, 2013, 07:17 UTC · re: Junio C Hamano · lore

[PATCH] tests: turn on test-lint by default

On Wed, Jan 02, 2013 at 06:02:48PM -0800, Junio C Hamano wrote:
Show 9 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
> 
> > When the dust has settled, we can either enable the check always,
> > or mention "make test-lint-shell-syntax" in the Documentation.
> 
> In the longer term, I'm pretty much in favor of enabling all the
> checks that are cheap by default, as that would help people catch
> easy mistakes while preparing their patches.  People do not tend to
> enable any check if it were optional.

That is fine with me, and I always intended that we turn the lint on by default at some point (I'm not really sure why we didn't -- looking at the list archives, I think I did not push it because it seemed like nobody was really that interested).

Certainly the two existing checks are cheap and do not produce false positives, and should be safe to turn on. Like this:

-- >8 --
Subject: [PATCH] tests: turn on test-lint by default

The test Makefile knows about a few "lint" checks for common errors. However, they are not enabled as part of "make test" by default, which means that many people do not bother running them. Since they are both quick to run and accurate (i.e., no false positives), there should be no harm in turning them on and helping submitters catch errors earlier.

We could just set:
  TEST_LINT = test-lint

to enable all tests. But that would be unnecessarily annoying later on if we add slower or less accurate tests that should not be part of the default. Instead, we name the tests individually.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/Makefile | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/Makefile +1 −0
diff --git a/t/Makefile b/t/Makefile
index 3025418..5c6de81 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -13,6 +13,7 @@ DEFAULT_TEST_TARGET ?= test
 RM ?= rm -f
 PROVE ?= prove
 DEFAULT_TEST_TARGET ?= test
+TEST_LINT ?= test-lint-duplicates test-lint-executable
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-- 
1.8.1.rc3.4.gf3a2f57





> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Junio C Hamano· Jan 3, 2013, 00:01 UTC · re: Jeff King · lore

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

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.

Junio C Hamano· Jan 3, 2013, 00:08 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 20 quoted lines
> 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.

I'll tentatively queue this instead. The log message has also been cleaned up a bit.

-- >8 --
From: Torsten Bögershausen <tboegi@web.de>
Date: Thu, 3 Jan 2013 00:20:19 +0100
Subject: [PATCH] test: Add check-non-portable-shell.pl

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

"echo -n" is an example of a shell command working on Linux, but not on Mac OS X.

These shell commands are checked and reported as error:
 - "echo -n" (printf should be used)
 - "sed -i" (GNUism; use a temp file instead)
 - "declare" (bashism, often used with arrays)
 - "which" (unreliable exit status and output; use type instead)
 - "test a == b" (bashism for "test a = b")
"make test-lint-shell-syntax" can be used to run only the check.
Helped-By: Jeff King <peff@peff.net>
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/Makefile                    |  8 ++++++--
 t/check-non-portable-shell.pl | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 2 deletions(-)
 create mode 100755 t/check-non-portable-shell.pl
Show changes to 2 files +33 −2

t/Makefile, t/check-non-portable-shell.pl

diff --git a/t/Makefile b/t/Makefile
index 3025418..a43becb 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -16,6 +16,7 @@ DEFAULT_TEST_TARGET ?= test
 
 # Shell quote;
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 
 T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh))
 TSVN = $(sort $(wildcard t91[0-9][0-9]-*.sh))
@@ -43,7 +44,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
 
 test-lint-duplicates:
 	@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -55,6 +56,9 @@ test-lint-executable:
 		test -z "$$bad" || { \
 		echo >&2 "non-executable tests:" $$bad; exit 1; }
 
+test-lint-shell-syntax:
+	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
+
 aggregate-results-and-cleanup: $(T)
 	$(MAKE) aggregate-results
 	$(MAKE) clean
@@ -87,7 +91,7 @@ test-results:
 	mkdir -p test-results
 
 test-results/git-smoke.tar.gz: test-results
-	$(PERL_PATH) ./harness \
+	'$(PERL_PATH_SQ)' ./harness \
 		--archive="test-results/git-smoke.tar.gz" \
 		$(T)
 
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
new file mode 100755
index 0000000..8b5a71d
--- /dev/null
+++ b/t/check-non-portable-shell.pl
@@ -0,0 +1,27 @@
+#!/usr/bin/perl
+
+# Test t0000..t9999.sh for non portable shell scripts
+# This script can be called with one or more filenames as parameters
+
+use strict;
+use warnings;
+
+my $exit_code=0;
+
+sub err {
+	my $msg = shift;
+	print "$ARGV:$.: error: $msg: $_\n";
+	$exit_code = 1;
+}
+
+while (<>) {
+	chomp;
+	/^\s*sed\s+-i/ and err 'sed -i is not portable';
+	/^\s*echo\s+-n/ and err 'echo -n is not portable (please use printf)';
+	/^\s*declare\s+/ and err 'arrays/declare not portable';
+	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
+	/test\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
+	# this resets our $. for each file
+	close ARGV if eof;
+}
+exit $exit_code;
-- 
1.8.1.203.gc241474
Torsten Bögershausen· Jan 7, 2013, 17:43 UTC · re: Junio C Hamano · lore

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

On 03.01.13 01:08, Junio C Hamano wrote:
Show 25 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> 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.
> 
> I'll tentatively queue this instead.  The log message has also been
> cleaned up a bit.

Sorry for late answer, but there is a problem (both linux and Mac OS X) :-( $ make test-lint does not do shel syntax check, neither $ make test-lint-shell-syntax

In the Makefile the the line 
	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
doesn't seem to anything (?)

Replacing @'$(PERL_PATH_SQ)' with $(PERL_PATH) gives the following, expected result: (a very long line starting like this:)

$ make test-lint-shell-syntax /usr/bin/perl check-non-portable-shell.pl t0000-basic.sh ......

confused... /Torsten

Junio C Hamano· Jan 7, 2013, 18:07 UTC · re: Torsten Bögershausen · lore

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

Torsten Bögershausen <tboegi@web.de> writes:
> Sorry for late answer, but there is a problem (both linux and Mac OS X) :-(
> $ make test-lint does not do shel syntax check, neither
> $ make test-lint-shell-syntax
In which directory?
    $ make -C t test-lint-shell-syntax
    ... passes silently ...
    $ ed t/t0000-basic.sh
    /test_expect_success/
    a
            which sh
    .
    w
    q
    $ make -C t test-lint-shell-syntax
    t0000-basic.sh:28: error: which is not portable (please use type):      which sh
    make: *** [test-lint-shell-syntax] Error 1
If you edit out '@' (but nothing else) from this line:
> 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)

and run the above again, you would see that it is running this shell command:

'/usr/bin/perl' check-non-portable-shell.pl t0000-basic.sh t0001-init.sh ...

If you introduce a Perl syntax error to check-non-portable-shell.pl, like this, you will get:

    $ make -C t test-lint-shell-syntax
    syntax error at check-non-portable-shell.pl line 11, near "whoa

So... is your shell broken? The above seems to work for dash, bash, ksh and zsh.

Torsten Bögershausen· Jan 8, 2013, 04:11 UTC · re: Junio C Hamano · lore

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

On 07.01.13 19:07, Junio C Hamano wrote:
Show 38 quoted lines
> Torsten Bögershausen <tboegi@web.de> writes:
> 
>> Sorry for late answer, but there is a problem (both linux and Mac OS X) :-(
>> $ make test-lint does not do shel syntax check, neither
>> $ make test-lint-shell-syntax
> 
> In which directory?
> 
>     $ make -C t test-lint-shell-syntax
>     ... passes silently ...
>     $ ed t/t0000-basic.sh
>     /test_expect_success/
>     a
>             which sh
>     .
>     w
>     q
>     $ make -C t test-lint-shell-syntax
>     t0000-basic.sh:28: error: which is not portable (please use type):      which sh
>     make: *** [test-lint-shell-syntax] Error 1
> 
> If you edit out '@' (but nothing else) from this line:
> 
>> 	@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T)
> 
> and run the above again, you would see that it is running this shell
> command:
> 
> '/usr/bin/perl' check-non-portable-shell.pl t0000-basic.sh t0001-init.sh ...
> 
> If you introduce a Perl syntax error to check-non-portable-shell.pl,
> like this, you will get:
> 
>     $ make -C t test-lint-shell-syntax
>     syntax error at check-non-portable-shell.pl line 11, near "whoa
> 
> So... is your shell broken?  The above seems to work for dash, bash,
> ksh and zsh.
Thanks for helping me out, and sorry for the noise.
My brain "went off track" while chasing a failure of t7400 on pu under Mac OS :-(

← back to recent threads