[v4,2/2] parse-options.c: add style checks for usage-strings

Commit Message

Abhradeep Chakraborty Feb. 25, 2022, 5:23 a.m. UTC

From: Abhradeep Chakraborty <[e-mail]>

`parse-options.c` doesn't check if the usage strings for option flags
are following the style guide or not. Style convention says, usage
strings should not start with capital letter (unless needed) and
it should not end with `.`.

Add checks to the `parse_options_check()` function to check usage
strings against the style convention.

Signed-off-by: Abhradeep Chakraborty <[e-mail]>
---
 parse-options.c               | 11 +++++++++++
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

Comments

Junio C Hamano Feb. 25, 2022, 6:13 a.m. UTC | #1

"Abhradeep Chakraborty via GitGitGadget" <[e-mail]>
writes:

> +
> +		// OPTION_GROUP should be ignored
> +		// if the first two characters of the help string are uppercase, then assume it is an
> +		// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
> +		// else assume the usage string is violating the style convention and throw error.

Style.

	/*
	 * This is how our multi-line comments
         * look like; with slash-asterisk that opens
         * and asterisk-slash that closes one on their
         * own lines.
	 */

Also avoid overly long lines.

> +		if (opts->type != OPTION_GROUP && opts->help &&
> +			opts->help[0] && isupper(opts->help[0]) &&
> +			!(opts->help[1] && isupper(opts->help[1])))
> +			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
> +		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
> +			err |= optbug(opts, xstrfmt("help should not end with a dot: %s", opts->help));

These two calls to optbug() use xstrfmt() to grab allocated pieces
of memory and pass it as a parameter to the function, which means
the string is leaked without any chance to be freed.

Do we care?

>  		if (opts->argh &&
>  		    strcspn(opts->argh, " _") != strlen(opts->argh))
>  			err |= optbug(opts, "multi-word argh should use dash to separate words");

The existing use of optbug() we see here does not share such a
problem.

Abhradeep Chakraborty Feb. 25, 2022, 8:08 a.m. UTC | #2

Junio C Hamano wrote:

> Style.
>
>	/*
>        * This is how our multi-line comments
>        * look like; with slash-asterisk that opens
>        * and asterisk-slash that closes one on their
>        * own lines.
>	 */
>
> Also avoid overly long lines.

Oh, sorry for that. I was in kind of a hurry ( today was
my semester exam), so I didn't look at the style guide.
Will fix it.

> These two calls to optbug() use xstrfmt() to grab allocated pieces
> of memory and pass it as a parameter to the function, which means
> the string is leaked without any chance to be freed.
>
> Do we care?
>
> >  		if (opts->argh &&
> >  		    strcspn(opts->argh, " _") != strlen(opts->argh))
> >  			err |= optbug(opts, "multi-word argh should use dash to separate words");
>
> The existing use of optbug() we see here does not share such a
> problem.

hmm, I wanted a formatting function to format (i.e. pass the
`opt->help` dynamically) the output string. The existing use of
`optbug()` that you specified has no `%s` formatter; it is a plain
string. That's why I used `xstrfmt()`. Moreover, it was in Ævar's
suggestion[1] -

> +		if (opts->help && ends_with(opts->help, "."))
> +			err |= optbug(opts, xstrfmt("argh should not end with a dot: %s", opts->help));

But I think, you're right. There is some memory leakage here.
Should I go with plain strings then? (i.e. "help should not end
with a dot" instead of `xstrfmt("help should not end with a dot:
%s", opts->help)`)

Thanks :)

[1] https://lore.kernel.org/git/[e-mail]/

Johannes Schindelin Feb. 25, 2022, 3:36 p.m. UTC | #3

Hi Abhradeep,

On Fri, 25 Feb 2022, Abhradeep Chakraborty via GitGitGadget wrote:

> From: Abhradeep Chakraborty <[e-mail]>
>
> `parse-options.c` doesn't check if the usage strings for option flags
> are following the style guide or not. Style convention says, usage
> strings should not start with capital letter (unless needed) and
> it should not end with `.`.
>
> Add checks to the `parse_options_check()` function to check usage
> strings against the style convention.

