Re: [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args

From: Jeff King <peff@peff.net>
Date: Nov 9, 2023, 18:55 UTC
Message-ID: <20231109185515.GD2711684@coredump.intra.peff.net>
In-Reply-To: <b1ea45b8a8884d09ab070bb0f099834447d28938.1699526999.git.ps@pks.im>

On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:

Show 6 quoted lines

> 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.

Show 5 quoted lines

>  # 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.

Show 6 quoted lines

> @@ -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 "||".

Show 7 quoted lines

>  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