On 2021-04-21 10:46:08+0200, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote: > > 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) && With: 7dbe8c8003, (check-non-portable-shell.pl: `wc -l` may have leading WS, 2017-12-21) Unless the situation has been changed, since. I think those tests with quoted "$(.. | wc -l)" has been deemed unportable and should be replaced with test_line_count anyway? Does "test -eq" strip spaces from integer strings? And I think we're working on moving "git" command to its own commmand instead of put it in the left of a pipe. 2 followed patch will clean them out -- Danh