As I just pointed out in
https://lore.kernel.org/git/[e-mail]/,
it seems that replacing the static check presented in v1 by a runtime
check needs to be reverted.

In addition to the example I presented, there is another compelling reason
to do so: with the static check, we can detect incorrect usage strings in
all code, even in code that is platform-dependent (such as in
`fsmonitor--daemon`).

Ciao,
Dscho

Abhradeep Chakraborty Feb. 25, 2022, 4:01 p.m. UTC | #4

Johannes Schindelin <[e-mail]> wrote:

> As I just pointed out in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejv=
> aqbjf.bet/,
> it seems that replacing the static check presented in v1 by a runtime
> check needs to be reverted.
>
> In addition to the example I presented, there is another compelling reason
> to do so: with the static check, we can detect incorrect usage strings in
> all code, even in code that is platform-dependent (such as in
> `fsmonitor--daemon`).

First of all, thank you so much for putting so much time to look into
my PR. I appriciate your research about various possible outcomes of this
Patch request.

I saw your mail where you listed some of the disadvantages of the current
version. I also agree with the arguments you provided and it is also true
that one wouldn't find any clue by seeing the output of the `CI` link
you mentioned (i.e. https://github.com/git/git/runs/5312914410?check_suite_focus=true).

Let's see what Junio, Ævar and others say about this.

Thanks :)

Junio C Hamano Feb. 25, 2022, 5:06 p.m. UTC | #5

Abhradeep Chakraborty <[e-mail]> writes:

>> These two calls to optbug() use xstrfmt() to grab allocated pieces
>> of memory and pass it as a parameter to the function, which means
>> the string is leaked without any chance to be freed.
>>
>> Do we care?
>>
>> >  		if (opts->argh &&
>> >  		    strcspn(opts->argh, " _") != strlen(opts->argh))
>> >  			err |= optbug(opts, "multi-word argh should use dash to separate words");
>>
>> The existing use of optbug() we see here does not share such a
>> problem.
>
> hmm, I wanted a formatting function to format (i.e. pass the
> `opt->help` dynamically) the output string. The existing use of
> `optbug()` that you specified has no `%s` formatter; it is a plain
> string. That's why I used `xstrfmt()`. Moreover, it was in Ævar's
> suggestion[1] -
>
>> +		if (opts->help && ends_with(opts->help, "."))
>> +			err |= optbug(opts, xstrfmt("argh should not end with a dot: %s", opts->help));
>
> But I think, you're right. There is some memory leakage here.
> Should I go with plain strings then? (i.e. "help should not end
> with a dot" instead of `xstrfmt("help should not end with a dot:
> %s", opts->help)`)

Sorry that I've given you a trick question, when I know you are
quite new to the community.

I think the right answer to "Do we care?" is "In this case, because
we are about to call exit(), we don't care.  The extra complexity
and code necessary to retain the memory we get from xstrfmt and free
it is not worth it."  It's not like we do this in a loop that iterates
unbounded number of times before the exit() happens (in which case
we should care).

Thanks.

Junio C Hamano Feb. 26, 2022, 1:36 a.m. UTC | #6

Johannes Schindelin <[e-mail]> writes:

>> Add checks to the `parse_options_check()` function to check usage
>> strings against the style convention.
>
> As I just pointed out in
> https://lore.kernel.org/git/[e-mail]/,
> it seems that replacing the static check presented in v1 by a runtime
> check needs to be reverted.

Sorry, but I am not sure how that conclusion follows from a breakage
in a topic in flight that was discovered by the check.

I do not know if a coccinelle based solution is sufficiently easy,
simple and robust enough to encourage us to scrap what has already
been proposed and reviewed, instead of leaving it as a topic for a
future incremental improvement that we can make on top.

> In addition to the example I presented, there is another compelling reason
> to do so: with the static check, we can detect incorrect usage strings in
> all code, even in code that is platform-dependent (such as in
> `fsmonitor--daemon`).

Yes and no.  

I would imagine that large enough platforms that have their own
conditionally compiled #ifdef/#endif block already have CI to build
their conditionally compiled block in practice.  I do not see the
above as a compelling reason to grow and shift the scope of these
two patches.

Thanks.

Abhradeep Chakraborty Feb. 26, 2022, 3:57 a.m. UTC | #7

Junio C Hamano <[e-mail]> wrote:

> Sorry that I've given you a trick question, when I know you are
> quite new to the community.

There is nothing to say `sorry`. Every review comment is teaching me
new things. E.g. If you didn't ask me this question, I would not go to
the codebase and see the proper handling of `xstrfmt`. So, thanks.

> I think the right answer to "Do we care?" is "In this case, because
> we are about to call exit(), we don't care.  The extra complexity
> and code necessary to retain the memory we get from xstrfmt and free
> it is not worth it."  It's not like we do this in a loop that iterates
> unbounded number of times before the exit() happens (in which case
> we should care).
>
> Thanks.

Got it.

Thanks :)

