Jeff King <peff@xxxxxxxx> 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?
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.
