Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Jeff King <hidden>
Date: 2023-11-09 18:55:17
On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
Functions in git-subtree.sh all assert that they are being passed the correct number of arguments. In cases where we accept a variable number 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.
OK. I think these ones really are safe, because they're only expanding $#, but I agree with the principle to follow the guidelines.
# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
process_subtree_split_trailer () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3It took me a minute to figure out why we were swapping "=" for "-ge". It is because we want to logical-OR the two conditions, but "assert" requires that we test one at a time. I think that is probably worth explaining in the commit message.
quoted hunk ↗ jump to hunk
@@ -916,7 +919,7 @@ cmd_split () { if test $# -eq 0 then rev=$(git rev-parse HEAD) - elif test $# -eq 1 -o $# -eq 2 + elif test $# -eq 1 || test $# -eq 2
OK, this one is a straight-forward use of "||".
cmd_merge () {
- test $# -eq 1 -o $# -eq 2 ||
+ if test $# -lt 1 || test $# -gt 2
+ then
die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+ fi
+
But here we swap "-eq" for other operators. We have to because we went
from "||" to an "if". I think what you have here is correct, but you
could also write:
if ! { test $# -eq 1 || test $# -eq 2; }
(I am OK with either, it just took me a minute to verify that your
conversion was correct. But that is a one-time issue now while
reviewing, and I think the code is readable going forward).
-Peff