Junio C Hamano Feb. 26, 2022, 6:08 a.m. UTC | #8

Junio C Hamano <[e-mail]> writes:

> I would imagine that large enough platforms that have their own
> conditionally compiled #ifdef/#endif block already have CI to build
> their conditionally compiled block in practice.  I do not see the
> above as a compelling reason to grow and shift the scope of these
> two patches.

Let's instead drop [2/2] for now.  I do not want us to go back to
shell script that pretends to know about C language, and I do not
want to block [1/2] by waiting for a replacement.  Fixes in [1/2]
are pretty much uncontroversial ones that can even be fast-tracked
down to 'master'.

Abhradeep Chakraborty Feb. 26, 2022, 6:57 a.m. UTC | #9

Junio C Hamano <[e-mail]> wrote:

> Let's instead drop [2/2] for now.  I do not want us to go back to
> shell script that pretends to know about C language, and I do not
> want to block [1/2] by waiting for a replacement.  Fixes in [1/2]
> are pretty much uncontroversial ones that can even be fast-tracked
> down to 'master'.

Though, as a new contributor, I felt bad about dropping the last
patch, but if you think the last patch request needs more discussion
( which I think is needed) - I also in favour of dropping the last
commit.

Would you do this on your side or I will re-submit it with the first
commit?

Thanks :)

Junio C Hamano Feb. 27, 2022, 7:15 p.m. UTC | #10

Abhradeep Chakraborty <[e-mail]> writes:

> Junio C Hamano <[e-mail]> wrote:
>
>> Let's instead drop [2/2] for now.  I do not want us to go back to
>> shell script that pretends to know about C language, and I do not
>> want to block [1/2] by waiting for a replacement.  Fixes in [1/2]
>> are pretty much uncontroversial ones that can even be fast-tracked
>> down to 'master'.
>
> Though, as a new contributor, I felt bad about dropping the last
> patch, but if you think the last patch request needs more discussion
> ( which I think is needed) - I also in favour of dropping the last
> commit.
>
> Would you do this on your side or I will re-submit it with the first
> commit?

Nah, I can just discard the second commit and keep the first one.

Abhradeep Chakraborty Feb. 28, 2022, 7:39 a.m. UTC | #11

Junio C Hamano <[e-mail]> wrote:

> Nah, I can just discard the second commit and keep the first one.

Okay, that's great. But one thing I want to ask - How the discussion
for `adding check for usage strings` will be held i.e. Whether the
idea is discarded for now.

If it is not discarded, then how to proceed? Johannes prefers the first
version and Ævar prefers the `add check to parse-options.c` version.

Thanks :)

Junio C Hamano Feb. 28, 2022, 5:48 p.m. UTC | #12

Abhradeep Chakraborty <[e-mail]> writes:

> Okay, that's great. But one thing I want to ask - How the discussion
> for `adding check for usage strings` will be held i.e. Whether the
> idea is discarded for now.
>
> If it is not discarded, then how to proceed? Johannes prefers the first
> version and Ævar prefers the `add check to parse-options.c` version.

