Prefer '[] && []' to '[ -a ]' and '[] || []' to '[ -o ]' in tests #810
Conversation
|
The change might be fine as such, but the summary is unreadable and overall the message largely fails to explain why should one syntax be preferred over another one. "Because covscan" is not a proper reason. test(1) man page gives a clue: |
pavlinamv
force-pushed
the
sc2166
branch
3 times, most recently
from
25493a9
to
742c797
Compare
|
When pushing updates to address comments, make a note of what you changed here. It's not always that obvious, and pushes without comments go easily unnoticed overall. ""Prefer '[] && []' to '[ -a ]' and '[] || []' to '[ -o ]'" describes the literal change done by the patch. This is not useful information because that's what the patch itself is for! The commit summary + message is about rationale of the change. "in tests" is also problematic because it too is unnecessarily ambiguous - does it refer to the tests in testsuite or something else? Something along the lines of "Eliminate use of ambiguous logical operators in script conditionals" would be at least closer to the mark. |
Prefer '[] && []' to '[ -a ]' and '[] || []' to '[ -o ]' in tests. -a and -o to mean AND and OR in a [ .. ] test expression is not well defined, and can cause incorrect results when arguments start with dashes or contain !. Moreover binary -a and -o are inherently ambiguous. test(1) man page recommends to use 'test EXPR1 && test EXPR2' or 'test EXPR1 || test EXPR2' instead. It corrects warnings [SC2166] spotted by covscan.
|
The commit message is changed according to the review. |
|
The checks are failing due to not rebasing to get the "rawhide workaround" in place. The commit message is still unnecessarily hard to read but I give up. |
It corrects warnings [SC2166] spotted by covscan:
warning: Prefer [ p ] && [ q ] as [ p -a q ] is not well defined. [SC2166]
warning: Prefer [ p ] || [ q ] as [ p -o q ] is not well defined. [SC2166]
Motivated by last the comment in #802. This should fix all [sc2166] warnings spotted by covscan on RPM version 4.14.2.