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

From: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Date: Mar 1, 2022, 06:38 UTC
Message-ID: <20220301063801.26732-1-chakrabortyabhradeep79@gmail.com>
In-Reply-To: <xmqqzgma287n.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

Show 16 quoted lines

>  (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 :)