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 3
It 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.
> @@ -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
