{"thread":{"id":"66257","subject":"[PATCH 0/6] Standardize early option scanning to fix argument parsing bugs","startedAt":"2026-09-02T16:11:15Z","lastAt":"2026-09-30T07:13:59Z","messageCount":21,"participants":["Christian Couder","Junio C Hamano","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"551779","messageId":"20260902161047.476753-1-christian.couder@gmail.com","threadId":"66257","inReplyTo":null,"subject":"[PATCH 0/6] Standardize early option scanning to fix argument parsing bugs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:41Z","receivedAt":"2026-09-02T16:11:15Z","isPatch":true,"body":"A number of commands perform an early scan of their arguments to look\nfor specific flags or structural separators (like `--`).\n\nThese hand-rolled early scans are often fragile. They especially fail\nto account for options that take their value as a separate\nargument. This leads to disagreements between the early scan and the\nactual parse_options() pass. For example, the early scanner might miss\na special option entirely, or mistakenly treat an option's value as\nthe `--` path separator.\n\nTo allow these commands to safely skip option values during their\nearly scans, this series introduces a new \"early-scan\" sub-API into\nthe existing \"parse-options\" API.\n\nThis is deliberately implemented as a new simple and fast scan, which\nhas some limitations, instead of a full refactor and reuse of the\nparse_options() code, because the limitations are not very significant\nin practice, while a full refactor and reuse of the parse_options()\ncode would be much more complex.\n\nThe current limitations of the new early scan code are:\n\n 1. short options are ignored,\n\n 2. options with PARSE_OPT_LASTARG_DEFAULT or PARSE_OPT_OPTARG are\n treated as not taking a separate value,\n\n 3. negated options (\"--no-...\") are not automatically generated,\n\n 4. abbreviated options will not be matched.\n\nNote that while the others could be real issues for some commands,\n\"3. negated options\" is not a practical issue because negated options\nnever consume a separate argument.\n\nThe early scan is performed by a new early_scan_options() function\nwhich takes a `const struct early_scan_option *options` array as\nargument. That array can be built either by hand or by a new\nearly_scan_options_from_options() function, which takes a\n`const struct option *options` array, when the command already uses\n`struct option`.\n\nThis allows us to use the new early-scan API even for commands that\ndon't use the parse-options API yet, and which are the majority of\ncommands performing an early scan.\n\nIn this series, only `git bisect`, `git rev-parse` and `git\nfast-import` are converted to the early-scan API, which fixes bugs in\nthose commands:\n\n - `git bisect start --term-good -- ` mistook the term name\n `--` for the revision/path separator, so was rejected\n as an invalid revision instead of being treated as a path.\n\n - `git rev-parse --default -- ` did the same, reporting\n \"bad revision \" while any other default value gives the\n usual more helpful \"ambiguous argument\" error.\n\n - `git fast-import --depth 5 --allow-unsafe-features` silently\n ignored `--allow-unsafe-features`, refusing unsafe features from\n the stream.\n\nAll of these commands call parse_options(), but for `git bisect` and\n`git rev-parse`, the specific functions doing the early scan\n(bisect_start() and cmd_rev_parse()'s main loop) parse their own\noptions by hand after the early scan and have no `struct option` array\nfor those options.\n\nIf bisect_start() and cmd_rev_parse() were converted to use\n`struct option`, they could use early_scan_options_from_options() and\nwould not be affected by limitations 1), 2) and 3) above, as both use\nthe early scan only to locate `--`.\n\nNote that using early_scan_options_from_options() rather than a\nhand-written table does not change how abbreviations are handled: the\nscan matches long names exactly either way. Limitation 4) would\nnevertheless become relevant to those commands, because such a\nconversion would also make parse_options() the parser for the options\nafter the early scan has first inspected them, and parse_options()\nresolves abbreviations while their current hand-rolled loops do not.\n\n`git diff`, `git column`, `git rev-list` and setup_revisions() in\n\"revision.c\" could also be converted to the early-scan API but aren't\nin this series for different reasons:\n\n - `git diff` has a number of short options like `-S`, `-G`, `-O`\n taking separate values.\n\n - `git column` scans `argv[1]` for `--command=` before reading the\n configuration. Because `--command` is an OPT_STRING,\n parse_options() also accepts `--command ` and abbreviations,\n so the two passes disagree. Converting it would fix that, but it\n changes user-visible behaviour in a command this series does not\n otherwise touch.\n\n - `git rev-list` and \"revision.c\" are about converting\n setup_revisions(), but converting it to `struct option` first is\n likely the better way forward.\n\nOverview of the patches:\n========================\n\n - Patch 1/6 introduces early_scan_options(), the early scanner that\n will be used instead of hand-rolled ones, along with its\n infrastructure.\n\n - Patches 2/6 and 3/6 use this scanner to fix bugs in `git bisect`\n and `git rev-parse` respectively.\n\n - Patch 4/6 refactors some existing code into a new\n parse_options_takes_argument() helper that will be used in the next\n patch.\n\n - Patch 5/6 introduces the new early_scan_options_from_options() as a\n bridge between the parse-options API and the early-scan API.\n\n - Patch 6/6 uses early_scan_options_from_options() to fix the early\n scan for `--allow-unsafe-features` in `git fast-import`.\n\nCI tests:\n=========\n\nThey all pass, see:\n\nhttps://github.com/chriscool/git/actions/runs/33612974808\n\n\nChristian Couder (6):\n parse-options: add early_scan_options()\n bisect: fix \"--\" detection when a term name is \"--\"\n rev-parse: fix \"--\" detection when it is an option value\n parse-options: add parse_options_takes_argument()\n parse-options: build early scan options from a struct option array\n fast-import: use early_scan_options() for --allow-unsafe-features\n\n Documentation/git-fast-import.adoc | 10 +-\n builtin/bisect.c | 27 ++++--\n builtin/fast-import.c | 46 +++++----\n builtin/rev-parse.c | 26 ++++--\n parse-options.c | 144 ++++++++++++++++++++++++++---\n parse-options.h | 92 ++++++++++++++++++\n t/helper/test-parse-options.c | 71 ++++++++++++++\n t/helper/test-tool.c | 2 +\n t/helper/test-tool.h | 2 +\n t/t0040-parse-options.sh | 103 +++++++++++++++++++++\n t/t1500-rev-parse.sh | 5 +\n t/t6030-bisect-porcelain.sh | 8 ++\n t/t9300-fast-import.sh | 14 +++\n 13 files changed, 503 insertions(+), 47 deletions(-)\n\n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551780","messageId":"20260902161047.476753-2-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 1/6] parse-options: add early_scan_options()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:42Z","receivedAt":"2026-09-02T16:11:17Z","isPatch":true,"body":"Some commands need to look at a few of their options before they can\nparse their command line for real, for example because the result\ndecides whether a repository is needed at all, or how the beginning of\ntheir input should be interpreted.\n\nSuch an early scan has to know which options take their value as a\nseparate argument, or it mistakes such a value for an option. Several\ncommands get this wrong, as they just walk their arguments comparing\nthem to the few option names they care about.\n\nLet's add early_scan_options() to help with this. Its callers describe\nthe options to look for, but also the ones that merely have to be\nskipped along with their value, so that the scan can walk the arguments\nwithout being fooled by option values.\n\nNote that abbreviated options are deliberately not recognized, as a\nscan cannot know about the options it hasn't been told about, and would\nthen resolve abbreviations differently from the actual option parsing.\n\nSo users must spell these specific options in full. This restriction\ncould be lifted in the future though, once the scanner is adapted to\naccept a command's full option array, as this would give it the\ncomplete context needed for safe abbreviation matching.\n\nSigned-off-by: Christian Couder \n---\n parse-options.c | 70 +++++++++++++++++++++++++++++++\n parse-options.h | 60 +++++++++++++++++++++++++++\n t/helper/test-parse-options.c | 39 ++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0040-parse-options.sh | 77 +++++++++++++++++++++++++++++++++++\n 6 files changed, 248 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4519ead9dc..b3d19446cd 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1244,6 +1244,76 @@ int parse_options(int argc, const char **argv,\n \treturn parse_options_end(&ctx);\n }\n \n+/*\n+ * Look for `arg` among `options`. On success, return the matching option\n+ * and set `value` to the value stuck to it, if any, or to NULL.\n+ */\n+static const struct early_scan_option *\n+find_early_scan_option(const char *arg,\n+\t\t const struct early_scan_option *options,\n+\t\t const char **value)\n+{\n+\tif (!skip_prefix(arg, \"--\", &arg))\n+\t\treturn NULL;\n+\n+\tfor (; options->name; options++) {\n+\t\tconst char *rest;\n+\n+\t\tif (!skip_prefix(arg, options->name, &rest))\n+\t\t\tcontinue;\n+\t\tif (!*rest) {\n+\t\t\t*value = NULL;\n+\t\t\treturn options;\n+\t\t}\n+\t\t/* Only an option taking a value can be stuck to one. */\n+\t\tif (*rest == '=' && options->takes_value) {\n+\t\t\t*value = rest + 1;\n+\t\t\treturn options;\n+\t\t}\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+int early_scan_options(int argc, const char **argv,\n+\t\t const struct early_scan_option *options,\n+\t\t enum early_scan_flags flags,\n+\t\t early_scan_fn *fn, void *data)\n+{\n+\tfor (int i = 0; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tconst char *value;\n+\t\tconst struct early_scan_option *opt;\n+\t\tint pos = i;\n+\n+\t\tif ((flags & EARLY_SCAN_STOP_AT_DASHDASH) &&\n+\t\t !strcmp(arg, \"--\"))\n+\t\t\treturn i;\n+\n+\t\topt = find_early_scan_option(arg, options, &value);\n+\t\tif (!opt) {\n+\t\t\tif ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&\n+\t\t\t (*arg != '-' || !arg[1]))\n+\t\t\t\treturn i;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * When an option takes a value, but that value is not\n+\t\t * stuck to it with '=', then the next argument is the\n+\t\t * value and it has to be skipped so that it isn't\n+\t\t * taken for an option itself.\n+\t\t */\n+\t\tif (opt->takes_value && !value && i + 1 < argc)\n+\t\t\tvalue = argv[++i];\n+\n+\t\tif (opt->wanted && fn(opt, value, pos, data))\n+\t\t\treturn i;\n+\t}\n+\n+\treturn argc;\n+}\n+\n static int usage_argh(const struct option *opts, FILE *outfile)\n {\n \tconst char *s;\ndiff --git a/parse-options.h b/parse-options.h\nindex d7f896a933..abc73d8399 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -491,6 +491,66 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\tBUG(\"option callback expects an argument\"); \\\n } while(0)\n \n+/*----- Early scan: scanning argv before the actual option parsing -----*/\n+\n+/*\n+ * Some commands need to look at a few options before they can parse\n+ * their command line for real, for example because the result decides\n+ * whether a repository is needed at all.\n+ *\n+ * Such an early scan has to know which options take their value as a\n+ * separate argument, or it could mistake such a value for an option. The\n+ * `struct early_scan_option` array passed to early_scan_options() below\n+ * describes the options to look for, as well as the ones that only need\n+ * to be skipped along with their value.\n+ */\n+struct early_scan_option {\n+\tconst char *name; \t/* Option name, without the leading dashes */\n+\tunsigned takes_value:1; /* \"--option=value\" or \"--option value\" expected? */\n+\tunsigned wanted:1; /* Report option to callback? */\n+};\n+\n+#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 }\n+#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 }\n+#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 }\n+#define EARLY_SCAN_END() { NULL }\n+\n+/*\n+ * Called by early_scan_options() for each argument matching a\n+ * `struct early_scan_option` that has its `wanted` bit set.\n+ *\n+ * `option` is the matching option, `value` its value or NULL if it\n+ * doesn't take one, and `pos` the index of the option in argv.\n+ *\n+ * Returning a non-zero value stops the scan.\n+ */\n+typedef int early_scan_fn(const struct early_scan_option *option,\n+\t\t\t const char *value, int pos, void *data);\n+\n+enum early_scan_flags {\n+\tEARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at \"--\" */\n+\tEARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1,\n+};\n+\n+/*\n+ * Scan `argv` for the options described by `options`, calling `fn`\n+ * for each of those that are `wanted`. `argv` is not modified.\n+ *\n+ * `fn` may be NULL when no option is `wanted`, which is useful to only\n+ * find out where the scan stops.\n+ *\n+ * Note that abbreviated options are not recognized, as a scan cannot\n+ * know about the options it hasn't been told about, and would then\n+ * resolve abbreviations differently from the actual option parsing.\n+ *\n+ * Returns the index at which the scan stopped, which is `argc` when the\n+ * whole array was scanned.\n+ */\n+int early_scan_options(int argc, const char **argv,\n+\t\t const struct early_scan_option *options,\n+\t\t enum early_scan_flags flags,\n+\t\t early_scan_fn *fn, void *data);\n+\n /*----- incremental advanced APIs -----*/\n \n struct parse_opt_cmdmode_list;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex f181f0c02d..96ab941d29 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -383,3 +383,42 @@ int cmd__parse_subcommand(int argc, const char **argv)\n \n \treturn parse_subcommand__cmd(argc, argv, test_flags);\n }\n+\n+static int show_early_option(const struct early_scan_option *opt,\n+\t\t\t const char *value, int pos, void *data UNUSED)\n+{\n+\tprintf(\"found: %s at %d\", opt->name, pos);\n+\tif (value)\n+\t\tprintf(\" value: %s\", value);\n+\tputchar('\\n');\n+\treturn 0;\n+}\n+\n+int cmd__early_scan_options(int argc, const char **argv)\n+{\n+\tstatic const struct early_scan_option options[] = {\n+\t\tEARLY_SCAN_WANT(\"wanted\"),\n+\t\tEARLY_SCAN_WANT_VALUE(\"wanted-value\"),\n+\t\tEARLY_SCAN_SKIP_VALUE(\"skipped-value\"),\n+\t\tEARLY_SCAN_END()\n+\t};\n+\tenum early_scan_flags flags = 0;\n+\tint stopped;\n+\n+\twhile (argc > 1 && *argv[1] == '-') {\n+\t\tif (!strcmp(argv[1], \"--stop-at-dashdash\"))\n+\t\t\tflags |= EARLY_SCAN_STOP_AT_DASHDASH;\n+\t\telse if (!strcmp(argv[1], \"--stop-at-non-option\"))\n+\t\t\tflags |= EARLY_SCAN_STOP_AT_NON_OPTION;\n+\t\telse\n+\t\t\tbreak;\n+\t\targc--;\n+\t\targv++;\n+\t}\n+\n+\tstopped = early_scan_options(argc - 1, argv + 1, options, flags,\n+\t\t\t\t show_early_option, NULL);\n+\tprintf(\"stopped at: %d of %d\\n\", stopped, argc - 1);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex b71a22b43b..5d2f5877d9 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -50,6 +50,7 @@ static struct test_cmd cmds[] = {\n \t{ \"pack-mtimes\", cmd__pack_mtimes },\n \t{ \"parse-options\", cmd__parse_options },\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n+\t{ \"early-scan-options\", cmd__early_scan_options },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n \t{ \"partial-clone\", cmd__partial_clone },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f2885b33d5..071306d52d 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -43,6 +43,7 @@ int cmd__pack_deltas(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_options_flags(int argc, const char **argv);\n+int cmd__early_scan_options(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__parse_subcommand(int argc, const char **argv);\n int cmd__partial_clone(int argc, const char **argv);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 449fff4d34..d760d8cfbd 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -845,4 +845,81 @@ test_expect_success 'u16 limits range' '\n \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n '\n \n+test_expect_success 'early_scan_options() finds a wanted option' '\n+\ttest-tool early-scan-options --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 0\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() reads a stuck or separate value' '\n+\ttest-tool early-scan-options --wanted-value=one >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted-value at 0 value: one\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --wanted-value two >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted-value at 0 value: two\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() skips the value of other options' '\n+\ttest-tool early-scan-options --skipped-value --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --skipped-value one --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() can stop at \"--\"' '\n+\ttest-tool early-scan-options --stop-at-dashdash -- --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --stop-at-dashdash \\\n+\t\t--skipped-value -- --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() can stop at a non-option' '\n+\ttest-tool early-scan-options --stop-at-non-option \\\n+\t\targ --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --stop-at-non-option \\\n+\t\t--skipped-value arg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() ignores abbreviated options' '\n+\ttest-tool early-scan-options --want >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551781","messageId":"20260902161047.476753-3-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 2/6] bisect: fix \"--\" detection when a term name is \"--\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:43Z","receivedAt":"2026-09-02T16:11:19Z","isPatch":true,"body":"`bisect_start()` walks its arguments twice. The second loop actually\nparses the options, and it knows that `--term-good`, `--term-old`,\n`--term-bad` and `--term-new` take their value as a separate argument,\nso it skips that value.\n\nThe first loop, which only looks for the \"--\" separating revisions from\npaths, doesn't know about these options. So when such an option is given\n\"--\" as its value, that \"--\" is mistaken for the separator and\n`has_double_dash` is wrongly set.\n\nThis matters because `has_double_dash` makes the second loop die on an\nargument that is not a valid revision, instead of treating it as the\nfirst path. So:\n\n $ git bisect start --term-good -- notarev\n fatal: 'notarev' does not appear to be a valid revision\n\nwhile the very same command line with any other term name happily takes\n\"notarev\" as a path.\n\nLet's fix this by using early_scan_options(), telling it about the\noptions taking their value as a separate argument, so that it can skip\nthose values.\n\nNote: One might argue that accepting a term name that looks like an\noption (such as \"--\") is a misfeature and should be forbidden entirely.\nHowever, whether we should tighten the validation rules for bisect\nterms is a separate UI issue that can be dealt with independently. For\nnow, this commit simply ensures the parser correctly implements the\nexisting rules.\n\nSigned-off-by: Christian Couder \n---\n builtin/bisect.c | 27 +++++++++++++++++++++------\n t/t6030-bisect-porcelain.sh | 8 ++++++++\n 2 files changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/bisect.c b/builtin/bisect.c\nindex 1cfb8a794b..ad089b289f 100644\n--- a/builtin/bisect.c\n+++ b/builtin/bisect.c\n@@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms,\n \treturn bisect_next(terms, prefix);\n }\n \n+/*\n+ * The options \"git bisect start\" accepts. Only the ones taking their\n+ * value as a separate argument matter to the scan looking for \"--\" below,\n+ * as their value has to be skipped along with them.\n+ */\n+static const struct early_scan_option bisect_start_early_options[] = {\n+\tEARLY_SCAN_SKIP_VALUE(\"term-good\"),\n+\tEARLY_SCAN_SKIP_VALUE(\"term-old\"),\n+\tEARLY_SCAN_SKIP_VALUE(\"term-bad\"),\n+\tEARLY_SCAN_SKIP_VALUE(\"term-new\"),\n+\tEARLY_SCAN_END()\n+};\n+\n static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \t\t\t\t const char **argv)\n {\n@@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n \n \t/*\n \t * Check for one bad and then some good revisions\n+\t *\n+\t * The scan below has to know about the options taking their value\n+\t * as a separate argument, or such a value that happens to be \"--\"\n+\t * would be mistaken for the \"--\" separating revisions from paths.\n \t */\n-\tfor (i = 0; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\thas_double_dash = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\ti = early_scan_options(argc, argv, bisect_start_early_options,\n+\t\t\t EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);\n+\tif (i < argc)\n+\t\thas_double_dash = 1;\n \n \tfor (i = 0; i < argc; i++) {\n \t\tconst char *arg = argv[i];\ndiff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\nindex a7588222a8..464ca53b42 100755\n--- a/t/t6030-bisect-porcelain.sh\n+++ b/t/t6030-bisect-porcelain.sh\n@@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'bisect start with \"--\" as a term name' '\n+\tgit bisect reset &&\n+\tgit bisect start --term-good -- hello &&\n+\tgit bisect terms --term-good >actual &&\n+\techo -- >expected &&\n+\ttest_cmp expected actual\n+'\n+\n # Bisect is started with --term-new and --term-old arguments,\n # then skip. The HEAD should be changed.\n test_expect_success 'bisect skip works with --term*' '\n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551782","messageId":"20260902161047.476753-4-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 3/6] rev-parse: fix \"--\" detection when it is an option value","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:44Z","receivedAt":"2026-09-02T16:11:21Z","isPatch":true,"body":"`cmd_rev_parse()` walks its arguments twice. The second loop actually\nparses the options, and it knows that `--default`, `--prefix` and\n`--resolve-git-dir` take their value as a separate argument, so it skips\nthat value.\n\nThe first loop, which only looks for the \"--\" separating revisions from\npaths, doesn't know about these options. So when such an option is given\n\"--\" as its value, that \"--\" is mistaken for the separator and\n`has_dashdash` is wrongly set.\n\nThis matters because `has_dashdash` makes the second loop die with a\n\"bad revision\" error on an argument that is neither a revision nor an\nexisting file, instead of reporting that the argument is ambiguous and\ntelling how to disambiguate it. So:\n\n $ git rev-parse --default -- notarev\n fatal: bad revision 'notarev'\n\nwhile the very same command line with any other default value gives the\nusual, much more helpful, \"ambiguous argument\" error.\n\nLet's fix this the same way as in a previous commit, by using\nearly_scan_options() and telling it about the options taking their value\nas a separate argument.\n\nSigned-off-by: Christian Couder \n---\n builtin/rev-parse.c | 26 ++++++++++++++++++++------\n t/t1500-rev-parse.sh | 5 +++++\n 2 files changed, 25 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 43693454d5..7ced82e25d 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -695,6 +695,17 @@ static void print_path(const char *path, const char *prefix,\n \tstrbuf_release(&sb);\n }\n \n+/*\n+ * The options taking their value as a separate argument, which the scan\n+ * looking for \"--\" below has to skip along with their value.\n+ */\n+static const struct early_scan_option rev_parse_early_options[] = {\n+\tEARLY_SCAN_SKIP_VALUE(\"default\"),\n+\tEARLY_SCAN_SKIP_VALUE(\"prefix\"),\n+\tEARLY_SCAN_SKIP_VALUE(\"resolve-git-dir\"),\n+\tEARLY_SCAN_END()\n+};\n+\n int cmd_rev_parse(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -724,12 +735,15 @@ int cmd_rev_parse(int argc,\n \tif (argc > 1 && !strcmp(\"-h\", argv[1]))\n \t\tusage(builtin_rev_parse_usage);\n \n-\tfor (i = 1; i < argc; i++) {\n-\t\tif (!strcmp(argv[i], \"--\")) {\n-\t\t\thas_dashdash = 1;\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\t/*\n+\t * The scan below has to know about the options taking their value\n+\t * as a separate argument, or such a value that happens to be \"--\"\n+\t * would be mistaken for the \"--\" separating revisions from paths.\n+\t */\n+\ti = early_scan_options(argc - 1, argv + 1, rev_parse_early_options,\n+\t\t\t EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);\n+\tif (i < argc - 1)\n+\t\thas_dashdash = 1;\n \n \t/* No options; just report on whether we're in a git repo or not. */\n \tif (argc == 1) {\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 4174ca40c3..897e9a7735 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -383,4 +383,9 @@ test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '\n \t)\n '\n \n+test_expect_success 'rev-parse with \"--\" as an option value' '\n+\ttest_must_fail git rev-parse --default -- notarev 2>err &&\n+\ttest_grep \"ambiguous argument .notarev.\" err\n+'\n+\n test_done\n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551783","messageId":"20260902161047.476753-5-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 4/6] parse-options: add parse_options_takes_argument()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:45Z","receivedAt":"2026-09-02T16:11:25Z","isPatch":true,"body":"Whether an option takes a value, and therefore consumes the next\nargument when that value is not stuck to it with an '=', is decided by\nits type and its flags. That rule is currently open-coded in\nshow_gitcomp(), which needs it to decide if it should append an '=' to\nthe option it completes.\n\nA following commit will need the same rule to find out which options an\nearly scan of the command line has to skip along with their value.\n\nSo let's factor that rule out into a new parse_options_takes_argument()\nfunction, and let's use it in show_gitcomp().\n\nNote that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the\nnext argument when it isn't the last one, so it is not considered as\ntaking a value, which is what show_gitcomp() already did.\n\nSigned-off-by: Christian Couder \n---\n parse-options.c | 35 ++++++++++++++++++++++-------------\n parse-options.h | 10 ++++++++++\n 2 files changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex b3d19446cd..70851a385b 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -841,6 +841,26 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t}\n }\n \n+int parse_options_takes_argument(const struct option *opt)\n+{\n+\tswitch (opt->type) {\n+\tcase OPTION_STRING:\n+\tcase OPTION_FILENAME:\n+\tcase OPTION_INTEGER:\n+\tcase OPTION_UNSIGNED:\n+\tcase OPTION_CALLBACK:\n+\t\tbreak;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n+\tif (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG |\n+\t\t\t PARSE_OPT_LASTARG_DEFAULT))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n static int show_gitcomp(const struct option *opts, int show_all)\n {\n \tconst struct option *original_opts = opts;\n@@ -862,20 +882,9 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\tbreak;\n \t\tcase OPTION_GROUP:\n \t\t\tcontinue;\n-\t\tcase OPTION_STRING:\n-\t\tcase OPTION_FILENAME:\n-\t\tcase OPTION_INTEGER:\n-\t\tcase OPTION_UNSIGNED:\n-\t\tcase OPTION_CALLBACK:\n-\t\t\tif (opts->flags & PARSE_OPT_NOARG)\n-\t\t\t\tbreak;\n-\t\t\tif (opts->flags & PARSE_OPT_OPTARG)\n-\t\t\t\tbreak;\n-\t\t\tif (opts->flags & PARSE_OPT_LASTARG_DEFAULT)\n-\t\t\t\tbreak;\n-\t\t\tsuffix = \"=\";\n-\t\t\tbreak;\n \t\tdefault:\n+\t\t\tif (parse_options_takes_argument(opts))\n+\t\t\t\tsuffix = \"=\";\n \t\t\tbreak;\n \t\t}\n \t\tif (opts->flags & PARSE_OPT_COMP_ARG)\ndiff --git a/parse-options.h b/parse-options.h\nindex abc73d8399..b96e93508e 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -420,6 +420,16 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const char * const usagestr[],\n \t\t enum parse_opt_flags flags);\n \n+/*\n+ * Return non-zero if `opt` takes a value, which means that it consumes\n+ * the next argument when that value is not stuck to it with an '='.\n+ *\n+ * Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the\n+ * next argument when it isn't the last one, so it is not considered as\n+ * taking a value here.\n+ */\n+int parse_options_takes_argument(const struct option *opt);\n+\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551784","messageId":"20260902161047.476753-6-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 5/6] parse-options: build early scan options from a struct option array","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:46Z","receivedAt":"2026-09-02T16:11:28Z","isPatch":true,"body":"A command that scans its arguments early has to know which options take\na value, so that it can skip that value instead of mistaking it for an\noption. When it also parses its options with the parse-options API, that\ninformation is already available in its `struct option` array, and\nduplicating it by hand in a `struct early_scan_option` array is both\ntedious and easy to get out of sync when an option is added.\n\nSo let's add early_scan_options_from_options() to build the latter array\nfrom the former, using parse_options_takes_argument() to find out which\noptions take a value. Its caller only has to name the options it wants\nto be reported.\n\nNote: This early scanner translation intentionally leaves out a few\ncomplex option types to keep the scan simple and fast:\n\n- Short options are ignored: early_scan_options_from_options()\n explicitly skips options without a `long_name`, and the scanner only\n looks for `--`. Properly handling short options would require parsing\n bundled flags (e.g., `-abc value`), which requires replicating the\n full parse_options() state machine.\n\n- Conditional values: Options with `PARSE_OPT_LASTARG_DEFAULT` or\n `PARSE_OPT_OPTARG` are treated as not taking a separate argument.\n Because the scanner does not evaluate context (like whether an\n argument is the final one in `argv`), it must err on the side of\n caution to avoid accidentally consuming the `--` separator or a path.\n\n- Abbreviated options remain unrecognized: Even though the scanner is\n now provided with the full option array, the underlying\n early_scan_options() engine still relies on exact string matches.\n Safely resolving abbreviations would require duplicating the\n ambiguity-checking logic from the main parser.\n\n- Negated options are not automatically derived: The scanner strictly\n matches the defined long name. It does not automatically recognize\n the `--no-` variants of boolean options. (This is harmless in\n practice for current callers, as negated options do not take values\n to skip, and boolean defaults align with the ignored state).\n\nThe above shortcomings can be addressed later, for example, when\ncommands that use short options or options with conditional values need\nan early scan or are ported to use `struct option`.\n\nDespite these limitations, this abstraction is a significant\nimprovement. It allows commands like `fast-import` to reuse their\nexisting `struct option` array for early scanning, ensuring the scanner\nand the main parser agree on which options take arguments, and\npreventing developers from having to maintain a separate, hardcoded\nlist that could drift out of sync.\n\nSigned-off-by: Christian Couder \n---\n parse-options.c | 39 +++++++++++++++++++++++++++++++++++\n parse-options.h | 22 ++++++++++++++++++++\n t/helper/test-parse-options.c | 32 ++++++++++++++++++++++++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0040-parse-options.sh | 26 +++++++++++++++++++++++\n 6 files changed, 121 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 70851a385b..6cdc9c64cc 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1323,6 +1323,45 @@ int early_scan_options(int argc, const char **argv,\n \treturn argc;\n }\n \n+struct early_scan_option *\n+early_scan_options_from_options(const struct option *options,\n+\t\t\t\tconst char **wanted)\n+{\n+\tstruct early_scan_option *early;\n+\tsize_t nr = 0;\n+\n+\tfor (const struct option *opt = options; opt->type != OPTION_END; opt++)\n+\t\tif (opt->long_name)\n+\t\t\tnr++;\n+\n+\tCALLOC_ARRAY(early, nr + 1);\n+\n+\tnr = 0;\n+\tfor (const struct option *opt = options; opt->type != OPTION_END; opt++) {\n+\t\tif (!opt->long_name)\n+\t\t\tcontinue;\n+\t\tearly[nr].name = opt->long_name;\n+\t\tearly[nr].takes_value = !!parse_options_takes_argument(opt);\n+\t\tnr++;\n+\t}\n+\n+\tfor (; wanted && *wanted; wanted++) {\n+\t\tsize_t i;\n+\n+\t\tfor (i = 0; i < nr; i++) {\n+\t\t\tif (strcmp(early[i].name, *wanted))\n+\t\t\t\tcontinue;\n+\t\t\tearly[i].wanted = 1;\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (i == nr)\n+\t\t\tBUG(\"wanted option '%s' is not in the options array\",\n+\t\t\t *wanted);\n+\t}\n+\n+\treturn early;\n+}\n+\n static int usage_argh(const struct option *opts, FILE *outfile)\n {\n \tconst char *s;\ndiff --git a/parse-options.h b/parse-options.h\nindex b96e93508e..fb81f2ed38 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -561,6 +561,28 @@ int early_scan_options(int argc, const char **argv,\n \t\t enum early_scan_flags flags,\n \t\t early_scan_fn *fn, void *data);\n \n+/*\n+ * Build the `struct early_scan_option` array to pass to\n+ * early_scan_options() from the `options` array that the actual option\n+ * parsing uses, so that both agree on which options take a value.\n+ *\n+ * Note some intentional limitations to keep the scan simple and fast:\n+ * short options are ignored, options with PARSE_OPT_LASTARG_DEFAULT or\n+ * PARSE_OPT_OPTARG are treated as not taking a separate value, negated\n+ * options (\"--no-...\") are not automatically generated, and abbreviated\n+ * options will not be matched.\n+ *\n+ * The options named in the NULL terminated `wanted` array get their\n+ * `wanted` bit set, the other ones are only there to be skipped along\n+ * with their value. It is a BUG() for a name in `wanted` not to appear\n+ * in `options`.\n+ *\n+ * The returned array is allocated and should be free()d by the caller.\n+ */\n+struct early_scan_option *\n+early_scan_options_from_options(const struct option *options,\n+\t\t\t\tconst char **wanted);\n+\n /*----- incremental advanced APIs -----*/\n \n struct parse_opt_cmdmode_list;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 96ab941d29..0187a25ccb 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -422,3 +422,35 @@ int cmd__early_scan_options(int argc, const char **argv)\n \n \treturn 0;\n }\n+\n+int cmd__early_scan_from_options(int argc, const char **argv)\n+{\n+\tint an_int = 0, a_bool = 0;\n+\tchar *a_string = NULL;\n+\tconst struct option options[] = {\n+\t\tOPT_STRING(0, \"string\", &a_string, \"str\", \"get a string\"),\n+\t\tOPT_INTEGER(0, \"int\", &an_int, \"get an integer\"),\n+\t\tOPT_BOOL(0, \"bool\", &a_bool, \"get a boolean\"),\n+\t\tOPT_STRING_F(0, \"optarg\", &a_string, \"str\",\n+\t\t\t \"string with an optional value\",\n+\t\t\t PARSE_OPT_OPTARG),\n+\t\tOPT_END()\n+\t};\n+\tstatic const char *wanted[] = { \"bool\", NULL };\n+\tstruct early_scan_option *early;\n+\tint stopped;\n+\n+\tearly = early_scan_options_from_options(options, wanted);\n+\n+\tfor (const struct early_scan_option *o = early; o->name; o++)\n+\t\tprintf(\"option: %s takes_value: %d wanted: %d\\n\",\n+\t\t o->name, o->takes_value, o->wanted);\n+\n+\tstopped = early_scan_options(argc - 1, argv + 1, early, 0,\n+\t\t\t\t show_early_option, NULL);\n+\tprintf(\"stopped at: %d of %d\\n\", stopped, argc - 1);\n+\n+\tfree(early);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 5d2f5877d9..f1b208a5af 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {\n \t{ \"parse-options\", cmd__parse_options },\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n \t{ \"early-scan-options\", cmd__early_scan_options },\n+\t{ \"early-scan-from-options\", cmd__early_scan_from_options },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n \t{ \"partial-clone\", cmd__partial_clone },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 071306d52d..97334ce3c6 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -44,6 +44,7 @@ int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_options_flags(int argc, const char **argv);\n int cmd__early_scan_options(int argc, const char **argv);\n+int cmd__early_scan_from_options(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__parse_subcommand(int argc, const char **argv);\n int cmd__partial_clone(int argc, const char **argv);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex d760d8cfbd..bb72a6544d 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -922,4 +922,30 @@ test_expect_success 'early_scan_options() ignores abbreviated options' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'early_scan_options_from_options() derives takes_value' '\n+\ttest-tool early-scan-from-options >actual &&\n+\tcat >expect <<-\\EOF &&\n+\toption: string takes_value: 1 wanted: 0\n+\toption: int takes_value: 1 wanted: 0\n+\toption: bool takes_value: 0 wanted: 1\n+\toption: optarg takes_value: 0 wanted: 0\n+\tstopped at: 0 of 0\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options_from_options() skips values' '\n+\ttest-tool early-scan-from-options --string --bool >out &&\n+\ttail -1 out >actual &&\n+\techo \"stopped at: 2 of 2\" >expect &&\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-from-options --string v --bool >out &&\n+\ttail -2 out >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: bool at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.55.0.787.g3f9e2241eb.dirty\n\n"},{"id":"551785","messageId":"20260902161047.476753-7-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH 6/6] fast-import: use early_scan_options() for --allow-unsafe-features","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-02T16:10:47Z","receivedAt":"2026-09-02T16:11:32Z","isPatch":true,"body":"The \"feature\" lines at the start of the stream are processed before the\ncommand line options are parsed, so cmd_fast_import() scans its\narguments early to find out if `--allow-unsafe-features` was given.\n\nThat scan doesn't know which options take their value as a separate\nargument, and it stops at the first argument that doesn't start with a\ndash. So it disagrees with parse_options(), which accepts values\nseparated from their option by a space, for a command line like\n\"--depth 5 --allow-unsafe-features\": the scan stops at \"5\" and never\nsees the option, so unsafe \"feature\" commands from the stream are\nrefused even though the option was given.\n\nLet's fix this by building the options for the scan from the same\n`struct option` array that parse_options() uses, so that both agree on\nwhich options take a value.\n\nNote that the scan still only matches the exact option spelling, while\nparse_options() also accepts unambiguous abbreviations, so the two still\ndisagree for a command line like \"--allow-unsafe\". This errs on the safe\nside, and is now documented as a restriction.\n\nSigned-off-by: Christian Couder \n---\n Documentation/git-fast-import.adoc | 10 +++----\n builtin/fast-import.c | 46 +++++++++++++++++++-----------\n t/t9300-fast-import.sh | 14 +++++++++\n 3 files changed, 48 insertions(+), 22 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\nindex fd165e11d2..9758ba5275 100644\n--- a/Documentation/git-fast-import.adoc\n+++ b/Documentation/git-fast-import.adoc\n@@ -66,12 +66,10 @@ fast-import stream! This option is enabled automatically for\n remote-helpers that use the `import` capability, as they are\n already trusted to run their own code.\n +\n-Note that this option has to be spelled in full, and has to appear\n-before any option whose value is separated from it by a space, for\n-the unsafe `feature` commands in the stream to be allowed. So\n-`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse\n-them, while `--allow-unsafe-features --depth 5` and\n-`--depth=5 --allow-unsafe-features` allow them.\n+Note that this option has to be spelled in full for the unsafe\n+`feature` commands in the stream to be allowed. So while\n+`--allow-unsafe` is accepted as an unambiguous abbreviation of this\n+option, it still refuses them.\n \n `--signed-tags=`::\n \tSpecify how to handle signed tags. Behaves in the same way as\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex fbd919982c..cf0504f01c 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -4120,12 +4120,29 @@ static int option_parse_quiet(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n+/*\n+ * The only option the early scan below is interested in, as it decides\n+ * whether unsafe \"feature\" commands from the stream are allowed.\n+ */\n+static const char *early_wanted[] = { \"allow-unsafe-features\", NULL };\n+\n+static int option_parse_early_allow_unsafe(\n+\t\tconst struct early_scan_option *opt UNUSED,\n+\t\tconst char *value UNUSED, int pos UNUSED, void *data)\n+{\n+\tstruct fast_import_state *state = data;\n+\n+\tstate->allow_unsafe_features = 1;\n+\treturn 0;\n+}\n+\n int cmd_fast_import(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n \t\t struct repository *repo)\n {\n \tstruct fast_import_state state;\n+\tstruct early_scan_option *early;\n \n \tstruct option fast_import_options[] = {\n \t\tOPT_GROUP(N_(\"Common\")),\n@@ -4218,23 +4235,20 @@ int cmd_fast_import(int argc,\n \t * line to override stream data). But we must do an early parse of any\n \t * command-line options that impact how we interpret the feature lines.\n \t *\n-\t * NEEDSWORK: This scan only matches the exact \"--allow-unsafe-features\"\n-\t * spelling and stops at the first argument that doesn't start with a\n-\t * dash. As parse_options() below also accepts unambiguous abbreviations\n-\t * and values separated by a space from their option, the two disagree\n-\t * for command lines like \"--allow-unsafe\" or \"--depth 5\n-\t * --allow-unsafe-features\": parse_options() accepts the option, but\n-\t * this scan doesn't see it, so unsafe features from the stream are\n-\t * still refused. This errs on the safe side, but should be fixed by\n-\t * teaching this scan about the options that take a value.\n+\t * NEEDSWORK: This scan only matches the exact\n+\t * \"--allow-unsafe-features\" spelling, while parse_options() below\n+\t * also accepts unambiguous abbreviations, so the two disagree for\n+\t * a command line like \"--allow-unsafe\": parse_options() accepts\n+\t * the option, but this scan doesn't see it, so unsafe features\n+\t * from the stream are still refused. This errs on the safe side.\n \t */\n-\tfor (int i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\t\tif (*arg != '-' || !strcmp(arg, \"--\"))\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--allow-unsafe-features\"))\n-\t\t\tstate.allow_unsafe_features = 1;\n-\t}\n+\tearly = early_scan_options_from_options(fast_import_options,\n+\t\t\t\t\t\tearly_wanted);\n+\tearly_scan_options(argc - 1, argv + 1, early,\n+\t\t\t EARLY_SCAN_STOP_AT_DASHDASH |\n+\t\t\t EARLY_SCAN_STOP_AT_NON_OPTION,\n+\t\t\t option_parse_early_allow_unsafe, &state);\n+\tfree(early);\n \n \trc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));\n \tfor (unsigned int i = 0; i < (cmd_save - 1); i++)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex d9de2ef0d8..1a37f2b8e6 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2344,6 +2344,20 @@ test_expect_success 'R: export-marks options can be overridden by commandline op\n \ttest_path_is_missing feature-sub\n '\n \n+test_expect_success 'R: --allow-unsafe-features found after a value' '\n+\techo \"feature import-marks-if-exists=nonexistent.marks\" >input &&\n+\tgit fast-import --allow-unsafe-features input &&\n+\ttest_must_fail git fast-import --allow-unsafe err &&\n+\ttest_grep \"forbidden in input without --allow-unsafe-features\" err\n+'\n+\n test_expect_success 'R: catch typo in marks file name' '\n \ttest_must_fail git fast-import --import-marks=nonexistent.marks writes:\n\n> A number of commands perform an early scan of their arguments to look\n> for specific flags or structural separators (like `--`).\n>\n> These hand-rolled early scans are often fragile. They especially fail\n> to account for options that take their value as a separate\n> argument. This leads to disagreements between the early scan and the\n> actual parse_options() pass. For example, the early scanner might miss\n> a special option entirely, or mistakenly treat an option's value as\n> the `--` path separator.\n>\n> To allow these commands to safely skip option values during their\n> early scans, this series introduces a new \"early-scan\" sub-API into\n> the existing \"parse-options\" API.\n\nYay.\n\n> This is deliberately implemented as a new simple and fast scan, which\n> has some limitations, instead of a full refactor and reuse of the\n> parse_options() code,\n\nSigh. In other words, we hate these ad-hoc prescan that are buggy\nbadly enough to replace them all with yet another ad-hoc prescan\nthat is know to behave differently from the real thing?\n\n> - `git bisect start --term-good -- ` mistook the term name\n> `--` for the revision/path separator, so was rejected\n> as an invalid revision instead of being treated as a path.\n\nSorry, I fail to see much practical value in this.\n\n> - `git rev-parse --default -- ` did the same, reporting\n> \"bad revision \" while any other default value gives the\n> usual more helpful \"ambiguous argument\" error.\n\nNeither in this one.\n\n> - `git fast-import --depth 5 --allow-unsafe-features` silently\n> ignored `--allow-unsafe-features`, refusing unsafe features from\n> the stream.\n\nOn the other hand, this may be a very good thing.\n\nIs the reason why the ad-hoc pre-scan failed to see it was because\nit did not realize 5 is a value to the --depth option?\n\n> All of these commands call parse_options(), but for `git bisect` and\n> `git rev-parse`, the specific functions doing the early scan\n> (bisect_start() and cmd_rev_parse()'s main loop) parse their own\n> options by hand after the early scan and have no `struct option` array\n> for those options.\n>\n> If bisect_start() and cmd_rev_parse() were converted to use\n> `struct option`, they could use early_scan_options_from_options() and\n> would not be affected by limitations 1), 2) and 3) above, as both use\n> the early scan only to locate `--`.\n\nI imagine that in the long term we would rather see a properly\nrefactored parse-options machinery perform the prescan (perhaps with\nsome kind of \"dry-run\" option given to the machinery) than yet\nanother ad-hoc parser like this topic introduces. It would be very\ngood if this interim solution at least took the same 'options[]'\narray so that when we have the real thing in the future we do not\nhave to redo the conversion effort.\n\nBy the way, how does this interact with your other topic that has\nbeen stalled for quite some time? Would moving this one forward\nhelp the other, or do they not have much relevance to each other? I\nwould rather not see two topics of non-trivial size stalled on a\nsingle author at the same time, so ...\n\nThanks.\n"},{"id":"551811","messageId":"xmqqy0djfgmt.fsf@gitster.g","threadId":"66257","inReplyTo":"20260902161047.476753-2-christian.couder@gmail.com","subject":"Re: [PATCH 1/6] parse-options: add early_scan_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-02T22:11:22Z","receivedAt":"2026-09-02T22:11:27Z","isPatch":true,"body":"Christian Couder writes:\n\n> So users must spell these specific options in full. This restriction\n> could be lifted in the future though, once the scanner is adapted to\n> accept a command's full option array, as this would give it the\n> complete context needed for safe abbreviation matching.\n\nIt is unfortunate that end-users cannot tell if they are dealing\nwith a system before of after \"once the scanner is adapted\"\nhappened, so they must be trained to always spell the options in\nfull to make use of the commands that use this feature. It at least\ndoes not regress relative to the ad-hoc early scanners these selected\ncommands have that do not even understand what they are parsing, so\nit may not be too bad.\n\nStepping back a bit, the burden on programmers to use this would be\nto write in a separate notation what options there are in addition\nto what they feed the real parse_options(), which cuts both ways in\nthe sense that because this does not take parse_options(), commands\nthat do not use parse_options() can still use it, but those that do\nalready use parse_options() need additional work to use eary_scan.\n\nAnd then once the scanner is adapted to accept the full option array,\nthe programmers only need to discard the struct early_scan_option[]\nthey wrote and replace it with the struct option[] they already have?\nOr would the calling convention to the scanner also change when it\nhappens (oother than replacing the pointer to struct early_scan_option[]\nwith another pointer to struct option[])?\n\n> +static const struct early_scan_option *\n> +find_early_scan_option(const char *arg,\n> +\t\t const struct early_scan_option *options,\n> +\t\t const char **value)\n\nBecause you return one single element from the incoming array of\noptions, it is mildly misleading to call the variable/parameter\n\"options\" here and everywhere else. Let's stick to \"arrays are \nnamed singular, so that option[4] names 4th option\" convention.\n\n> +{\n> +\tif (!skip_prefix(arg, \"--\", &arg))\n> +\t\treturn NULL;\n> +\n> +\tfor (; options->name; options++) {\n> +\t\tconst char *rest;\n> +\n> +\t\tif (!skip_prefix(arg, options->name, &rest))\n> +\t\t\tcontinue;\n\n\"--option\" on the command line, after getting stripped the leading\n\"--\", may begin with \"option\", and that name may be in the option[]\ntable, in which case ...\n\n> +\t\tif (!*rest) {\n> +\t\t\t*value = NULL;\n> +\t\t\treturn options;\n> +\t\t}\n\n... we found a hit. But shouldn't option->takes_value be consulted\nbefore we return to signal the caller that the next arg is an option\nvalue before we return from here? It looks a bit uneven as we do\nthat for stuck form \"--option=value\" here.\n\n> +\t\t/* Only an option taking a value can be stuck to one. */\n> +\t\tif (*rest == '=' && options->takes_value) {\n> +\t\t\t*value = rest + 1;\n> +\t\t\treturn options;\n> +\t\t}\n\nAnd if the option[] table had \"opt\", then \"--option\" on the command\nline may begin with \"--opt\" but \"ion\" is an excess that is not a\nstuck value, so we do not consider it as a match. OK.\n\n> +\t}\n> +\treturn NULL;\n> +}\n\nIf we are to write a separate function anyway, I wonder how much\nmore work to write a early_scan_option() parser that does take a\nreal \"struct option[]\" array. Its elements already know if they\ntake a value or not. For expediency, it may be OK to start by\nsimplified parser that does not handle unique prefix and other\ncomplexities like callback functions of the real parser, but at\nleast it would reduce the burden on the programmers quite a bit if\nwe used the real struct option[] array, I suspect.\n\n\n"},{"id":"551813","messageId":"xmqqse3rffr4.fsf@gitster.g","threadId":"66257","inReplyTo":"20260902161047.476753-3-christian.couder@gmail.com","subject":"Re: [PATCH 2/6] bisect: fix \"--\" detection when a term name is \"--\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-02T22:30:23Z","receivedAt":"2026-09-02T22:30:26Z","isPatch":true,"body":"Christian Couder writes:\n\n> `bisect_start()` walks its arguments twice. The second loop actually\n> parses the options, and it knows that `--term-good`, `--term-old`,\n> `--term-bad` and `--term-new` take their value as a separate argument,\n> so it skips that value.\n>\n> The first loop, which only looks for the \"--\" separating revisions from\n> paths, doesn't know about these options. So when such an option is given\n> \"--\" as its value, that \"--\" is mistaken for the separator and\n> `has_double_dash` is wrongly set.\n\nIt may be theoretically true, but I wonder how much practical value\nit has to correctly parse \"--term-good --\" as \"Ah, the user wants to\nmark good revisions as '--' instead of 'good' or 'old'\"? Even\nthough \"refs/bisect/--\" is *not* forbidden, how likely is it for\nusers to do that?\n\nThis is not like \"git grep -e --\" which does have much more pracical\nvalue.\n\n\n\n> builtin/bisect.c | 27 +++++++++++++++++++++------\n> t/t6030-bisect-porcelain.sh | 8 ++++++++\n> 2 files changed, 29 insertions(+), 6 deletions(-)\n>\n> diff --git a/builtin/bisect.c b/builtin/bisect.c\n> index 1cfb8a794b..ad089b289f 100644\n> --- a/builtin/bisect.c\n> +++ b/builtin/bisect.c\n> @@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms,\n> \treturn bisect_next(terms, prefix);\n> }\n> \n> +/*\n> + * The options \"git bisect start\" accepts. Only the ones taking their\n> + * value as a separate argument matter to the scan looking for \"--\" below,\n> + * as their value has to be skipped along with them.\n> + */\n> +static const struct early_scan_option bisect_start_early_options[] = {\n> +\tEARLY_SCAN_SKIP_VALUE(\"term-good\"),\n> +\tEARLY_SCAN_SKIP_VALUE(\"term-old\"),\n> +\tEARLY_SCAN_SKIP_VALUE(\"term-bad\"),\n> +\tEARLY_SCAN_SKIP_VALUE(\"term-new\"),\n> +\tEARLY_SCAN_END()\n> +};\n> +\n> static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n> \t\t\t\t const char **argv)\n> {\n> @@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,\n> \n> \t/*\n> \t * Check for one bad and then some good revisions\n> +\t *\n> +\t * The scan below has to know about the options taking their value\n> +\t * as a separate argument, or such a value that happens to be \"--\"\n> +\t * would be mistaken for the \"--\" separating revisions from paths.\n> \t */\n> -\tfor (i = 0; i < argc; i++) {\n> -\t\tif (!strcmp(argv[i], \"--\")) {\n> -\t\t\thas_double_dash = 1;\n> -\t\t\tbreak;\n> -\t\t}\n> -\t}\n> +\ti = early_scan_options(argc, argv, bisect_start_early_options,\n> +\t\t\t EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);\n> +\tif (i < argc)\n> +\t\thas_double_dash = 1;\n> \n> \tfor (i = 0; i < argc; i++) {\n> \t\tconst char *arg = argv[i];\n> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh\n> index a7588222a8..464ca53b42 100755\n> --- a/t/t6030-bisect-porcelain.sh\n> +++ b/t/t6030-bisect-porcelain.sh\n> @@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'bisect start with \"--\" as a term name' '\n> +\tgit bisect reset &&\n> +\tgit bisect start --term-good -- hello &&\n> +\tgit bisect terms --term-good >actual &&\n> +\techo -- >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> # Bisect is started with --term-new and --term-old arguments,\n> # then skip. The HEAD should be changed.\n> test_expect_success 'bisect skip works with --term*' '\n"},{"id":"551922","messageId":"xmqqwlt18z3q.fsf@gitster.g","threadId":"66257","inReplyTo":"20260902161047.476753-7-christian.couder@gmail.com","subject":"Re: [PATCH 6/6] fast-import: use early_scan_options() for --allow-unsafe-features","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-04T03:38:49Z","receivedAt":"2026-09-04T03:38:52Z","isPatch":true,"body":"Christian Couder writes:\n\n> The \"feature\" lines at the start of the stream are processed before the\n> command line options are parsed, so cmd_fast_import() scans its\n> arguments early to find out if `--allow-unsafe-features` was given.\n>\n> That scan doesn't know which options take their value as a separate\n> argument, and it stops at the first argument that doesn't start with a\n> dash. So it disagrees with parse_options(), which accepts values\n> separated from their option by a space, for a command line like\n> \"--depth 5 --allow-unsafe-features\": the scan stops at \"5\" and never\n> sees the option, so unsafe \"feature\" commands from the stream are\n> refused even though the option was given.\n\nWell explained.\n\n> @@ -4218,23 +4235,20 @@ int cmd_fast_import(int argc,\n> \t * line to override stream data). But we must do an early parse of any\n> \t * command-line options that impact how we interpret the feature lines.\n> \t *\n> +\t * NEEDSWORK: This scan only matches the exact\n> +\t * \"--allow-unsafe-features\" spelling, while parse_options() below\n> +\t * also accepts unambiguous abbreviations, so the two disagree for\n> +\t * a command line like \"--allow-unsafe\": parse_options() accepts\n> +\t * the option, but this scan doesn't see it, so unsafe features\n> +\t * from the stream are still refused. This errs on the safe side.\n> \t */\n> -\tfor (int i = 1; i < argc; i++) {\n> -\t\tconst char *arg = argv[i];\n> -\t\tif (*arg != '-' || !strcmp(arg, \"--\"))\n> -\t\t\tbreak;\n> -\t\tif (!strcmp(arg, \"--allow-unsafe-features\"))\n> -\t\t\tstate.allow_unsafe_features = 1;\n> -\t}\n\nThis is the ad-hoc one that does not know --depth takes a value\nafter it.\n\n> +\tearly = early_scan_options_from_options(fast_import_options,\n> +\t\t\t\t\t\tearly_wanted);\n> +\tearly_scan_options(argc - 1, argv + 1, early,\n> +\t\t\t EARLY_SCAN_STOP_AT_DASHDASH |\n> +\t\t\t EARLY_SCAN_STOP_AT_NON_OPTION,\n> +\t\t\t option_parse_early_allow_unsafe, &state);\n> +\tfree(early);\n\nInteresting. This one now \"knows\" enough to skip what comes after\n\"--depth\" that takes an option ;-) And it is perfectly fine if we\nskip over \"--depth hello\" to find \"--allow-unsafe\", as such a \"oops\nwe require number but hello is not a number\" will be caught by the\nreal parser anyway.\n\nNicely done.\n"},{"id":"553034","messageId":"20260923080928.1534413-1-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260902161047.476753-1-christian.couder@gmail.com","subject":"[PATCH v2 0/3] Standardize early option scanning","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:09:25Z","receivedAt":"2026-09-23T08:09:47Z","isPatch":true,"body":"A number of commands perform an early scan of their arguments to look\nfor specific flags or structural separators (like `--`).\n\nThese hand-rolled early scans are often fragile. They especially fail\nto account for options that take their value as a separate\nargument. This leads to disagreements between the early scan and the\nactual parse_options() pass. For example, the early scanner might miss\na special option entirely, or mistakenly treat an option's value as\nthe `--` path separator.\n\nTo allow these commands to safely skip option values during their\nearly scans, this series introduces a new \"early-scan\" sub-API into\nthe existing \"parse-options\" API.\n\nThis is deliberately implemented as a new simple and fast scan, which\nhas some limitations, instead of a full refactor and reuse of the\nparse_options() code, because the limitations are not very significant\nin practice, while a full refactor and reuse of the parse_options()\ncode would be much more complex.\n\nThe current limitations of the new early scan code are:\n\n 1. short options are ignored,\n\n 2. an option with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT never\n has the following argument skipped as its value. This matches\n parse_options() for PARSE_OPT_OPTARG, but not for\n PARSE_OPT_LASTARG_DEFAULT, which consumes that argument when the\n option isn't the last one,\n\n 3. negated options (\"--no-...\") are not matched,\n\n 4. abbreviated options will not be matched,\n\n 5. subcommands (OPTION_SUBCOMMAND) are never matched, so an option\n marked with PARSE_OPT_EARLY cannot be a subcommand,\n\n 6. aliases (OPTION_ALIAS) are not resolved to the option they stand\n for, so the separate value of an alias of an option taking a value\n is not skipped.\n\nNote that while the others could be real issues for some commands,\n\"3. negated options\" and \"5. subcommands\" are not practical issues\nbecause negated options never consume a separate argument, and\nparse_options() doesn't match subcommands as \"--\" either.\n\nThe early scan is performed by a new early_scan_options() function\nwhich takes a regular `const struct option *options` array as\nargument.\n\nThis requires that the command already uses `struct option` and the\nparse-options API to parse its arguments. As the majority of commands\nperforming an early scan don't use the parse-options API yet, they\nwill have to be converted to use it before they can use\nearly_scan_options().\n\nIn this series, only `git fast-import` is converted to the early-scan\nAPI, which fixes a bug as:\n\n `git fast-import --depth 5 --allow-unsafe-features`\n\nsilently ignored `--allow-unsafe-features`, refusing unsafe features\nfrom the stream.\n\nOverview of the patches\n=======================\n\n - Patch 1/3 refactors some existing code into a new\n parse_options_takes_argument() helper that will be used in the next\n patch.\n\n - Patch 2/3 introduces early_scan_options(), the early scanner that\n will be used instead of hand-rolled ones, along with its\n infrastructure.\n\n - Patch 3/3 uses early_scan_options() to fix the early scan for\n `--allow-unsafe-features` in `git fast-import`.\n\nChanges since v1\n================\n\nThanks to Junio who reviewed v1.\n\nThere are a lot of important changes since v1:\n\n - Now the early-scan API requires the parse-options API to be already\n used, and early_scan_options() accepts a regular `struct option *`\n instead of a dedicated `struct early_scan_option *`.\n\n (In v1, early_scan_options() took a dedicated\n `struct early_scan_option *` array, which the caller either wrote by\n hand, when it didn't use the parse-options API, or derived from its\n `struct option *` array with early_scan_options_from_options().)\n\n This simplifies things significantly, but requires that code doing\n an early scan be ported to the parse-options API if it doesn't use\n it yet.\n\n - bisect_start() and cmd_rev_parse() are not converted anymore to the\n new early-scan API. Converting them didn't bring much value, and\n they can still be converted in the future after they are converted\n to the parse-options API.\n\n - Options that should be looked up during the early scan are now\n marked with a new PARSE_OPT_EARLY flag (which is documented with\n the other per-option flags in\n \"Documentation/technical/api-parse-options.adoc\") in\n `struct option`.\n\n (In v1, a `wanted` flag in `struct early_scan_option` was used for\n this, and this flag could be set by passing a `const char **wanted`\n to early_scan_options_from_options().)\n\n - parse_options_check() now rejects PARSE_OPT_EARLY on an option\n without a long name, and on a subcommand, as the scan can never\n match either of them.\n\n (In v1, early_scan_options_from_options() raised a BUG() when a\n name in `wanted` was not in the option array.)\n\n - The EARLY_SCAN_STOP_AT_DASHDASH flag has been removed, and the scan\n now always stops at both `--` and `--end-of-options`, as\n parse_options() always stops parsing options at them whatever its\n flags. (PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only\n decide if the terminator is left in argv, not if it terminates.)\n\n (In v1, the scan walked past `--end-of-options`, so\n `git fast-import --end-of-options --allow-unsafe-features` allowed\n unsafe stream features before parse_options() rejected the command\n line.)\n\n - \"--=\" is now matched for any option that is not\n PARSE_OPT_NOARG, instead of only for options taking a separate\n value. Whether the next argument is consumed still depends on\n parse_options_takes_argument().\n\n (In v1, an option with PARSE_OPT_OPTARG or\n PARSE_OPT_LASTARG_DEFAULT was missed entirely in that form, while\n parse_options() accepts it.)\n\n - The different patches changed in the following way:\n\n - Patch 4/6 is now patch 1/3.\n\n - Patches 1/6 and 5/6 have been squashed and heavily modified to\n create patch 2/3.\n\n - Patches 2/6 and 3/6 have been removed as bisect_start() and\n cmd_rev_parse() are not converted anymore to the new early-scan\n API.\n\n - Patch 6/6 is now patch 3/3.\n\nCI tests:\n=========\n\nThey all pass, see:\n\nhttps://github.com/chriscool/git/actions/runs/35749173266\n\nRange-diff since v1\n===================\n\n4: 1ea80545c4 = 1: 32adff46ce parse-options: add parse_options_takes_argument()\n1: acb475f98d ! 2: 82569fa602 parse-options: add early_scan_options()\n @@ Commit message\n commands get this wrong, as they just walk their arguments comparing\n them to the few option names they care about.\n \n - Let's add early_scan_options() to help with this. Its callers describe\n - the options to look for, but also the ones that merely have to be\n - skipped along with their value, so that the scan can walk the arguments\n - without being fooled by option values.\n + Let's add early_scan_options() to help with this. It walks the\n + arguments using the very same `struct option` array that the command\n + already passes to parse_options(), and uses the\n + parse_options_takes_argument() helper added in a previous commit, so\n + that the scan and the actual parsing agree on which options take a\n + value.\n \n - Note that abbreviated options are deliberately not recognized, as a\n - scan cannot know about the options it hasn't been told about, and would\n - then resolve abbreviations differently from the actual option parsing.\n + The options the caller wants to be told about are marked with a new\n + PARSE_OPT_EARLY flag, so that nothing has to be spelled out a second\n + time, and so that the mark cannot drift away from the option it refers\n + to.\n \n - So users must spell these specific options in full. This restriction\n - could be lifted in the future though, once the scanner is adapted to\n - accept a command's full option array, as this would give it the\n - complete context needed for safe abbreviation matching.\n + Using a per-option flag for this is not new as that flag space already\n + holds flags that the parsing loop itself ignores, like\n + PARSE_OPT_NOCOMPLETE and PARSE_OPT_COMP_ARG, which only the completion\n + helper looks at, or PARSE_OPT_HIDDEN and PARSE_OPT_LITERAL_ARGHELP,\n + which only the usage output looks at.\n +\n + The scan is deliberately kept much simpler than parse_options(),\n + instead of teaching the latter to perform a side effect free \"dry\n + run\". Such a dry run would have to avoid writing through `opt->value`,\n + calling option callbacks, dying on an invalid value, handling `--help`\n + and tracking command mode conflicts, so it would be a much larger\n + refactoring. If parse_options() learns to do it in the future though,\n + the commands converted now would keep both their option array and their\n + PARSE_OPT_EARLY marks, so their conversion would not have to be redone.\n +\n + One consequence of staying simple is that abbreviated options are\n + still not matched, even though the scan is now given the command's\n + full option array. Resolving them the way parse_options() does would\n + mean duplicating the ambiguity detection that parse_long_opt()\n + performs. So the scan can fail to see an option that parse_options()\n + would accept, and its callers have to cope with that, typically by\n + erring on the safe side. This and the other differences with\n + parse_options() are documented in \"parse-options.h\".\n +\n + In practice, despite these limitations, early scans using\n + early_scan_options() should still be safer and cleaner than the\n + ad-hoc hand-rolled scans they are meant to replace, which don't know\n + about option values at all and therefore disagree with\n + parse_options() in ways that create plain bugs.\n \n Signed-off-by: Christian Couder \n \n + ## Documentation/technical/api-parse-options.adoc ##\n +@@ Documentation/technical/api-parse-options.adoc: are the bitwise-or of:\n + \tInternal flag, set on options that were expanded from a\n + \tconfigured alias. It should not be set by callers.\n + \n ++`PARSE_OPT_EARLY`::\n ++\tReport this option to `early_scan_options()`, which looks at a\n ++\tfew options before parsing the command line for real. Ignored\n ++\tby `parse_options()` itself.\n ++\n + `PARSE_OPT_NOCOMPLETE`::\n + \tDo not offer this option for completion.\n + \n +\n ## parse-options.c ##\n +@@ parse-options.c: static void parse_options_check(const struct option *opts)\n + \t\t opts->long_name))\n + \t\t\toptbug(opts, \"uses feature \"\n + \t\t\t \"not supported for dashless options\");\n ++\t\tif ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name)\n ++\t\t\toptbug(opts, \"uses PARSE_OPT_EARLY, which needs a long name\");\n + \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n + \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n + \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n +@@ parse-options.c: static void parse_options_check(const struct option *opts)\n + \t\tcase OPTION_SUBCOMMAND:\n + \t\t\tif (!opts->value || !opts->subcommand_fn)\n + \t\t\t\toptbug(opts, \"OPTION_SUBCOMMAND needs a value and a subcommand function\");\n ++\t\t\tif (opts->flags & PARSE_OPT_EARLY)\n ++\t\t\t\toptbug(opts, \"OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY\");\n + \t\t\tif (!subcommand_value)\n + \t\t\t\tsubcommand_value = opts->value;\n + \t\t\telse if (subcommand_value != opts->value)\n @@ parse-options.c: int parse_options(int argc, const char **argv,\n \treturn parse_options_end(&ctx);\n }\n \n +/*\n -+ * Look for `arg` among `options`. On success, return the matching option\n ++ * Look for `arg` among `option`. On success, return the matching option\n + * and set `value` to the value stuck to it, if any, or to NULL.\n + */\n -+static const struct early_scan_option *\n -+find_early_scan_option(const char *arg,\n -+\t\t const struct early_scan_option *options,\n -+\t\t const char **value)\n ++static const struct option *find_early_scan_option(const char *arg,\n ++\t\t\t\t\t\t const struct option *option,\n ++\t\t\t\t\t\t const char **value)\n +{\n +\tif (!skip_prefix(arg, \"--\", &arg))\n +\t\treturn NULL;\n +\n -+\tfor (; options->name; options++) {\n ++\tfor (const struct option *opt = option; opt->type != OPTION_END; opt++) {\n +\t\tconst char *rest;\n +\n -+\t\tif (!skip_prefix(arg, options->name, &rest))\n ++\t\tif (opt->type == OPTION_SUBCOMMAND)\n ++\t\t\tcontinue;\n ++\t\tif (!opt->long_name)\n ++\t\t\tcontinue;\n ++\t\tif (!skip_prefix(arg, opt->long_name, &rest))\n +\t\t\tcontinue;\n ++\n +\t\tif (!*rest) {\n +\t\t\t*value = NULL;\n -+\t\t\treturn options;\n ++\t\t\treturn opt;\n +\t\t}\n -+\t\t/* Only an option taking a value can be stuck to one. */\n -+\t\tif (*rest == '=' && options->takes_value) {\n ++\t\t/* Only an option that can take a value may have one stuck to it. */\n ++\t\tif (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {\n +\t\t\t*value = rest + 1;\n -+\t\t\treturn options;\n ++\t\t\treturn opt;\n +\t\t}\n +\t}\n +\n @@ parse-options.c: int parse_options(int argc, const char **argv,\n +}\n +\n +int early_scan_options(int argc, const char **argv,\n -+\t\t const struct early_scan_option *options,\n ++\t\t const struct option *option,\n +\t\t enum early_scan_flags flags,\n +\t\t early_scan_fn *fn, void *data)\n +{\n +\tfor (int i = 0; i < argc; i++) {\n +\t\tconst char *arg = argv[i];\n +\t\tconst char *value;\n -+\t\tconst struct early_scan_option *opt;\n ++\t\tconst struct option *opt;\n +\t\tint pos = i;\n +\n -+\t\tif ((flags & EARLY_SCAN_STOP_AT_DASHDASH) &&\n -+\t\t !strcmp(arg, \"--\"))\n ++\t\t/*\n ++\t\t * parse_options() always stops parsing options at these,\n ++\t\t * whatever its flags, so nothing after them is an option.\n ++\t\t */\n ++\t\tif (!strcmp(arg, \"--\") || !strcmp(arg, \"--end-of-options\"))\n +\t\t\treturn i;\n +\n -+\t\topt = find_early_scan_option(arg, options, &value);\n ++\t\topt = find_early_scan_option(arg, option, &value);\n +\t\tif (!opt) {\n +\t\t\tif ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&\n +\t\t\t (*arg != '-' || !arg[1]))\n @@ parse-options.c: int parse_options(int argc, const char **argv,\n +\t\t * value and it has to be skipped so that it isn't\n +\t\t * taken for an option itself.\n +\t\t */\n -+\t\tif (opt->takes_value && !value && i + 1 < argc)\n ++\t\tif (parse_options_takes_argument(opt) && !value && i + 1 < argc)\n +\t\t\tvalue = argv[++i];\n +\n -+\t\tif (opt->wanted && fn(opt, value, pos, data))\n ++\t\tif (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))\n +\t\t\treturn i;\n +\t}\n +\n @@ parse-options.c: int parse_options(int argc, const char **argv,\n \tconst char *s;\n \n ## parse-options.h ##\n +@@ parse-options.h: enum parse_opt_option_flags {\n + \tPARSE_OPT_NODASH = 1 << 5,\n + \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n + \tPARSE_OPT_FROM_ALIAS = 1 << 7,\n ++\tPARSE_OPT_EARLY = 1 << 8,\t/* only for early_scan_options() */\n + \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n + \tPARSE_OPT_COMP_ARG = 1 << 10,\n + \tPARSE_OPT_CMDMODE = 1 << 11,\n @@ parse-options.h: static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\tBUG(\"option callback expects an argument\"); \\\n } while(0)\n @@ parse-options.h: static inline void die_for_incompatible_opt2(int opt1, const ch\n + * whether a repository is needed at all.\n + *\n + * Such an early scan has to know which options take their value as a\n -+ * separate argument, or it could mistake such a value for an option. The\n -+ * `struct early_scan_option` array passed to early_scan_options() below\n -+ * describes the options to look for, as well as the ones that only need\n -+ * to be skipped along with their value.\n ++ * separate argument, or it could mistake such a value for an\n ++ * option. The functions below allow performing such early scans\n ++ * without being fooled by option values.\n + */\n -+struct early_scan_option {\n -+\tconst char *name; \t/* Option name, without the leading dashes */\n -+\tunsigned takes_value:1; /* \"--option=value\" or \"--option value\" expected? */\n -+\tunsigned wanted:1; /* Report option to callback? */\n -+};\n -+\n -+#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 }\n -+#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 }\n -+#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 }\n -+#define EARLY_SCAN_END() { NULL }\n +\n +/*\n + * Called by early_scan_options() for each argument matching a\n -+ * `struct early_scan_option` that has its `wanted` bit set.\n ++ * `struct option` with PARSE_OPT_EARLY set.\n + *\n + * `option` is the matching option, `value` its value or NULL if it\n + * doesn't take one, and `pos` the index of the option in argv.\n + *\n + * Returning a non-zero value stops the scan.\n + */\n -+typedef int early_scan_fn(const struct early_scan_option *option,\n -+\t\t\t const char *value, int pos, void *data);\n ++typedef int early_scan_fn(const struct option *option, const char *value,\n ++\t\t\t int pos, void *data);\n +\n +enum early_scan_flags {\n -+\tEARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at \"--\" */\n -+\tEARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1,\n ++\tEARLY_SCAN_STOP_AT_NON_OPTION = 1 << 0, /* Stop at any non option */\n +};\n +\n +/*\n -+ * Scan `argv` for the options described by `options`, calling `fn`\n -+ * for each of those that are `wanted`. `argv` is not modified.\n ++ * Scan `argv` for the options described by `option`, calling `fn` for\n ++ * each of those that have PARSE_OPT_EARLY set. `argv` is not\n ++ * modified.\n + *\n -+ * `fn` may be NULL when no option is `wanted`, which is useful to only\n -+ * find out where the scan stops.\n ++ * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is\n ++ * useful to only find out where the scan stops.\n + *\n -+ * Note that abbreviated options are not recognized, as a scan cannot\n -+ * know about the options it hasn't been told about, and would then\n -+ * resolve abbreviations differently from the actual option parsing.\n ++ * The scan always stops at \"--\" and at \"--end-of-options\", as\n ++ * parse_options() always stops parsing options there too, whatever its\n ++ * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only\n ++ * decide if the terminator is left in argv, not if it terminates.\n + *\n + * Returns the index at which the scan stopped, which is `argc` when the\n + * whole array was scanned.\n ++ *\n ++ * This scan is for now deliberately much simpler than\n ++ * parse_options(), so it differs from it in the following ways:\n ++ *\n ++ * - Only the long form of an option is matched, and it has to be\n ++ * spelled in full: short options and abbreviations are ignored.\n ++ *\n ++ * - Negated forms (\"--no-\") are not matched. This is harmless,\n ++ * as they never take a value to skip.\n ++ *\n ++ * - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are\n ++ * treated as not taking a separate value.\n ++ *\n ++ * - OPTION_SUBCOMMAND entries are skipped.\n ++ *\n ++ * - OPTION_ALIAS entries are not resolved to the option they stand\n ++ * for.\n ++ *\n ++ * So the scan can fail to see an option that parse_options() would\n ++ * accept, and callers have to cope with that, typically by erring on\n ++ * the safe side.\n + */\n +int early_scan_options(int argc, const char **argv,\n -+\t\t const struct early_scan_option *options,\n ++\t\t const struct option *option,\n +\t\t enum early_scan_flags flags,\n +\t\t early_scan_fn *fn, void *data);\n +\n @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **\n \treturn parse_subcommand__cmd(argc, argv, test_flags);\n }\n +\n -+static int show_early_option(const struct early_scan_option *opt,\n -+\t\t\t const char *value, int pos, void *data UNUSED)\n ++static int show_early_option(const struct option *opt, const char *value,\n ++\t\t\t int pos, void *data UNUSED)\n +{\n -+\tprintf(\"found: %s at %d\", opt->name, pos);\n ++\tprintf(\"found: %s at %d\", opt->long_name, pos);\n +\tif (value)\n +\t\tprintf(\" value: %s\", value);\n +\tputchar('\\n');\n @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **\n +\n +int cmd__early_scan_options(int argc, const char **argv)\n +{\n -+\tstatic const struct early_scan_option options[] = {\n -+\t\tEARLY_SCAN_WANT(\"wanted\"),\n -+\t\tEARLY_SCAN_WANT_VALUE(\"wanted-value\"),\n -+\t\tEARLY_SCAN_SKIP_VALUE(\"skipped-value\"),\n -+\t\tEARLY_SCAN_END()\n ++\tchar *a_string = NULL;\n ++\tint an_int = 0, a_bool = 0, a_short = 0;\n ++\n ++\tconst struct option option[] = {\n ++\t\tOPT_GROUP(\"early scan test options\"),\n ++\t\tOPT_BOOL_F(0, \"wanted\", &a_bool,\n ++\t\t\t \"wanted option taking no value\",\n ++\t\t\t PARSE_OPT_EARLY),\n ++\t\tOPT_STRING_F(0, \"wanted-value\", &a_string, \"str\",\n ++\t\t\t \"wanted option taking a value\",\n ++\t\t\t PARSE_OPT_EARLY),\n ++\t\tOPT_STRING(0, \"skipped-value\", &a_string, \"str\",\n ++\t\t\t \"option whose value has to be skipped\"),\n ++\t\tOPT_INTEGER(0, \"number\", &an_int,\n ++\t\t\t \"option taking an integer value\"),\n ++\t\tOPT_STRING_F(0, \"optarg\", &a_string, \"str\",\n ++\t\t\t \"option with an optional value\",\n ++\t\t\t PARSE_OPT_OPTARG),\n ++\t\tOPT_STRING_F(0, \"lastarg\", &a_string, \"str\",\n ++\t\t\t \"option with a last argument default\",\n ++\t\t\t PARSE_OPT_LASTARG_DEFAULT),\n ++\t\tOPT_STRING_F(0, \"early-optarg\", &a_string, \"str\",\n ++\t\t\t \"early option with an optional value\",\n ++\t\t\t PARSE_OPT_EARLY | PARSE_OPT_OPTARG),\n ++\t\tOPT_STRING_F(0, \"early-lastarg\", &a_string, \"str\",\n ++\t\t\t \"early option with a last argument default\",\n ++\t\t\t PARSE_OPT_EARLY | PARSE_OPT_LASTARG_DEFAULT),\n ++\t\tOPT_BOOL('s', NULL, &a_short, \"short only option\"),\n ++\t\tOPT_END()\n +\t};\n ++\n +\tenum early_scan_flags flags = 0;\n +\tint stopped;\n +\n +\twhile (argc > 1 && *argv[1] == '-') {\n -+\t\tif (!strcmp(argv[1], \"--stop-at-dashdash\"))\n -+\t\t\tflags |= EARLY_SCAN_STOP_AT_DASHDASH;\n -+\t\telse if (!strcmp(argv[1], \"--stop-at-non-option\"))\n ++\t\tif (!strcmp(argv[1], \"--stop-at-non-option\"))\n +\t\t\tflags |= EARLY_SCAN_STOP_AT_NON_OPTION;\n +\t\telse\n +\t\t\tbreak;\n @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **\n +\t\targv++;\n +\t}\n +\n -+\tstopped = early_scan_options(argc - 1, argv + 1, options, flags,\n -+\t\t\t\t show_early_option, NULL);\n ++\tstopped = early_scan_options(argc - 1, argv + 1, option, flags,\n ++\t\t\t\t show_early_option, NULL);\n +\tprintf(\"stopped at: %d of %d\\n\", stopped, argc - 1);\n +\n +\treturn 0;\n @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '\n +\ttest_cmp expect actual\n +'\n +\n -+test_expect_success 'early_scan_options() can stop at \"--\"' '\n -+\ttest-tool early-scan-options --stop-at-dashdash -- --wanted >actual &&\n ++test_expect_success 'early_scan_options() always stops at \"--\"' '\n ++\ttest-tool early-scan-options -- --wanted >actual &&\n +\tcat >expect <<-\\EOF &&\n +\tstopped at: 0 of 2\n +\tEOF\n +\ttest_cmp expect actual &&\n -+\ttest-tool early-scan-options --stop-at-dashdash \\\n -+\t\t--skipped-value -- --wanted >actual &&\n ++\ttest-tool early-scan-options --stop-at-non-option -- --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tstopped at: 0 of 2\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --skipped-value -- --wanted >actual &&\n +\tcat >expect <<-\\EOF &&\n +\tfound: wanted at 2\n +\tstopped at: 3 of 3\n @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '\n +\ttest_cmp expect actual\n +'\n +\n ++test_expect_success 'early_scan_options() always stops at \"--end-of-options\"' '\n ++\ttest-tool early-scan-options --end-of-options --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tstopped at: 0 of 2\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --stop-at-non-option \\\n ++\t\t--end-of-options --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tstopped at: 0 of 2\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n +test_expect_success 'early_scan_options() can stop at a non-option' '\n +\ttest-tool early-scan-options --stop-at-non-option \\\n +\t\targ --wanted >actual &&\n @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '\n +\tEOF\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success 'early_scan_options() takes values from struct option' '\n ++\ttest-tool early-scan-options --number --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --number=5 --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'early_scan_options() does not skip an optional value' '\n ++\ttest-tool early-scan-options --optarg --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --lastarg --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'early_scan_options() matches a stuck optional value' '\n ++\ttest-tool early-scan-options --early-optarg=one >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: early-optarg at 0 value: one\n ++\tstopped at: 1 of 1\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --early-lastarg=two >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: early-lastarg at 0 value: two\n ++\tstopped at: 1 of 1\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'early_scan_options() does not take a separate optional value' '\n ++\ttest-tool early-scan-options --early-optarg --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: early-optarg at 0\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual &&\n ++\ttest-tool early-scan-options --early-lastarg --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: early-lastarg at 0\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'early_scan_options() ignores options without a long name' '\n ++\ttest-tool early-scan-options -s --wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tfound: wanted at 1\n ++\tstopped at: 2 of 2\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'early_scan_options() ignores negated options' '\n ++\ttest-tool early-scan-options --no-wanted >actual &&\n ++\tcat >expect <<-\\EOF &&\n ++\tstopped at: 1 of 1\n ++\tEOF\n ++\ttest_cmp expect actual\n ++'\n +\n test_done\n2: 2adb3b6229 < -: ---------- bisect: fix \"--\" detection when a term name is \"--\"\n3: 8bfbd627a8 < -: ---------- rev-parse: fix \"--\" detection when it is an option value\n5: 69cb1339c6 < -: ---------- parse-options: build early scan options from a struct option array\n6: a55b275327 ! 3: 8e5092d421 fast-import: use early_scan_options() for --allow-unsafe-features\n @@ Commit message\n sees the option, so unsafe \"feature\" commands from the stream are\n refused even though the option was given.\n \n - Let's fix this by building the options for the scan from the same\n - `struct option` array that parse_options() uses, so that both agree on\n - which options take a value.\n + Let's fix this by using early_scan_options(), which scans the very\n + same `struct option` array that parse_options() uses, so that both\n + agree on which options take a value, and by marking\n + `--allow-unsafe-features` with PARSE_OPT_EARLY so that the scan\n + reports it.\n \n Note that the scan still only matches the exact option spelling, while\n parse_options() also accepts unambiguous abbreviations, so the two still\n @@ Documentation/git-fast-import.adoc: fast-import stream! This option is enabled a\n -them, while `--allow-unsafe-features --depth 5` and\n -`--depth=5 --allow-unsafe-features` allow them.\n +Note that this option has to be spelled in full for the unsafe\n -+`feature` commands in the stream to be allowed. So while\n -+`--allow-unsafe` is accepted as an unambiguous abbreviation of this\n -+option, it still refuses them.\n ++`feature` commands in the stream to be allowed. So `--allow-unsafe`\n ++is accepted as an unambiguous abbreviation of this option, but the\n ++unsafe `feature` commands are still refused.\n \n `--signed-tags=`::\n \tSpecify how to handle signed tags. Behaves in the same way as\n @@ builtin/fast-import.c: static int option_parse_quiet(const struct option *opt UN\n \treturn 0;\n }\n \n -+/*\n -+ * The only option the early scan below is interested in, as it decides\n -+ * whether unsafe \"feature\" commands from the stream are allowed.\n -+ */\n -+static const char *early_wanted[] = { \"allow-unsafe-features\", NULL };\n -+\n -+static int option_parse_early_allow_unsafe(\n -+\t\tconst struct early_scan_option *opt UNUSED,\n -+\t\tconst char *value UNUSED, int pos UNUSED, void *data)\n ++static int option_parse_early_allow_unsafe(const struct option *option,\n ++\t\t\t\t\t const char *value UNUSED,\n ++\t\t\t\t\t int pos UNUSED, void *data)\n +{\n +\tstruct fast_import_state *state = data;\n +\n -+\tstate->allow_unsafe_features = 1;\n ++\tif (!strcmp(option->long_name, \"allow-unsafe-features\"))\n ++\t\tstate->allow_unsafe_features = 1;\n +\treturn 0;\n +}\n +\n int cmd_fast_import(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n - \t\t struct repository *repo)\n - {\n - \tstruct fast_import_state state;\n -+\tstruct early_scan_option *early;\n - \n - \tstruct option fast_import_options[] = {\n - \t\tOPT_GROUP(N_(\"Common\")),\n +@@ builtin/fast-import.c: int cmd_fast_import(int argc,\n + \t\tOPT_HIDDEN_GROUP(N_(\"Advanced\")),\n + \t\tOPT_BOOL_F(0, \"allow-unsafe-features\", &state.allow_unsafe_features,\n + \t\t\t N_(\"allow unsafe mark commands from the stream\"),\n +-\t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),\n ++\t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY),\n + \t\tOPT_CALLBACK_F(0, \"export-pack-edges\", &state, N_(\"file\"),\n + \t\t\t N_(\"dump edge commits to \"),\n + \t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG,\n @@ builtin/fast-import.c: int cmd_fast_import(int argc,\n \t * line to override stream data). But we must do an early parse of any\n \t * command-line options that impact how we interpret the feature lines.\n @@ builtin/fast-import.c: int cmd_fast_import(int argc,\n -\t\tif (!strcmp(arg, \"--allow-unsafe-features\"))\n -\t\t\tstate.allow_unsafe_features = 1;\n -\t}\n -+\tearly = early_scan_options_from_options(fast_import_options,\n -+\t\t\t\t\t\tearly_wanted);\n -+\tearly_scan_options(argc - 1, argv + 1, early,\n -+\t\t\t EARLY_SCAN_STOP_AT_DASHDASH |\n ++\tearly_scan_options(argc - 1, argv + 1, fast_import_options,\n +\t\t\t EARLY_SCAN_STOP_AT_NON_OPTION,\n +\t\t\t option_parse_early_allow_unsafe, &state);\n -+\tfree(early);\n \n \trc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));\n \tfor (unsigned int i = 0; i < (cmd_save - 1); i++)\n\nChristian Couder (3):\n parse-options: add parse_options_takes_argument()\n parse-options: add early_scan_options()\n fast-import: use early_scan_options() for --allow-unsafe-features\n\n Documentation/git-fast-import.adoc | 10 +-\n .../technical/api-parse-options.adoc | 5 +\n builtin/fast-import.c | 38 ++--\n parse-options.c | 116 ++++++++++--\n parse-options.h | 82 +++++++++\n t/helper/test-parse-options.c | 62 +++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0040-parse-options.sh | 173 ++++++++++++++++++\n t/t9300-fast-import.sh | 14 ++\n 10 files changed, 466 insertions(+), 36 deletions(-)\n\n\nbase-commit: d38352cd43ab9745686d697872408bc3249a153f\n-- \n2.56.0.rc2\n\n"},{"id":"553033","messageId":"20260923080928.1534413-2-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260923080928.1534413-1-christian.couder@gmail.com","subject":"[PATCH v2 1/3] parse-options: add parse_options_takes_argument()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:09:26Z","receivedAt":"2026-09-23T08:09:48Z","isPatch":true,"body":"Whether an option takes a value, and therefore consumes the next\nargument when that value is not stuck to it with an '=', is decided by\nits type and its flags. That rule is currently open-coded in\nshow_gitcomp(), which needs it to decide if it should append an '=' to\nthe option it completes.\n\nA following commit will need the same rule to find out which options an\nearly scan of the command line has to skip along with their value.\n\nSo let's factor that rule out into a new parse_options_takes_argument()\nfunction, and let's use it in show_gitcomp().\n\nNote that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the\nnext argument when it isn't the last one, so it is not considered as\ntaking a value, which is what show_gitcomp() already did.\n\nSigned-off-by: Christian Couder \n---\n parse-options.c | 35 ++++++++++++++++++++++-------------\n parse-options.h | 10 ++++++++++\n 2 files changed, 32 insertions(+), 13 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4519ead9dc..a132c1ea12 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -841,6 +841,26 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,\n \t}\n }\n \n+int parse_options_takes_argument(const struct option *opt)\n+{\n+\tswitch (opt->type) {\n+\tcase OPTION_STRING:\n+\tcase OPTION_FILENAME:\n+\tcase OPTION_INTEGER:\n+\tcase OPTION_UNSIGNED:\n+\tcase OPTION_CALLBACK:\n+\t\tbreak;\n+\tdefault:\n+\t\treturn 0;\n+\t}\n+\n+\tif (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG |\n+\t\t\t PARSE_OPT_LASTARG_DEFAULT))\n+\t\treturn 0;\n+\n+\treturn 1;\n+}\n+\n static int show_gitcomp(const struct option *opts, int show_all)\n {\n \tconst struct option *original_opts = opts;\n@@ -862,20 +882,9 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\t\tbreak;\n \t\tcase OPTION_GROUP:\n \t\t\tcontinue;\n-\t\tcase OPTION_STRING:\n-\t\tcase OPTION_FILENAME:\n-\t\tcase OPTION_INTEGER:\n-\t\tcase OPTION_UNSIGNED:\n-\t\tcase OPTION_CALLBACK:\n-\t\t\tif (opts->flags & PARSE_OPT_NOARG)\n-\t\t\t\tbreak;\n-\t\t\tif (opts->flags & PARSE_OPT_OPTARG)\n-\t\t\t\tbreak;\n-\t\t\tif (opts->flags & PARSE_OPT_LASTARG_DEFAULT)\n-\t\t\t\tbreak;\n-\t\t\tsuffix = \"=\";\n-\t\t\tbreak;\n \t\tdefault:\n+\t\t\tif (parse_options_takes_argument(opts))\n+\t\t\t\tsuffix = \"=\";\n \t\t\tbreak;\n \t\t}\n \t\tif (opts->flags & PARSE_OPT_COMP_ARG)\ndiff --git a/parse-options.h b/parse-options.h\nindex d7f896a933..f29e73f85c 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -420,6 +420,16 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \t\t const char * const usagestr[],\n \t\t enum parse_opt_flags flags);\n \n+/*\n+ * Return non-zero if `opt` takes a value, which means that it consumes\n+ * the next argument when that value is not stuck to it with an '='.\n+ *\n+ * Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the\n+ * next argument when it isn't the last one, so it is not considered as\n+ * taking a value here.\n+ */\n+int parse_options_takes_argument(const struct option *opt);\n+\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n-- \n2.56.0.rc2\n\n"},{"id":"553035","messageId":"20260923080928.1534413-3-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260923080928.1534413-1-christian.couder@gmail.com","subject":"[PATCH v2 2/3] parse-options: add early_scan_options()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:09:27Z","receivedAt":"2026-09-23T08:09:50Z","isPatch":true,"body":"Some commands need to look at a few of their options before they can\nparse their command line for real, for example because the result\ndecides whether a repository is needed at all, or how the beginning of\ntheir input should be interpreted.\n\nSuch an early scan has to know which options take their value as a\nseparate argument, or it mistakes such a value for an option. Several\ncommands get this wrong, as they just walk their arguments comparing\nthem to the few option names they care about.\n\nLet's add early_scan_options() to help with this. It walks the\narguments using the very same `struct option` array that the command\nalready passes to parse_options(), and uses the\nparse_options_takes_argument() helper added in a previous commit, so\nthat the scan and the actual parsing agree on which options take a\nvalue.\n\nThe options the caller wants to be told about are marked with a new\nPARSE_OPT_EARLY flag, so that nothing has to be spelled out a second\ntime, and so that the mark cannot drift away from the option it refers\nto.\n\nUsing a per-option flag for this is not new as that flag space already\nholds flags that the parsing loop itself ignores, like\nPARSE_OPT_NOCOMPLETE and PARSE_OPT_COMP_ARG, which only the completion\nhelper looks at, or PARSE_OPT_HIDDEN and PARSE_OPT_LITERAL_ARGHELP,\nwhich only the usage output looks at.\n\nThe scan is deliberately kept much simpler than parse_options(),\ninstead of teaching the latter to perform a side effect free \"dry\nrun\". Such a dry run would have to avoid writing through `opt->value`,\ncalling option callbacks, dying on an invalid value, handling `--help`\nand tracking command mode conflicts, so it would be a much larger\nrefactoring. If parse_options() learns to do it in the future though,\nthe commands converted now would keep both their option array and their\nPARSE_OPT_EARLY marks, so their conversion would not have to be redone.\n\nOne consequence of staying simple is that abbreviated options are\nstill not matched, even though the scan is now given the command's\nfull option array. Resolving them the way parse_options() does would\nmean duplicating the ambiguity detection that parse_long_opt()\nperforms. So the scan can fail to see an option that parse_options()\nwould accept, and its callers have to cope with that, typically by\nerring on the safe side. This and the other differences with\nparse_options() are documented in \"parse-options.h\".\n\nIn practice, despite these limitations, early scans using\nearly_scan_options() should still be safer and cleaner than the\nad-hoc hand-rolled scans they are meant to replace, which don't know\nabout option values at all and therefore disagree with\nparse_options() in ways that create plain bugs.\n\nSigned-off-by: Christian Couder \n---\n .../technical/api-parse-options.adoc | 5 +\n parse-options.c | 81 ++++++++\n parse-options.h | 72 ++++++++\n t/helper/test-parse-options.c | 62 +++++++\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n t/t0040-parse-options.sh | 173 ++++++++++++++++++\n 7 files changed, 395 insertions(+)\n\ndiff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc\nindex 95b7924e84..f59d8e90a5 100644\n--- a/Documentation/technical/api-parse-options.adoc\n+++ b/Documentation/technical/api-parse-options.adoc\n@@ -197,6 +197,11 @@ are the bitwise-or of:\n \tInternal flag, set on options that were expanded from a\n \tconfigured alias. It should not be set by callers.\n \n+`PARSE_OPT_EARLY`::\n+\tReport this option to `early_scan_options()`, which looks at a\n+\tfew options before parsing the command line for real. Ignored\n+\tby `parse_options()` itself.\n+\n `PARSE_OPT_NOCOMPLETE`::\n \tDo not offer this option for completion.\n \ndiff --git a/parse-options.c b/parse-options.c\nindex a132c1ea12..559dad9061 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -669,6 +669,8 @@ static void parse_options_check(const struct option *opts)\n \t\t opts->long_name))\n \t\t\toptbug(opts, \"uses feature \"\n \t\t\t \"not supported for dashless options\");\n+\t\tif ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name)\n+\t\t\toptbug(opts, \"uses PARSE_OPT_EARLY, which needs a long name\");\n \t\tif (opts->type == OPTION_SET_INT && !opts->defval &&\n \t\t opts->long_name && !(opts->flags & PARSE_OPT_NONEG))\n \t\t\toptbug(opts, \"OPTION_SET_INT 0 should not be negatable\");\n@@ -706,6 +708,8 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_SUBCOMMAND:\n \t\t\tif (!opts->value || !opts->subcommand_fn)\n \t\t\t\toptbug(opts, \"OPTION_SUBCOMMAND needs a value and a subcommand function\");\n+\t\t\tif (opts->flags & PARSE_OPT_EARLY)\n+\t\t\t\toptbug(opts, \"OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY\");\n \t\t\tif (!subcommand_value)\n \t\t\t\tsubcommand_value = opts->value;\n \t\t\telse if (subcommand_value != opts->value)\n@@ -1253,6 +1257,83 @@ int parse_options(int argc, const char **argv,\n \treturn parse_options_end(&ctx);\n }\n \n+/*\n+ * Look for `arg` among `option`. On success, return the matching option\n+ * and set `value` to the value stuck to it, if any, or to NULL.\n+ */\n+static const struct option *find_early_scan_option(const char *arg,\n+\t\t\t\t\t\t const struct option *option,\n+\t\t\t\t\t\t const char **value)\n+{\n+\tif (!skip_prefix(arg, \"--\", &arg))\n+\t\treturn NULL;\n+\n+\tfor (const struct option *opt = option; opt->type != OPTION_END; opt++) {\n+\t\tconst char *rest;\n+\n+\t\tif (opt->type == OPTION_SUBCOMMAND)\n+\t\t\tcontinue;\n+\t\tif (!opt->long_name)\n+\t\t\tcontinue;\n+\t\tif (!skip_prefix(arg, opt->long_name, &rest))\n+\t\t\tcontinue;\n+\n+\t\tif (!*rest) {\n+\t\t\t*value = NULL;\n+\t\t\treturn opt;\n+\t\t}\n+\t\t/* Only an option that can take a value may have one stuck to it. */\n+\t\tif (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {\n+\t\t\t*value = rest + 1;\n+\t\t\treturn opt;\n+\t\t}\n+\t}\n+\n+\treturn NULL;\n+}\n+\n+int early_scan_options(int argc, const char **argv,\n+\t\t const struct option *option,\n+\t\t enum early_scan_flags flags,\n+\t\t early_scan_fn *fn, void *data)\n+{\n+\tfor (int i = 0; i < argc; i++) {\n+\t\tconst char *arg = argv[i];\n+\t\tconst char *value;\n+\t\tconst struct option *opt;\n+\t\tint pos = i;\n+\n+\t\t/*\n+\t\t * parse_options() always stops parsing options at these,\n+\t\t * whatever its flags, so nothing after them is an option.\n+\t\t */\n+\t\tif (!strcmp(arg, \"--\") || !strcmp(arg, \"--end-of-options\"))\n+\t\t\treturn i;\n+\n+\t\topt = find_early_scan_option(arg, option, &value);\n+\t\tif (!opt) {\n+\t\t\tif ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&\n+\t\t\t (*arg != '-' || !arg[1]))\n+\t\t\t\treturn i;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\t/*\n+\t\t * When an option takes a value, but that value is not\n+\t\t * stuck to it with '=', then the next argument is the\n+\t\t * value and it has to be skipped so that it isn't\n+\t\t * taken for an option itself.\n+\t\t */\n+\t\tif (parse_options_takes_argument(opt) && !value && i + 1 < argc)\n+\t\t\tvalue = argv[++i];\n+\n+\t\tif (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))\n+\t\t\treturn i;\n+\t}\n+\n+\treturn argc;\n+}\n+\n static int usage_argh(const struct option *opts, FILE *outfile)\n {\n \tconst char *s;\ndiff --git a/parse-options.h b/parse-options.h\nindex f29e73f85c..3ef64744a4 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -51,6 +51,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NODASH = 1 << 5,\n \tPARSE_OPT_LITERAL_ARGHELP = 1 << 6,\n \tPARSE_OPT_FROM_ALIAS = 1 << 7,\n+\tPARSE_OPT_EARLY = 1 << 8,\t/* only for early_scan_options() */\n \tPARSE_OPT_NOCOMPLETE = 1 << 9,\n \tPARSE_OPT_COMP_ARG = 1 << 10,\n \tPARSE_OPT_CMDMODE = 1 << 11,\n@@ -501,6 +502,77 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,\n \t\tBUG(\"option callback expects an argument\"); \\\n } while(0)\n \n+/*----- Early scan: scanning argv before the actual option parsing -----*/\n+\n+/*\n+ * Some commands need to look at a few options before they can parse\n+ * their command line for real, for example because the result decides\n+ * whether a repository is needed at all.\n+ *\n+ * Such an early scan has to know which options take their value as a\n+ * separate argument, or it could mistake such a value for an\n+ * option. The functions below allow performing such early scans\n+ * without being fooled by option values.\n+ */\n+\n+/*\n+ * Called by early_scan_options() for each argument matching a\n+ * `struct option` with PARSE_OPT_EARLY set.\n+ *\n+ * `option` is the matching option, `value` its value or NULL if it\n+ * doesn't take one, and `pos` the index of the option in argv.\n+ *\n+ * Returning a non-zero value stops the scan.\n+ */\n+typedef int early_scan_fn(const struct option *option, const char *value,\n+\t\t\t int pos, void *data);\n+\n+enum early_scan_flags {\n+\tEARLY_SCAN_STOP_AT_NON_OPTION = 1 << 0, /* Stop at any non option */\n+};\n+\n+/*\n+ * Scan `argv` for the options described by `option`, calling `fn` for\n+ * each of those that have PARSE_OPT_EARLY set. `argv` is not\n+ * modified.\n+ *\n+ * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is\n+ * useful to only find out where the scan stops.\n+ *\n+ * The scan always stops at \"--\" and at \"--end-of-options\", as\n+ * parse_options() always stops parsing options there too, whatever its\n+ * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only\n+ * decide if the terminator is left in argv, not if it terminates.\n+ *\n+ * Returns the index at which the scan stopped, which is `argc` when the\n+ * whole array was scanned.\n+ *\n+ * This scan is for now deliberately much simpler than\n+ * parse_options(), so it differs from it in the following ways:\n+ *\n+ * - Only the long form of an option is matched, and it has to be\n+ * spelled in full: short options and abbreviations are ignored.\n+ *\n+ * - Negated forms (\"--no-\") are not matched. This is harmless,\n+ * as they never take a value to skip.\n+ *\n+ * - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are\n+ * treated as not taking a separate value.\n+ *\n+ * - OPTION_SUBCOMMAND entries are skipped.\n+ *\n+ * - OPTION_ALIAS entries are not resolved to the option they stand\n+ * for.\n+ *\n+ * So the scan can fail to see an option that parse_options() would\n+ * accept, and callers have to cope with that, typically by erring on\n+ * the safe side.\n+ */\n+int early_scan_options(int argc, const char **argv,\n+\t\t const struct option *option,\n+\t\t enum early_scan_flags flags,\n+\t\t early_scan_fn *fn, void *data);\n+\n /*----- incremental advanced APIs -----*/\n \n struct parse_opt_cmdmode_list;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex f181f0c02d..83522714c8 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -383,3 +383,65 @@ int cmd__parse_subcommand(int argc, const char **argv)\n \n \treturn parse_subcommand__cmd(argc, argv, test_flags);\n }\n+\n+static int show_early_option(const struct option *opt, const char *value,\n+\t\t\t int pos, void *data UNUSED)\n+{\n+\tprintf(\"found: %s at %d\", opt->long_name, pos);\n+\tif (value)\n+\t\tprintf(\" value: %s\", value);\n+\tputchar('\\n');\n+\treturn 0;\n+}\n+\n+int cmd__early_scan_options(int argc, const char **argv)\n+{\n+\tchar *a_string = NULL;\n+\tint an_int = 0, a_bool = 0, a_short = 0;\n+\n+\tconst struct option option[] = {\n+\t\tOPT_GROUP(\"early scan test options\"),\n+\t\tOPT_BOOL_F(0, \"wanted\", &a_bool,\n+\t\t\t \"wanted option taking no value\",\n+\t\t\t PARSE_OPT_EARLY),\n+\t\tOPT_STRING_F(0, \"wanted-value\", &a_string, \"str\",\n+\t\t\t \"wanted option taking a value\",\n+\t\t\t PARSE_OPT_EARLY),\n+\t\tOPT_STRING(0, \"skipped-value\", &a_string, \"str\",\n+\t\t\t \"option whose value has to be skipped\"),\n+\t\tOPT_INTEGER(0, \"number\", &an_int,\n+\t\t\t \"option taking an integer value\"),\n+\t\tOPT_STRING_F(0, \"optarg\", &a_string, \"str\",\n+\t\t\t \"option with an optional value\",\n+\t\t\t PARSE_OPT_OPTARG),\n+\t\tOPT_STRING_F(0, \"lastarg\", &a_string, \"str\",\n+\t\t\t \"option with a last argument default\",\n+\t\t\t PARSE_OPT_LASTARG_DEFAULT),\n+\t\tOPT_STRING_F(0, \"early-optarg\", &a_string, \"str\",\n+\t\t\t \"early option with an optional value\",\n+\t\t\t PARSE_OPT_EARLY | PARSE_OPT_OPTARG),\n+\t\tOPT_STRING_F(0, \"early-lastarg\", &a_string, \"str\",\n+\t\t\t \"early option with a last argument default\",\n+\t\t\t PARSE_OPT_EARLY | PARSE_OPT_LASTARG_DEFAULT),\n+\t\tOPT_BOOL('s', NULL, &a_short, \"short only option\"),\n+\t\tOPT_END()\n+\t};\n+\n+\tenum early_scan_flags flags = 0;\n+\tint stopped;\n+\n+\twhile (argc > 1 && *argv[1] == '-') {\n+\t\tif (!strcmp(argv[1], \"--stop-at-non-option\"))\n+\t\t\tflags |= EARLY_SCAN_STOP_AT_NON_OPTION;\n+\t\telse\n+\t\t\tbreak;\n+\t\targc--;\n+\t\targv++;\n+\t}\n+\n+\tstopped = early_scan_options(argc - 1, argv + 1, option, flags,\n+\t\t\t\t show_early_option, NULL);\n+\tprintf(\"stopped at: %d of %d\\n\", stopped, argc - 1);\n+\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex b71a22b43b..5d2f5877d9 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -50,6 +50,7 @@ static struct test_cmd cmds[] = {\n \t{ \"pack-mtimes\", cmd__pack_mtimes },\n \t{ \"parse-options\", cmd__parse_options },\n \t{ \"parse-options-flags\", cmd__parse_options_flags },\n+\t{ \"early-scan-options\", cmd__early_scan_options },\n \t{ \"parse-pathspec-file\", cmd__parse_pathspec_file },\n \t{ \"parse-subcommand\", cmd__parse_subcommand },\n \t{ \"partial-clone\", cmd__partial_clone },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex f2885b33d5..071306d52d 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -43,6 +43,7 @@ int cmd__pack_deltas(int argc, const char **argv);\n int cmd__pack_mtimes(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\n int cmd__parse_options_flags(int argc, const char **argv);\n+int cmd__early_scan_options(int argc, const char **argv);\n int cmd__parse_pathspec_file(int argc, const char** argv);\n int cmd__parse_subcommand(int argc, const char **argv);\n int cmd__partial_clone(int argc, const char **argv);\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 449fff4d34..b796d96b9a 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -845,4 +845,177 @@ test_expect_success 'u16 limits range' '\n \ttest_grep \"value 65536 for option .u16. not in range \\[0,65535\\]\" err\n '\n \n+test_expect_success 'early_scan_options() finds a wanted option' '\n+\ttest-tool early-scan-options --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 0\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() reads a stuck or separate value' '\n+\ttest-tool early-scan-options --wanted-value=one >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted-value at 0 value: one\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --wanted-value two >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted-value at 0 value: two\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() skips the value of other options' '\n+\ttest-tool early-scan-options --skipped-value --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --skipped-value one --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() always stops at \"--\"' '\n+\ttest-tool early-scan-options -- --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --stop-at-non-option -- --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --skipped-value -- --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() always stops at \"--end-of-options\"' '\n+\ttest-tool early-scan-options --end-of-options --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --stop-at-non-option \\\n+\t\t--end-of-options --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() can stop at a non-option' '\n+\ttest-tool early-scan-options --stop-at-non-option \\\n+\t\targ --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 0 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --stop-at-non-option \\\n+\t\t--skipped-value arg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 2\n+\tstopped at: 3 of 3\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() ignores abbreviated options' '\n+\ttest-tool early-scan-options --want >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() takes values from struct option' '\n+\ttest-tool early-scan-options --number --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --number=5 --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() does not skip an optional value' '\n+\ttest-tool early-scan-options --optarg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --lastarg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() matches a stuck optional value' '\n+\ttest-tool early-scan-options --early-optarg=one >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: early-optarg at 0 value: one\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --early-lastarg=two >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: early-lastarg at 0 value: two\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() does not take a separate optional value' '\n+\ttest-tool early-scan-options --early-optarg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: early-optarg at 0\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual &&\n+\ttest-tool early-scan-options --early-lastarg --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: early-lastarg at 0\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() ignores options without a long name' '\n+\ttest-tool early-scan-options -s --wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfound: wanted at 1\n+\tstopped at: 2 of 2\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'early_scan_options() ignores negated options' '\n+\ttest-tool early-scan-options --no-wanted >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tstopped at: 1 of 1\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.56.0.rc2\n\n"},{"id":"553036","messageId":"20260923080928.1534413-4-christian.couder@gmail.com","threadId":"66257","inReplyTo":"20260923080928.1534413-1-christian.couder@gmail.com","subject":"[PATCH v2 3/3] fast-import: use early_scan_options() for --allow-unsafe-features","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:09:28Z","receivedAt":"2026-09-23T08:09:51Z","isPatch":true,"body":"The \"feature\" lines at the start of the stream are processed before the\ncommand line options are parsed, so cmd_fast_import() scans its\narguments early to find out if `--allow-unsafe-features` was given.\n\nThat scan doesn't know which options take their value as a separate\nargument, and it stops at the first argument that doesn't start with a\ndash. So it disagrees with parse_options(), which accepts values\nseparated from their option by a space, for a command line like\n\"--depth 5 --allow-unsafe-features\": the scan stops at \"5\" and never\nsees the option, so unsafe \"feature\" commands from the stream are\nrefused even though the option was given.\n\nLet's fix this by using early_scan_options(), which scans the very\nsame `struct option` array that parse_options() uses, so that both\nagree on which options take a value, and by marking\n`--allow-unsafe-features` with PARSE_OPT_EARLY so that the scan\nreports it.\n\nNote that the scan still only matches the exact option spelling, while\nparse_options() also accepts unambiguous abbreviations, so the two still\ndisagree for a command line like \"--allow-unsafe\". This errs on the safe\nside, and is now documented as a restriction.\n\nSigned-off-by: Christian Couder \n---\n Documentation/git-fast-import.adoc | 10 ++++----\n builtin/fast-import.c | 38 +++++++++++++++++-------------\n t/t9300-fast-import.sh | 14 +++++++++++\n 3 files changed, 39 insertions(+), 23 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc\nindex fd165e11d2..c04b8fe502 100644\n--- a/Documentation/git-fast-import.adoc\n+++ b/Documentation/git-fast-import.adoc\n@@ -66,12 +66,10 @@ fast-import stream! This option is enabled automatically for\n remote-helpers that use the `import` capability, as they are\n already trusted to run their own code.\n +\n-Note that this option has to be spelled in full, and has to appear\n-before any option whose value is separated from it by a space, for\n-the unsafe `feature` commands in the stream to be allowed. So\n-`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse\n-them, while `--allow-unsafe-features --depth 5` and\n-`--depth=5 --allow-unsafe-features` allow them.\n+Note that this option has to be spelled in full for the unsafe\n+`feature` commands in the stream to be allowed. So `--allow-unsafe`\n+is accepted as an unambiguous abbreviation of this option, but the\n+unsafe `feature` commands are still refused.\n \n `--signed-tags=`::\n \tSpecify how to handle signed tags. Behaves in the same way as\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex fbd919982c..7f36b828ce 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -4120,6 +4120,17 @@ static int option_parse_quiet(const struct option *opt UNUSED,\n \treturn 0;\n }\n \n+static int option_parse_early_allow_unsafe(const struct option *option,\n+\t\t\t\t\t const char *value UNUSED,\n+\t\t\t\t\t int pos UNUSED, void *data)\n+{\n+\tstruct fast_import_state *state = data;\n+\n+\tif (!strcmp(option->long_name, \"allow-unsafe-features\"))\n+\t\tstate->allow_unsafe_features = 1;\n+\treturn 0;\n+}\n+\n int cmd_fast_import(int argc,\n \t\t const char **argv,\n \t\t const char *prefix,\n@@ -4184,7 +4195,7 @@ int cmd_fast_import(int argc,\n \t\tOPT_HIDDEN_GROUP(N_(\"Advanced\")),\n \t\tOPT_BOOL_F(0, \"allow-unsafe-features\", &state.allow_unsafe_features,\n \t\t\t N_(\"allow unsafe mark commands from the stream\"),\n-\t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),\n+\t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY),\n \t\tOPT_CALLBACK_F(0, \"export-pack-edges\", &state, N_(\"file\"),\n \t\t\t N_(\"dump edge commits to \"),\n \t\t\t PARSE_OPT_HIDDEN | PARSE_OPT_NONEG,\n@@ -4218,23 +4229,16 @@ int cmd_fast_import(int argc,\n \t * line to override stream data). But we must do an early parse of any\n \t * command-line options that impact how we interpret the feature lines.\n \t *\n-\t * NEEDSWORK: This scan only matches the exact \"--allow-unsafe-features\"\n-\t * spelling and stops at the first argument that doesn't start with a\n-\t * dash. As parse_options() below also accepts unambiguous abbreviations\n-\t * and values separated by a space from their option, the two disagree\n-\t * for command lines like \"--allow-unsafe\" or \"--depth 5\n-\t * --allow-unsafe-features\": parse_options() accepts the option, but\n-\t * this scan doesn't see it, so unsafe features from the stream are\n-\t * still refused. This errs on the safe side, but should be fixed by\n-\t * teaching this scan about the options that take a value.\n+\t * NEEDSWORK: This scan only matches the exact\n+\t * \"--allow-unsafe-features\" spelling, while parse_options() below\n+\t * also accepts unambiguous abbreviations, so the two disagree for\n+\t * a command line like \"--allow-unsafe\": parse_options() accepts\n+\t * the option, but this scan doesn't see it, so unsafe features\n+\t * from the stream are still refused. This errs on the safe side.\n \t */\n-\tfor (int i = 1; i < argc; i++) {\n-\t\tconst char *arg = argv[i];\n-\t\tif (*arg != '-' || !strcmp(arg, \"--\"))\n-\t\t\tbreak;\n-\t\tif (!strcmp(arg, \"--allow-unsafe-features\"))\n-\t\t\tstate.allow_unsafe_features = 1;\n-\t}\n+\tearly_scan_options(argc - 1, argv + 1, fast_import_options,\n+\t\t\t EARLY_SCAN_STOP_AT_NON_OPTION,\n+\t\t\t option_parse_early_allow_unsafe, &state);\n \n \trc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));\n \tfor (unsigned int i = 0; i < (cmd_save - 1); i++)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex d9de2ef0d8..1a37f2b8e6 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2344,6 +2344,20 @@ test_expect_success 'R: export-marks options can be overridden by commandline op\n \ttest_path_is_missing feature-sub\n '\n \n+test_expect_success 'R: --allow-unsafe-features found after a value' '\n+\techo \"feature import-marks-if-exists=nonexistent.marks\" >input &&\n+\tgit fast-import --allow-unsafe-features input &&\n+\ttest_must_fail git fast-import --allow-unsafe err &&\n+\ttest_grep \"forbidden in input without --allow-unsafe-features\" err\n+'\n+\n test_expect_success 'R: catch typo in marks file name' '\n \ttest_must_fail git fast-import --import-marks=nonexistent.marks wrote:\n>\n> Christian Couder writes:\n\n> > To allow these commands to safely skip option values during their\n> > early scans, this series introduces a new \"early-scan\" sub-API into\n> > the existing \"parse-options\" API.\n>\n> Yay.\n>\n> > This is deliberately implemented as a new simple and fast scan, which\n> > has some limitations, instead of a full refactor and reuse of the\n> > parse_options() code,\n>\n> Sigh. In other words, we hate these ad-hoc prescan that are buggy\n> badly enough to replace them all with yet another ad-hoc prescan\n> that is know to behave differently from the real thing?\n\nYes, because the limitations of the new scan are not very significant\nin practice while refactoring the real thing (so that it can perform\nan early scan without side effects) would be much more complex.\n\n> > - `git bisect start --term-good -- ` mistook the term name\n> > `--` for the revision/path separator, so was rejected\n> > as an invalid revision instead of being treated as a path.\n>\n> Sorry, I fail to see much practical value in this.\n>\n> > - `git rev-parse --default -- ` did the same, reporting\n> > \"bad revision \" while any other default value gives the\n> > usual more helpful \"ambiguous argument\" error.\n>\n> Neither in this one.\n\nI have removed those from the series in the v2 I just sent.\n\n> > - `git fast-import --depth 5 --allow-unsafe-features` silently\n> > ignored `--allow-unsafe-features`, refusing unsafe features from\n> > the stream.\n>\n> On the other hand, this may be a very good thing.\n>\n> Is the reason why the ad-hoc pre-scan failed to see it was because\n> it did not realize 5 is a value to the --depth option?\n\nYes.\n\n> > All of these commands call parse_options(), but for `git bisect` and\n> > `git rev-parse`, the specific functions doing the early scan\n> > (bisect_start() and cmd_rev_parse()'s main loop) parse their own\n> > options by hand after the early scan and have no `struct option` array\n> > for those options.\n> >\n> > If bisect_start() and cmd_rev_parse() were converted to use\n> > `struct option`, they could use early_scan_options_from_options() and\n> > would not be affected by limitations 1), 2) and 3) above, as both use\n> > the early scan only to locate `--`.\n>\n> I imagine that in the long term we would rather see a properly\n> refactored parse-options machinery perform the prescan (perhaps with\n> some kind of \"dry-run\" option given to the machinery) than yet\n> another ad-hoc parser like this topic introduces. It would be very\n> good if this interim solution at least took the same 'options[]'\n> array so that when we have the real thing in the future we do not\n> have to redo the conversion effort.\n\nThis is what is implemented in the v2 I just sent. So yeah, when a\nrefactored parse-options machinery will be able to perform the\nprescan, we will be able to use it to replace the early-scan parser\nwithout changing or converting the callers.\n\n> By the way, how does this interact with your other topic that has\n> been stalled for quite some time? Would moving this one forward\n> help the other, or do they not have much relevance to each other? I\n> would rather not see two topics of non-trivial size stalled on a\n> single author at the same time, so ...\n\nThey are separate topics and I alternate between them. I was recently\nbusy with travelling to the Git Merge and was a bit sick before that,\nbut hopefully I should be able to spend more time on them in the next\nweeks. Also it seems to me that both topics have advanced to a point\nwhere not a lot of big changes are needed. So they should move forward\nquite fast now.\n"},{"id":"553038","messageId":"CAP8UFD0KP+e4EYVAKW1+6n3og1nzi_+Utr59Vgo8Fz0G=WZ-Qw@mail.gmail.com","threadId":"66257","inReplyTo":"xmqqy0djfgmt.fsf@gitster.g","subject":"Re: [PATCH 1/6] parse-options: add early_scan_options()","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:10:37Z","receivedAt":"2026-09-23T08:10:50Z","isPatch":true,"body":"On Thu, Sep 3, 2026 at 12:11 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > So users must spell these specific options in full. This restriction\n> > could be lifted in the future though, once the scanner is adapted to\n> > accept a command's full option array, as this would give it the\n> > complete context needed for safe abbreviation matching.\n>\n> It is unfortunate that end-users cannot tell if they are dealing\n> with a system before of after \"once the scanner is adapted\"\n> happened, so they must be trained to always spell the options in\n> full to make use of the commands that use this feature. It at least\n> does not regress relative to the ad-hoc early scanners these selected\n> commands have that do not even understand what they are parsing, so\n> it may not be too bad.\n>\n> Stepping back a bit, the burden on programmers to use this would be\n> to write in a separate notation what options there are in addition\n> to what they feed the real parse_options(), which cuts both ways in\n> the sense that because this does not take parse_options(), commands\n> that do not use parse_options() can still use it, but those that do\n> already use parse_options() need additional work to use eary_scan.\n>\n> And then once the scanner is adapted to accept the full option array,\n> the programmers only need to discard the struct early_scan_option[]\n> they wrote and replace it with the struct option[] they already have?\n> Or would the calling convention to the scanner also change when it\n> happens (oother than replacing the pointer to struct early_scan_option[]\n> with another pointer to struct option[])?\n\nI agree that what was implemented in v1 (to be able to accommodate\nearly scans that do not use parse_options()) didn't bring much\npractical value, was a bit complex and required some churn when the\nearly scan would have been converted to use parse_options(). So, in\nthe v2 I just sent, it addresses only the early scan where\nparse_options() is used, which simplifies a lot of things.\n\n> > +static const struct early_scan_option *\n> > +find_early_scan_option(const char *arg,\n> > + const struct early_scan_option *options,\n> > + const char **value)\n>\n> Because you return one single element from the incoming array of\n> options, it is mildly misleading to call the variable/parameter\n> \"options\" here and everywhere else. Let's stick to \"arrays are\n> named singular, so that option[4] names 4th option\" convention.\n\nRight, I have changed the argument to `const struct option *option`.\n\n> > +{\n> > + if (!skip_prefix(arg, \"--\", &arg))\n> > + return NULL;\n> > +\n> > + for (; options->name; options++) {\n> > + const char *rest;\n> > +\n> > + if (!skip_prefix(arg, options->name, &rest))\n> > + continue;\n>\n> \"--option\" on the command line, after getting stripped the leading\n> \"--\", may begin with \"option\", and that name may be in the option[]\n> table, in which case ...\n>\n> > + if (!*rest) {\n> > + *value = NULL;\n> > + return options;\n> > + }\n>\n> ... we found a hit. But shouldn't option->takes_value be consulted\n> before we return to signal the caller that the next arg is an option\n> value before we return from here? It looks a bit uneven as we do\n> that for stuck form \"--option=value\" here.\n\nYeah, we found that `arg` exactly matches this option whether or not\nit takes a value, but the value is not here.\n\nWhether the next argument has to be skipped is decided by the caller:\n\n if (parse_options_takes_argument(opt) && !value && i + 1 < argc)\n value = argv[++i];\n\nfind_early_scan_option() cannot do that itself, as it has neither\nargv, argc nor the current index.\n\nSo signalling to the caller would be redundant, because the caller\nalready holds the matched option and can ask directly.\n\nBut maybe I should add a comment on the line before `if (!*rest) {`\nsaying that skipping a separate value is the caller's job?\n\n> > + /* Only an option taking a value can be stuck to one. */\n> > + if (*rest == '=' && options->takes_value) {\n> > + *value = rest + 1;\n> > + return options;\n> > + }\n>\n> And if the option[] table had \"opt\", then \"--option\" on the command\n> line may begin with \"--opt\" but \"ion\" is an excess that is not a\n> stuck value, so we do not consider it as a match. OK.\n\nNow using `takes_value` in the `*rest == '='` case wasn't quite right,\nas parse_options_takes_argument() returns 0 for PARSE_OPT_OPTARG and\nPARSE_OPT_LASTARG_DEFAULT, but parse_options() does accept a stuck\nvalue for both.\n\nSo in v2 we use the same condition parse_options() uses:\n\n /* Only an option that can take a value may have one stuck to it. */\n if (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {\n *value = rest + 1;\n return opt;\n }\n\n> > + }\n> > + return NULL;\n> > +}\n>\n> If we are to write a separate function anyway, I wonder how much\n> more work to write a early_scan_option() parser that does take a\n> real \"struct option[]\" array. Its elements already know if they\n> take a value or not. For expediency, it may be OK to start by\n> simplified parser that does not handle unique prefix and other\n> complexities like callback functions of the real parser, but at\n> least it would reduce the burden on the programmers quite a bit if\n> we used the real struct option[] array, I suspect.\n\nThis is what v2 does, and I agree that it simplifies things.\n"},{"id":"553039","messageId":"CAP8UFD3sh9Ejfgv7CB33LRU_z3i662_+tjdW7JqwRrzexWX_Ow@mail.gmail.com","threadId":"66257","inReplyTo":"xmqqse3rffr4.fsf@gitster.g","subject":"Re: [PATCH 2/6] bisect: fix \"--\" detection when a term name is \"--\"","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2026-09-23T08:11:32Z","receivedAt":"2026-09-23T08:11:44Z","isPatch":true,"body":"On Thu, Sep 3, 2026 at 12:30 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > `bisect_start()` walks its arguments twice. The second loop actually\n> > parses the options, and it knows that `--term-good`, `--term-old`,\n> > `--term-bad` and `--term-new` take their value as a separate argument,\n> > so it skips that value.\n> >\n> > The first loop, which only looks for the \"--\" separating revisions from\n> > paths, doesn't know about these options. So when such an option is given\n> > \"--\" as its value, that \"--\" is mistaken for the separator and\n> > `has_double_dash` is wrongly set.\n>\n> It may be theoretically true, but I wonder how much practical value\n> it has to correctly parse \"--term-good --\" as \"Ah, the user wants to\n> mark good revisions as '--' instead of 'good' or 'old'\"? Even\n> though \"refs/bisect/--\" is *not* forbidden, how likely is it for\n> users to do that?\n>\n> This is not like \"git grep -e --\" which does have much more pracical\n> value.\n\nRight, this patch and the next one have been removed from v2.\n\nIn the future we can still convert bisect_start() to the parse-options\nAPI, and then use the early-scan API to look for \"--\" in a bit cleaner\nway.\n"},{"id":"553082","messageId":"xmqqqzijc22h.fsf@gitster.g","threadId":"66257","inReplyTo":"CAP8UFD0KP+e4EYVAKW1+6n3og1nzi_+Utr59Vgo8Fz0G=WZ-Qw@mail.gmail.com","subject":"Re: [PATCH 1/6] parse-options: add early_scan_options()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-23T17:25:26Z","receivedAt":"2026-09-23T17:25:29Z","isPatch":true,"body":"Christian Couder writes:\n\n> Whether the next argument has to be skipped is decided by the caller:\n>\n> if (parse_options_takes_argument(opt) && !value && i + 1 < argc)\n> value = argv[++i];\n>\n> find_early_scan_option() cannot do that itself, as it has neither\n> argv, argc nor the current index.\n>\n> So signalling to the caller would be redundant, because the caller\n> already holds the matched option and can ask directly.\n>\n> But maybe I should add a comment on the line before `if (!*rest) {`\n> saying that skipping a separate value is the caller's job?\n\nNot really. I was hinting if it is cleaner to have the callee do the\nskipping so that caller does not have to worry about it. After all,\nthe job of the early-scan machinery is to scan the options reliably\nto find something later in the command line argument array. The\nless the caller needs to do, the easier the machinery is to use.\n\n>> > + /* Only an option taking a value can be stuck to one. */\n>> > + if (*rest == '=' && options->takes_value) {\n>> > + *value = rest + 1;\n>> > + return options;\n>> > + }\n>>\n>> And if the option[] table had \"opt\", then \"--option\" on the command\n>> line may begin with \"--opt\" but \"ion\" is an excess that is not a\n>> stuck value, so we do not consider it as a match. OK.\n>\n> Now using `takes_value` in the `*rest == '='` case wasn't quite right,\n> as parse_options_takes_argument() returns 0 for PARSE_OPT_OPTARG and\n> PARSE_OPT_LASTARG_DEFAULT, but parse_options() does accept a stuck\n> value for both.\n>\n> So in v2 we use the same condition parse_options() uses:\n\nMy giving an opaque hint pays off sometimes ;-)\n\nThanks.\n"},{"id":"553084","messageId":"xmqqmrt7c1zf.fsf@gitster.g","threadId":"66257","inReplyTo":"CAP8UFD3sh9Ejfgv7CB33LRU_z3i662_+tjdW7JqwRrzexWX_Ow@mail.gmail.com","subject":"Re: [PATCH 2/6] bisect: fix \"--\" detection when a term name is \"--\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-23T17:27:16Z","receivedAt":"2026-09-23T17:27:19Z","isPatch":true,"body":"Christian Couder writes:\n\n> In the future we can still convert bisect_start() to the parse-options\n> API, and then use the early-scan API to look for \"--\" in a bit cleaner\n> way.\n\nYeah, when that happens, I can imagine that we can make detection of\n\"--\" to come for free as a side effect of using parse_options().\n\nThanks.\n"},{"id":"553674","messageId":"84f9d1c9-30b7-4d8b-82d6-9afd16d0ab08@gmail.com","threadId":"66257","inReplyTo":"20260923080928.1534413-3-christian.couder@gmail.com","subject":"Re: [PATCH v2 2/3] parse-options: add early_scan_options()","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2026-09-30T07:13:53Z","receivedAt":"2026-09-30T07:13:59Z","isPatch":true,"body":"On 9/23/26 13:39, Christian Couder wrote:\n>\n > [ snip ]\n >\n> One consequence of staying simple is that abbreviated options are\n> still not matched, even though the scan is now given the command's\n> full option array. Resolving them the way parse_options() does would\n> mean duplicating the ambiguity detection that parse_long_opt()\n> performs. So the scan can fail to see an option that parse_options()\n> would accept, and its callers have to cope with that, typically by\n> erring on the safe side. This and the other differences with\n> parse_options() are documented in \"parse-options.h\".\n> \n\nI think not handling abbreviations could also have another potential \nproblem. Consider a command as follows:\n\n $ git fast-import --quiet --export-pack --allow-unsafe-features\n\nHere `--export-pack` is an abbreviation of `--export-pack-edges`. So, \nthe arg next to it should ideally be considered as a value for it but\ngiven the correct \"ignore\" logic, we will happily interpret is an \nargument which misaligns with parse_options()'s behaviour.\n\nIn the ideal world, we could say such weird names for files is unlikely \nand this isn't such a big concern. But given it is the scope of this \nseries to make early scan more reliable, I think we should consider how \nto handle this better.\n\nWould it make sense to actually err on the safe side and just stop \nwalking the args as soon as we notice an unrecognized argument? This \nwill the ensure the walk never misinterpret a value for an argument.\n\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index a132c1ea12..559dad9061 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n>\n > [ snip ]\n >\n> +int early_scan_options(int argc, const char **argv,\n> +\t\t const struct option *option,\n> +\t\t enum early_scan_flags flags,\n> +\t\t early_scan_fn *fn, void *data)\n> +{\n> +\tfor (int i = 0; i < argc; i++) {\n> +\t\tconst char *arg = argv[i];\n> +\t\tconst char *value;\n> +\t\tconst struct option *opt;\n> +\t\tint pos = i;\n> +\n> +\t\t/*\n> +\t\t * parse_options() always stops parsing options at these,\n> +\t\t * whatever its flags, so nothing after them is an option.\n> +\t\t */\n> +\t\tif (!strcmp(arg, \"--\") || !strcmp(arg, \"--end-of-options\"))\n> +\t\t\treturn i;\n> +\n> +\t\topt = find_early_scan_option(arg, option, &value);\n> +\t\tif (!opt) {\n> +\t\t\tif ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&\n> +\t\t\t (*arg != '-' || !arg[1]))\n> +\t\t\t\treturn i;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * When an option takes a value, but that value is not\n> +\t\t * stuck to it with '=', then the next argument is the\n> +\t\t * value and it has to be skipped so that it isn't\n> +\t\t * taken for an option itself.\n> +\t\t */\n> +\t\tif (parse_options_takes_argument(opt) && !value && i + 1 < argc)\n> +\t\t\tvalue = argv[++i];\n> +\n\nThe 'i +1 < argc' part is an appropriate guard to have. But this means a \ncommand such as the following:\n\n test-tool early-scan-options --wanted-value\n\n... would reult in 'value' being NULL. I suppose this is kind of \nexpected for the early scan code and is not something we need to worry \nabout?\n\n> +\t\tif (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))\n> +\t\t\treturn i;\n> +\t}\n> +\n> +\treturn argc;\n> +}\n> +\n> static int usage_argh(const struct option *opts, FILE *outfile)\n> {\n> \tconst char *s;\n> diff --git a/parse-options.h b/parse-options.h\n> index f29e73f85c..3ef64744a4 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n >\n> [ snip ]\n>\n> +/*\n> + * Scan `argv` for the options described by `option`, calling `fn` for\n> + * each of those that have PARSE_OPT_EARLY set. `argv` is not\n> + * modified.\n> + *\n> + * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is\n> + * useful to only find out where the scan stops.\n> + *\n> + * The scan always stops at \"--\" and at \"--end-of-options\", as\n> + * parse_options() always stops parsing options there too, whatever its\n> + * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only\n> + * decide if the terminator is left in argv, not if it terminates.\n> + *\n> + * Returns the index at which the scan stopped, which is `argc` when the\n> + * whole array was scanned.\n> + *\n\nAs for the return index, when the callback stops the scan the index \nreturned is that of the option's value rather than the option itself. \nWould it be better to capture this more clearly?\n\nAlso, would it be helpful to also have a test for this?\n\n> + * This scan is for now deliberately much simpler than\n> + * parse_options(), so it differs from it in the following ways:\n> + *\n> + * - Only the long form of an option is matched, and it has to be\n> + * spelled in full: short options and abbreviations are ignored.\n> + *\n> + * - Negated forms (\"--no-\") are not matched. This is harmless,\n> + * as they never take a value to skip.\n> + *\n> + * - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are\n> + * treated as not taking a separate value.\n> + *\n> + * - OPTION_SUBCOMMAND entries are skipped.\n> + *\n> + * - OPTION_ALIAS entries are not resolved to the option they stand\n> + * for.\n> + *\n> + * So the scan can fail to see an option that parse_options() would\n> + * accept, and callers have to cope with that, typically by erring on\n> + * the safe side.\n> + */\n> +int early_scan_options(int argc, const char **argv,\n> +\t\t const struct option *option,\n> +\t\t enum early_scan_flags flags,\n> +\t\t early_scan_fn *fn, void *data);\n >\n> [ snip ]>\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index 449fff4d34..b796d96b9a 100755\n>\n > [ snip ]> +\n> +test_expect_success 'early_scan_options() takes values from struct option' '\n> +\ttest-tool early-scan-options --number --wanted >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tstopped at: 2 of 2\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\ttest-tool early-scan-options --number=5 --wanted >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfound: wanted at 1\n> +\tstopped at: 2 of 2\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nCompared to others, I'm not quite sure this test is testing something \nspecial. Do we need it?\n\n> +test_expect_success 'early_scan_options() does not skip an optional value' '\n> +\ttest-tool early-scan-options --optarg --wanted >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfound: wanted at 1\n> +\tstopped at: 2 of 2\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\ttest-tool early-scan-options --lastarg --wanted >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfound: wanted at 1\n> +\tstopped at: 2 of 2\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'early_scan_options() matches a stuck optional value' '\n> +\ttest-tool early-scan-options --early-optarg=one >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfound: early-optarg at 0 value: one\n> +\tstopped at: 1 of 1\n> +\tEOF\n> +\ttest_cmp expect actual &&\n> +\ttest-tool early-scan-options --early-lastarg=two >actual &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfound: early-lastarg at 0 value: two\n> +\tstopped at: 1 of 1\n> +\tEOF\n> +\ttest_cmp expect actual\n> +'\n\nWould the following be a useful part to also add to the above?\n\n test-tool early-scan-options --optarg=5 --wanted >actual &&\n cat >expect <<-\\EOF &&\n found: wanted at 1\n stopped at: 2 of 2\n EOF\n test_cmp expect actual\n\n-- \nSivaraam\n\n"}]}