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: 2282 bytes --]

On Thu, Nov 09, 2023 at 01:55:15PM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 11:53:35AM +0100, Patrick Steinhardt wrote:
> 
> > 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.
> 
> >  # 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 really hate to admit how long I've pondered over this patch series in
total, up to the point where I did a `git rebase --reset-author-date` at
the end just so that it's not obvious. So I totally get everyone who
needs to stop and think for a bit here.

Will adapt the commit message.

Patrick

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

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