From bce62444c9dabc2171c7c5a3c6ddc3af0aa329ee Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:42 +0200 Subject: [PATCH 1/6] 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. Its callers describe the options to look for, but also the ones that merely have to be skipped along with their value, so that the scan can walk the arguments without being fooled by option values. Note that abbreviated options are deliberately not recognized, as a scan cannot know about the options it hasn't been told about, and would then resolve abbreviations differently from the actual option parsing. So users must spell these specific options in full. This restriction could be lifted in the future though, once the scanner is adapted to accept a command's full option array, as this would give it the complete context needed for safe abbreviation matching. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- parse-options.c | 70 +++++++++++++++++++++++++++++++ parse-options.h | 60 +++++++++++++++++++++++++++ t/helper/test-parse-options.c | 39 ++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0040-parse-options.sh | 77 +++++++++++++++++++++++++++++++++++ 6 files changed, 248 insertions(+) diff --git a/parse-options.c b/parse-options.c index 4519ead9dc..b3d19446cd 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1244,6 +1244,76 @@ int parse_options(int argc, const char **argv, return parse_options_end(&ctx); } +/* + * Look for `arg` among `options`. On success, return the matching option + * and set `value` to the value stuck to it, if any, or to NULL. + */ +static const struct early_scan_option * +find_early_scan_option(const char *arg, + const struct early_scan_option *options, + const char **value) +{ + if (!skip_prefix(arg, "--", &arg)) + return NULL; + + for (; options->name; options++) { + const char *rest; + + if (!skip_prefix(arg, options->name, &rest)) + continue; + if (!*rest) { + *value = NULL; + return options; + } + /* Only an option taking a value can be stuck to one. */ + if (*rest == '=' && options->takes_value) { + *value = rest + 1; + return options; + } + } + + return NULL; +} + +int early_scan_options(int argc, const char **argv, + const struct early_scan_option *options, + enum early_scan_flags flags, + early_scan_fn *fn, void *data) +{ + for (int i = 0; i < argc; i++) { + const char *arg = argv[i]; + const char *value; + const struct early_scan_option *opt; + int pos = i; + + if ((flags & EARLY_SCAN_STOP_AT_DASHDASH) && + !strcmp(arg, "--")) + return i; + + opt = find_early_scan_option(arg, options, &value); + if (!opt) { + if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) && + (*arg != '-' || !arg[1])) + return i; + continue; + } + + /* + * When an option takes a value, but that value is not + * stuck to it with '=', then the next argument is the + * value and it has to be skipped so that it isn't + * taken for an option itself. + */ + if (opt->takes_value && !value && i + 1 < argc) + value = argv[++i]; + + if (opt->wanted && fn(opt, value, pos, data)) + return i; + } + + return argc; +} + static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; diff --git a/parse-options.h b/parse-options.h index d7f896a933..abc73d8399 100644 --- a/parse-options.h +++ b/parse-options.h @@ -491,6 +491,66 @@ 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 + * `struct early_scan_option` array passed to early_scan_options() below + * describes the options to look for, as well as the ones that only need + * to be skipped along with their value. + */ +struct early_scan_option { + const char *name; /* Option name, without the leading dashes */ + unsigned takes_value:1; /* "--option=value" or "--option value" expected? */ + unsigned wanted:1; /* Report option to callback? */ +}; + +#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 } +#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 } +#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 } +#define EARLY_SCAN_END() { NULL } + +/* + * Called by early_scan_options() for each argument matching a + * `struct early_scan_option` that has its `wanted` bit set. + * + * `option` is the matching option, `value` its value or NULL if it + * doesn't take one, and `pos` the index of the option in argv. + * + * Returning a non-zero value stops the scan. + */ +typedef int early_scan_fn(const struct early_scan_option *option, + const char *value, int pos, void *data); + +enum early_scan_flags { + EARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at "--" */ + EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1, +}; + +/* + * Scan `argv` for the options described by `options`, calling `fn` + * for each of those that are `wanted`. `argv` is not modified. + * + * `fn` may be NULL when no option is `wanted`, which is useful to only + * find out where the scan stops. + * + * Note that abbreviated options are not recognized, as a scan cannot + * know about the options it hasn't been told about, and would then + * resolve abbreviations differently from the actual option parsing. + * + * Returns the index at which the scan stopped, which is `argc` when the + * whole array was scanned. + */ +int early_scan_options(int argc, const char **argv, + const struct early_scan_option *options, + 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..96ab941d29 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -383,3 +383,42 @@ int cmd__parse_subcommand(int argc, const char **argv) return parse_subcommand__cmd(argc, argv, test_flags); } + +static int show_early_option(const struct early_scan_option *opt, + const char *value, int pos, void *data UNUSED) +{ + printf("found: %s at %d", opt->name, pos); + if (value) + printf(" value: %s", value); + putchar('\n'); + return 0; +} + +int cmd__early_scan_options(int argc, const char **argv) +{ + static const struct early_scan_option options[] = { + EARLY_SCAN_WANT("wanted"), + EARLY_SCAN_WANT_VALUE("wanted-value"), + EARLY_SCAN_SKIP_VALUE("skipped-value"), + EARLY_SCAN_END() + }; + enum early_scan_flags flags = 0; + int stopped; + + while (argc > 1 && *argv[1] == '-') { + if (!strcmp(argv[1], "--stop-at-dashdash")) + flags |= EARLY_SCAN_STOP_AT_DASHDASH; + else if (!strcmp(argv[1], "--stop-at-non-option")) + flags |= EARLY_SCAN_STOP_AT_NON_OPTION; + else + break; + argc--; + argv++; + } + + stopped = early_scan_options(argc - 1, argv + 1, options, flags, + show_early_option, NULL); + 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..d760d8cfbd 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -845,4 +845,81 @@ 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_expect_success 'early_scan_options() can stop at "--"' ' + test-tool early-scan-options --stop-at-dashdash -- --wanted >actual && + cat >expect <<-\EOF && + stopped at: 0 of 2 + EOF + test_cmp expect actual && + test-tool early-scan-options --stop-at-dashdash \ + --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() 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() ignores abbreviated options' ' + test-tool early-scan-options --want >actual && + cat >expect <<-\EOF && + stopped at: 1 of 1 + EOF + test_cmp expect actual +' + test_done From 3c795025064789c934ecc242aedb65b84da13fa7 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:43 +0200 Subject: [PATCH 2/6] bisect: fix "--" detection when a term name is "--" `bisect_start()` walks its arguments twice. The second loop actually parses the options, and it knows that `--term-good`, `--term-old`, `--term-bad` and `--term-new` take their value as a separate argument, so it skips that value. The first loop, which only looks for the "--" separating revisions from paths, doesn't know about these options. So when such an option is given "--" as its value, that "--" is mistaken for the separator and `has_double_dash` is wrongly set. This matters because `has_double_dash` makes the second loop die on an argument that is not a valid revision, instead of treating it as the first path. So: $ git bisect start --term-good -- notarev fatal: 'notarev' does not appear to be a valid revision while the very same command line with any other term name happily takes "notarev" as a path. Let's fix this by using early_scan_options(), telling it about the options taking their value as a separate argument, so that it can skip those values. Note: One might argue that accepting a term name that looks like an option (such as "--") is a misfeature and should be forbidden entirely. However, whether we should tighten the validation rules for bisect terms is a separate UI issue that can be dealt with independently. For now, this commit simply ensures the parser correctly implements the existing rules. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/bisect.c | 27 +++++++++++++++++++++------ t/t6030-bisect-porcelain.sh | 8 ++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/builtin/bisect.c b/builtin/bisect.c index 1cfb8a794b..ad089b289f 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, return bisect_next(terms, prefix); } +/* + * The options "git bisect start" accepts. Only the ones taking their + * value as a separate argument matter to the scan looking for "--" below, + * as their value has to be skipped along with them. + */ +static const struct early_scan_option bisect_start_early_options[] = { + EARLY_SCAN_SKIP_VALUE("term-good"), + EARLY_SCAN_SKIP_VALUE("term-old"), + EARLY_SCAN_SKIP_VALUE("term-bad"), + EARLY_SCAN_SKIP_VALUE("term-new"), + EARLY_SCAN_END() +}; + static enum bisect_error bisect_start(struct bisect_terms *terms, int argc, const char **argv) { @@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc, /* * Check for one bad and then some good revisions + * + * The scan below has to know about the options taking their value + * as a separate argument, or such a value that happens to be "--" + * would be mistaken for the "--" separating revisions from paths. */ - for (i = 0; i < argc; i++) { - if (!strcmp(argv[i], "--")) { - has_double_dash = 1; - break; - } - } + i = early_scan_options(argc, argv, bisect_start_early_options, + EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL); + if (i < argc) + has_double_dash = 1; for (i = 0; i < argc; i++) { const char *arg = argv[i]; diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index a7588222a8..464ca53b42 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' ' test_cmp expected actual ' +test_expect_success 'bisect start with "--" as a term name' ' + git bisect reset && + git bisect start --term-good -- hello && + git bisect terms --term-good >actual && + echo -- >expected && + test_cmp expected actual +' + # Bisect is started with --term-new and --term-old arguments, # then skip. The HEAD should be changed. test_expect_success 'bisect skip works with --term*' ' From 28c803e36e73f307c0d45116f27618ac29691faa Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:44 +0200 Subject: [PATCH 3/6] rev-parse: fix "--" detection when it is an option value `cmd_rev_parse()` walks its arguments twice. The second loop actually parses the options, and it knows that `--default`, `--prefix` and `--resolve-git-dir` take their value as a separate argument, so it skips that value. The first loop, which only looks for the "--" separating revisions from paths, doesn't know about these options. So when such an option is given "--" as its value, that "--" is mistaken for the separator and `has_dashdash` is wrongly set. This matters because `has_dashdash` makes the second loop die with a "bad revision" error on an argument that is neither a revision nor an existing file, instead of reporting that the argument is ambiguous and telling how to disambiguate it. So: $ git rev-parse --default -- notarev fatal: bad revision 'notarev' while the very same command line with any other default value gives the usual, much more helpful, "ambiguous argument" error. Let's fix this the same way as in a previous commit, by using early_scan_options() and telling it about the options taking their value as a separate argument. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/rev-parse.c | 26 ++++++++++++++++++++------ t/t1500-rev-parse.sh | 5 +++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 43693454d5..7ced82e25d 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -695,6 +695,17 @@ static void print_path(const char *path, const char *prefix, strbuf_release(&sb); } +/* + * The options taking their value as a separate argument, which the scan + * looking for "--" below has to skip along with their value. + */ +static const struct early_scan_option rev_parse_early_options[] = { + EARLY_SCAN_SKIP_VALUE("default"), + EARLY_SCAN_SKIP_VALUE("prefix"), + EARLY_SCAN_SKIP_VALUE("resolve-git-dir"), + EARLY_SCAN_END() +}; + int cmd_rev_parse(int argc, const char **argv, const char *prefix, @@ -724,12 +735,15 @@ int cmd_rev_parse(int argc, if (argc > 1 && !strcmp("-h", argv[1])) usage(builtin_rev_parse_usage); - for (i = 1; i < argc; i++) { - if (!strcmp(argv[i], "--")) { - has_dashdash = 1; - break; - } - } + /* + * The scan below has to know about the options taking their value + * as a separate argument, or such a value that happens to be "--" + * would be mistaken for the "--" separating revisions from paths. + */ + i = early_scan_options(argc - 1, argv + 1, rev_parse_early_options, + EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL); + if (i < argc - 1) + has_dashdash = 1; /* No options; just report on whether we're in a git repo or not. */ if (argc == 1) { diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index 4174ca40c3..897e9a7735 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -383,4 +383,9 @@ test_expect_success ':/ and HEAD^{/} favor more recent matching commits' ' ) ' +test_expect_success 'rev-parse with "--" as an option value' ' + test_must_fail git rev-parse --default -- notarev 2>err && + test_grep "ambiguous argument .notarev." err +' + test_done From 21814f1477b8017eecb99608e57fc197146d8d75 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:45 +0200 Subject: [PATCH 4/6] 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, so it is not considered as taking a value, which is what show_gitcomp() already did. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- parse-options.c | 35 ++++++++++++++++++++++------------- parse-options.h | 10 ++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/parse-options.c b/parse-options.c index b3d19446cd..70851a385b 100644 --- a/parse-options.c +++ b/parse-options.c @@ -841,6 +841,26 @@ 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 | + PARSE_OPT_LASTARG_DEFAULT)) + return 0; + + return 1; +} + static int show_gitcomp(const struct option *opts, int show_all) { const struct option *original_opts = opts; @@ -862,20 +882,9 @@ 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)) + suffix = "="; break; } if (opts->flags & PARSE_OPT_COMP_ARG) diff --git a/parse-options.h b/parse-options.h index abc73d8399..b96e93508e 100644 --- a/parse-options.h +++ b/parse-options.h @@ -420,6 +420,16 @@ 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 only consumes the + * next argument when it isn't the last one, so it is not considered as + * taking a value here. + */ +int parse_options_takes_argument(const struct option *opt); + NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); From 3d417fc968ebea3584e9e2da5dc0e765390acafc Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:46 +0200 Subject: [PATCH 5/6] parse-options: build early scan options from a struct option array A command that scans its arguments early has to know which options take a value, so that it can skip that value instead of mistaking it for an option. When it also parses its options with the parse-options API, that information is already available in its `struct option` array, and duplicating it by hand in a `struct early_scan_option` array is both tedious and easy to get out of sync when an option is added. So let's add early_scan_options_from_options() to build the latter array from the former, using parse_options_takes_argument() to find out which options take a value. Its caller only has to name the options it wants to be reported. Note: This early scanner translation intentionally leaves out a few complex option types to keep the scan simple and fast: - Short options are ignored: early_scan_options_from_options() explicitly skips options without a `long_name`, and the scanner only looks for `--`. Properly handling short options would require parsing bundled flags (e.g., `-abc value`), which requires replicating the full parse_options() state machine. - Conditional values: Options with `PARSE_OPT_LASTARG_DEFAULT` or `PARSE_OPT_OPTARG` are treated as not taking a separate argument. Because the scanner does not evaluate context (like whether an argument is the final one in `argv`), it must err on the side of caution to avoid accidentally consuming the `--` separator or a path. - Abbreviated options remain unrecognized: Even though the scanner is now provided with the full option array, the underlying early_scan_options() engine still relies on exact string matches. Safely resolving abbreviations would require duplicating the ambiguity-checking logic from the main parser. - Negated options are not automatically derived: The scanner strictly matches the defined long name. It does not automatically recognize the `--no-` variants of boolean options. (This is harmless in practice for current callers, as negated options do not take values to skip, and boolean defaults align with the ignored state). The above shortcomings can be addressed later, for example, when commands that use short options or options with conditional values need an early scan or are ported to use `struct option`. Despite these limitations, this abstraction is a significant improvement. It allows commands like `fast-import` to reuse their existing `struct option` array for early scanning, ensuring the scanner and the main parser agree on which options take arguments, and preventing developers from having to maintain a separate, hardcoded list that could drift out of sync. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- parse-options.c | 39 +++++++++++++++++++++++++++++++++++ parse-options.h | 22 ++++++++++++++++++++ t/helper/test-parse-options.c | 32 ++++++++++++++++++++++++++++ t/helper/test-tool.c | 1 + t/helper/test-tool.h | 1 + t/t0040-parse-options.sh | 26 +++++++++++++++++++++++ 6 files changed, 121 insertions(+) diff --git a/parse-options.c b/parse-options.c index 70851a385b..6cdc9c64cc 100644 --- a/parse-options.c +++ b/parse-options.c @@ -1323,6 +1323,45 @@ int early_scan_options(int argc, const char **argv, return argc; } +struct early_scan_option * +early_scan_options_from_options(const struct option *options, + const char **wanted) +{ + struct early_scan_option *early; + size_t nr = 0; + + for (const struct option *opt = options; opt->type != OPTION_END; opt++) + if (opt->long_name) + nr++; + + CALLOC_ARRAY(early, nr + 1); + + nr = 0; + for (const struct option *opt = options; opt->type != OPTION_END; opt++) { + if (!opt->long_name) + continue; + early[nr].name = opt->long_name; + early[nr].takes_value = !!parse_options_takes_argument(opt); + nr++; + } + + for (; wanted && *wanted; wanted++) { + size_t i; + + for (i = 0; i < nr; i++) { + if (strcmp(early[i].name, *wanted)) + continue; + early[i].wanted = 1; + break; + } + if (i == nr) + BUG("wanted option '%s' is not in the options array", + *wanted); + } + + return early; +} + static int usage_argh(const struct option *opts, FILE *outfile) { const char *s; diff --git a/parse-options.h b/parse-options.h index b96e93508e..fb81f2ed38 100644 --- a/parse-options.h +++ b/parse-options.h @@ -561,6 +561,28 @@ int early_scan_options(int argc, const char **argv, enum early_scan_flags flags, early_scan_fn *fn, void *data); +/* + * Build the `struct early_scan_option` array to pass to + * early_scan_options() from the `options` array that the actual option + * parsing uses, so that both agree on which options take a value. + * + * Note some intentional limitations to keep the scan simple and fast: + * short options are ignored, options with PARSE_OPT_LASTARG_DEFAULT or + * PARSE_OPT_OPTARG are treated as not taking a separate value, negated + * options ("--no-...") are not automatically generated, and abbreviated + * options will not be matched. + * + * The options named in the NULL terminated `wanted` array get their + * `wanted` bit set, the other ones are only there to be skipped along + * with their value. It is a BUG() for a name in `wanted` not to appear + * in `options`. + * + * The returned array is allocated and should be free()d by the caller. + */ +struct early_scan_option * +early_scan_options_from_options(const struct option *options, + const char **wanted); + /*----- 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 96ab941d29..0187a25ccb 100644 --- a/t/helper/test-parse-options.c +++ b/t/helper/test-parse-options.c @@ -422,3 +422,35 @@ int cmd__early_scan_options(int argc, const char **argv) return 0; } + +int cmd__early_scan_from_options(int argc, const char **argv) +{ + int an_int = 0, a_bool = 0; + char *a_string = NULL; + const struct option options[] = { + OPT_STRING(0, "string", &a_string, "str", "get a string"), + OPT_INTEGER(0, "int", &an_int, "get an integer"), + OPT_BOOL(0, "bool", &a_bool, "get a boolean"), + OPT_STRING_F(0, "optarg", &a_string, "str", + "string with an optional value", + PARSE_OPT_OPTARG), + OPT_END() + }; + static const char *wanted[] = { "bool", NULL }; + struct early_scan_option *early; + int stopped; + + early = early_scan_options_from_options(options, wanted); + + for (const struct early_scan_option *o = early; o->name; o++) + printf("option: %s takes_value: %d wanted: %d\n", + o->name, o->takes_value, o->wanted); + + stopped = early_scan_options(argc - 1, argv + 1, early, 0, + show_early_option, NULL); + printf("stopped at: %d of %d\n", stopped, argc - 1); + + free(early); + + return 0; +} diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c index 5d2f5877d9..f1b208a5af 100644 --- a/t/helper/test-tool.c +++ b/t/helper/test-tool.c @@ -51,6 +51,7 @@ static struct test_cmd cmds[] = { { "parse-options", cmd__parse_options }, { "parse-options-flags", cmd__parse_options_flags }, { "early-scan-options", cmd__early_scan_options }, + { "early-scan-from-options", cmd__early_scan_from_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 071306d52d..97334ce3c6 100644 --- a/t/helper/test-tool.h +++ b/t/helper/test-tool.h @@ -44,6 +44,7 @@ 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__early_scan_from_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 d760d8cfbd..bb72a6544d 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -922,4 +922,30 @@ test_expect_success 'early_scan_options() ignores abbreviated options' ' test_cmp expect actual ' +test_expect_success 'early_scan_options_from_options() derives takes_value' ' + test-tool early-scan-from-options >actual && + cat >expect <<-\EOF && + option: string takes_value: 1 wanted: 0 + option: int takes_value: 1 wanted: 0 + option: bool takes_value: 0 wanted: 1 + option: optarg takes_value: 0 wanted: 0 + stopped at: 0 of 0 + EOF + test_cmp expect actual +' + +test_expect_success 'early_scan_options_from_options() skips values' ' + test-tool early-scan-from-options --string --bool >out && + tail -1 out >actual && + echo "stopped at: 2 of 2" >expect && + test_cmp expect actual && + test-tool early-scan-from-options --string v --bool >out && + tail -2 out >actual && + cat >expect <<-\EOF && + found: bool at 2 + stopped at: 3 of 3 + EOF + test_cmp expect actual +' + test_done From aaca161e1ecad97f85f79b52fc6dd16f04156ffc Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Wed, 2 Sep 2026 18:10:47 +0200 Subject: [PATCH 6/6] 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 building the options for the scan from the same `struct option` array that parse_options() uses, so that both agree on which options take a value. Note that the scan still only matches the exact option spelling, while parse_options() also accepts unambiguous abbreviations, so the two still disagree for a command line like "--allow-unsafe". This errs on the safe side, and is now documented as a restriction. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- Documentation/git-fast-import.adoc | 10 +++---- builtin/fast-import.c | 46 +++++++++++++++++++----------- t/t9300-fast-import.sh | 14 +++++++++ 3 files changed, 48 insertions(+), 22 deletions(-) diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc index fd165e11d2..9758ba5275 100644 --- a/Documentation/git-fast-import.adoc +++ b/Documentation/git-fast-import.adoc @@ -66,12 +66,10 @@ 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. +Note that this option has to be spelled in full for the unsafe +`feature` commands in the stream to be allowed. So while +`--allow-unsafe` is accepted as an unambiguous abbreviation of this +option, it still refuses 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..cf0504f01c 100644 --- a/builtin/fast-import.c +++ b/builtin/fast-import.c @@ -4120,12 +4120,29 @@ static int option_parse_quiet(const struct option *opt UNUSED, return 0; } +/* + * The only option the early scan below is interested in, as it decides + * whether unsafe "feature" commands from the stream are allowed. + */ +static const char *early_wanted[] = { "allow-unsafe-features", NULL }; + +static int option_parse_early_allow_unsafe( + const struct early_scan_option *opt UNUSED, + const char *value UNUSED, int pos UNUSED, void *data) +{ + struct fast_import_state *state = data; + + state->allow_unsafe_features = 1; + return 0; +} + int cmd_fast_import(int argc, const char **argv, const char *prefix, struct repository *repo) { struct fast_import_state state; + struct early_scan_option *early; struct option fast_import_options[] = { OPT_GROUP(N_("Common")), @@ -4218,23 +4235,20 @@ 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. + * NEEDSWORK: This scan only matches the exact + * "--allow-unsafe-features" spelling, while parse_options() below + * also accepts unambiguous abbreviations, so the two disagree for + * a command line like "--allow-unsafe": 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. */ - 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 = early_scan_options_from_options(fast_import_options, + early_wanted); + early_scan_options(argc - 1, argv + 1, early, + EARLY_SCAN_STOP_AT_DASHDASH | + EARLY_SCAN_STOP_AT_NON_OPTION, + option_parse_early_allow_unsafe, &state); + free(early); 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..1a37f2b8e6 100755 --- a/t/t9300-fast-import.sh +++ b/t/t9300-fast-import.sh @@ -2344,6 +2344,20 @@ 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 && + test_must_fail git fast-import --allow-unsafe 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