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

Hi Junio,

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

Johannes Schindelin [off-list ref] writes:

quoted

Hi Ævar,

On Tue, 31 Aug 2021, Ævar Arnfjörð Bjarmason wrote:

quoted

On Mon, Aug 30 2021, Derrick Stolee via GitGitGadget wrote:

quoted

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

quoted

quoted

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

quoted

quoted

quoted

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

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