Hi Junio,
On Fri, 3 Sep 2021, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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.
>
> >> > + 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
