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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 28, 2022, 17:48 UTC
Message-ID: <xmqqzgma287n.fsf@gitster.g>
In-Reply-To: <20220228073908.20553-1-chakrabortyabhradeep79@gmail.com>

Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com> writes:

Show 6 quoted lines

> 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.