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

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
May 9, 2019, 12:33 UTC
Message-ID
<87ftpnhknn.fsf@evledraar.gmail.com>
In-Reply-To
<20190509102037.27044-1-sunshine@sunshineco.com>
On Thu, May 09 2019, Eric Sunshine wrote:
Show 75 quoted lines
> 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.

Previous: Eric SunshineNext: Eric Sunshine
Message 7 of 9 in “Git build on antique PowerMac”
  1. Jeffrey WaltonMay 5, 2019
  2. SZEDER GáborMay 5, 2019
  3. Eric SunshineMay 5, 2019
  4. Ævar Arnfjörð BjarmasonMay 8, 2019
  5. Eric SunshineMay 9, 2019
  6. check-non-portable-shell: support Perl versions older than 5.10Eric Sunshine, May 9, 2019
  7. Ævar Arnfjörð BjarmasonMay 9, 2019
  8. Eric SunshineMay 10, 2019
  9. check-non-portable-shell: support Perl versions older than 5.10Eric Sunshine, May 11, 2019

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.