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

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 10, 2023, 10:18 UTC
Message-ID: <ZU4DiTQbKyuuT55k@tanuki>
In-Reply-To: <xmqq8r76zg1j.fsf@gitster.g>

On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:

Show 33 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >>  # 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.
> 
> I wish we could write something like
> 
> 	assert test $# -ge 2 && test $# -le 3
> 
> (and I'd allow double quoting the whole thing after assert if
> needed) but we cannot do so without tweaking the implementation of
> assert.
> 
> >
> >> @@ -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 "||".
> 
> Yes, but why not consistently use the range notation like the
> earlier one here, or below?

I opted to go for the "obvious" conversion, if there was one easily available, to make the diff easier to read. We could of course use a range notation like this:

 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 || test $# -eq 2
+	elif test $# -ge 1 && test $# -le 2
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"

Or :

 		rev=$(git rev-parse HEAD)
-	elif test $# -eq 1 || test $# -eq 2
+	elif ! { test $# -lt 1 || test $# -gt 2; }
 	then
 		rev=$(git rev-parse -q --verify "$1^{commit}") ||
 			die "fatal: '$1' does not refer to a commit"

But both of these are not consistent with the other cases due to the negation here, and both of them are harder to read in my opinion. So I'm not sure whether we gain anything by trying to make this a bit more consistent with the other conversions.

Patrick

Show 13 quoted lines

> 	elif test $# -ge 1 && test $# -le 2
> 
> >>  cmd_merge () {
> >> -	test $# -eq 1 -o $# -eq 2 ||
> >> +	if test $# -lt 1 || test $# -gt 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).
> 
> Yeah, the end result looks good.
> 
> Thanks, both.