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

[Jeff King <peff@peff.net>]

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