Re: [PATCH 3/4] check-non-portable-shell: improve `VAR=val shell-func` detection

["Rubén Justo" <rjusto@gmail.com>]

On Mon, Jul 22, 2024 at 02:59:13AM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Unlike "VAR=val cmd" one-shot environment variable assignments which
> exist only for the invocation of 'cmd', those assigned by "VAR=val
> shell-func" exist within the running shell and continue to do so until
> the process exits. check-non-portable-shell.pl warns when it detects
> such usage since, more often than not, the author who writes such an
> invocation is unaware of the undesirable behavior.
> 
> However, a limitation of the check is that it only detects such
> invocations when variable assignment (i.e. `VAR=val`) is the first
> thing on the line. Thus, it can easily be fooled by an invocation such
> as:
> 
>     echo X | VAR=val shell-func
> 
> Address this shortcoming by loosening the check so that the variable
> assignment can be recognized even when not at the beginning of the line.
> 
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>  t/check-non-portable-shell.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
> index b2b28c2ced..44b23d6ddd 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -49,7 +49,7 @@ sub err {
>  	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)';
>  	/\blocal\s+[A-Za-z0-9_]*=\$([A-Za-z0-9_{]|[(][^(])/ and
>  		err q(quote "$val" in 'local var=$val');
> -	/^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and
> +	/\b([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and !/test_env.+=/ and exists($func{$4}) and

Losing "^\s*" means we'll cause false positives, such as:

    # VAR=VAL shell-func
    echo VAR=VAL shell-func

Regardless of that, the regex will continue to pose problems with:

  VAR=$OTHER_VALUE shell-func
  VAR=$(cmd) shell-func
  VAR=VAL\ UE shell-func
  VAR="\"val\" shell-func UE" non-shell-func 

Which, of course, should be cases that should be written in a more
orthodox way. 

But we will start to detect errors like the ones mentioned in the
message, which are more likely to happen.

I think this change is a good step forward, and I'm happy with it as it
is.

Thanks

>  		err '"FOO=bar shell_func" assignment extends beyond "shell_func"';
>  	$line = '';
>  	# this resets our $. for each file
> -- 
> 2.45.2
>