Re: [PATCH] check-non-portable-shell: support Perl versions older than 5.10

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, May 09 2019, Eric Sunshine wrote:

> For thoroughness when checking for one-shot environment variable
> assignments at shell function call sites, check-non-portable-shell
> stitches together incomplete lines (those ending with backslash). This
> allows it to correctly flag such undesirable usage even when the
> variable assignment and function call are split across lines, for
> example:
>
>     FOO=bar \
>     func
>
> where 'func' is a shell function.
>
> The stitching is accomplished like this:
>
>     while (<>) {
>         chomp;
>         # stitch together incomplete lines (those ending with "\")
>         while (s/\\$//) {
>             $_ .= readline;
>             chomp;
>         }
>         # detect unportable/undesirable shell constructs
>         ...
>     }
>
> Although this implementation is well supported in reasonably modern Perl
> versions (5.10 and later), it fails in a couple ways with older versions
> (such as Perl 5.8 shipped with ancient Mac OS 10.5).
>
> In particular, in older Perl versions, 'readline' is not connected to
> the file handle associated with the "magic" while (<>) {...} construct,
> so 'readline' throws a "readline() on unopened filehandle" error.
> Furthermore, $_ assigned by the outer while-loop is read-only, so the
> attempt to modify it via "$_ .= readline" in the inner while-loop fails
> with a "Modification of a read-only value" error.
>
> Avoid both problems by collecting the stitched-together line in a
> variable other than $_ and dropping the inner loop entirely.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index 166d64d4a2..60e607ba42 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -27,14 +27,14 @@ sub err {
>  	close $f;
>  }
>
> +my $line = '';
>  while (<>) {
>  	chomp;
> +	$line .= $_;
>  	# stitch together incomplete lines (those ending with "\")
> -	while (s/\\$//) {
> -		$_ .= readline;
> -		chomp;
> -	}
> +	next if $line =~ s/\\$//;
>
> +	local $_ = $line;
>  	/\bcp\s+-a/ and err 'cp -a is not portable';
>  	/\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
>  	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
> @@ -48,6 +48,7 @@ sub err {
>  	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>  	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
>  		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
> +	$line = '';
>  	# this resets our $. for each file
>  	close ARGV if eof;
>  }

This fix is fine, but just for the record: There's no problem with
assigning to $_, it just throws an error about $_ *because* of the
readline() issue, i.e. it'll fail, clobber $_ to a read-only value, and
off we go.

So just assigning to $_ is fine, and you don't need to localize it.

Anyway, I tested this on 5.8, it works, then looked at the output and
wondered if I could improve it, came up with this:

    diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
    index 60e607ba42..d5fd0a3050 100755
    --- a/t/check-non-portable-shell.pl
    +++ b/t/check-non-portable-shell.pl
    @@ -8,13 +8,26 @@

     my $exit_code=0;
     my %func;
    +my $start_nr = 0;
    +my $line = '';

     sub err {
     	my $msg = shift;
    -	s/^\s+//;
    -	s/\s+$//;
    -	s/\s+/ /g;
    -	print "$ARGV:$.: error: $msg: $_\n";
    +	if (/\n/) {
    +		$. = $start_nr;
    +		my ($ws) = $_ =~ /^(\s+)/;
    +		for (split /^/) {
    +			s/^\Q$ws\E//;
    +			print "$ARGV:$.: error: $msg: $_";
    +			$.++;
    +		}
    +		print "\n";
    +	} else {
    +		s/^\s+//;
    +		s/\s+$//;
    +		s/\s+/ /g;
    +		print "$ARGV:$.: error: $msg: $_\n";
    +	}
     	$exit_code = 1;
     }

    @@ -27,14 +40,16 @@ sub err {
     	close $f;
     }

    -my $line = '';
     while (<>) {
     	chomp;
    -	$line .= $_;
     	# stitch together incomplete lines (those ending with "\")
    -	next if $line =~ s/\\$//;
    -
    -	local $_ = $line;
    +	if (s/\\$//) {
    +		$start_nr ||= $.;
    +		$line .= "$_\n";
    +		next;
    +	} else {
    +		$_ = $line . $_;
    +	}
     	/\bcp\s+-a/ and err 'cp -a is not portable';
     	/\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
     	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
    @@ -48,7 +63,11 @@ sub err {
     	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
     	/^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
     		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
    +
    +	# No longer spanning lines
    +	$start_nr = 0;
     	$line = '';
    +
     	# this resets our $. for each file
     	close ARGV if eof;
     }
    diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
    index c03054c538..b4af7032ad 100755
    --- a/t/t0000-basic.sh
    +++ b/t/t0000-basic.sh
    @@ -156,8 +156,11 @@ test_expect_success 'pretend we have a fully passing test suite' "
     "

     test_expect_success 'pretend we have a partially passing test suite' "
    -	test_must_fail run_sub_test_lib_test \
    -		partial-pass '2/3 tests passing' <<-\\EOF &&
    +	test_must_fail penis run_sub_test_lib_test \
    +		partial-pass '2/3 tests passing' <<-\\EOF \
    +		partial-pass '2/3 tests passing' <<-\\EOF \
    +		partial-pass '2/3 tests passing' <<-\\EOF \
    +		cp -a hi there &&
     	test_expect_success 'passing test #1' 'true'
     	test_expect_success 'failing test #2' 'false'
     	test_expect_success 'passing test #3' 'true'
    diff --git a/t/t0001-init.sh b/t/t0001-init.sh
    index 1f462204ea..a25ac208e5 100755
    --- a/t/t0001-init.sh
    +++ b/t/t0001-init.sh
    @@ -122,6 +122,7 @@ test_expect_success 'plain bare with GIT_WORK_TREE' '

     test_expect_success 'GIT_DIR bare' '
     	mkdir git-dir-bare.git &&
    +	cp -a foo bar &&
     	GIT_DIR=git-dir-bare.git git init &&
     	check_config git-dir-bare.git true unset
     '

I.e. now for these multi-line issues we'll print the whole offending
multi-line invocation

    $ ~/g/perl/miniperl -I ~/g/perl/lib check-non-portable-shell.pl t[0-9]*.sh
    t0000-basic.sh:159: error: cp -a is not portable: test_must_fail penis run_sub_test_lib_test
    t0000-basic.sh:160: error: cp -a is not portable:       partial-pass '2/3 tests passing' <<-\\EOF
    t0000-basic.sh:161: error: cp -a is not portable:       partial-pass '2/3 tests passing' <<-\\EOF
    t0000-basic.sh:162: error: cp -a is not portable:       partial-pass '2/3 tests passing' <<-\\EOF
    t0000-basic.sh:163: error: cp -a is not portable:       cp -a hi there &&
    t0001-init.sh:125: error: cp -a is not portable: cp -a foo bar &&

I figured it was better than the current output just squashing such a
long line together, i.e. it'll print this now (before/after this patch):

    $ ~/g/perl/miniperl -I ~/g/perl/lib check-non-portable-shell.pl t[0-9]*.sh
    t0000-basic.sh:163: error: cp -a is not portable: test_must_fail penis run_sub_test_lib_test partial-pass '2/3 tests passing' <<-\\EOF partial-pass '2/3 tests passing' <<-\\EOF partial-pass '2/3 tests passing' <<-\\EOF cp -a hi there &&
    t0001-init.sh:125: error: cp -a is not portable: cp -a foo bar &&

There's ways to make that WIP patch shorter etc. I wasn't trying to golf
it, also I think we can get rid of that s/\s+$// if we split up lines
like this.