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.
