Re: [PATCH v2 4/5] check-non-portable-shell: suggest alternative for `VAR=val shell-func`
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Jul 26, 2024, 13:11 UTC
- Message-ID
- <9d96b89f-f4e4-49f4-aa59-2c229d3988e4@gmail.com>
- In-Reply-To
- <20240726081522.28015-5-ericsunshine@charter.net>
On Fri, Jul 26, 2024 at 04:15:21AM -0400, Eric Sunshine wrote:
Show 31 quoted lines
> 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.
Show 6 quoted lines
> $line = ''; > # this resets our $. for each file > close ARGV if eof; > -- > 2.45.2 >