Re: [PATCH v3 01/12] check-non-portable-shell: check for "test <cond> -a/-o <cond>"

On Wed, Apr 21 2021, Junio C Hamano wrote:

Ævar Arnfjörð Bjarmason  [off-list ref] writes:

quoted

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:

quoted

+	/\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).