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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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.