Johannes Schindelin [off-list ref] writes:
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.
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.
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.
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