My take on it is that the "first version" that uses an ad-hoc shell
script will not become acceptably robust.  If coccinelle or other
static analyzer can help us check more reliably, that would be great
because we won't incur runtime cost of checking, like the embedded
check we added in the latest version that we are tentatively removing.

I also think Dscho simply overreacted only because the check broke
an in-flight topic that is from his group, which is not universally
built, and the tests in it was written in such a way that the error
output from the embedded check was not immediately available when
run in the CI, making it harder to debug.  None of that is a fault
in the approach of using the embedded check.

If the embedded check were there from the beginning, together with
tons of the existing checks done by parse_options_check(), the
developers themselves of the in-flight topic(s) would have caught
the problem, even before it hit the public CI.  I am very sure Dscho
wouldn't have complained or even noticed that you added a new check
to the parse_options_check().

So from my point of view, plan should be

 (0) I have been assuming that the check we removed tentatively is
     correct and the breakage in in-flight topic caught usage
     strings that were malformed.  If not, we need to tweak it to
     make sure it does not produce false positives.

 (1) Help Microsoft folks fix the in-flight topic with faulty usage
     strings.

 (2) Rethink if parse_options_check() can be made optional at
     runtime, which would (a) allow our test to enable it, and allow
     us to test all code paths that use parse_options() centrally,
     and (b) allow us to bypass the check while the end-user runs
     "git", to avoid overhead of checking the same option[] array,
     which does not change between invocations of "git", over and
     over again all over the world.

     We may add the check back to parse_options_check() after doing
     the above.  There are already tons of "check sanity of what is
     inside option[]" in there, and it would be beneficial if we can
     separate out from parse_options_start() the sanity checking
     code, regardless of this topic.

 (3) While (2) is ongoing, we can let people also explore static
     analysis possibilities.

Ævar Arnfjörð Bjarmason Feb. 28, 2022, 7:32 p.m. UTC | #13

On Mon, Feb 28 2022, Junio C Hamano wrote:

> [...]
> So from my point of view, plan should be
>
>  (0) I have been assuming that the check we removed tentatively is
>      correct and the breakage in in-flight topic caught usage
>      strings that were malformed.  If not, we need to tweak it to
>      make sure it does not produce false positives.
>
>  (1) Help Microsoft folks fix the in-flight topic with faulty usage
>      strings.

Agreed.

>  (2) Rethink if parse_options_check() can be made optional at
>      runtime, which would (a) allow our test to enable it, and allow
>      us to test all code paths that use parse_options() centrally,
>      and (b) allow us to bypass the check while the end-user runs
>      "git", to avoid overhead of checking the same option[] array,
>      which does not change between invocations of "git", over and
>      over again all over the world.
>
>      We may add the check back to parse_options_check() after doing
>      the above.  There are already tons of "check sanity of what is
>      inside option[]" in there, and it would be beneficial if we can
>      separate out from parse_options_start() the sanity checking
>      code, regardless of this topic.

This is a good idea, but while t0012-help.sh catches most of it, it
doesn't cover e.g. sub-commands that call parse_options() in N functions
one after the other.

We could that in t0012-help.sh with (pseudocode):

    for subcmd write verify ...
    do
        test_expect_success '...' 'git commit-graph $subcmd -h'
    done

etc., but that still won't catch *all* of it, and we don't have a way to
spew out "what commands use subcommands".

Hence why we need to run the rest of the test suite, and why these
things aren't some one-off GIT_TEST_ mode or t/helper/*.c code already.

>  (3) While (2) is ongoing, we can let people also explore static
>      analysis possibilities.

I think with in-flight concerns with (0) and (1) addressed what we have
here is really good enough for now, and we could just add it to the
existing parse_options_check() without needing (2) and (3) at this
point.

Abhradeep Chakraborty March 1, 2022, 6:38 a.m. UTC | #14

Junio C Hamano <[e-mail]> wrote:

