| Message ID | 20240726081522.28015-5-ericsunshine@charter.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | improve one-shot variable detection with shell function | expand |
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 >
On Fri, Jul 26, 2024 at 9:11 AM Rubén Justo <rjusto@gmail.com> wrote: > On Fri, Jul 26, 2024 at 04:15:21AM -0400, Eric Sunshine wrote: > > - 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. I did consider combining the two patches but decided against it. Despite the fact that both patches touch the same line/message, they really are two distinct "fixes" as evidenced by the fact that the explanation provided by each commit message is entirely orthogonal to the other.
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)'; $line = ''; # this resets our $. for each file close ARGV if eof;