Re: [PATCH 10/15] scalar: implement the `run` command

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Sep 8, 2021, 19:11 UTC
Message-ID: <nycvar.QRO.7.76.6.2109082100180.55@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqk0jxft1p.fsf@gitster.g>

Hi Junio,

On Fri, 3 Sep 2021, Junio C Hamano wrote:

Show 43 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > Hi Ævar,
> >
> > On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:
> >>
> >> > +	const char *usagestr[] = { NULL, NULL };
> >>
> >> Missing usage strings?
> >
> > This command will show a generated usage, i.e. a non-static string. It
> > therefore cannot be specified here already. See the `strbuf_*()` calls
> > populating `buf` and the `usagestr[0] = buf.buf;` assignment.
> >
> >> > +	if (argc == 0)
> >>
> >> Style nit (per style guide): s/argc == 0/!argc/g.
> >
> > It is true that we often do this, but in this instance it would be
> > misleading: `argc` is a counter, not a Boolean.
>
> That argument could be a plausible excuse to deviate from the style
> if it were
>
> 	if (argc == 0)
> 		do no args case;
> 	else if (argc == 1)
> 		do one arg case;
> 	else if (argc == 2)
> 		do two args case;
> 	...
>
> Replacing the first one with "if (!argc)" may make it less readable.
>
> But I do not think the reasoning applies here
>
> 	if (argc == 0)
> 		do a thing that applies only to no args case;
>
> without "else".  This is talking about "do we have any argument? Yes
> or no?" Boolean here.

Well, I offer a differing opinion. But you're right, we are at least consistent in Git's source code in using `!i` where other projects would use `i == 0`, and consistency is definitely something I'd like to see more in Git, not less.

So I changed it as you suggested.

Show 16 quoted lines

>
> >> > +	if (!strcmp("all", argv[0]))
> >> > +		i = -1;
> >>
> >> Style nit (per style guide): missing braces here.
> >
> > The style guide specifically allows my preference to leave single-line
> > blocks without curlies.
>
> Actually, the exception goes the other way, no?
>
> We generally want to avoid such an unnecessary braces around a
> single statement block.  But when we have an else clause that has a
> block with multiple statements (hence braces are required), as an
> exception, the guide asks you to write braces around the body of the
> if side for consistency.

You're right. I am somehow still using the previous style where we _required_ single-line blocks _not_ to have curly brackets (see e.g. aa1c48df817 ([PATCH] ls-tree enhancements, 2005-04-15), the `else` part of the added `if (! eltbuf)` block).

Show 5 quoted lines

>
> When you only have just a couple of lines on the "else {}" side, I
> do not think it matters too much either way for readability, though.
> I cannot see the "else" side in the above clause, but IIRC it wasn't
> just a few lines, was it?

It depends what you count as "just a few lines". There are seven lines enclosed within the curly brackets of the `else` block.

But as much as I enjoy thorough reviews of the Scalar code, I am failing at getting excited about code style discussions, therefore I simply went with your suggestion to enclose even the single-line block in curly brackets.

Thanks, Dscho