> I  also think Dscho simply overreacted only because the check broke
> an in-flight topic that is from his group, which is not universally
> built, and the tests in it was written in such a way that the error
> output from the embedded check was not immediately available when
> run in the CI, making it harder to debug.  None of that is a fault
> in the approach of using the embedded check.
>
> If the embedded check were there from the beginning, together with
> tons of the existing checks done by parse_options_check(), the
> developers themselves of the in-flight topic(s) would have caught
> the problem, even before it hit the public CI.  I am very sure Dscho
> wouldn't have complained or even noticed that you added a new check
> to the parse_options_check().

Hmm, that's true.

>  (2) Rethink if parse_options_check() can be made optional at
>      runtime, which would (a) allow our test to enable it, and allow
>      us to test all code paths that use parse_options() centrally,
>      and (b) allow us to bypass the check while the end-user runs
>      "git", to avoid overhead of checking the same option[] array,
>      which does not change between invocations of "git", over and
>      over again all over the world.
>
>      We may add the check back to parse_options_check() after doing
>      the above.  There are already tons of "check sanity of what is
>      inside option[]" in there, and it would be beneficial if we can
>      separate out from parse_options_start() the sanity checking
>      code, regardless of this topic.
>
>  (3) While (2) is ongoing, we can let people also explore static
>      analysis possibilities.

I agree with you. But I think these two points(specially (2)) deserve
a dedicated discussion/patch thread. Because, the latest version of this
patch series (actually this patch series itself) only cares about the
`usage strings`.

So, I argue you to not discard the last commit for now. As you said `There are
already tons of "check sanity of what is inside option[]"`, integrating
one more sanity check would not affect it. I am saying it not because
I made that commit. The discussion or patch integration of (2) and (3)
may take few weeks (or more than a month may be; I also would like to
take part/contribute to that discussion/PR). I fear that another
set of invalid usage-strings would be added in that time. In that case,
we have to make another commit/PR for correcting those strings - disrupting
the purpose of this first commit you are willing to merge.

As Ævar also said - 

> I think with in-flight concerns with (0) and (1) addressed what we have
> here is really good enough for now, and we could just add it to the
> existing parse_options_check() without needing (2) and (3) at this
> point.

Thanks :)

Junio C Hamano March 1, 2022, 11:12 a.m. UTC | #15

Abhradeep Chakraborty <[e-mail]> writes:

>>  (2) Rethink if parse_options_check() can be made optional at
>> ...
>>  (3) While (2) is ongoing, we can let people also explore static
>>      analysis possibilities.
>
> I agree with you. But I think these two points(specially (2)) deserve
> a dedicated discussion/patch thread. Because, the latest version of this
> patch series (actually this patch series itself) only cares about the
> `usage strings`.

Yes, absolutely.  So applying [2/2] in haste is not a good idea at
all.  Before we accumulate more cruft on top, we should stop and
think if the approach we are taking is sensible to begin with, or
we'll make an already bad situation even worse.

Johannes Schindelin March 1, 2022, 7:37 p.m. UTC | #16

Hi Junio,

On Mon, 28 Feb 2022, Junio C Hamano wrote:

> Abhradeep Chakraborty <[e-mail]> writes:
>
> > Okay, that's great. But one thing I want to ask - How the discussion
> > for `adding check for usage strings` will be held i.e. Whether the
> > idea is discarded for now.
> >
> > If it is not discarded, then how to proceed? Johannes prefers the first
> > version and Ævar prefers the `add check to parse-options.c` version.
>
> My take on it is that the "first version" that uses an ad-hoc shell
> script will not become acceptably robust.

It is unfortunate that the challenge had been characterized as "parse C",
when in reality we are talking about highly idiomatic code. It's not like
we accept arbitrary input in the `OPT_...()` lines. We _really_ want the
option usage string to be a string that is enclosed in `N_()`.

Additionally, this is about Git's own code, not arbitrary C code provided
by users. That makes that shell script more on par with `t/chainlint.sed`
than with `contrib/coccinelle/*`.

Having said that...

> If coccinelle or other static analyzer can help us check more reliably,
> that would be great because we won't incur runtime cost of checking,
> like the embedded check we added in the latest version that we are
> tentatively removing.

