| Message ID | patch-01.16-8e4b4a2a216-20210412T110456Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | test-lib.sh: new test_commit args, simplification & fixes | expand |
On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > Expand the t/check-non-portable-shell.pl checks to complain about the > use of "-a" and "-o" to the "test" shell built-in to to mean "and" and > "or", as opposed to using two "test" invocations with "&&" or "||". > > There aren't any portability issues with using that construct that I > know of, but since Junio expressed a dislike of it in [1] and we've > currently got no such constructs let's add it to the lint checking. I > had various in-flight and WIP patches that used this construct. It's not only Junio's dislike of `-a` and `-o` but also that they have long been considered obsolescent by POSIX[1]. GNU has likewise warned against it[2] for a few decades. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16 [2]: https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Limitations-of-Builtins.html > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl > @@ -35,6 +35,7 @@ sub err { > + # Portability issues > @@ -48,6 +49,9 @@ sub err { > + # Coding style preferences Given that these flags are considered obsolescent, thus may someday be dropped from some implementations, these seem a better fit for the "portability issues" section than the "coding style preferences" section. (I'd probably just drop those section titles, though.) > + /\btest\s+-[a-z]\s+.*?\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"'; > + /\btest\s+-[a-z]\s+.*?\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"'; These will only match the simplistic forms of `test -X blah` (where "-X" is some single letter option), but will miss expressions such as `test "$foo" = bar`. Crafting a regex to match more generally would be non-trivial, so this simpler match is a reasonable start. Okay.
"Eric Sunshine" <sunshine@sunshineco.com> wrote: > On Mon, Apr 12, 2021 at 7:09 AM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: > > Expand the t/check-non-portable-shell.pl checks to complain about the > > use of "-a" and "-o" to the "test" shell built-in to to mean "and" and > > "or", as opposed to using two "test" invocations with "&&" or "||". > > There aren't any portability issues with using that construct that I > > know of, but since Junio expressed a dislike of it in [1] and we've > > currently got no such constructs let's add it to the lint checking. I > > had various in-flight and WIP patches that used this construct. > It's not only Junio's dislike of `-a` and `-o` but also that they have > long been considered obsolescent by POSIX[1]. GNU has likewise warned > against it[2] for a few decades. > [1]: > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html#tag_20_128_16 > [2]: > https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Limitations-of-Builtins.html And also in Git's Documentation/CodingGuidelines, which motivates the advice with an example: test -n "$x" -a "$a" = "$b" is buggy and breaks when $x is "=", but test -n "$x" && test "$a" = "$b" does not have such a problem.
Matthieu Moy <git@matthieu-moy.fr> writes: > And also in Git's Documentation/CodingGuidelines, which motivates the advice with an example: > > test -n "$x" -a "$a" = "$b" > > is buggy and breaks when $x is "=", but > > test -n "$x" && test "$a" = "$b" > > does not have such a problem. Good advice to cite Documentation/CodingGuidelines in the log message. Thanks (and nice to hear from you again ;-).
Eric Sunshine <sunshine@sunshineco.com> writes: > Given that these flags are considered obsolescent, thus may someday be > dropped from some implementations, these seem a better fit for the > "portability issues" section than the "coding style preferences" > section. (I'd probably just drop those section titles, though.) > >> + /\btest\s+-[a-z]\s+.*?\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"'; >> + /\btest\s+-[a-z]\s+.*?\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"'; > > These will only match the simplistic forms of `test -X blah` (where > "-X" is some single letter option), but will miss expressions such as > `test "$foo" = bar`. Crafting a regex to match more generally would be > non-trivial, so this simpler match is a reasonable start. Okay. Would it be a trivial improvement to do 'test', followed by anything other than '&' or '|', and then followed by '-a' or '-o' instead?
On Mon, Apr 12, 2021 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >> + /\btest\s+-[a-z]\s+.*?\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"'; > >> + /\btest\s+-[a-z]\s+.*?\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"'; > > > > These will only match the simplistic forms of `test -X blah` (where > > "-X" is some single letter option), but will miss expressions such as > > `test "$foo" = bar`. Crafting a regex to match more generally would be > > non-trivial, so this simpler match is a reasonable start. Okay. > > Would it be a trivial improvement to do > > 'test', followed by anything other than '&' or '|', and then > followed by '-a' or '-o' > > instead? That seems plausible and trivial enough. In fact, I think it eliminates a source of false-positives that Ævar's pattern can report, such as: test whatever && ls -a foo and test whatever && foo -o outfile among others.
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index fd3303552be..a5367346255 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -35,6 +35,7 @@ sub err { next if $line =~ s/\\$//; $_ = $line; + # Portability issues /\bcp\s+-a/ and err 'cp -a is not portable'; /\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)'; /\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)'; @@ -48,6 +49,9 @@ sub err { /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and err '"FOO=bar shell_func" assignment extends beyond "shell_func"'; + # Coding style preferences + /\btest\s+-[a-z]\s+.*?\s+-a\s+/ and err '"test A && test B" preferred to "test A -a B"'; + /\btest\s+-[a-z]\s+.*?\s+-o\s+/ and err '"test A || test B" preferred to "test A -o B"'; $line = ''; # this resets our $. for each file close ARGV if eof;
Expand the t/check-non-portable-shell.pl checks to complain about the use of "-a" and "-o" to the "test" shell built-in to to mean "and" and "or", as opposed to using two "test" invocations with "&&" or "||". There aren't any portability issues with using that construct that I know of, but since Junio expressed a dislike of it in [1] and we've currently got no such constructs let's add it to the lint checking. I had various in-flight and WIP patches that used this construct. 1. https://lore.kernel.org/git/xmqqa6qkb5fi.fsf@gitster.g/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/check-non-portable-shell.pl | 4 ++++ 1 file changed, 4 insertions(+)