On Wed, Apr 21 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> 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`. We stop at "&" or "|" to try not to overmatch >> things like: >> >> test whatever && ls -a foo >> test whatever && foo -o outfile > > I still do not understand why you have to insist on dashed operator > as the first thing given to "test", like this: > >> + /\btest\s+-[a-z]\s+[^&|]+\s+-a\s+/ and err '"test A && test B" ... >> + /\btest\s+-[a-z]\s+[^&|]+\s+-o\s+/ and err '"test A || test B" ... > > IOW, what over-matching would we get if we simplified the condition > like so? > > /\btest\s+[^&|]+\s+-a\s/ > /\btest\s+[^&|]+\s+-o\s/ > > The one in the patch would miss things like > > test "$a" = "$b" -o "$a" -lt "$b" > test "$n" -a "$n" -lt 4 > > but the only thing that we care about is that a command that started > with "test " has "-a" or "-o" before we see "&" or "|", no? Applying your suggestion results in these false positives: t4038-diff-combined.sh:135: error: "test A && test B" preferred to "test A -a B": git commit -m "test space change" -a && t4038-diff-combined.sh:147: error: "test A && test B" preferred to "test A -a B": git commit -m "test other space changes" -a && t6400-merge-df.sh:89: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l) t6400-merge-df.sh:91: error: "test A || test B" preferred to "test A -o B": test 1 -eq $(git ls-files -o | wc -l) t6400-merge-df.sh:110: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l) t6400-merge-df.sh:112: error: "test A || test B" preferred to "test A -o B": test 1 -eq $(git ls-files -o | wc -l) t6402-merge-rename.sh:639: error: "test A || test B" preferred to "test A -o B": test 0 -eq "$(git ls-files -o | wc -l)" t6402-merge-rename.sh:646: error: "test A || test B" preferred to "test A -o B": test 2 -eq "$(git ls-files -o | wc -l)" t6402-merge-rename.sh:686: error: "test A || test B" preferred to "test A -o B": test 0 -eq "$(git ls-files -o | wc -l)" && t6402-merge-rename.sh:865: error: "test A || test B" preferred to "test A -o B": test 0 -eq $(git ls-files -o | wc -l) && annotate-tests.sh:201: error: "test A && test B" preferred to "test A -a B": GIT_AUTHOR_NAME="E" GIT_AUTHOR_EMAIL="E at test dot git" git commit -a -m "norobots" I'll just drop this from the re-roll of this series. Maybe you/someone has a better suggestion for something that's simple but still catches these cases, but in any case I'd like to not have this series blocked on this minor thing (which none of the rest of it needs).
