| Message ID | cover.1699609940.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | Replace use of `test <expr> -o/a <expr>` | expand |
On Fri, Nov 10, 2023 at 11:01:11AM +0100, Patrick Steinhardt wrote: > this is the second version of my patch series that replaces all uses of > `test <expr> -o/a <expr`. > > Changes compared to v1: > > - I've expanded a bit on why we want to do these conversions in the > first place in the first commit message. > > - Dropped a needless subshell and added missing quoting while at it. > > - Explained why we need to decompose the asserts in the second patch > into two asserts. These look OK to me. I mentioned a small nit on the first patch, but I am OK with ignoring it (and we are reaching diminishing returns polishing an otherwise trivial series). -Peff
Hi, this is the second version of my patch series that replaces all uses of `test <expr> -o/a <expr`. Changes compared to v1: - I've expanded a bit on why we want to do these conversions in the first place in the first commit message. - Dropped a needless subshell and added missing quoting while at it. - Explained why we need to decompose the asserts in the second patch into two asserts. Thanks! Patrick Patrick Steinhardt (4): global: convert trivial usages of `test <expr> -a/-o <expr>` contrib/subtree: stop using `-o` to test for number of args contrib/subtree: convert subtree type check to use case statement Makefile: stop using `test -o` when unlinking duplicate executables GIT-VERSION-GEN | 2 +- Makefile | 2 +- configure.ac | 2 +- contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++----------- t/perf/perf-lib.sh | 2 +- t/perf/run | 9 +++++---- t/valgrind/valgrind.sh | 2 +- 7 files changed, 33 insertions(+), 20 deletions(-) Range-diff against v1: 1: c5e627eb3fe ! 1: 2967c8ebb46 global: convert trivial usages of `test <expr> -a/-o <expr>` @@ Commit message these to instead instead concatenate multiple invocations of `test` via `&&` and `||`, respectively. + While not all of the converted instances can cause ambiguity, it is + worth getting rid of all of them regardless: + + - It becomes easier to reason about the code as we do not have to + argue why one use of `-a`/`-o` is okay while another one isn't. + + - We don't encourage people to use these expressions. + Signed-off-by: Patrick Steinhardt <ps@pks.im> ## GIT-VERSION-GEN ## @@ GIT-VERSION-GEN: LF=' then VN=$(cat version) || VN="$DEF_VER" -elif test -d ${GIT_DIR:-.git} -o -f .git && -+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) && ++elif { test -d "${GIT_DIR:-.git}" || test -f .git; } && VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) && case "$VN" in *$LF*) (exit 1) ;; 2: b1ea45b8a88 ! 2: 977132d2236 contrib/subtree: stop using `-o` to test for number of args @@ Commit message of arguments we assert this via a single call to `test` with `-o`, which is discouraged by our coding guidelines. - Convert these cases to stop doing so. + Convert these cases to stop doing so. This requires us to decompose + assertions of the style `assert test $# = 2 -o $# = 3` into two calls + because we have no easy way to logically chain statements passed to the + assert function. Signed-off-by: Patrick Steinhardt <ps@pks.im> 3: 7c54d9070fa = 3: 761cde1b341 contrib/subtree: convert subtree type check to use case statement 4: bc9489ca5b8 = 4: 5326d86888a Makefile: stop using `test -o` when unlinking duplicate executables base-commit: dadef801b365989099a9929e995589e455c51fed