I think that Julia gave us enough to work with, so we can (ab-)use
Coccinelle for static usage string checks, and we should probably do that,
too.

> I also think Dscho simply overreacted only because the check broke
> an in-flight topic that is from his group, which is not universally
> built, and the tests in it was written in such a way that the error
> output from the embedded check was not immediately available when
> run in the CI, making it harder to debug.  None of that is a fault
> in the approach of using the embedded check.

No, I would have reacted the same way if I had seen the failures in any
other topic, with an equally trivial fix that blooms into 42 separate test
case failures.

This explosion made me realize _why_ I found the suggestion to patch
`parse_options()` iffy in the first place: it replaces a static check with
a runtime check, which is almost always something that is regretted later.

And since Abhradeep is a new contributor, I found it important to steer
the direction toward sound advice that they can use over and over again
over the course of their career: whenever possible, prefer static checks
over runtime ones.

> If the embedded check were there from the beginning, together with
> tons of the existing checks done by parse_options_check(), the
> developers themselves of the in-flight topic(s) would have caught
> the problem, even before it hit the public CI.  I am very sure Dscho
> wouldn't have complained or even noticed that you added a new check
> to the parse_options_check().

Indeed, if no static check had been proposed first, I would not have
caught on to the unfortunate suggestion to use a runtime check _instead_.

> So from my point of view, plan should be
>
>  (0) I have been assuming that the check we removed tentatively is
>      correct and the breakage in in-flight topic caught usage
>      strings that were malformed.  If not, we need to tweak it to
>      make sure it does not produce false positives.
>
>  (1) Help Microsoft folks fix the in-flight topic with faulty usage
>      strings.

You're so sweet, but I already did that in parallel.

>
>  (2) Rethink if parse_options_check() can be made optional at
>      runtime, which would (a) allow our test to enable it, and allow
>      us to test all code paths that use parse_options() centrally,
>      and (b) allow us to bypass the check while the end-user runs
>      "git", to avoid overhead of checking the same option[] array,
>      which does not change between invocations of "git", over and
>      over again all over the world.
>
>      We may add the check back to parse_options_check() after doing
>      the above.  There are already tons of "check sanity of what is
>      inside option[]" in there, and it would be beneficial if we can
>      separate out from parse_options_start() the sanity checking
>      code, regardless of this topic.
>
>  (3) While (2) is ongoing, we can let people also explore static
>      analysis possibilities.

Of course, if we can convince Coccinelle (together with Python) to give us
the static check, we might very well be able to port more of
`parse_options_check()` from runtime checks to static ones, which would be
a clear win.

If that is possible, we could save ourselves a lot of time by skipping (2)
altogether.

And as I said, Julia's advice looked really good. If only I wasn't
desperately short on time, I would have given it a try because it sounds
not only fun but also very, very useful in Git's context.

Ciao,
Dscho

Abhradeep Chakraborty March 3, 2022, 5:34 p.m. UTC | #17

Johannes Schindelin <[e-mail]> wrote:

> And since Abhradeep is a new contributor, I found it important to steer
> the direction toward sound advice that they can use over and over again
> over the course of their career: whenever possible, prefer static checks
> over runtime ones.

Thanks Johannes for the advice. I will always remember it ^^

> Of course, if we can convince Coccinelle (together with Python) to give us
> the static check, we might very well be able to port more of
> `parse_options_check()` from runtime checks to static ones, which would be
> a clear win.
>
> If that is possible, we could save ourselves a lot of time by skipping (2)
> altogether.
>
> And as I said, Julia's advice looked really good. If only I wasn't
> desperately short on time, I would have given it a try because it sounds
> not only fun but also very, very useful in Git's context.

Since Junio and you both have an interest in Coccinelle, if you allow,
I want to look into it.

Thanks :)

Junio C Hamano March 3, 2022, 10:30 p.m. UTC | #18

Abhradeep Chakraborty <[e-mail]> writes:

