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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2509 bytes --]

On Fri, Nov 10, 2023 at 08:02:32AM +0900, Junio C Hamano wrote:
> 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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]