[PATCH] parse-options: make parse_options_check() test-only
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 1, 2022, 20:08 UTC
- Message-ID
- <xmqqr17lphav.fsf_-_@gitster.g>
- In-Reply-To
- <xmqqzgma287n.fsf@gitster.g>
The array of options given to the parse-options API is sanity checked for reuse of a single-letter option for multiple entries and other programmer mistakes by calling parse_options_check() from parse_options_start(). This allows our developers to catch silly mistakes early, but all callers of parse-options API pays this cost. Once the set of options in an array is validated and passes this check, until a programmer modifies the array, there is no way for it to fail the check, which is wasteful.
Introduce the GIT_TEST_PARSE_OPTIONS_CHECK environment variable and make the sanity check only when it is set to true. Set it in t/test-lib.sh so that our tests will continue to catch buggy options arrays.
Signed-off-by: Junio C Hamano <gitster@pobox.com> ---
> (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 looked too easy and there may be some pitfalls, but I am
hoping that we will know soon enough by floating a weather
balloon like this.parse-options.c | 12 +++++++++++- t/README | 5 +++++ t/test-lib.sh | 3 +++ 3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c index 6e57744fd2..02cfe3f2cd 100644 --- a/parse-options.c +++ b/parse-options.c @@ -439,6 +439,14 @@ static void check_typos(const char *arg, const struct option *options) } } +/* + * Check the sanity of contents of opts[] array to find programmer + * mistakes (like duplicated short options). + * + * This function is supposed to be no-op when it returns without + * dying, making a call from parse_options_start_1() to it optional + * in end-user builds. + */ static void parse_options_check(const struct option *opts) { int err = 0; @@ -523,7 +531,9 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx, if ((flags & PARSE_OPT_ONE_SHOT) && (flags & PARSE_OPT_KEEP_ARGV0)) BUG("Can't keep argv0 if you don't have it"); - parse_options_check(options); + + if (git_env_bool("GIT_TEST_PARSE_OPTIONS_CHECK", 0)) + parse_options_check(options); } void parse_options_start(struct parse_opt_ctx_t *ctx, diff --git a/t/README b/t/README index f48e0542cd..b7285531f2 100644 --- a/t/README +++ b/t/README @@ -472,6 +472,11 @@ a test and then fails then the whole test run will abort. This can help to make sure the expected tests are executed and not silently skipped when their dependency breaks or is simply not present in a new environment. +GIT_TEST_PARSE_OPTIONS_CHECK=<boolean>, when true, makes all options +array passed to the parse-options API to be sanity checked. This +environment variable is set to true by test-lib.sh unless it is set. + + Naming Tests ------------ diff --git a/t/test-lib.sh b/t/test-lib.sh index e4716b0b86..805f495fd4 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -474,6 +474,9 @@ export GIT_DEFAULT_HASH GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}" export GIT_TEST_MERGE_ALGORITHM +: ${GIT_TEST_PARSE_OPTIONS_CHECK:=1} +export GIT_TEST_PARSE_OPTIONS_CHECK + # Tests using GIT_TRACE typically don't want <timestamp> <file>:<line> output GIT_TRACE_BARE=1 export GIT_TRACE_BARE
-- 2.35.1-354-g715d08a9e5