Re: [PATCH v2 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`

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

On Fri, Jul 26, 2024 at 04:15:21AM -0400, Eric Sunshine wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
> 
> Most problems reported by check-non-portable-shell are accompanied by
> advice suggesting how the test author can repair the problem. For
> instance:
> 
>     error: egrep/fgrep obsolescent (use grep -E/-F)
> 
> However, when one-shot variable assignment is detected when calling a
> shell function (i.e. `VAR=val shell-func`), the problem is reported, but
> no advice is given. The lack of advice is particularly egregious since
> neither the problem nor the workaround are likely well-known by
> newcomers to the project writing tests for the first time. Address this
> shortcoming by recommending the use of `test_env` which is tailor made
> for this specific use-case.
> 
> 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 179efaa39d..903af14294 100755
> --- a/t/check-non-portable-shell.pl
> +++ b/t/check-non-portable-shell.pl
> @@ -50,7 +50,7 @@ sub err {
>  	/\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
> -		err '"FOO=bar shell_func" is not portable';
> +		err '"FOO=bar shell_func" is not portable (use test_env FOO=bar shell_func)';

When someone blames this line in the future, the message of this commit
will appear and be informative.  However, I think the message of the
previous patch [3/5], which also touches this line, would also be
relevant for this context.  And it won't be so obvious to get to that
message.  Therefore, it might be worth combining this commit with the
previous one.  But I'm not sure the change is worth it to have a new
iteration of this series.

Anyway, the change in the err() is a convenient improvement.

>  	$line = '';
>  	# this resets our $. for each file
>  	close ARGV if eof;
> -- 
> 2.45.2
>