From b8604e85aec871221cc6b220f1d423a9f5bdc740 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 12 Oct 2026 09:44:10 +0200 Subject: [PATCH 1/4] parse-options: add parse_options_takes_argument() Whether an option takes a value, and therefore consumes the next argument when that value is not stuck to it with an '=', is decided by its type and its flags. That rule is currently open-coded in show_gitcomp(), which needs it to decide if it should append an '=' to the option it completes. A following commit will need the same rule to find out which options an early scan of the command line has to skip along with their value. So let's factor that rule out into a new parse_options_takes_argument() function, and let's use it in show_gitcomp(). Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the next argument when it isn't the last one. It is considered as taking a value by parse_options_takes_argument(), as an early scan can easily check whether there is a next argument, and would otherwise wrongly take that argument for an option. show_gitcomp() keeps not appending an '=' to such an option though, as it already did, since its value is optional. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- parse-options.c | 35 ++++++++++++++++++++++------------- parse-options.h | 11 +++++++++++ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/parse-options.c b/parse-options.c index 4519ead9dc..d7cbb5a343 100644 --- a/parse-options.c +++ b/parse-options.c @@ -841,6 +841,25 @@ static void show_negated_gitcomp(const struct option *opts, int show_all, } } +int parse_options_takes_argument(const struct option *opt) +{ + switch (opt->type) { + case OPTION_STRING: + case OPTION_FILENAME: + case OPTION_INTEGER: + case OPTION_UNSIGNED: + case OPTION_CALLBACK: + break; + default: + return 0; + } + + if (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG)) + return 0; + + return 1; +} + static int show_gitcomp(const struct option *opts, int show_all) { const struct option *original_opts = opts; @@ -862,20 +881,10 @@ static int show_gitcomp(const struct option *opts, int show_all) break; case OPTION_GROUP: continue; - case OPTION_STRING: - case OPTION_FILENAME: - case OPTION_INTEGER: - case OPTION_UNSIGNED: - case OPTION_CALLBACK: - if (opts->flags & PARSE_OPT_NOARG) - break; - if (opts->flags & PARSE_OPT_OPTARG) - break; - if (opts->flags & PARSE_OPT_LASTARG_DEFAULT) - break; - suffix = "="; - break; default: + if (parse_options_takes_argument(opts) && + !(opts->flags & PARSE_OPT_LASTARG_DEFAULT)) + suffix = "="; break; } if (opts->flags & PARSE_OPT_COMP_ARG) diff --git a/parse-options.h b/parse-options.h index d7f896a933..589f8d9008 100644 --- a/parse-options.h +++ b/parse-options.h @@ -420,6 +420,17 @@ int parse_options(int argc, const char **argv, const char *prefix, const char * const usagestr[], enum parse_opt_flags flags); +/* + * Return non-zero if `opt` takes a value, which means that it consumes + * the next argument when that value is not stuck to it with an '='. + * + * Note that an option with PARSE_OPT_LASTARG_DEFAULT is considered as + * taking a value here, even though it only consumes the next argument + * when it isn't the last one. Callers that care have to check for that + * flag themselves. + */ +int parse_options_takes_argument(const struct option *opt); + NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); From 6e59330766a6331d7bf3fd299956feeb6bfff787 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 12 Oct 2026 09:44:11 +0200 Subject: [PATCH 2/4] parse-options: extract lookup_long_opt() from parse_long_opt() In a following commit, an early scan of the command line will need to look up long options the same way parse_options() does, including unambiguous abbreviations and negated forms, but without any side effect: it must neither set option values nor call die() or error(). parse_long_opt() can't be used for that, as it does both the look up and the actions that follow it: it calls get_value() on the option it finds, die()s when abbreviations are disallowed, and error()s on an ambiguous abbreviation. Let's extract a new lookup_long_opt() helper from parse_long_opt() to perform only the look up part, and let parse_long_opt() act on what lookup_long_opt() returns. There is no behavior change intended. In particular an exact match is still preferred to any abbreviation, abbreviations are still rejected when disallowed only if there is no exact match, and the ambiguity error message is unchanged. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- parse-options.c | 105 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 81 insertions(+), 24 deletions(-) diff --git a/parse-options.c b/parse-options.c index d7cbb5a343..a98b958423 100644 --- a/parse-options.c +++ b/parse-options.c @@ -516,16 +516,46 @@ static void register_abbrev(struct parse_opt_ctx_t *p, abbrev->flags = flags; } -static enum parse_opt_result parse_long_opt( - struct parse_opt_ctx_t *p, const char *arg, - const struct option *options) +enum long_opt_lookup { + LONG_OPT_EXACT, /* exact match found */ + LONG_OPT_ABBREV, /* unambiguous abbreviation found */ + LONG_OPT_AMBIGUOUS, /* more than one candidate abbreviation */ + LONG_OPT_UNKNOWN, /* nothing matches */ +}; + +/* + * Look up the long option named by `arg` (without its leading "--") + * without acting on it. (Neither prints nor dies.) + * + * Return value: + * - On LONG_OPT_EXACT, `found` is the exact matching option and its flags + * (including OPT_UNSET for a negated form), and `stuck` points after + * the '=' or is NULL. + * - On LONG_OPT_ABBREV, `found` is the unambiguous abbreviation, same as + * above for its flags and `stuck`. + * - On LONG_OPT_AMBIGUOUS, `found` and `other` are two candidates of an + * ambiguous abbreviation. + * - On LONG_OPT_UNKNOWN, nothing matches, no result. + * + * `other` is only meaningful for LONG_OPT_AMBIGUOUS. + */ +static enum long_opt_lookup lookup_long_opt(struct parse_opt_ctx_t *p, + const char *arg, + const struct option *options, + struct parsed_option *found, + struct parsed_option *other, + const char **stuck) { + enum opt_parsed flags = OPT_LONG; const char *arg_end = strchrnul(arg, '='); const char *arg_start = arg; - enum opt_parsed flags = OPT_LONG; int arg_starts_with_no_no = 0; - struct parsed_option abbrev = { .option = NULL, .flags = OPT_LONG }; - struct parsed_option ambiguous = { .option = NULL, .flags = OPT_LONG }; + + found->option = NULL; + found->flags = OPT_LONG; + other->option = NULL; + other->flags = OPT_LONG; + *stuck = NULL; if (skip_prefix(arg_start, "no-", &arg_start)) { if (skip_prefix(arg_start, "no-", &arg_start)) @@ -554,43 +584,70 @@ static enum parse_opt_result parse_long_opt( if (skip_prefix(arg_start, long_name, &rest)) { if (*rest == '=') - p->opt = rest + 1; + *stuck = rest + 1; else if (*rest) continue; - return get_value(p, options, flags ^ opt_flags); + found->option = options; + found->flags = flags ^ opt_flags; + return LONG_OPT_EXACT; } /* abbreviated? */ if (!strncmp(long_name, arg_start, arg_end - arg_start)) register_abbrev(p, options, flags ^ opt_flags, - &abbrev, &ambiguous); + found, other); /* negated and abbreviated very much? */ if (allow_unset && starts_with("no-", arg)) register_abbrev(p, options, OPT_UNSET ^ opt_flags, - &abbrev, &ambiguous); + found, other); } - if (disallow_abbreviated_options && (ambiguous.option || abbrev.option)) + if (other->option) + return LONG_OPT_AMBIGUOUS; + + if (found->option) { + if (*arg_end) + *stuck = arg_end + 1; + return LONG_OPT_ABBREV; + } + + return LONG_OPT_UNKNOWN; +} + +static enum parse_opt_result parse_long_opt(struct parse_opt_ctx_t *p, + const char *arg, + const struct option *options) +{ + struct parsed_option found; + struct parsed_option other; + const char *stuck; + enum long_opt_lookup result = lookup_long_opt(p, arg, options, + &found, &other, &stuck); + + if (result == LONG_OPT_UNKNOWN) + return PARSE_OPT_UNKNOWN; + + if (disallow_abbreviated_options && + (result == LONG_OPT_ABBREV || result == LONG_OPT_AMBIGUOUS)) { + const char *arg_end = strchrnul(arg, '='); die("disallowed abbreviated or ambiguous option '%.*s'", (int)(arg_end - arg), arg); + } - if (ambiguous.option) { - error(_("ambiguous option: %s " - "(could be --%s%s or --%s%s)"), - arg, - (ambiguous.flags & OPT_UNSET) ? "no-" : "", - ambiguous.option->long_name, - (abbrev.flags & OPT_UNSET) ? "no-" : "", - abbrev.option->long_name); + if (result == LONG_OPT_AMBIGUOUS) { + error(_("ambiguous option: %s (could be --%s%s or --%s%s)"), arg, + (other.flags & OPT_UNSET) ? "no-" : "", other.option->long_name, + (found.flags & OPT_UNSET) ? "no-" : "", found.option->long_name); return PARSE_OPT_HELP_ERROR; } - if (abbrev.option) { - if (*arg_end) - p->opt = arg_end + 1; - return get_value(p, abbrev.option, abbrev.flags); + + if (result == LONG_OPT_ABBREV || result == LONG_OPT_EXACT) { + p->opt = stuck; + return get_value(p, found.option, found.flags); } - return PARSE_OPT_UNKNOWN; + + BUG("unknown enum long_opt_lookup value '%d'", result); } static enum parse_opt_result parse_nodash_opt(struct parse_opt_ctx_t *p, From 09a0a25219a67b9d9536e506acef8b4467064d3b Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 12 Oct 2026 09:44:12 +0200 Subject: [PATCH 3/4] parse-options: add early_scan_options() Some commands need to look at a few of their options before they can parse their command line for real, for example because the result decides whether a repository is needed at all, or how the beginning of their input should be interpreted. Such an early scan has to know which options take their value as a separate argument, or it mistakes such a value for an option. Several commands get this wrong, as they just walk their arguments comparing them to the few option names they care about. Let's add early_scan_options() to help with this. It walks the arguments using the very same `struct option` array that the command already passes to parse_options(). It looks up long options with the lookup_long_opt() helper extracted from parse_long_opt() in a previous commit, so that negated forms, unambiguous abbreviations and aliases are recognized the same way, and it uses the parse_options_takes_argument() helper added in another previous commit, so that the scan and the actual parsing agree on which options take a value. The options the caller wants to be told about are marked with a new PARSE_OPT_EARLY flag, so that nothing has to be spelled out a second time, and so that the mark cannot drift away from the option it refers to. Using a per-option flag for this is not new as that flag space already holds flags that the parsing loop itself ignores, like PARSE_OPT_NOCOMPLETE and PARSE_OPT_COMP_ARG, which only the completion helper looks at, or PARSE_OPT_HIDDEN and PARSE_OPT_LITERAL_ARGHELP, which only the usage output looks at. The scan is deliberately kept much simpler than parse_options(), instead of teaching the latter to perform a side effect free "dry run". Such a dry run would have to avoid writing through `opt->value`, calling option callbacks, dying on an invalid value, handling `--help` and tracking command mode conflicts, so it would be a much larger refactoring. If parse_options() learns to do it in the future though, the commands converted now would keep both their option array and their PARSE_OPT_EARLY marks, so their conversion would not have to be redone. One consequence of staying simple is that the scan doesn't try to handle what it cannot reliably skip, like short options, unknown options, ambiguous abbreviations or options with a low-level callback, which may consume any number of arguments. It stops at them instead, so that it never takes an option's value for an option. This matters, as for example with: git fast-import --quiet --export-pack --allow-unsafe-features parse_options() takes `--allow-unsafe-features` as the value of `--export-pack-edges`, which `--export-pack` abbreviates. A scan that just skipped `--export-pack` as an unknown option would wrongly see `--allow-unsafe-features` there. So the scan can fail to see an option that parse_options() would accept, but it doesn't report an option that parse_options() would not, and its callers can safely err on the side of not having seen an option. The differences with parse_options() are documented in "parse-options.h". In practice, despite these limitations, early scans using early_scan_options() should still be safer and cleaner than the ad-hoc hand-rolled scans they are meant to replace, which don't know about option values at all and therefore disagree with parse_options() in ways that create plain bugs. Helped-by: Kaartic Sivaraam Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- .../technical/api-parse-options.adoc | 5 + parse-options.c | 138 ++++++++ parse-options.h | 91 +++++ t/helper/test-parse-options.c | 96 ++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0040-parse-options.sh | 321 ++++++++++++++++++ 7 files changed, 653 insertions(+) diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc index 95b7924e84..f59d8e90a5 100644 --- a/Documentation/technical/api-parse-options.adoc +++ b/Documentation/technical/api-parse-options.adoc @@ -197,6 +197,11 @@ are the bitwise-or of: Internal flag, set on options that were expanded from a configured alias. It should not be set by callers. +`PARSE_OPT_EARLY`:: + Report this option to `early_scan_options()`, which looks at a + few options before parsing the command line for real. Ignored + by `parse_options()` itself. + `PARSE_OPT_NOCOMPLETE`:: Do not offer this option for completion. diff --git a/parse-options.c b/parse-options.c index a98b958423..c03c396aa6 100644 --- a/parse-options.c +++ b/parse-options.c @@ -726,6 +726,8 @@ static void parse_options_check(const struct option *opts) opts->long_name)) optbug(opts, "uses feature " "not supported for dashless options"); + if ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name) + optbug(opts, "uses PARSE_OPT_EARLY, which needs a long name"); if (opts->type == OPTION_SET_INT && !opts->defval && opts->long_name && !(opts->flags & PARSE_OPT_NONEG)) optbug(opts, "OPTION_SET_INT 0 should not be negatable"); @@ -763,6 +765,8 @@ static void parse_options_check(const struct option *opts) case OPTION_SUBCOMMAND: if (!opts->value || !opts->subcommand_fn) optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function"); + if (opts->flags & PARSE_OPT_EARLY) + optbug(opts, "OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY"); if (!subcommand_value) subcommand_value = opts->value; else if (subcommand_value != opts->value) @@ -1310,6 +1314,140 @@ int parse_options(int argc, const char **argv, return parse_options_end(&ctx); } +/* + * Look for `name`, a long option without its leading "--", among + * `option`, the way parse_options() would. Return true if it names an + * option exactly, or as an unambiguous abbreviation that is not + * disallowed, with `found` set to that option and its flags, and + * `stuck` to the value stuck to it, if any, or to NULL. + */ +static bool find_early_scan_option(struct parse_opt_ctx_t *p, + const char *name, + const struct option *option, + bool disallow_abbrev, + struct parsed_option *found, + const char **stuck) +{ + struct parsed_option other; + enum long_opt_lookup result = lookup_long_opt(p, name, option, + found, &other, stuck); + + switch (result) { + case LONG_OPT_EXACT: + return true; + case LONG_OPT_ABBREV: + return !disallow_abbrev; + case LONG_OPT_AMBIGUOUS: + case LONG_OPT_UNKNOWN: + return false; + } + + BUG("unknown enum long_opt_lookup value '%d'", result); +} + +struct early_scan_state { + struct parse_opt_ctx_t ctx; + const struct option *option; + enum early_scan_flags flags; + early_scan_fn *fn; + void *data; + bool disallow_abbrev; +}; + +/* + * Look at argv[*i]. Return 'true' to go on scanning, with *i on the + * last argument consumed. Return 'false' to stop, with *i set to the + * first argument not consumed. + */ +static bool early_scan_one_option(struct early_scan_state *s, + int argc, const char **argv, int *i) +{ + const char *name; + const char *value; + struct parsed_option found; + bool unset; + int pos = *i; + const char *arg = argv[pos]; + + /* + * parse_options() always stops parsing options at these, + * whatever its flags, so nothing after them is an option. + */ + if (!strcmp(arg, "--") || !strcmp(arg, "--end-of-options")) + return false; + + /* non-option */ + if (*arg != '-' || !arg[1]) + return !(s->flags & EARLY_SCAN_STOP_AT_NON_OPTION); + + /* short option */ + if (!skip_prefix(arg, "--", &name)) + return false; + + /* unknown, ambiguous or disallowed abbreviation */ + if (!find_early_scan_option(&s->ctx, name, s->option, + s->disallow_abbrev, &found, &value)) + return false; + + /* A low-level callback may consume any number of arguments. */ + if (found.option->ll_callback) + return false; + + unset = !!(found.flags & OPT_UNSET); + + /* parse_options() rejects these, so don't go further. */ + if (value && (unset || (found.option->flags & PARSE_OPT_NOARG))) + return false; + + /* + * Like get_arg(), take the next argument as the value when it is + * not stuck to the option. When there is no next argument, + * parse_options() uses the default value of an option with + * PARSE_OPT_LASTARG_DEFAULT, but errors out for other options. + */ + if (!value && !unset && parse_options_takes_argument(found.option)) { + if (pos + 1 < argc) + value = argv[++*i]; + else if (!(found.option->flags & PARSE_OPT_LASTARG_DEFAULT)) + return false; + } + + if ((found.option->flags & PARSE_OPT_EARLY) && + s->fn(found.option, value, unset, pos, s->data)) { + /* the option, and its value if any, have been consumed */ + ++*i; + return false; + } + + return true; +} + +int early_scan_options(int argc, const char **argv, + const struct option *option, + enum early_scan_flags flags, + early_scan_fn *fn, void *data) +{ + struct early_scan_state s = { + .flags = flags, + .fn = fn, + .data = data, + .disallow_abbrev = + git_env_bool("GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS", 0), + }; + struct option *real_option = preprocess_options(&s.ctx, option); + int i; + + s.option = real_option ? real_option : option; + + for (i = 0; i < argc; i++) + if (!early_scan_one_option(&s, argc, argv, &i)) + break; + + free_preprocessed_options(real_option); + free(s.ctx.alias_groups); + return i; +} + static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; diff --git a/parse-options.h b/parse-options.h index 589f8d9008..f39b9a779a 100644 --- a/parse-options.h +++ b/parse-options.h @@ -51,6 +51,7 @@ enum parse_opt_option_flags { PARSE_OPT_NODASH = 1 << 5, PARSE_OPT_LITERAL_ARGHELP = 1 << 6, PARSE_OPT_FROM_ALIAS = 1 << 7, + PARSE_OPT_EARLY = 1 << 8, /* only for early_scan_options() */ PARSE_OPT_NOCOMPLETE = 1 << 9, PARSE_OPT_COMP_ARG = 1 << 10, PARSE_OPT_CMDMODE = 1 << 11, @@ -502,6 +503,96 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name, BUG("option callback expects an argument"); \ } while(0) +/*----- Early scan: scanning argv before the actual option parsing -----*/ + +/* + * Some commands need to look at a few options before they can parse + * their command line for real, for example because the result decides + * whether a repository is needed at all. + * + * Such an early scan has to know which options take their value as a + * separate argument, or it could mistake such a value for an + * option. The functions below allow performing such early scans + * without being fooled by option values. + */ + +/* + * Called by early_scan_options() for each argument matching a + * `struct option` with PARSE_OPT_EARLY set. + * + * - `option` is the matching option, + * - `value` its value, or NULL if it doesn't take one or if its value + * is optional and wasn't given, as with PARSE_OPT_OPTARG, or with + * PARSE_OPT_LASTARG_DEFAULT when the option is the last argument, + * - `unset` is 'true' for the negated form of the option (like + * for parse_opt_cb()), in which case `value` is always NULL, and + * - `pos` the index of the option in argv. + * + * Returning a non-zero value stops the scan right after the option and + * its value, if any. + */ +typedef int early_scan_fn(const struct option *option, const char *value, + bool unset, int pos, void *data); + +enum early_scan_flags { + EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 0, /* Stop at any non option */ +}; + +/* + * Scan `argv` for the options described by `option`, calling `fn` for + * each of those that have PARSE_OPT_EARLY set. `argv` is not + * modified. + * + * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is + * useful to only find out where the scan stops. + * + * The scan always stops at "--" and at "--end-of-options", as + * parse_options() always stops parsing options there too, whatever its + * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only + * decide if the terminator is left in argv, not if it terminates. + * + * Returns the index of the first argument not consumed by the scan, + * which is `argc` when the whole array was scanned. + * + * Long options are looked up the same way as parse_options() does, + * so their negated forms, their unambiguous abbreviations, unless + * GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS is set, and the OPTION_ALIAS + * entries standing for them are all recognized, and their value is + * skipped when they take one. + * + * To never misinterpret an option value as an option though, the scan + * stops at any argument it cannot reliably skip, that is: + * + * - a short option, or a bundle of them, + * + * - an unknown option, or an ambiguous or disallowed abbreviation, + * + * - an option with a low-level callback, as such a callback may + * consume any number of arguments, + * + * - an option with a value stuck to it that parse_options() would + * reject, like a negated option or a PARSE_OPT_NOARG one, + * + * - an option requiring a value when it is the last argument, which + * parse_options() would also reject. + * + * In all these cases, the scan can fail to see an option that + * parse_options() would accept, but it doesn't report an option that + * parse_options() would not, so that callers err on the safe side. + * + * This requires the scan to be told about the way the later + * parse_options() call treats its arguments though: callers should + * pass EARLY_SCAN_STOP_AT_NON_OPTION when parse_options() is called + * with PARSE_OPT_STOP_AT_NON_OPTION or with subcommands, and should + * not use the scan when parse_options() is called with + * PARSE_OPT_KEEP_UNKNOWN_OPT, which disables abbreviations there but + * not here. + */ +int early_scan_options(int argc, const char **argv, + const struct option *option, + enum early_scan_flags flags, + early_scan_fn *fn, void *data); + /*----- incremental advanced APIs -----*/ struct parse_opt_cmdmode_list; diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c index f181f0c02d..de3dd5751c 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -383,3 +383,99 @@ int cmd__parse_subcommand(int argc, const char **argv) return parse_subcommand__cmd(argc, argv, test_flags); } + +static enum parse_opt_result early_ll_callback(struct parse_opt_ctx_t *ctx UNUSED, + const struct option *opt UNUSED, + const char *arg UNUSED, + int unset UNUSED) +{ + return 0; +} + +static int show_early_option(const struct option *opt, const char *value, + bool unset, int pos, void *data) +{ + int *stop_at_found = data; + + printf("found: %s at %d", opt->long_name, pos); + if (unset) + printf(" unset"); + if (value) + printf(" value: %s", value); + putchar('\n'); + return *stop_at_found; +} + +int cmd__early_scan_options(int argc, const char **argv) +{ + char *a_string = NULL; + int an_int = 0, a_bool = 0, a_short = 0; + + const struct option option[] = { + OPT_GROUP("early scan test options"), + OPT_BOOL_F(0, "wanted", &a_bool, + "wanted option taking no value", + PARSE_OPT_EARLY), + OPT_STRING_F(0, "wanted-value", &a_string, "str", + "wanted option taking a value", + PARSE_OPT_EARLY), + OPT_STRING(0, "skipped-value", &a_string, "str", + "option whose value has to be skipped"), + OPT_ALIAS(0, "alias-of-skipped", "skipped-value"), + OPT_INTEGER(0, "number", &an_int, + "option taking an integer value"), + OPT_STRING_F(0, "optarg", &a_string, "str", + "option with an optional value", + PARSE_OPT_OPTARG), + OPT_STRING_F(0, "lastarg", &a_string, "str", + "option with a last argument default", + PARSE_OPT_LASTARG_DEFAULT), + OPT_STRING_F(0, "early-optarg", &a_string, "str", + "early option with an optional value", + PARSE_OPT_EARLY | PARSE_OPT_OPTARG), + OPT_STRING_F(0, "early-lastarg", &a_string, "str", + "early option with a last argument default", + PARSE_OPT_EARLY | PARSE_OPT_LASTARG_DEFAULT), + OPT_BOOL_F(0, "no-early", &a_bool, + "early option whose name starts with 'no-'", + PARSE_OPT_EARLY), + OPT_BOOL('s', NULL, &a_short, "short only option"), + { + .type = OPTION_LOWLEVEL_CALLBACK, + .long_name = "low-level", + .help = "option with a low-level callback", + .flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG, + .ll_callback = early_ll_callback, + }, + { + .type = OPTION_CALLBACK, + .long_name = "ll-value", + .argh = "str", + .help = "option with a value and a low-level callback", + .flags = PARSE_OPT_NONEG, + .ll_callback = early_ll_callback, + }, + OPT_END() + }; + + enum early_scan_flags flags = 0; + int stop_at_found = 0; + int stopped; + + while (argc > 1 && *argv[1] == '-') { + if (!strcmp(argv[1], "--stop-at-non-option")) + flags |= EARLY_SCAN_STOP_AT_NON_OPTION; + else if (!strcmp(argv[1], "--stop-at-found")) + stop_at_found = 1; + else + break; + argc--; + argv++; + } + + stopped = early_scan_options(argc - 1, argv + 1, option, flags, + show_early_option, &stop_at_found); + printf("stopped at: %d of %d\n", stopped, argc - 1); + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index b71a22b43b..5d2f5877d9 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -50,6 +50,7 @@ static struct test_cmd cmds[] = { { "pack-mtimes", cmd__pack_mtimes }, { "parse-options", cmd__parse_options }, { "parse-options-flags", cmd__parse_options_flags }, + { "early-scan-options", cmd__early_scan_options }, { "parse-pathspec-file", cmd__parse_pathspec_file }, { "parse-subcommand", cmd__parse_subcommand }, { "partial-clone", cmd__partial_clone }, diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h index f2885b33d5..071306d52d 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -43,6 +43,7 @@ int cmd__pack_deltas(int argc, const char **argv); int cmd__pack_mtimes(int argc, const char **argv); int cmd__parse_options(int argc, const char **argv); int cmd__parse_options_flags(int argc, const char **argv); +int cmd__early_scan_options(int argc, const char **argv); int cmd__parse_pathspec_file(int argc, const char** argv); int cmd__parse_subcommand(int argc, const char **argv); int cmd__partial_clone(int argc, const char **argv); diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 449fff4d34..c44b12e46d 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -845,4 +845,325 @@ test_expect_success 'u16 limits range' ' test_grep "value 65536 for option .u16. not in range \[0,65535\]" err ' +test_expect_success 'early_scan_options() finds a wanted option' ' + test-tool early-scan-options --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 0 + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() reads a stuck or separate value' ' + test-tool early-scan-options --wanted-value=one >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: one + stopped at: 1 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --wanted-value two >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: two + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() skips the value of other options' ' + test-tool early-scan-options --skipped-value --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --skipped-value one --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual && + test-tool early-scan-options --number --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at a last option missing its value' ' + test-tool early-scan-options --wanted-value >actual && + cat >expect <<-\EOF && + stopped at: 0 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --wanted --skipped-value >actual && + cat >expect <<-\EOF && + found: wanted at 0 + stopped at: 1 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() always stops at "--"' ' + test-tool early-scan-options -- --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-non-option -- --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --skipped-value -- --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() always stops at "--end-of-options"' ' + test-tool early-scan-options --end-of-options --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-non-option \ + --end-of-options --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() can stop at a non-option' ' + test-tool early-scan-options --stop-at-non-option \ + arg --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-non-option \ + --skipped-value arg --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() matches unambiguous abbreviations' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool early-scan-options --wanted-v=x >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: x + stopped at: 1 of 1 + EOF + test_cmp expect actual && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool early-scan-options --wanted-v x --wanted >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: x + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() skips the value of an abbreviation' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool early-scan-options --skipped --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at an ambiguous abbreviation' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + test-tool early-scan-options --want --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at disallowed abbreviations' ' + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ + test-tool early-scan-options --wanted-v=x >actual && + cat >expect <<-\EOF && + stopped at: 0 of 1 + EOF + test_cmp expect actual && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ + test-tool early-scan-options --skipped --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at an unknown option' ' + test-tool early-scan-options --bogus --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() resolves aliases' ' + test-tool early-scan-options --alias-of-skipped --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --alias-of-skipped x --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() does not skip an optional value' ' + test-tool early-scan-options --optarg --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 1 + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --optarg=5 --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 1 + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() skips a last argument default value' ' + test-tool early-scan-options --lastarg --wanted >actual && + cat >expect <<-\EOF && + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --wanted --lastarg >actual && + cat >expect <<-\EOF && + found: wanted at 0 + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() matches a stuck optional value' ' + test-tool early-scan-options --early-optarg=one >actual && + cat >expect <<-\EOF && + found: early-optarg at 0 value: one + stopped at: 1 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --early-lastarg=two >actual && + cat >expect <<-\EOF && + found: early-lastarg at 0 value: two + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() does not take a separate optional value' ' + test-tool early-scan-options --early-optarg --wanted >actual && + cat >expect <<-\EOF && + found: early-optarg at 0 + found: wanted at 1 + stopped at: 2 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() takes a last argument default value' ' + test-tool early-scan-options --early-lastarg --wanted >actual && + cat >expect <<-\EOF && + found: early-lastarg at 0 value: --wanted + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --early-lastarg >actual && + cat >expect <<-\EOF && + found: early-lastarg at 0 + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at short options' ' + test-tool early-scan-options -s --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops at low-level callbacks' ' + test-tool early-scan-options --low-level --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --ll-value x --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() stops after what the callback consumed' ' + test-tool early-scan-options --stop-at-found --wanted --wanted >actual && + cat >expect <<-\EOF && + found: wanted at 0 + stopped at: 1 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-found \ + --wanted-value x --wanted >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 value: x + stopped at: 2 of 3 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() reports negated options' ' + test-tool early-scan-options --no-wanted >actual && + cat >expect <<-\EOF && + found: wanted at 0 unset + stopped at: 1 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --early >actual && + cat >expect <<-\EOF && + found: no-early at 0 unset + stopped at: 1 of 1 + EOF + test_cmp expect actual && + test-tool early-scan-options --no-early >actual && + cat >expect <<-\EOF && + found: no-early at 0 + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options() does not take a value for negated options' ' + test-tool early-scan-options --no-wanted-value --wanted >actual && + cat >expect <<-\EOF && + found: wanted-value at 0 unset + found: wanted at 1 + stopped at: 2 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --no-wanted-value=x --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual +' + test_done From dabb68a20dc66b553606d05f447762434b82c403 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Mon, 12 Oct 2026 09:44:13 +0200 Subject: [PATCH 4/4] fast-import: use early_scan_options() for --allow-unsafe-features The "feature" lines at the start of the stream are processed before the command line options are parsed, so cmd_fast_import() scans its arguments early to find out if `--allow-unsafe-features` was given. That scan doesn't know which options take their value as a separate argument, and it stops at the first argument that doesn't start with a dash. So it disagrees with parse_options(), which accepts values separated from their option by a space, for a command line like "--depth 5 --allow-unsafe-features": the scan stops at "5" and never sees the option, so unsafe "feature" commands from the stream are refused even though the option was given. Let's fix this by using early_scan_options(), which scans the very same `struct option` array that parse_options() uses, so that both agree on which options take a value, and by marking `--allow-unsafe-features` with PARSE_OPT_EARLY so that the scan reports it. As early_scan_options() looks up long options the same way as parse_options(), the two also agree on abbreviations now. So `--allow-unsafe` allows unsafe "feature" commands, while "--quiet --export-pack --allow-unsafe-features" refuses them, as parse_options() takes `--allow-unsafe-features` there as the value of `--export-pack-edges`. (The old scan wrongly allowed them in this case, as all the arguments start with a dash.) The note about having to spell the option in full in the documentation is therefore removed, and so is the NEEDSWORK comment about it. Helped-by: Kaartic Sivaraam Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/git-fast-import.adoc | 7 ------ builtin/fast-import.c | 37 ++++++++++++++++-------------- t/t9300-fast-import.sh | 29 +++++++++++++++++++++++ 3 files changed, 49 insertions(+), 24 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index fd165e11d2..7c5900e048 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -65,13 +65,6 @@ Only enable this option if you trust the program generating the fast-import stream! This option is enabled automatically for remote-helpers that use the `import` capability, as they are already trusted to run their own code. -+ -Note that this option has to be spelled in full, and has to appear -before any option whose value is separated from it by a space, for -the unsafe `feature` commands in the stream to be allowed. So -`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse -them, while `--allow-unsafe-features --depth 5` and -`--depth=5 --allow-unsafe-features` allow them. `--signed-tags=`:: Specify how to handle signed tags. Behaves in the same way as diff --git a/builtin/fast-import.c b/builtin/fast-import.c index fbd919982c..2e667af349 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -4120,6 +4120,18 @@ static int option_parse_quiet(const struct option *opt UNUSED, return 0; } +static int option_parse_early_allow_unsafe(const struct option *option, + const char *value UNUSED, + bool unset, int pos UNUSED, + void *data) +{ + struct fast_import_state *state = data; + + if (!strcmp(option->long_name, "allow-unsafe-features")) + state->allow_unsafe_features = !unset; + return 0; +} + int cmd_fast_import(int argc, const char **argv, const char *prefix, @@ -4184,7 +4196,7 @@ int cmd_fast_import(int argc, OPT_HIDDEN_GROUP(N_("Advanced")), OPT_BOOL_F(0, "allow-unsafe-features", &state.allow_unsafe_features, N_("allow unsafe mark commands from the stream"), - PARSE_OPT_HIDDEN | PARSE_OPT_NONEG), + PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY), OPT_CALLBACK_F(0, "export-pack-edges", &state, N_("file"), N_("dump edge commits to "), PARSE_OPT_HIDDEN | PARSE_OPT_NONEG, @@ -4218,23 +4230,14 @@ int cmd_fast_import(int argc, * line to override stream data). But we must do an early parse of any * command-line options that impact how we interpret the feature lines. * - * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features" - * spelling and stops at the first argument that doesn't start with a - * dash. As parse_options() below also accepts unambiguous abbreviations - * and values separated by a space from their option, the two disagree - * for command lines like "--allow-unsafe" or "--depth 5 - * --allow-unsafe-features": parse_options() accepts the option, but - * this scan doesn't see it, so unsafe features from the stream are - * still refused. This errs on the safe side, but should be fixed by - * teaching this scan about the options that take a value. + * The scan uses the same options as parse_options() below, so the + * two agree on which options take a value and on abbreviations. In + * case of doubt, the scan stops early, so unsafe features are then + * refused rather than wrongly allowed. */ - for (int i = 1; i < argc; i++) { - const char *arg = argv[i]; - if (*arg != '-' || !strcmp(arg, "--")) - break; - if (!strcmp(arg, "--allow-unsafe-features")) - state.allow_unsafe_features = 1; - } + early_scan_options(argc - 1, argv + 1, fast_import_options, + EARLY_SCAN_STOP_AT_NON_OPTION, + option_parse_early_allow_unsafe, &state); rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free)); for (unsigned int i = 0; i < (cmd_save - 1); i++) diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh index d9de2ef0d8..85059a5385 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2344,6 +2344,35 @@ test_expect_success 'R: export-marks options can be overridden by commandline op test_path_is_missing feature-sub ' +test_expect_success 'R: --allow-unsafe-features found after a value' ' + echo "feature import-marks-if-exists=nonexistent.marks" >input && + git fast-import --allow-unsafe-features input && + GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + git fast-import --allow-unsafe input && + test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \ + git fast-import --allow-unsafe err && + test_grep "forbidden in input without --allow-unsafe-features" err +' + +test_expect_success 'R: --allow-unsafe-features as the value of an abbreviation' ' + echo "feature import-marks-if-exists=nonexistent.marks" >input && + test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \ + git fast-import --quiet --export-pack \ + --allow-unsafe-features err && + test_grep "forbidden in input without --allow-unsafe-features" err +' + test_expect_success 'R: catch typo in marks file name' ' test_must_fail git fast-import --import-marks=nonexistent.marks