On Sat, Jul 20, 2024 at 6:37 PM Rubén Justo <rjusto@xxxxxxxxx> wrote: > On Wed, Jul 17, 2024 at 04:09:17PM -0400, Eric Sunshine wrote: > > > >> - test_write_lines P q | GIT_PAGER="head -n 1" test_terminal git add -p > > > It's also curious that t/check-non-portable-shell.pl didn't catch this > > use of one-shot assignment when calling a shell function[*]. > > It would have been great if it had caught that error. > > As a reference: > VAR=value func # this error is caught > echo 1 | > VAR=value func # this one is also caught > echo 1 | VAR=value func # this one isn't Thanks for providing this summary; it saved me the effort of digging back through this discussion / patch series. > Maybe, catch this errors expanding the regular expression we have in > `check-non-portable-shell.pl` isn't the best approach. We might need > something more sophisticated, like what we have in `chainlint.pl`. The idea has been expressed previously of subsuming all the check-non-portable-shell.pl checks into chainlint.pl some day, thus allowing check-non-portable-shell.pl to be retired. In fact, it was mentioned again quite recently[1]. However, this particular check (detecting `VAR=val shell-func`) poses an extra complication which would require some specialized additional mechanism in chainlint.pl. In particular, in `VAR=val symbol`, in order to distinguish when `symbol` is an external command versus a shell-function, it is necessary to scan for function definitions not just in the script being checked, but also in all scripts included (recursively) by the script being checked. So, it's probably possible to do but ought to be done carefully. > Perhaps someone with experience in those scripts could give us this > capability :-) I posted a series[2] which addresses this shortcoming by enhancing check-non-portable-shell.pl. [1]: https://lore.kernel.org/git/CAPig+cTFZuU7zM7poqk4HeK09zn8bFrO37eUZiaGmeJ0yecpiw@xxxxxxxxxxxxxx/ [2]: https://lore.kernel.org/git/20240722065915.80760-1-ericsunshine@xxxxxxxxxxx/T/
