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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 9, 2023, 23:02 UTC
Message-ID: <xmqq8r76zg1j.fsf@gitster.g>
In-Reply-To: <20231109185515.GD2711684@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

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

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.

Show 9 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 "||".

Yes, but why not consistently use the range notation like the earlier one here, or below?

	elif test $# -ge 1 && test $# -le 2

Show 7 quoted lines

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