> Johannes Schindelin <[e-mail]> wrote:
>
>> And since Abhradeep is a new contributor, I found it important to steer
>> the direction toward sound advice that they can use over and over again
>> over the course of their career: whenever possible, prefer static checks
>> over runtime ones.
>
> Thanks Johannes for the advice. I will always remember it ^^

Yup, if we can have static and dynamic checks of the same quality,
static checks are always better alternative.  In this case, runtime
check would probably be an expedite solution suitable for a shorter
term to fill the gap, as a static check with the same quality as it
would probably need some time to develop.

> Since Junio and you both have an interest in Coccinelle, if you allow,
> I want to look into it.

I do not have any particular interest.  If it is a tool fit for the
task, it would be good to use it, that's all ;-)

Abhradeep Chakraborty March 4, 2022, 2:21 p.m. UTC | #19

Junio C Hamano <<[e-mail]> wrote:

> Yup, if we can have static and dynamic checks of the same quality,
> static checks are always better alternative.  In this case, runtime
> check would probably be an expedite solution suitable for a shorter
> term to fill the gap, as a static check with the same quality as it
> would probably need some time to develop.

Got it!

> I do not have any particular interest.  If it is a tool fit for the
> task, it would be good to use it, that's all ;-)

Okay, then I would like to research if that is a good fit. Johannes
is pretty confident about it though.

Thanks :)

Johannes Schindelin March 7, 2022, 4:12 p.m. UTC | #20

Hi,

On Fri, 4 Mar 2022, Abhradeep Chakraborty wrote:

> Junio C Hamano <<[e-mail]> wrote:
>
> > Yup, if we can have static and dynamic checks of the same quality,
> > static checks are always better alternative.  In this case, runtime
> > check would probably be an expedite solution suitable for a shorter
> > term to fill the gap, as a static check with the same quality as it
> > would probably need some time to develop.
>
> Got it!

While the runtime check would address the concern in the short run, paving
the path for future static checks revolving around the same area will pay
off quite happily.

> > I do not have any particular interest.  If it is a tool fit for the
> > task, it would be good to use it, that's all ;-)
>
> Okay, then I would like to research if that is a good fit. Johannes
> is pretty confident about it though.

Yes, he is.

And he wishes he had the time to work on it himself because it sounds like
a really fun (if challenging) project.

In other words: If you ever get stuck somewhere along the lines, please do
push up a work-in-progress branch and reach out here so that I or others
can help.

Ciao,
Dscho

Abhradeep Chakraborty March 8, 2022, 5:44 a.m. UTC | #21

Johannes Schindelin <[e-mail]> wrote:

> Yes, he is.

> And he wishes he had the time to work on it himself because it sounds like
> a really fun (if challenging) project.
>
> In other words: If you ever get stuck somewhere along the lines, please do
> push up a work-in-progress branch and reach out here so that I or others
> can help.

Thank you so much ^^

Patch

diff --git a/parse-options.c b/parse-options.c
index 2437ad3bcdd..acd9ddbb372 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -492,6 +492,17 @@  static void parse_options_check(const struct option *opts)
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
+
+		// OPTION_GROUP should be ignored
+		// if the first two characters of the help string are uppercase, then assume it is an
+		// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
+		// else assume the usage string is violating the style convention and throw error.
+		if (opts->type != OPTION_GROUP && opts->help &&
+			opts->help[0] && isupper(opts->help[0]) &&
+			!(opts->help[1] && isupper(opts->help[1])))
+			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
+		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
+			err |= optbug(opts, xstrfmt("help should not end with a dot: %s", opts->help));
 		if (opts->argh &&
 		    strcspn(opts->argh, " _") != strlen(opts->argh))
 			err |= optbug(opts, "multi-word argh should use dash to separate words");
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 284fe18e726..2a07e130b96 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -53,7 +53,7 @@  test_expect_success 'setup optionspec-only-hidden-switches' '
 |
 |some-command does foo and bar!
 |--
-|hidden1* A hidden switch
+|hidden1* a hidden switch
 EOF
 '
 
@@ -131,7 +131,7 @@  test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --hidden1             a hidden switch
 |
 |EOF
 END_EXPECT