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

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

[Torsten Bögershausen]

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

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

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

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

> +######################################################################
> +# 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:

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

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

[Torsten Bögershausen]

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

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

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

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

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

[Jeff King]

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)

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

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

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Torsten Bögershausen]

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

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

[Jeff King]

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?

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

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

[Torsten Bögershausen]

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

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

[Junio C Hamano]

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

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

[Torsten Bögershausen]

On 03.01.13 01:16, Junio C Hamano wrote:
> 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.

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

[Junio C Hamano]

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.

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

[Jeff King]

On Wed, Jan 02, 2013 at 06:02:48PM -0800, Junio C Hamano wrote:

> 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(+)

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Junio C Hamano]

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.

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

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

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

[Torsten Bögershausen]

On 03.01.13 01:08, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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.

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

[Torsten Bögershausen]

On 07.01.13 19:07, Junio C Hamano wrote:
> 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 :-(