Junio C Hamano <gitster@xxxxxxxxx> 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 :)
