On Wed, Apr 21 2021, Đoàn Trần Công Danh wrote:
> 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
I think those patches are good in their own right, i.e. replacing things
with more incremental helpers and test_cmp-like functions.
But I believe the code you're changing is not non-portable. It was using
the output of "wc -l" with the "=" operator that wasn't portable.
These ones are all occurances that use "-eq".
And:
test "0" -eq " 0"
etc., is true, which is why these pass on OSX and beyond.
