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

[Junio C Hamano <gitster@pobox.com>]

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.

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

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?

Thanks.