Re: [PATCH] parse-options: make parse_options_check() test-only

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2022, 18:59 UTC
Message-ID: <xmqq1qzkmb8q.fsf@gitster.g>
In-Reply-To: <220302.86r17k7gry.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 10 quoted lines

>> Meaning that a parse-options array can be fed by "rev-parse --parseopt"
>> and having the sanity check enabled does help the use case?  Even there,
>> I would say that once the script writer finishes developing the script
>> that uses "rev-parse --parseopt", setting the parseopt input in stone,
>> there is no need to check the same thing over and over again.  Am I
>> mistaken?  Does "rev-parse --parseopt" that is fed the same input
>> sometimes trigger the sanity check and sometimes not?
>
> If we're declaring that "git rev-parse --parseopt" is something that was
> only ever intended for in-tree usage sure, that should hold true.

Show 8 quoted lines

> So out-of-tree users wouldn't guard against
> GIT_TEST_PARSE_OPTIONS_CHECK, and I wouldn't be surprised if we could
> e.g. segfault on some subsequent code if some of the sanity checks
> aren't happening anymore.
> ...
> No, I'd be quite happy if we declared that it's for our use only, and
> could remove it when the last in-tree *.sh user went away. there's a bit
> of complexity in parse_options() required only for its use....

I do not see any need for such a declaration. We are not changing the behaviour of "git rev-parse --parseopt" plumbing command at all for those who feed valid input to it.

"rev-parse --parseopt" users can keep using their scripts just the same as before, debugging their scripts to catch silly mistakes like duplicated short options may become slightly harder, but they still have a way to ask for the same debugging support available.

Yes, I am saying that is perfectly fine, and both in-tree and out-of-tree users have a way to reinstate the sanity checks. I also do not mind if your proposal were one of these:

 * introduce --parseopt-with-sanity-check to "rev-parse" and arrange
   the parse_options_check() call to be made when the command was
   invoked with it; or

 * introduce --parse-opt-without-sanity-check to "rev-parse", and
   arrange the parse_options_check() call to be still made when
   "--parse-opt" is used.  Those who finished developing their
   scripts can rewrite their --parse-opt to "without" version for
   conceptual cleanliness.

> So the performance cost is trivial & not worth worrying about.

I already said I am not worried about it, didn't I? These numbers do not matter in this discussion.