[PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

[PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

[Christian Couder]

A number of commands perform an early scan of their arguments to look
for specific flags or structural separators (like `--`).

These hand-rolled early scans are often fragile. They especially fail
to account for options that take their value as a separate
argument. This leads to disagreements between the early scan and the
actual parse_options() pass. For example, the early scanner might miss
a special option entirely, or mistakenly treat an option's value as
the `--` path separator.

To allow these commands to safely skip option values during their
early scans, this series introduces a new "early-scan" sub-API into
the existing "parse-options" API.

This is deliberately implemented as a new simple and fast scan, which
has some limitations, instead of a full refactor and reuse of the
parse_options() code, because the limitations are not very significant
in practice, while a full refactor and reuse of the parse_options()
code would be much more complex.

The current limitations of the new early scan code are:

 1. short options are ignored,

 2. options with PARSE_OPT_LASTARG_DEFAULT or PARSE_OPT_OPTARG are
 treated as not taking a separate value,

 3. negated options ("--no-...") are not automatically generated,

 4. abbreviated options will not be matched.

Note that while the others could be real issues for some commands,
"3. negated options" is not a practical issue because negated options
never consume a separate argument.

The early scan is performed by a new early_scan_options() function
which takes a `const struct early_scan_option *options` array as
argument. That array can be built either by hand or by a new
early_scan_options_from_options() function, which takes a
`const struct option *options` array, when the command already uses
`struct option`.

This allows us to use the new early-scan API even for commands that
don't use the parse-options API yet, and which are the majority of
commands performing an early scan.

In this series, only `git bisect`, `git rev-parse` and `git
fast-import` are converted to the early-scan API, which fixes bugs in
those commands:

 - `git bisect start --term-good -- <not-a-rev>` mistook the term name
   `--` for the revision/path separator, so <not-a-rev> was rejected
   as an invalid revision instead of being treated as a path.

 - `git rev-parse --default -- <not-a-rev>` did the same, reporting
   "bad revision <notarev>" while any other default value gives the
   usual more helpful "ambiguous argument" error.

 - `git fast-import --depth 5 --allow-unsafe-features` silently
   ignored `--allow-unsafe-features`, refusing unsafe features from
   the stream.

All of these commands call parse_options(), but for `git bisect` and
`git rev-parse`, the specific functions doing the early scan
(bisect_start() and cmd_rev_parse()'s main loop) parse their own
options by hand after the early scan and have no `struct option` array
for those options.

If bisect_start() and cmd_rev_parse() were converted to use
`struct option`, they could use early_scan_options_from_options() and
would not be affected by limitations 1), 2) and 3) above, as both use
the early scan only to locate `--`.

Note that using early_scan_options_from_options() rather than a
hand-written table does not change how abbreviations are handled: the
scan matches long names exactly either way. Limitation 4) would
nevertheless become relevant to those commands, because such a
conversion would also make parse_options() the parser for the options
after the early scan has first inspected them, and parse_options()
resolves abbreviations while their current hand-rolled loops do not.

`git diff`, `git column`, `git rev-list` and setup_revisions() in
"revision.c" could also be converted to the early-scan API but aren't
in this series for different reasons:

 - `git diff` has a number of short options like `-S`, `-G`, `-O`
   taking separate values.

 - `git column` scans `argv[1]` for `--command=` before reading the
   configuration. Because `--command` is an OPT_STRING,
   parse_options() also accepts `--command <name>` and abbreviations,
   so the two passes disagree. Converting it would fix that, but it
   changes user-visible behaviour in a command this series does not
   otherwise touch.

 - `git rev-list` and "revision.c" are about converting
   setup_revisions(), but converting it to `struct option` first is
   likely the better way forward.

Overview of the patches:
========================

 - Patch 1/6 introduces early_scan_options(), the early scanner that
   will be used instead of hand-rolled ones, along with its
   infrastructure.

 - Patches 2/6 and 3/6 use this scanner to fix bugs in `git bisect`
   and `git rev-parse` respectively.

 - Patch 4/6 refactors some existing code into a new
   parse_options_takes_argument() helper that will be used in the next
   patch.

 - Patch 5/6 introduces the new early_scan_options_from_options() as a
   bridge between the parse-options API and the early-scan API.

 - Patch 6/6 uses early_scan_options_from_options() to fix the early
   scan for `--allow-unsafe-features` in `git fast-import`.

CI tests:
=========

They all pass, see:

https://github.com/chriscool/git/actions/runs/33612974808


Christian Couder (6):
  parse-options: add early_scan_options()
  bisect: fix "--" detection when a term name is "--"
  rev-parse: fix "--" detection when it is an option value
  parse-options: add parse_options_takes_argument()
  parse-options: build early scan options from a struct option array
  fast-import: use early_scan_options() for --allow-unsafe-features

 Documentation/git-fast-import.adoc |  10 +-
 builtin/bisect.c                   |  27 ++++--
 builtin/fast-import.c              |  46 +++++----
 builtin/rev-parse.c                |  26 ++++--
 parse-options.c                    | 144 ++++++++++++++++++++++++++---
 parse-options.h                    |  92 ++++++++++++++++++
 t/helper/test-parse-options.c      |  71 ++++++++++++++
 t/helper/test-tool.c               |   2 +
 t/helper/test-tool.h               |   2 +
 t/t0040-parse-options.sh           | 103 +++++++++++++++++++++
 t/t1500-rev-parse.sh               |   5 +
 t/t6030-bisect-porcelain.sh        |   8 ++
 t/t9300-fast-import.sh             |  14 +++
 13 files changed, 503 insertions(+), 47 deletions(-)

-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 1/6] parse-options: add early_scan_options()

[Christian Couder]

Some commands need to look at a few of their options before they can
parse their command line for real, for example because the result
decides whether a repository is needed at all, or how the beginning of
their input should be interpreted.

Such an early scan has to know which options take their value as a
separate argument, or it mistakes such a value for an option. Several
commands get this wrong, as they just walk their arguments comparing
them to the few option names they care about.

Let's add early_scan_options() to help with this. Its callers describe
the options to look for, but also the ones that merely have to be
skipped along with their value, so that the scan can walk the arguments
without being fooled by option values.

Note that abbreviated options are deliberately not recognized, as a
scan cannot know about the options it hasn't been told about, and would
then resolve abbreviations differently from the actual option parsing.

So users must spell these specific options in full. This restriction
could be lifted in the future though, once the scanner is adapted to
accept a command's full option array, as this would give it the
complete context needed for safe abbreviation matching.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 parse-options.c               | 70 +++++++++++++++++++++++++++++++
 parse-options.h               | 60 +++++++++++++++++++++++++++
 t/helper/test-parse-options.c | 39 ++++++++++++++++++
 t/helper/test-tool.c          |  1 +
 t/helper/test-tool.h          |  1 +
 t/t0040-parse-options.sh      | 77 +++++++++++++++++++++++++++++++++++
 6 files changed, 248 insertions(+)

diff --git a/parse-options.c b/parse-options.c
index 4519ead9dc..b3d19446cd 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1244,6 +1244,76 @@ int parse_options(int argc, const char **argv,
 	return parse_options_end(&ctx);
 }
 
+/*
+ * Look for `arg` among `options`. On success, return the matching option
+ * and set `value` to the value stuck to it, if any, or to NULL.
+ */
+static const struct early_scan_option *
+find_early_scan_option(const char *arg,
+		       const struct early_scan_option *options,
+		       const char **value)
+{
+	if (!skip_prefix(arg, "--", &arg))
+		return NULL;
+
+	for (; options->name; options++) {
+		const char *rest;
+
+		if (!skip_prefix(arg, options->name, &rest))
+			continue;
+		if (!*rest) {
+			*value = NULL;
+			return options;
+		}
+		/* Only an option taking a value can be stuck to one. */
+		if (*rest == '=' && options->takes_value) {
+			*value = rest + 1;
+			return options;
+		}
+	}
+
+	return NULL;
+}
+
+int early_scan_options(int argc, const char **argv,
+		       const struct early_scan_option *options,
+		       enum early_scan_flags flags,
+		       early_scan_fn *fn, void *data)
+{
+	for (int i = 0; i < argc; i++) {
+		const char *arg = argv[i];
+		const char *value;
+		const struct early_scan_option *opt;
+		int pos = i;
+
+		if ((flags & EARLY_SCAN_STOP_AT_DASHDASH) &&
+		    !strcmp(arg, "--"))
+			return i;
+
+		opt = find_early_scan_option(arg, options, &value);
+		if (!opt) {
+			if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&
+			    (*arg != '-' || !arg[1]))
+				return i;
+			continue;
+		}
+
+		/*
+		 * When an option takes a value, but that value is not
+		 * stuck to it with '=', then the next argument is the
+		 * value and it has to be skipped so that it isn't
+		 * taken for an option itself.
+		 */
+		if (opt->takes_value && !value && i + 1 < argc)
+			value = argv[++i];
+
+		if (opt->wanted && fn(opt, value, pos, data))
+			return i;
+	}
+
+	return argc;
+}
+
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
diff --git a/parse-options.h b/parse-options.h
index d7f896a933..abc73d8399 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -491,6 +491,66 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 		BUG("option callback expects an argument"); \
 } while(0)
 
+/*----- Early scan: scanning argv before the actual option parsing -----*/
+
+/*
+ * Some commands need to look at a few options before they can parse
+ * their command line for real, for example because the result decides
+ * whether a repository is needed at all.
+ *
+ * Such an early scan has to know which options take their value as a
+ * separate argument, or it could mistake such a value for an option. The
+ * `struct early_scan_option` array passed to early_scan_options() below
+ * describes the options to look for, as well as the ones that only need
+ * to be skipped along with their value.
+ */
+struct early_scan_option {
+	const char *name; 	/* Option name, without the leading dashes */
+	unsigned takes_value:1; /* "--option=value" or "--option value" expected? */
+	unsigned wanted:1;      /* Report option to callback? */
+};
+
+#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 }
+#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 }
+#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 }
+#define EARLY_SCAN_END() { NULL }
+
+/*
+ * Called by early_scan_options() for each argument matching a
+ * `struct early_scan_option` that has its `wanted` bit set.
+ *
+ * `option` is the matching option, `value` its value or NULL if it
+ * doesn't take one, and `pos` the index of the option in argv.
+ *
+ * Returning a non-zero value stops the scan.
+ */
+typedef int early_scan_fn(const struct early_scan_option *option,
+			  const char *value, int pos, void *data);
+
+enum early_scan_flags {
+	EARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at "--" */
+	EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1,
+};
+
+/*
+ * Scan `argv` for the options described by `options`, calling `fn`
+ * for each of those that are `wanted`. `argv` is not modified.
+ *
+ * `fn` may be NULL when no option is `wanted`, which is useful to only
+ * find out where the scan stops.
+ *
+ * Note that abbreviated options are not recognized, as a scan cannot
+ * know about the options it hasn't been told about, and would then
+ * resolve abbreviations differently from the actual option parsing.
+ *
+ * Returns the index at which the scan stopped, which is `argc` when the
+ * whole array was scanned.
+ */
+int early_scan_options(int argc, const char **argv,
+		       const struct early_scan_option *options,
+		       enum early_scan_flags flags,
+		       early_scan_fn *fn, void *data);
+
 /*----- incremental advanced APIs -----*/
 
 struct parse_opt_cmdmode_list;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index f181f0c02d..96ab941d29 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -383,3 +383,42 @@ int cmd__parse_subcommand(int argc, const char **argv)
 
 	return parse_subcommand__cmd(argc, argv, test_flags);
 }
+
+static int show_early_option(const struct early_scan_option *opt,
+			     const char *value, int pos, void *data UNUSED)
+{
+	printf("found: %s at %d", opt->name, pos);
+	if (value)
+		printf(" value: %s", value);
+	putchar('\n');
+	return 0;
+}
+
+int cmd__early_scan_options(int argc, const char **argv)
+{
+	static const struct early_scan_option options[] = {
+		EARLY_SCAN_WANT("wanted"),
+		EARLY_SCAN_WANT_VALUE("wanted-value"),
+		EARLY_SCAN_SKIP_VALUE("skipped-value"),
+		EARLY_SCAN_END()
+	};
+	enum early_scan_flags flags = 0;
+	int stopped;
+
+	while (argc > 1 && *argv[1] == '-') {
+		if (!strcmp(argv[1], "--stop-at-dashdash"))
+			flags |= EARLY_SCAN_STOP_AT_DASHDASH;
+		else if (!strcmp(argv[1], "--stop-at-non-option"))
+			flags |= EARLY_SCAN_STOP_AT_NON_OPTION;
+		else
+			break;
+		argc--;
+		argv++;
+	}
+
+	stopped = early_scan_options(argc - 1, argv + 1, options, flags,
+				    show_early_option, NULL);
+	printf("stopped at: %d of %d\n", stopped, argc - 1);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b71a22b43b..5d2f5877d9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -50,6 +50,7 @@ static struct test_cmd cmds[] = {
 	{ "pack-mtimes", cmd__pack_mtimes },
 	{ "parse-options", cmd__parse_options },
 	{ "parse-options-flags", cmd__parse_options_flags },
+	{ "early-scan-options", cmd__early_scan_options },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
 	{ "partial-clone", cmd__partial_clone },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index f2885b33d5..071306d52d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -43,6 +43,7 @@ int cmd__pack_deltas(int argc, const char **argv);
 int cmd__pack_mtimes(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
 int cmd__parse_options_flags(int argc, const char **argv);
+int cmd__early_scan_options(int argc, const char **argv);
 int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__parse_subcommand(int argc, const char **argv);
 int cmd__partial_clone(int argc, const char **argv);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 449fff4d34..d760d8cfbd 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -845,4 +845,81 @@ test_expect_success 'u16 limits range' '
 	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
 '
 
+test_expect_success 'early_scan_options() finds a wanted option' '
+	test-tool early-scan-options --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 0
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() reads a stuck or separate value' '
+	test-tool early-scan-options --wanted-value=one >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted-value at 0 value: one
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --wanted-value two >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted-value at 0 value: two
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() skips the value of other options' '
+	test-tool early-scan-options --skipped-value --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --skipped-value one --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() can stop at "--"' '
+	test-tool early-scan-options --stop-at-dashdash -- --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --stop-at-dashdash \
+		--skipped-value -- --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() can stop at a non-option' '
+	test-tool early-scan-options --stop-at-non-option \
+		arg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --stop-at-non-option \
+		--skipped-value arg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() ignores abbreviated options' '
+	test-tool early-scan-options --want >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 2/6] bisect: fix "--" detection when a term name is "--"

[Christian Couder]

`bisect_start()` walks its arguments twice. The second loop actually
parses the options, and it knows that `--term-good`, `--term-old`,
`--term-bad` and `--term-new` take their value as a separate argument,
so it skips that value.

The first loop, which only looks for the "--" separating revisions from
paths, doesn't know about these options. So when such an option is given
"--" as its value, that "--" is mistaken for the separator and
`has_double_dash` is wrongly set.

This matters because `has_double_dash` makes the second loop die on an
argument that is not a valid revision, instead of treating it as the
first path. So:

  $ git bisect start --term-good -- notarev
  fatal: 'notarev' does not appear to be a valid revision

while the very same command line with any other term name happily takes
"notarev" as a path.

Let's fix this by using early_scan_options(), telling it about the
options taking their value as a separate argument, so that it can skip
those values.

Note: One might argue that accepting a term name that looks like an
option (such as "--") is a misfeature and should be forbidden entirely.
However, whether we should tighten the validation rules for bisect
terms is a separate UI issue that can be dealt with independently. For
now, this commit simply ensures the parser correctly implements the
existing rules.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 builtin/bisect.c            | 27 +++++++++++++++++++++------
 t/t6030-bisect-porcelain.sh |  8 ++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 1cfb8a794b..ad089b289f 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
 	return bisect_next(terms, prefix);
 }
 
+/*
+ * The options "git bisect start" accepts. Only the ones taking their
+ * value as a separate argument matter to the scan looking for "--" below,
+ * as their value has to be skipped along with them.
+ */
+static const struct early_scan_option bisect_start_early_options[] = {
+	EARLY_SCAN_SKIP_VALUE("term-good"),
+	EARLY_SCAN_SKIP_VALUE("term-old"),
+	EARLY_SCAN_SKIP_VALUE("term-bad"),
+	EARLY_SCAN_SKIP_VALUE("term-new"),
+	EARLY_SCAN_END()
+};
+
 static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 				      const char **argv)
 {
@@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
 
 	/*
 	 * Check for one bad and then some good revisions
+	 *
+	 * The scan below has to know about the options taking their value
+	 * as a separate argument, or such a value that happens to be "--"
+	 * would be mistaken for the "--" separating revisions from paths.
 	 */
-	for (i = 0; i < argc; i++) {
-		if (!strcmp(argv[i], "--")) {
-			has_double_dash = 1;
-			break;
-		}
-	}
+	i = early_scan_options(argc, argv, bisect_start_early_options,
+			       EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);
+	if (i < argc)
+		has_double_dash = 1;
 
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index a7588222a8..464ca53b42 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'bisect start with "--" as a term name' '
+	git bisect reset &&
+	git bisect start --term-good -- hello &&
+	git bisect terms --term-good >actual &&
+	echo -- >expected &&
+	test_cmp expected actual
+'
+
 # Bisect is started with --term-new and --term-old arguments,
 # then skip. The HEAD should be changed.
 test_expect_success 'bisect skip works with --term*' '
-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 3/6] rev-parse: fix "--" detection when it is an option value

[Christian Couder]

`cmd_rev_parse()` walks its arguments twice. The second loop actually
parses the options, and it knows that `--default`, `--prefix` and
`--resolve-git-dir` take their value as a separate argument, so it skips
that value.

The first loop, which only looks for the "--" separating revisions from
paths, doesn't know about these options. So when such an option is given
"--" as its value, that "--" is mistaken for the separator and
`has_dashdash` is wrongly set.

This matters because `has_dashdash` makes the second loop die with a
"bad revision" error on an argument that is neither a revision nor an
existing file, instead of reporting that the argument is ambiguous and
telling how to disambiguate it. So:

  $ git rev-parse --default -- notarev
  fatal: bad revision 'notarev'

while the very same command line with any other default value gives the
usual, much more helpful, "ambiguous argument" error.

Let's fix this the same way as in a previous commit, by using
early_scan_options() and telling it about the options taking their value
as a separate argument.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 builtin/rev-parse.c  | 26 ++++++++++++++++++++------
 t/t1500-rev-parse.sh |  5 +++++
 2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 43693454d5..7ced82e25d 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -695,6 +695,17 @@ static void print_path(const char *path, const char *prefix,
 	strbuf_release(&sb);
 }
 
+/*
+ * The options taking their value as a separate argument, which the scan
+ * looking for "--" below has to skip along with their value.
+ */
+static const struct early_scan_option rev_parse_early_options[] = {
+	EARLY_SCAN_SKIP_VALUE("default"),
+	EARLY_SCAN_SKIP_VALUE("prefix"),
+	EARLY_SCAN_SKIP_VALUE("resolve-git-dir"),
+	EARLY_SCAN_END()
+};
+
 int cmd_rev_parse(int argc,
 		  const char **argv,
 		  const char *prefix,
@@ -724,12 +735,15 @@ int cmd_rev_parse(int argc,
 	if (argc > 1 && !strcmp("-h", argv[1]))
 		usage(builtin_rev_parse_usage);
 
-	for (i = 1; i < argc; i++) {
-		if (!strcmp(argv[i], "--")) {
-			has_dashdash = 1;
-			break;
-		}
-	}
+	/*
+	 * The scan below has to know about the options taking their value
+	 * as a separate argument, or such a value that happens to be "--"
+	 * would be mistaken for the "--" separating revisions from paths.
+	 */
+	i = early_scan_options(argc - 1, argv + 1, rev_parse_early_options,
+			       EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);
+	if (i < argc - 1)
+		has_dashdash = 1;
 
 	/* No options; just report on whether we're in a git repo or not. */
 	if (argc == 1) {
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 4174ca40c3..897e9a7735 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -383,4 +383,9 @@ test_expect_success ':/ and HEAD^{/} favor more recent matching commits' '
 	)
 '
 
+test_expect_success 'rev-parse with "--" as an option value' '
+	test_must_fail git rev-parse --default -- notarev 2>err &&
+	test_grep "ambiguous argument .notarev." err
+'
+
 test_done
-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 4/6] parse-options: add parse_options_takes_argument()

[Christian Couder]

Whether an option takes a value, and therefore consumes the next
argument when that value is not stuck to it with an '=', is decided by
its type and its flags. That rule is currently open-coded in
show_gitcomp(), which needs it to decide if it should append an '=' to
the option it completes.

A following commit will need the same rule to find out which options an
early scan of the command line has to skip along with their value.

So let's factor that rule out into a new parse_options_takes_argument()
function, and let's use it in show_gitcomp().

Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the
next argument when it isn't the last one, so it is not considered as
taking a value, which is what show_gitcomp() already did.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 parse-options.c | 35 ++++++++++++++++++++++-------------
 parse-options.h | 10 ++++++++++
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index b3d19446cd..70851a385b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -841,6 +841,26 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
 	}
 }
 
+int parse_options_takes_argument(const struct option *opt)
+{
+	switch (opt->type) {
+	case OPTION_STRING:
+	case OPTION_FILENAME:
+	case OPTION_INTEGER:
+	case OPTION_UNSIGNED:
+	case OPTION_CALLBACK:
+		break;
+	default:
+		return 0;
+	}
+
+	if (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG |
+			  PARSE_OPT_LASTARG_DEFAULT))
+		return 0;
+
+	return 1;
+}
+
 static int show_gitcomp(const struct option *opts, int show_all)
 {
 	const struct option *original_opts = opts;
@@ -862,20 +882,9 @@ static int show_gitcomp(const struct option *opts, int show_all)
 			break;
 		case OPTION_GROUP:
 			continue;
-		case OPTION_STRING:
-		case OPTION_FILENAME:
-		case OPTION_INTEGER:
-		case OPTION_UNSIGNED:
-		case OPTION_CALLBACK:
-			if (opts->flags & PARSE_OPT_NOARG)
-				break;
-			if (opts->flags & PARSE_OPT_OPTARG)
-				break;
-			if (opts->flags & PARSE_OPT_LASTARG_DEFAULT)
-				break;
-			suffix = "=";
-			break;
 		default:
+			if (parse_options_takes_argument(opts))
+				suffix = "=";
 			break;
 		}
 		if (opts->flags & PARSE_OPT_COMP_ARG)
diff --git a/parse-options.h b/parse-options.h
index abc73d8399..b96e93508e 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -420,6 +420,16 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  const char * const usagestr[],
 		  enum parse_opt_flags flags);
 
+/*
+ * Return non-zero if `opt` takes a value, which means that it consumes
+ * the next argument when that value is not stuck to it with an '='.
+ *
+ * Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the
+ * next argument when it isn't the last one, so it is not considered as
+ * taking a value here.
+ */
+int parse_options_takes_argument(const struct option *opt);
+
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 5/6] parse-options: build early scan options from a struct option array

[Christian Couder]

A command that scans its arguments early has to know which options take
a value, so that it can skip that value instead of mistaking it for an
option. When it also parses its options with the parse-options API, that
information is already available in its `struct option` array, and
duplicating it by hand in a `struct early_scan_option` array is both
tedious and easy to get out of sync when an option is added.

So let's add early_scan_options_from_options() to build the latter array
from the former, using parse_options_takes_argument() to find out which
options take a value. Its caller only has to name the options it wants
to be reported.

Note: This early scanner translation intentionally leaves out a few
complex option types to keep the scan simple and fast:

- Short options are ignored: early_scan_options_from_options()
  explicitly skips options without a `long_name`, and the scanner only
  looks for `--`. Properly handling short options would require parsing
  bundled flags (e.g., `-abc value`), which requires replicating the
  full parse_options() state machine.

- Conditional values: Options with `PARSE_OPT_LASTARG_DEFAULT` or
  `PARSE_OPT_OPTARG` are treated as not taking a separate argument.
  Because the scanner does not evaluate context (like whether an
  argument is the final one in `argv`), it must err on the side of
  caution to avoid accidentally consuming the `--` separator or a path.

- Abbreviated options remain unrecognized: Even though the scanner is
  now provided with the full option array, the underlying
  early_scan_options() engine still relies on exact string matches.
  Safely resolving abbreviations would require duplicating the
  ambiguity-checking logic from the main parser.

- Negated options are not automatically derived: The scanner strictly
  matches the defined long name. It does not automatically recognize
  the `--no-<name>` variants of boolean options. (This is harmless in
  practice for current callers, as negated options do not take values
  to skip, and boolean defaults align with the ignored state).

The above shortcomings can be addressed later, for example, when
commands that use short options or options with conditional values need
an early scan or are ported to use `struct option`.

Despite these limitations, this abstraction is a significant
improvement. It allows commands like `fast-import` to reuse their
existing `struct option` array for early scanning, ensuring the scanner
and the main parser agree on which options take arguments, and
preventing developers from having to maintain a separate, hardcoded
list that could drift out of sync.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 parse-options.c               | 39 +++++++++++++++++++++++++++++++++++
 parse-options.h               | 22 ++++++++++++++++++++
 t/helper/test-parse-options.c | 32 ++++++++++++++++++++++++++++
 t/helper/test-tool.c          |  1 +
 t/helper/test-tool.h          |  1 +
 t/t0040-parse-options.sh      | 26 +++++++++++++++++++++++
 6 files changed, 121 insertions(+)

diff --git a/parse-options.c b/parse-options.c
index 70851a385b..6cdc9c64cc 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1323,6 +1323,45 @@ int early_scan_options(int argc, const char **argv,
 	return argc;
 }
 
+struct early_scan_option *
+early_scan_options_from_options(const struct option *options,
+				const char **wanted)
+{
+	struct early_scan_option *early;
+	size_t nr = 0;
+
+	for (const struct option *opt = options; opt->type != OPTION_END; opt++)
+		if (opt->long_name)
+			nr++;
+
+	CALLOC_ARRAY(early, nr + 1);
+
+	nr = 0;
+	for (const struct option *opt = options; opt->type != OPTION_END; opt++) {
+		if (!opt->long_name)
+			continue;
+		early[nr].name = opt->long_name;
+		early[nr].takes_value = !!parse_options_takes_argument(opt);
+		nr++;
+	}
+
+	for (; wanted && *wanted; wanted++) {
+		size_t i;
+
+		for (i = 0; i < nr; i++) {
+			if (strcmp(early[i].name, *wanted))
+				continue;
+			early[i].wanted = 1;
+			break;
+		}
+		if (i == nr)
+			BUG("wanted option '%s' is not in the options array",
+			    *wanted);
+	}
+
+	return early;
+}
+
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
diff --git a/parse-options.h b/parse-options.h
index b96e93508e..fb81f2ed38 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -561,6 +561,28 @@ int early_scan_options(int argc, const char **argv,
 		       enum early_scan_flags flags,
 		       early_scan_fn *fn, void *data);
 
+/*
+ * Build the `struct early_scan_option` array to pass to
+ * early_scan_options() from the `options` array that the actual option
+ * parsing uses, so that both agree on which options take a value.
+ *
+ * Note some intentional limitations to keep the scan simple and fast:
+ * short options are ignored, options with PARSE_OPT_LASTARG_DEFAULT or
+ * PARSE_OPT_OPTARG are treated as not taking a separate value, negated
+ * options ("--no-...") are not automatically generated, and abbreviated
+ * options will not be matched.
+ *
+ * The options named in the NULL terminated `wanted` array get their
+ * `wanted` bit set, the other ones are only there to be skipped along
+ * with their value. It is a BUG() for a name in `wanted` not to appear
+ * in `options`.
+ *
+ * The returned array is allocated and should be free()d by the caller.
+ */
+struct early_scan_option *
+early_scan_options_from_options(const struct option *options,
+				const char **wanted);
+
 /*----- incremental advanced APIs -----*/
 
 struct parse_opt_cmdmode_list;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 96ab941d29..0187a25ccb 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -422,3 +422,35 @@ int cmd__early_scan_options(int argc, const char **argv)
 
 	return 0;
 }
+
+int cmd__early_scan_from_options(int argc, const char **argv)
+{
+	int an_int = 0, a_bool = 0;
+	char *a_string = NULL;
+	const struct option options[] = {
+		OPT_STRING(0, "string", &a_string, "str", "get a string"),
+		OPT_INTEGER(0, "int", &an_int, "get an integer"),
+		OPT_BOOL(0, "bool", &a_bool, "get a boolean"),
+		OPT_STRING_F(0, "optarg", &a_string, "str",
+			     "string with an optional value",
+			     PARSE_OPT_OPTARG),
+		OPT_END()
+	};
+	static const char *wanted[] = { "bool", NULL };
+	struct early_scan_option *early;
+	int stopped;
+
+	early = early_scan_options_from_options(options, wanted);
+
+	for (const struct early_scan_option *o = early; o->name; o++)
+		printf("option: %s takes_value: %d wanted: %d\n",
+		       o->name, o->takes_value, o->wanted);
+
+	stopped = early_scan_options(argc - 1, argv + 1, early, 0,
+				     show_early_option, NULL);
+	printf("stopped at: %d of %d\n", stopped, argc - 1);
+
+	free(early);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 5d2f5877d9..f1b208a5af 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -51,6 +51,7 @@ static struct test_cmd cmds[] = {
 	{ "parse-options", cmd__parse_options },
 	{ "parse-options-flags", cmd__parse_options_flags },
 	{ "early-scan-options", cmd__early_scan_options },
+	{ "early-scan-from-options", cmd__early_scan_from_options },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
 	{ "partial-clone", cmd__partial_clone },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 071306d52d..97334ce3c6 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -44,6 +44,7 @@ int cmd__pack_mtimes(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
 int cmd__parse_options_flags(int argc, const char **argv);
 int cmd__early_scan_options(int argc, const char **argv);
+int cmd__early_scan_from_options(int argc, const char **argv);
 int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__parse_subcommand(int argc, const char **argv);
 int cmd__partial_clone(int argc, const char **argv);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index d760d8cfbd..bb72a6544d 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -922,4 +922,30 @@ test_expect_success 'early_scan_options() ignores abbreviated options' '
 	test_cmp expect actual
 '
 
+test_expect_success 'early_scan_options_from_options() derives takes_value' '
+	test-tool early-scan-from-options >actual &&
+	cat >expect <<-\EOF &&
+	option: string takes_value: 1 wanted: 0
+	option: int takes_value: 1 wanted: 0
+	option: bool takes_value: 0 wanted: 1
+	option: optarg takes_value: 0 wanted: 0
+	stopped at: 0 of 0
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options_from_options() skips values' '
+	test-tool early-scan-from-options --string --bool >out &&
+	tail -1 out >actual &&
+	echo "stopped at: 2 of 2" >expect &&
+	test_cmp expect actual &&
+	test-tool early-scan-from-options --string v --bool >out &&
+	tail -2 out >actual &&
+	cat >expect <<-\EOF &&
+	found: bool at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.55.0.787.g3f9e2241eb.dirty

[PATCH 6/6] fast-import: use early_scan_options() for --allow-unsafe-features

[Christian Couder]

The "feature" lines at the start of the stream are processed before the
command line options are parsed, so cmd_fast_import() scans its
arguments early to find out if `--allow-unsafe-features` was given.

That scan doesn't know which options take their value as a separate
argument, and it stops at the first argument that doesn't start with a
dash. So it disagrees with parse_options(), which accepts values
separated from their option by a space, for a command line like
"--depth 5 --allow-unsafe-features": the scan stops at "5" and never
sees the option, so unsafe "feature" commands from the stream are
refused even though the option was given.

Let's fix this by building the options for the scan from the same
`struct option` array that parse_options() uses, so that both agree on
which options take a value.

Note that the scan still only matches the exact option spelling, while
parse_options() also accepts unambiguous abbreviations, so the two still
disagree for a command line like "--allow-unsafe". This errs on the safe
side, and is now documented as a restriction.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 Documentation/git-fast-import.adoc | 10 +++----
 builtin/fast-import.c              | 46 +++++++++++++++++++-----------
 t/t9300-fast-import.sh             | 14 +++++++++
 3 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
index fd165e11d2..9758ba5275 100644
--- a/Documentation/git-fast-import.adoc
+++ b/Documentation/git-fast-import.adoc
@@ -66,12 +66,10 @@ fast-import stream! This option is enabled automatically for
 remote-helpers that use the `import` capability, as they are
 already trusted to run their own code.
 +
-Note that this option has to be spelled in full, and has to appear
-before any option whose value is separated from it by a space, for
-the unsafe `feature` commands in the stream to be allowed. So
-`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse
-them, while `--allow-unsafe-features --depth 5` and
-`--depth=5 --allow-unsafe-features` allow them.
+Note that this option has to be spelled in full for the unsafe
+`feature` commands in the stream to be allowed. So while
+`--allow-unsafe` is accepted as an unambiguous abbreviation of this
+option, it still refuses them.
 
 `--signed-tags=<mode>`::
 	Specify how to handle signed tags. Behaves in the same way as
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index fbd919982c..cf0504f01c 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -4120,12 +4120,29 @@ static int option_parse_quiet(const struct option *opt UNUSED,
 	return 0;
 }
 
+/*
+ * The only option the early scan below is interested in, as it decides
+ * whether unsafe "feature" commands from the stream are allowed.
+ */
+static const char *early_wanted[] = { "allow-unsafe-features", NULL };
+
+static int option_parse_early_allow_unsafe(
+		const struct early_scan_option *opt UNUSED,
+		const char *value UNUSED, int pos UNUSED, void *data)
+{
+	struct fast_import_state *state = data;
+
+	state->allow_unsafe_features = 1;
+	return 0;
+}
+
 int cmd_fast_import(int argc,
 		    const char **argv,
 		    const char *prefix,
 		    struct repository *repo)
 {
 	struct fast_import_state state;
+	struct early_scan_option *early;
 
 	struct option fast_import_options[] = {
 		OPT_GROUP(N_("Common")),
@@ -4218,23 +4235,20 @@ int cmd_fast_import(int argc,
 	 * line to override stream data). But we must do an early parse of any
 	 * command-line options that impact how we interpret the feature lines.
 	 *
-	 * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features"
-	 * spelling and stops at the first argument that doesn't start with a
-	 * dash. As parse_options() below also accepts unambiguous abbreviations
-	 * and values separated by a space from their option, the two disagree
-	 * for command lines like "--allow-unsafe" or "--depth 5
-	 * --allow-unsafe-features": parse_options() accepts the option, but
-	 * this scan doesn't see it, so unsafe features from the stream are
-	 * still refused. This errs on the safe side, but should be fixed by
-	 * teaching this scan about the options that take a value.
+	 * NEEDSWORK: This scan only matches the exact
+	 * "--allow-unsafe-features" spelling, while parse_options() below
+	 * also accepts unambiguous abbreviations, so the two disagree for
+	 * a command line like "--allow-unsafe": parse_options() accepts
+	 * the option, but this scan doesn't see it, so unsafe features
+	 * from the stream are still refused. This errs on the safe side.
 	 */
-	for (int i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (*arg != '-' || !strcmp(arg, "--"))
-			break;
-		if (!strcmp(arg, "--allow-unsafe-features"))
-			state.allow_unsafe_features = 1;
-	}
+	early = early_scan_options_from_options(fast_import_options,
+						early_wanted);
+	early_scan_options(argc - 1, argv + 1, early,
+			   EARLY_SCAN_STOP_AT_DASHDASH |
+			   EARLY_SCAN_STOP_AT_NON_OPTION,
+			   option_parse_early_allow_unsafe, &state);
+	free(early);
 
 	rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
 	for (unsigned int i = 0; i < (cmd_save - 1); i++)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index d9de2ef0d8..1a37f2b8e6 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2344,6 +2344,20 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
 	test_path_is_missing feature-sub
 '
 
+test_expect_success 'R: --allow-unsafe-features found after a value' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	git fast-import --allow-unsafe-features <input &&
+	git fast-import --depth=5 --allow-unsafe-features <input &&
+	git fast-import --depth 5 --allow-unsafe-features <input &&
+	git fast-import --date-format raw --allow-unsafe-features <input
+'
+
+test_expect_success 'R: --allow-unsafe-features has to be spelled in full' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	test_must_fail git fast-import --allow-unsafe <input 2>err &&
+	test_grep "forbidden in input without --allow-unsafe-features" err
+'
+
 test_expect_success 'R: catch typo in marks file name' '
 	test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
 	echo "feature import-marks=nonexistent.marks" |
-- 
2.55.0.787.g3f9e2241eb.dirty

Re: [PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> A number of commands perform an early scan of their arguments to look
> for specific flags or structural separators (like `--`).
>
> These hand-rolled early scans are often fragile. They especially fail
> to account for options that take their value as a separate
> argument. This leads to disagreements between the early scan and the
> actual parse_options() pass. For example, the early scanner might miss
> a special option entirely, or mistakenly treat an option's value as
> the `--` path separator.
>
> To allow these commands to safely skip option values during their
> early scans, this series introduces a new "early-scan" sub-API into
> the existing "parse-options" API.

Yay.

> This is deliberately implemented as a new simple and fast scan, which
> has some limitations, instead of a full refactor and reuse of the
> parse_options() code,

Sigh.  In other words, we hate these ad-hoc prescan that are buggy
badly enough to replace them all with yet another ad-hoc prescan
that is know to behave differently from the real thing?

>  - `git bisect start --term-good -- <not-a-rev>` mistook the term name
>    `--` for the revision/path separator, so <not-a-rev> was rejected
>    as an invalid revision instead of being treated as a path.

Sorry, I fail to see much practical value in this.

>  - `git rev-parse --default -- <not-a-rev>` did the same, reporting
>    "bad revision <notarev>" while any other default value gives the
>    usual more helpful "ambiguous argument" error.

Neither in this one.

>  - `git fast-import --depth 5 --allow-unsafe-features` silently
>    ignored `--allow-unsafe-features`, refusing unsafe features from
>    the stream.

On the other hand, this may be a very good thing.

Is the reason why the ad-hoc pre-scan failed to see it was because
it did not realize 5 is a value to the --depth option?

> All of these commands call parse_options(), but for `git bisect` and
> `git rev-parse`, the specific functions doing the early scan
> (bisect_start() and cmd_rev_parse()'s main loop) parse their own
> options by hand after the early scan and have no `struct option` array
> for those options.
>
> If bisect_start() and cmd_rev_parse() were converted to use
> `struct option`, they could use early_scan_options_from_options() and
> would not be affected by limitations 1), 2) and 3) above, as both use
> the early scan only to locate `--`.

I imagine that in the long term we would rather see a properly
refactored parse-options machinery perform the prescan (perhaps with
some kind of "dry-run" option given to the machinery) than yet
another ad-hoc parser like this topic introduces.  It would be very
good if this interim solution at least took the same 'options[]'
array so that when we have the real thing in the future we do not
have to redo the conversion effort.

By the way, how does this interact with your other topic that has
been stalled for quite some time?  Would moving this one forward
help the other, or do they not have much relevance to each other?  I
would rather not see two topics of non-trivial size stalled on a
single author at the same time, so ...

Thanks.

Re: [PATCH 1/6] parse-options: add early_scan_options()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> So users must spell these specific options in full. This restriction
> could be lifted in the future though, once the scanner is adapted to
> accept a command's full option array, as this would give it the
> complete context needed for safe abbreviation matching.

It is unfortunate that end-users cannot tell if they are dealing
with a system before of after "once the scanner is adapted"
happened, so they must be trained to always spell the options in
full to make use of the commands that use this feature.  It at least
does not regress relative to the ad-hoc early scanners these selected
commands have that do not even understand what they are parsing, so
it may not be too bad.

Stepping back a bit, the burden on programmers to use this would be
to write in a separate notation what options there are in addition
to what they feed the real parse_options(), which cuts both ways in
the sense that because this does not take parse_options(), commands
that do not use parse_options() can still use it, but those that do
already use parse_options() need additional work to use eary_scan.

And then once the scanner is adapted to accept the full option array,
the programmers only need to discard the struct early_scan_option[]
they wrote and replace it with the struct option[] they already have?
Or would the calling convention to the scanner also change when it
happens (oother than replacing the pointer to struct early_scan_option[]
with another pointer to struct option[])?

> +static const struct early_scan_option *
> +find_early_scan_option(const char *arg,
> +		       const struct early_scan_option *options,
> +		       const char **value)

Because you return one single element from the incoming array of
options, it is mildly misleading to call the variable/parameter
"options" here and everywhere else.  Let's stick to "arrays are 
named singular, so that option[4] names 4th option" convention.

> +{
> +	if (!skip_prefix(arg, "--", &arg))
> +		return NULL;
> +
> +	for (; options->name; options++) {
> +		const char *rest;
> +
> +		if (!skip_prefix(arg, options->name, &rest))
> +			continue;

"--option" on the command line, after getting stripped the leading
"--", may begin with "option", and that name may be in the option[]
table, in which case ...

> +		if (!*rest) {
> +			*value = NULL;
> +			return options;
> +		}

... we found a hit.  But shouldn't option->takes_value be consulted
before we return to signal the caller that the next arg is an option
value before we return from here?  It looks a bit uneven as we do
that for stuck form "--option=value" here.

> +		/* Only an option taking a value can be stuck to one. */
> +		if (*rest == '=' && options->takes_value) {
> +			*value = rest + 1;
> +			return options;
> +		}

And if the option[] table had "opt", then "--option" on the command
line may begin with "--opt" but "ion" is an excess that is not a
stuck value, so we do not consider it as a match.  OK.

> +	}
> +	return NULL;
> +}

If we are to write a separate function anyway, I wonder how much
more work to write a early_scan_option() parser that does take a
real "struct option[]" array.  Its elements already know if they
take a value or not.  For expediency, it may be OK to start by
simplified parser that does not handle unique prefix and other
complexities like callback functions of the real parser, but at
least it would reduce the burden on the programmers quite a bit if
we used the real struct option[] array, I suspect.

Re: [PATCH 2/6] bisect: fix "--" detection when a term name is "--"

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> `bisect_start()` walks its arguments twice. The second loop actually
> parses the options, and it knows that `--term-good`, `--term-old`,
> `--term-bad` and `--term-new` take their value as a separate argument,
> so it skips that value.
>
> The first loop, which only looks for the "--" separating revisions from
> paths, doesn't know about these options. So when such an option is given
> "--" as its value, that "--" is mistaken for the separator and
> `has_double_dash` is wrongly set.

It may be theoretically true, but I wonder how much practical value
it has to correctly parse "--term-good --" as "Ah, the user wants to
mark good revisions as '--' instead of 'good' or 'old'"?  Even
though "refs/bisect/--" is *not* forbidden, how likely is it for
users to do that?

This is not like "git grep -e --" which does have much more pracical
value.



>  builtin/bisect.c            | 27 +++++++++++++++++++++------
>  t/t6030-bisect-porcelain.sh |  8 ++++++++
>  2 files changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 1cfb8a794b..ad089b289f 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -803,6 +803,19 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
>  	return bisect_next(terms, prefix);
>  }
>  
> +/*
> + * The options "git bisect start" accepts. Only the ones taking their
> + * value as a separate argument matter to the scan looking for "--" below,
> + * as their value has to be skipped along with them.
> + */
> +static const struct early_scan_option bisect_start_early_options[] = {
> +	EARLY_SCAN_SKIP_VALUE("term-good"),
> +	EARLY_SCAN_SKIP_VALUE("term-old"),
> +	EARLY_SCAN_SKIP_VALUE("term-bad"),
> +	EARLY_SCAN_SKIP_VALUE("term-new"),
> +	EARLY_SCAN_END()
> +};
> +
>  static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
>  				      const char **argv)
>  {
> @@ -825,13 +838,15 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
>  
>  	/*
>  	 * Check for one bad and then some good revisions
> +	 *
> +	 * The scan below has to know about the options taking their value
> +	 * as a separate argument, or such a value that happens to be "--"
> +	 * would be mistaken for the "--" separating revisions from paths.
>  	 */
> -	for (i = 0; i < argc; i++) {
> -		if (!strcmp(argv[i], "--")) {
> -			has_double_dash = 1;
> -			break;
> -		}
> -	}
> +	i = early_scan_options(argc, argv, bisect_start_early_options,
> +			       EARLY_SCAN_STOP_AT_DASHDASH, NULL, NULL);
> +	if (i < argc)
> +		has_double_dash = 1;
>  
>  	for (i = 0; i < argc; i++) {
>  		const char *arg = argv[i];
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index a7588222a8..464ca53b42 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -1297,6 +1297,14 @@ test_expect_success 'bisect start takes options and revs in any order' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'bisect start with "--" as a term name' '
> +	git bisect reset &&
> +	git bisect start --term-good -- hello &&
> +	git bisect terms --term-good >actual &&
> +	echo -- >expected &&
> +	test_cmp expected actual
> +'
> +
>  # Bisect is started with --term-new and --term-old arguments,
>  # then skip. The HEAD should be changed.
>  test_expect_success 'bisect skip works with --term*' '

Re: [PATCH 6/6] fast-import: use early_scan_options() for --allow-unsafe-features

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> The "feature" lines at the start of the stream are processed before the
> command line options are parsed, so cmd_fast_import() scans its
> arguments early to find out if `--allow-unsafe-features` was given.
>
> That scan doesn't know which options take their value as a separate
> argument, and it stops at the first argument that doesn't start with a
> dash. So it disagrees with parse_options(), which accepts values
> separated from their option by a space, for a command line like
> "--depth 5 --allow-unsafe-features": the scan stops at "5" and never
> sees the option, so unsafe "feature" commands from the stream are
> refused even though the option was given.

Well explained.

> @@ -4218,23 +4235,20 @@ int cmd_fast_import(int argc,
>  	 * line to override stream data). But we must do an early parse of any
>  	 * command-line options that impact how we interpret the feature lines.
>  	 *
> +	 * NEEDSWORK: This scan only matches the exact
> +	 * "--allow-unsafe-features" spelling, while parse_options() below
> +	 * also accepts unambiguous abbreviations, so the two disagree for
> +	 * a command line like "--allow-unsafe": parse_options() accepts
> +	 * the option, but this scan doesn't see it, so unsafe features
> +	 * from the stream are still refused. This errs on the safe side.
>  	 */
> -	for (int i = 1; i < argc; i++) {
> -		const char *arg = argv[i];
> -		if (*arg != '-' || !strcmp(arg, "--"))
> -			break;
> -		if (!strcmp(arg, "--allow-unsafe-features"))
> -			state.allow_unsafe_features = 1;
> -	}

This is the ad-hoc one that does not know --depth takes a value
after it.

> +	early = early_scan_options_from_options(fast_import_options,
> +						early_wanted);
> +	early_scan_options(argc - 1, argv + 1, early,
> +			   EARLY_SCAN_STOP_AT_DASHDASH |
> +			   EARLY_SCAN_STOP_AT_NON_OPTION,
> +			   option_parse_early_allow_unsafe, &state);
> +	free(early);

Interesting.  This one now "knows" enough to skip what comes after
"--depth" that takes an option ;-)  And it is perfectly fine if we
skip over "--depth hello" to find "--allow-unsafe", as such a "oops
we require number but hello is not a number" will be caught by the
real parser anyway.

Nicely done.

[PATCH v2 0/3] Standardize early option scanning

[Christian Couder]

A number of commands perform an early scan of their arguments to look
for specific flags or structural separators (like `--`).

These hand-rolled early scans are often fragile. They especially fail
to account for options that take their value as a separate
argument. This leads to disagreements between the early scan and the
actual parse_options() pass. For example, the early scanner might miss
a special option entirely, or mistakenly treat an option's value as
the `--` path separator.

To allow these commands to safely skip option values during their
early scans, this series introduces a new "early-scan" sub-API into
the existing "parse-options" API.

This is deliberately implemented as a new simple and fast scan, which
has some limitations, instead of a full refactor and reuse of the
parse_options() code, because the limitations are not very significant
in practice, while a full refactor and reuse of the parse_options()
code would be much more complex.

The current limitations of the new early scan code are:

 1. short options are ignored,

 2. an option with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT never
 has the following argument skipped as its value. This matches
 parse_options() for PARSE_OPT_OPTARG, but not for
 PARSE_OPT_LASTARG_DEFAULT, which consumes that argument when the
 option isn't the last one,

 3. negated options ("--no-...") are not matched,

 4. abbreviated options will not be matched,

 5. subcommands (OPTION_SUBCOMMAND) are never matched, so an option
 marked with PARSE_OPT_EARLY cannot be a subcommand,

 6. aliases (OPTION_ALIAS) are not resolved to the option they stand
 for, so the separate value of an alias of an option taking a value
 is not skipped.

Note that while the others could be real issues for some commands,
"3. negated options" and "5. subcommands" are not practical issues
because negated options never consume a separate argument, and
parse_options() doesn't match subcommands as "--<name>" either.

The early scan is performed by a new early_scan_options() function
which takes a regular `const struct option *options` array as
argument.

This requires that the command already uses `struct option` and the
parse-options API to parse its arguments. As the majority of commands
performing an early scan don't use the parse-options API yet, they
will have to be converted to use it before they can use
early_scan_options().

In this series, only `git fast-import` is converted to the early-scan
API, which fixes a bug as:

  `git fast-import --depth 5 --allow-unsafe-features`

silently ignored `--allow-unsafe-features`, refusing unsafe features
from the stream.

Overview of the patches
=======================

 - Patch 1/3 refactors some existing code into a new
   parse_options_takes_argument() helper that will be used in the next
   patch.

 - Patch 2/3 introduces early_scan_options(), the early scanner that
   will be used instead of hand-rolled ones, along with its
   infrastructure.

 - Patch 3/3 uses early_scan_options() to fix the early scan for
   `--allow-unsafe-features` in `git fast-import`.

Changes since v1
================

Thanks to Junio who reviewed v1.

There are a lot of important changes since v1:

 - Now the early-scan API requires the parse-options API to be already
   used, and early_scan_options() accepts a regular `struct option *`
   instead of a dedicated `struct early_scan_option *`.

   (In v1, early_scan_options() took a dedicated
   `struct early_scan_option *` array, which the caller either wrote by
   hand, when it didn't use the parse-options API, or derived from its
   `struct option *` array with early_scan_options_from_options().)

   This simplifies things significantly, but requires that code doing
   an early scan be ported to the parse-options API if it doesn't use
   it yet.

 - bisect_start() and cmd_rev_parse() are not converted anymore to the
   new early-scan API. Converting them didn't bring much value, and
   they can still be converted in the future after they are converted
   to the parse-options API.

 - Options that should be looked up during the early scan are now
   marked with a new PARSE_OPT_EARLY flag (which is documented with
   the other per-option flags in
   "Documentation/technical/api-parse-options.adoc") in
   `struct option`.

   (In v1, a `wanted` flag in `struct early_scan_option` was used for
   this, and this flag could be set by passing a `const char **wanted`
   to early_scan_options_from_options().)

 - parse_options_check() now rejects PARSE_OPT_EARLY on an option
   without a long name, and on a subcommand, as the scan can never
   match either of them.

   (In v1, early_scan_options_from_options() raised a BUG() when a
   name in `wanted` was not in the option array.)

 - The EARLY_SCAN_STOP_AT_DASHDASH flag has been removed, and the scan
   now always stops at both `--` and `--end-of-options`, as
   parse_options() always stops parsing options at them whatever its
   flags. (PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only
   decide if the terminator is left in argv, not if it terminates.)

   (In v1, the scan walked past `--end-of-options`, so
   `git fast-import --end-of-options --allow-unsafe-features` allowed
   unsafe stream features before parse_options() rejected the command
   line.)

 - "--<name>=<value>" is now matched for any option that is not
   PARSE_OPT_NOARG, instead of only for options taking a separate
   value. Whether the next argument is consumed still depends on
   parse_options_takes_argument().

   (In v1, an option with PARSE_OPT_OPTARG or
   PARSE_OPT_LASTARG_DEFAULT was missed entirely in that form, while
   parse_options() accepts it.)

 - The different patches changed in the following way:

   - Patch 4/6 is now patch 1/3.

   - Patches 1/6 and 5/6 have been squashed and heavily modified to
     create patch 2/3.

   - Patches 2/6 and 3/6 have been removed as bisect_start() and
     cmd_rev_parse() are not converted anymore to the new early-scan
     API.

   - Patch 6/6 is now patch 3/3.

CI tests:
=========

They all pass, see:

https://github.com/chriscool/git/actions/runs/35749173266

Range-diff since v1
===================

4:  1ea80545c4 = 1:  32adff46ce parse-options: add parse_options_takes_argument()
1:  acb475f98d ! 2:  82569fa602 parse-options: add early_scan_options()
    @@ Commit message
         commands get this wrong, as they just walk their arguments comparing
         them to the few option names they care about.
     
    -    Let's add early_scan_options() to help with this. Its callers describe
    -    the options to look for, but also the ones that merely have to be
    -    skipped along with their value, so that the scan can walk the arguments
    -    without being fooled by option values.
    +    Let's add early_scan_options() to help with this. It walks the
    +    arguments using the very same `struct option` array that the command
    +    already passes to parse_options(), and uses the
    +    parse_options_takes_argument() helper added in a previous commit, so
    +    that the scan and the actual parsing agree on which options take a
    +    value.
     
    -    Note that abbreviated options are deliberately not recognized, as a
    -    scan cannot know about the options it hasn't been told about, and would
    -    then resolve abbreviations differently from the actual option parsing.
    +    The options the caller wants to be told about are marked with a new
    +    PARSE_OPT_EARLY flag, so that nothing has to be spelled out a second
    +    time, and so that the mark cannot drift away from the option it refers
    +    to.
     
    -    So users must spell these specific options in full. This restriction
    -    could be lifted in the future though, once the scanner is adapted to
    -    accept a command's full option array, as this would give it the
    -    complete context needed for safe abbreviation matching.
    +    Using a per-option flag for this is not new as that flag space already
    +    holds flags that the parsing loop itself ignores, like
    +    PARSE_OPT_NOCOMPLETE and PARSE_OPT_COMP_ARG, which only the completion
    +    helper looks at, or PARSE_OPT_HIDDEN and PARSE_OPT_LITERAL_ARGHELP,
    +    which only the usage output looks at.
    +
    +    The scan is deliberately kept much simpler than parse_options(),
    +    instead of teaching the latter to perform a side effect free "dry
    +    run". Such a dry run would have to avoid writing through `opt->value`,
    +    calling option callbacks, dying on an invalid value, handling `--help`
    +    and tracking command mode conflicts, so it would be a much larger
    +    refactoring. If parse_options() learns to do it in the future though,
    +    the commands converted now would keep both their option array and their
    +    PARSE_OPT_EARLY marks, so their conversion would not have to be redone.
    +
    +    One consequence of staying simple is that abbreviated options are
    +    still not matched, even though the scan is now given the command's
    +    full option array. Resolving them the way parse_options() does would
    +    mean duplicating the ambiguity detection that parse_long_opt()
    +    performs. So the scan can fail to see an option that parse_options()
    +    would accept, and its callers have to cope with that, typically by
    +    erring on the safe side. This and the other differences with
    +    parse_options() are documented in "parse-options.h".
    +
    +    In practice, despite these limitations, early scans using
    +    early_scan_options() should still be safer and cleaner than the
    +    ad-hoc hand-rolled scans they are meant to replace, which don't know
    +    about option values at all and therefore disagree with
    +    parse_options() in ways that create plain bugs.
     
         Signed-off-by: Christian Couder <christian.couder@gmail.com>
     
    + ## Documentation/technical/api-parse-options.adoc ##
    +@@ Documentation/technical/api-parse-options.adoc: are the bitwise-or of:
    + 	Internal flag, set on options that were expanded from a
    + 	configured alias. It should not be set by callers.
    + 
    ++`PARSE_OPT_EARLY`::
    ++	Report this option to `early_scan_options()`, which looks at a
    ++	few options before parsing the command line for real. Ignored
    ++	by `parse_options()` itself.
    ++
    + `PARSE_OPT_NOCOMPLETE`::
    + 	Do not offer this option for completion.
    + 
    +
      ## parse-options.c ##
    +@@ parse-options.c: static void parse_options_check(const struct option *opts)
    + 		     opts->long_name))
    + 			optbug(opts, "uses feature "
    + 			       "not supported for dashless options");
    ++		if ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name)
    ++			optbug(opts, "uses PARSE_OPT_EARLY, which needs a long name");
    + 		if (opts->type == OPTION_SET_INT && !opts->defval &&
    + 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
    + 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
    +@@ parse-options.c: static void parse_options_check(const struct option *opts)
    + 		case OPTION_SUBCOMMAND:
    + 			if (!opts->value || !opts->subcommand_fn)
    + 				optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function");
    ++			if (opts->flags & PARSE_OPT_EARLY)
    ++				optbug(opts, "OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY");
    + 			if (!subcommand_value)
    + 				subcommand_value = opts->value;
    + 			else if (subcommand_value != opts->value)
     @@ parse-options.c: int parse_options(int argc, const char **argv,
      	return parse_options_end(&ctx);
      }
      
     +/*
    -+ * Look for `arg` among `options`. On success, return the matching option
    ++ * Look for `arg` among `option`. On success, return the matching option
     + * and set `value` to the value stuck to it, if any, or to NULL.
     + */
    -+static const struct early_scan_option *
    -+find_early_scan_option(const char *arg,
    -+		       const struct early_scan_option *options,
    -+		       const char **value)
    ++static const struct option *find_early_scan_option(const char *arg,
    ++						   const struct option *option,
    ++						   const char **value)
     +{
     +	if (!skip_prefix(arg, "--", &arg))
     +		return NULL;
     +
    -+	for (; options->name; options++) {
    ++	for (const struct option *opt = option; opt->type != OPTION_END; opt++) {
     +		const char *rest;
     +
    -+		if (!skip_prefix(arg, options->name, &rest))
    ++		if (opt->type == OPTION_SUBCOMMAND)
    ++			continue;
    ++		if (!opt->long_name)
    ++			continue;
    ++		if (!skip_prefix(arg, opt->long_name, &rest))
     +			continue;
    ++
     +		if (!*rest) {
     +			*value = NULL;
    -+			return options;
    ++			return opt;
     +		}
    -+		/* Only an option taking a value can be stuck to one. */
    -+		if (*rest == '=' && options->takes_value) {
    ++		/* Only an option that can take a value may have one stuck to it. */
    ++		if (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {
     +			*value = rest + 1;
    -+			return options;
    ++			return opt;
     +		}
     +	}
     +
    @@ parse-options.c: int parse_options(int argc, const char **argv,
     +}
     +
     +int early_scan_options(int argc, const char **argv,
    -+		       const struct early_scan_option *options,
    ++		       const struct option *option,
     +		       enum early_scan_flags flags,
     +		       early_scan_fn *fn, void *data)
     +{
     +	for (int i = 0; i < argc; i++) {
     +		const char *arg = argv[i];
     +		const char *value;
    -+		const struct early_scan_option *opt;
    ++		const struct option *opt;
     +		int pos = i;
     +
    -+		if ((flags & EARLY_SCAN_STOP_AT_DASHDASH) &&
    -+		    !strcmp(arg, "--"))
    ++		/*
    ++		 * parse_options() always stops parsing options at these,
    ++		 * whatever its flags, so nothing after them is an option.
    ++		 */
    ++		if (!strcmp(arg, "--") || !strcmp(arg, "--end-of-options"))
     +			return i;
     +
    -+		opt = find_early_scan_option(arg, options, &value);
    ++		opt = find_early_scan_option(arg, option, &value);
     +		if (!opt) {
     +			if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&
     +			    (*arg != '-' || !arg[1]))
    @@ parse-options.c: int parse_options(int argc, const char **argv,
     +		 * value and it has to be skipped so that it isn't
     +		 * taken for an option itself.
     +		 */
    -+		if (opt->takes_value && !value && i + 1 < argc)
    ++		if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
     +			value = argv[++i];
     +
    -+		if (opt->wanted && fn(opt, value, pos, data))
    ++		if (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))
     +			return i;
     +	}
     +
    @@ parse-options.c: int parse_options(int argc, const char **argv,
      	const char *s;
     
      ## parse-options.h ##
    +@@ parse-options.h: enum parse_opt_option_flags {
    + 	PARSE_OPT_NODASH = 1 << 5,
    + 	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
    + 	PARSE_OPT_FROM_ALIAS = 1 << 7,
    ++	PARSE_OPT_EARLY = 1 << 8,	/* only for early_scan_options() */
    + 	PARSE_OPT_NOCOMPLETE = 1 << 9,
    + 	PARSE_OPT_COMP_ARG = 1 << 10,
    + 	PARSE_OPT_CMDMODE = 1 << 11,
     @@ parse-options.h: static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
      		BUG("option callback expects an argument"); \
      } while(0)
    @@ parse-options.h: static inline void die_for_incompatible_opt2(int opt1, const ch
     + * whether a repository is needed at all.
     + *
     + * Such an early scan has to know which options take their value as a
    -+ * separate argument, or it could mistake such a value for an option. The
    -+ * `struct early_scan_option` array passed to early_scan_options() below
    -+ * describes the options to look for, as well as the ones that only need
    -+ * to be skipped along with their value.
    ++ * separate argument, or it could mistake such a value for an
    ++ * option. The functions below allow performing such early scans
    ++ * without being fooled by option values.
     + */
    -+struct early_scan_option {
    -+	const char *name; 	/* Option name, without the leading dashes */
    -+	unsigned takes_value:1; /* "--option=value" or "--option value" expected? */
    -+	unsigned wanted:1;      /* Report option to callback? */
    -+};
    -+
    -+#define EARLY_SCAN_SKIP_VALUE(n) { .name = (n), .takes_value = 1 }
    -+#define EARLY_SCAN_WANT(n) { .name = (n), .wanted = 1 }
    -+#define EARLY_SCAN_WANT_VALUE(n) { .name = (n), .takes_value = 1, .wanted = 1 }
    -+#define EARLY_SCAN_END() { NULL }
     +
     +/*
     + * Called by early_scan_options() for each argument matching a
    -+ * `struct early_scan_option` that has its `wanted` bit set.
    ++ * `struct option` with PARSE_OPT_EARLY set.
     + *
     + * `option` is the matching option, `value` its value or NULL if it
     + * doesn't take one, and `pos` the index of the option in argv.
     + *
     + * Returning a non-zero value stops the scan.
     + */
    -+typedef int early_scan_fn(const struct early_scan_option *option,
    -+			  const char *value, int pos, void *data);
    ++typedef int early_scan_fn(const struct option *option, const char *value,
    ++			  int pos, void *data);
     +
     +enum early_scan_flags {
    -+	EARLY_SCAN_STOP_AT_DASHDASH = 1 << 0, /* Stop at "--" */
    -+	EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 1,
    ++	EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 0, /* Stop at any non option */
     +};
     +
     +/*
    -+ * Scan `argv` for the options described by `options`, calling `fn`
    -+ * for each of those that are `wanted`. `argv` is not modified.
    ++ * Scan `argv` for the options described by `option`, calling `fn` for
    ++ * each of those that have PARSE_OPT_EARLY set. `argv` is not
    ++ * modified.
     + *
    -+ * `fn` may be NULL when no option is `wanted`, which is useful to only
    -+ * find out where the scan stops.
    ++ * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is
    ++ * useful to only find out where the scan stops.
     + *
    -+ * Note that abbreviated options are not recognized, as a scan cannot
    -+ * know about the options it hasn't been told about, and would then
    -+ * resolve abbreviations differently from the actual option parsing.
    ++ * The scan always stops at "--" and at "--end-of-options", as
    ++ * parse_options() always stops parsing options there too, whatever its
    ++ * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only
    ++ * decide if the terminator is left in argv, not if it terminates.
     + *
     + * Returns the index at which the scan stopped, which is `argc` when the
     + * whole array was scanned.
    ++ *
    ++ * This scan is for now deliberately much simpler than
    ++ * parse_options(), so it differs from it in the following ways:
    ++ *
    ++ *  - Only the long form of an option is matched, and it has to be
    ++ *    spelled in full: short options and abbreviations are ignored.
    ++ *
    ++ *  - Negated forms ("--no-<name>") are not matched. This is harmless,
    ++ *    as they never take a value to skip.
    ++ *
    ++ *  - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are
    ++ *    treated as not taking a separate value.
    ++ *
    ++ *  - OPTION_SUBCOMMAND entries are skipped.
    ++ *
    ++ *  - OPTION_ALIAS entries are not resolved to the option they stand
    ++ *    for.
    ++ *
    ++ * So the scan can fail to see an option that parse_options() would
    ++ * accept, and callers have to cope with that, typically by erring on
    ++ * the safe side.
     + */
     +int early_scan_options(int argc, const char **argv,
    -+		       const struct early_scan_option *options,
    ++		       const struct option *option,
     +		       enum early_scan_flags flags,
     +		       early_scan_fn *fn, void *data);
     +
    @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **
      	return parse_subcommand__cmd(argc, argv, test_flags);
      }
     +
    -+static int show_early_option(const struct early_scan_option *opt,
    -+			     const char *value, int pos, void *data UNUSED)
    ++static int show_early_option(const struct option *opt, const char *value,
    ++			     int pos, void *data UNUSED)
     +{
    -+	printf("found: %s at %d", opt->name, pos);
    ++	printf("found: %s at %d", opt->long_name, pos);
     +	if (value)
     +		printf(" value: %s", value);
     +	putchar('\n');
    @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **
     +
     +int cmd__early_scan_options(int argc, const char **argv)
     +{
    -+	static const struct early_scan_option options[] = {
    -+		EARLY_SCAN_WANT("wanted"),
    -+		EARLY_SCAN_WANT_VALUE("wanted-value"),
    -+		EARLY_SCAN_SKIP_VALUE("skipped-value"),
    -+		EARLY_SCAN_END()
    ++	char *a_string = NULL;
    ++	int an_int = 0, a_bool = 0, a_short = 0;
    ++
    ++	const struct option option[] = {
    ++		OPT_GROUP("early scan test options"),
    ++		OPT_BOOL_F(0, "wanted", &a_bool,
    ++			   "wanted option taking no value",
    ++			   PARSE_OPT_EARLY),
    ++		OPT_STRING_F(0, "wanted-value", &a_string, "str",
    ++			     "wanted option taking a value",
    ++			     PARSE_OPT_EARLY),
    ++		OPT_STRING(0, "skipped-value", &a_string, "str",
    ++			   "option whose value has to be skipped"),
    ++		OPT_INTEGER(0, "number", &an_int,
    ++			    "option taking an integer value"),
    ++		OPT_STRING_F(0, "optarg", &a_string, "str",
    ++			     "option with an optional value",
    ++			     PARSE_OPT_OPTARG),
    ++		OPT_STRING_F(0, "lastarg", &a_string, "str",
    ++			     "option with a last argument default",
    ++			     PARSE_OPT_LASTARG_DEFAULT),
    ++		OPT_STRING_F(0, "early-optarg", &a_string, "str",
    ++			     "early option with an optional value",
    ++			     PARSE_OPT_EARLY | PARSE_OPT_OPTARG),
    ++		OPT_STRING_F(0, "early-lastarg", &a_string, "str",
    ++			     "early option with a last argument default",
    ++			     PARSE_OPT_EARLY | PARSE_OPT_LASTARG_DEFAULT),
    ++		OPT_BOOL('s', NULL, &a_short, "short only option"),
    ++		OPT_END()
     +	};
    ++
     +	enum early_scan_flags flags = 0;
     +	int stopped;
     +
     +	while (argc > 1 && *argv[1] == '-') {
    -+		if (!strcmp(argv[1], "--stop-at-dashdash"))
    -+			flags |= EARLY_SCAN_STOP_AT_DASHDASH;
    -+		else if (!strcmp(argv[1], "--stop-at-non-option"))
    ++		if (!strcmp(argv[1], "--stop-at-non-option"))
     +			flags |= EARLY_SCAN_STOP_AT_NON_OPTION;
     +		else
     +			break;
    @@ t/helper/test-parse-options.c: int cmd__parse_subcommand(int argc, const char **
     +		argv++;
     +	}
     +
    -+	stopped = early_scan_options(argc - 1, argv + 1, options, flags,
    -+				    show_early_option, NULL);
    ++	stopped = early_scan_options(argc - 1, argv + 1, option, flags,
    ++				     show_early_option, NULL);
     +	printf("stopped at: %d of %d\n", stopped, argc - 1);
     +
     +	return 0;
    @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '
     +	test_cmp expect actual
     +'
     +
    -+test_expect_success 'early_scan_options() can stop at "--"' '
    -+	test-tool early-scan-options --stop-at-dashdash -- --wanted >actual &&
    ++test_expect_success 'early_scan_options() always stops at "--"' '
    ++	test-tool early-scan-options -- --wanted >actual &&
     +	cat >expect <<-\EOF &&
     +	stopped at: 0 of 2
     +	EOF
     +	test_cmp expect actual &&
    -+	test-tool early-scan-options --stop-at-dashdash \
    -+		--skipped-value -- --wanted >actual &&
    ++	test-tool early-scan-options --stop-at-non-option -- --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	stopped at: 0 of 2
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --skipped-value -- --wanted >actual &&
     +	cat >expect <<-\EOF &&
     +	found: wanted at 2
     +	stopped at: 3 of 3
    @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '
     +	test_cmp expect actual
     +'
     +
    ++test_expect_success 'early_scan_options() always stops at "--end-of-options"' '
    ++	test-tool early-scan-options --end-of-options --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	stopped at: 0 of 2
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --stop-at-non-option \
    ++		--end-of-options --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	stopped at: 0 of 2
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
     +test_expect_success 'early_scan_options() can stop at a non-option' '
     +	test-tool early-scan-options --stop-at-non-option \
     +		arg --wanted >actual &&
    @@ t/t0040-parse-options.sh: test_expect_success 'u16 limits range' '
     +	EOF
     +	test_cmp expect actual
     +'
    ++
    ++test_expect_success 'early_scan_options() takes values from struct option' '
    ++	test-tool early-scan-options --number --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --number=5 --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'early_scan_options() does not skip an optional value' '
    ++	test-tool early-scan-options --optarg --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --lastarg --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'early_scan_options() matches a stuck optional value' '
    ++	test-tool early-scan-options --early-optarg=one >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: early-optarg at 0 value: one
    ++	stopped at: 1 of 1
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --early-lastarg=two >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: early-lastarg at 0 value: two
    ++	stopped at: 1 of 1
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'early_scan_options() does not take a separate optional value' '
    ++	test-tool early-scan-options --early-optarg --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: early-optarg at 0
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual &&
    ++	test-tool early-scan-options --early-lastarg --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: early-lastarg at 0
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'early_scan_options() ignores options without a long name' '
    ++	test-tool early-scan-options -s --wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	found: wanted at 1
    ++	stopped at: 2 of 2
    ++	EOF
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'early_scan_options() ignores negated options' '
    ++	test-tool early-scan-options --no-wanted >actual &&
    ++	cat >expect <<-\EOF &&
    ++	stopped at: 1 of 1
    ++	EOF
    ++	test_cmp expect actual
    ++'
     +
      test_done
2:  2adb3b6229 < -:  ---------- bisect: fix "--" detection when a term name is "--"
3:  8bfbd627a8 < -:  ---------- rev-parse: fix "--" detection when it is an option value
5:  69cb1339c6 < -:  ---------- parse-options: build early scan options from a struct option array
6:  a55b275327 ! 3:  8e5092d421 fast-import: use early_scan_options() for --allow-unsafe-features
    @@ Commit message
         sees the option, so unsafe "feature" commands from the stream are
         refused even though the option was given.
     
    -    Let's fix this by building the options for the scan from the same
    -    `struct option` array that parse_options() uses, so that both agree on
    -    which options take a value.
    +    Let's fix this by using early_scan_options(), which scans the very
    +    same `struct option` array that parse_options() uses, so that both
    +    agree on which options take a value, and by marking
    +    `--allow-unsafe-features` with PARSE_OPT_EARLY so that the scan
    +    reports it.
     
         Note that the scan still only matches the exact option spelling, while
         parse_options() also accepts unambiguous abbreviations, so the two still
    @@ Documentation/git-fast-import.adoc: fast-import stream! This option is enabled a
     -them, while `--allow-unsafe-features --depth 5` and
     -`--depth=5 --allow-unsafe-features` allow them.
     +Note that this option has to be spelled in full for the unsafe
    -+`feature` commands in the stream to be allowed. So while
    -+`--allow-unsafe` is accepted as an unambiguous abbreviation of this
    -+option, it still refuses them.
    ++`feature` commands in the stream to be allowed. So `--allow-unsafe`
    ++is accepted as an unambiguous abbreviation of this option, but the
    ++unsafe `feature` commands are still refused.
      
      `--signed-tags=<mode>`::
      	Specify how to handle signed tags. Behaves in the same way as
    @@ builtin/fast-import.c: static int option_parse_quiet(const struct option *opt UN
      	return 0;
      }
      
    -+/*
    -+ * The only option the early scan below is interested in, as it decides
    -+ * whether unsafe "feature" commands from the stream are allowed.
    -+ */
    -+static const char *early_wanted[] = { "allow-unsafe-features", NULL };
    -+
    -+static int option_parse_early_allow_unsafe(
    -+		const struct early_scan_option *opt UNUSED,
    -+		const char *value UNUSED, int pos UNUSED, void *data)
    ++static int option_parse_early_allow_unsafe(const struct option *option,
    ++					   const char *value UNUSED,
    ++					   int pos UNUSED, void *data)
     +{
     +	struct fast_import_state *state = data;
     +
    -+	state->allow_unsafe_features = 1;
    ++	if (!strcmp(option->long_name, "allow-unsafe-features"))
    ++		state->allow_unsafe_features = 1;
     +	return 0;
     +}
     +
      int cmd_fast_import(int argc,
      		    const char **argv,
      		    const char *prefix,
    - 		    struct repository *repo)
    - {
    - 	struct fast_import_state state;
    -+	struct early_scan_option *early;
    - 
    - 	struct option fast_import_options[] = {
    - 		OPT_GROUP(N_("Common")),
    +@@ builtin/fast-import.c: int cmd_fast_import(int argc,
    + 		OPT_HIDDEN_GROUP(N_("Advanced")),
    + 		OPT_BOOL_F(0, "allow-unsafe-features", &state.allow_unsafe_features,
    + 			   N_("allow unsafe mark commands from the stream"),
    +-			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
    ++			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY),
    + 		OPT_CALLBACK_F(0, "export-pack-edges", &state, N_("file"),
    + 			       N_("dump edge commits to <file>"),
    + 			       PARSE_OPT_HIDDEN | PARSE_OPT_NONEG,
     @@ builtin/fast-import.c: int cmd_fast_import(int argc,
      	 * line to override stream data). But we must do an early parse of any
      	 * command-line options that impact how we interpret the feature lines.
    @@ builtin/fast-import.c: int cmd_fast_import(int argc,
     -		if (!strcmp(arg, "--allow-unsafe-features"))
     -			state.allow_unsafe_features = 1;
     -	}
    -+	early = early_scan_options_from_options(fast_import_options,
    -+						early_wanted);
    -+	early_scan_options(argc - 1, argv + 1, early,
    -+			   EARLY_SCAN_STOP_AT_DASHDASH |
    ++	early_scan_options(argc - 1, argv + 1, fast_import_options,
     +			   EARLY_SCAN_STOP_AT_NON_OPTION,
     +			   option_parse_early_allow_unsafe, &state);
    -+	free(early);
      
      	rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
      	for (unsigned int i = 0; i < (cmd_save - 1); i++)

Christian Couder (3):
  parse-options: add parse_options_takes_argument()
  parse-options: add early_scan_options()
  fast-import: use early_scan_options() for --allow-unsafe-features

 Documentation/git-fast-import.adoc            |  10 +-
 .../technical/api-parse-options.adoc          |   5 +
 builtin/fast-import.c                         |  38 ++--
 parse-options.c                               | 116 ++++++++++--
 parse-options.h                               |  82 +++++++++
 t/helper/test-parse-options.c                 |  62 +++++++
 t/helper/test-tool.c                          |   1 +
 t/helper/test-tool.h                          |   1 +
 t/t0040-parse-options.sh                      | 173 ++++++++++++++++++
 t/t9300-fast-import.sh                        |  14 ++
 10 files changed, 466 insertions(+), 36 deletions(-)


base-commit: d38352cd43ab9745686d697872408bc3249a153f
-- 
2.56.0.rc2

[PATCH v2 1/3] parse-options: add parse_options_takes_argument()

[Christian Couder]

Whether an option takes a value, and therefore consumes the next
argument when that value is not stuck to it with an '=', is decided by
its type and its flags. That rule is currently open-coded in
show_gitcomp(), which needs it to decide if it should append an '=' to
the option it completes.

A following commit will need the same rule to find out which options an
early scan of the command line has to skip along with their value.

So let's factor that rule out into a new parse_options_takes_argument()
function, and let's use it in show_gitcomp().

Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the
next argument when it isn't the last one, so it is not considered as
taking a value, which is what show_gitcomp() already did.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 parse-options.c | 35 ++++++++++++++++++++++-------------
 parse-options.h | 10 ++++++++++
 2 files changed, 32 insertions(+), 13 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 4519ead9dc..a132c1ea12 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -841,6 +841,26 @@ static void show_negated_gitcomp(const struct option *opts, int show_all,
 	}
 }
 
+int parse_options_takes_argument(const struct option *opt)
+{
+	switch (opt->type) {
+	case OPTION_STRING:
+	case OPTION_FILENAME:
+	case OPTION_INTEGER:
+	case OPTION_UNSIGNED:
+	case OPTION_CALLBACK:
+		break;
+	default:
+		return 0;
+	}
+
+	if (opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_OPTARG |
+			  PARSE_OPT_LASTARG_DEFAULT))
+		return 0;
+
+	return 1;
+}
+
 static int show_gitcomp(const struct option *opts, int show_all)
 {
 	const struct option *original_opts = opts;
@@ -862,20 +882,9 @@ static int show_gitcomp(const struct option *opts, int show_all)
 			break;
 		case OPTION_GROUP:
 			continue;
-		case OPTION_STRING:
-		case OPTION_FILENAME:
-		case OPTION_INTEGER:
-		case OPTION_UNSIGNED:
-		case OPTION_CALLBACK:
-			if (opts->flags & PARSE_OPT_NOARG)
-				break;
-			if (opts->flags & PARSE_OPT_OPTARG)
-				break;
-			if (opts->flags & PARSE_OPT_LASTARG_DEFAULT)
-				break;
-			suffix = "=";
-			break;
 		default:
+			if (parse_options_takes_argument(opts))
+				suffix = "=";
 			break;
 		}
 		if (opts->flags & PARSE_OPT_COMP_ARG)
diff --git a/parse-options.h b/parse-options.h
index d7f896a933..f29e73f85c 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -420,6 +420,16 @@ int parse_options(int argc, const char **argv, const char *prefix,
 		  const char * const usagestr[],
 		  enum parse_opt_flags flags);
 
+/*
+ * Return non-zero if `opt` takes a value, which means that it consumes
+ * the next argument when that value is not stuck to it with an '='.
+ *
+ * Note that an option with PARSE_OPT_LASTARG_DEFAULT only consumes the
+ * next argument when it isn't the last one, so it is not considered as
+ * taking a value here.
+ */
+int parse_options_takes_argument(const struct option *opt);
+
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
-- 
2.56.0.rc2

[PATCH v2 2/3] parse-options: add early_scan_options()

[Christian Couder]

Some commands need to look at a few of their options before they can
parse their command line for real, for example because the result
decides whether a repository is needed at all, or how the beginning of
their input should be interpreted.

Such an early scan has to know which options take their value as a
separate argument, or it mistakes such a value for an option. Several
commands get this wrong, as they just walk their arguments comparing
them to the few option names they care about.

Let's add early_scan_options() to help with this. It walks the
arguments using the very same `struct option` array that the command
already passes to parse_options(), and uses the
parse_options_takes_argument() helper added in a previous commit, so
that the scan and the actual parsing agree on which options take a
value.

The options the caller wants to be told about are marked with a new
PARSE_OPT_EARLY flag, so that nothing has to be spelled out a second
time, and so that the mark cannot drift away from the option it refers
to.

Using a per-option flag for this is not new as that flag space already
holds flags that the parsing loop itself ignores, like
PARSE_OPT_NOCOMPLETE and PARSE_OPT_COMP_ARG, which only the completion
helper looks at, or PARSE_OPT_HIDDEN and PARSE_OPT_LITERAL_ARGHELP,
which only the usage output looks at.

The scan is deliberately kept much simpler than parse_options(),
instead of teaching the latter to perform a side effect free "dry
run". Such a dry run would have to avoid writing through `opt->value`,
calling option callbacks, dying on an invalid value, handling `--help`
and tracking command mode conflicts, so it would be a much larger
refactoring. If parse_options() learns to do it in the future though,
the commands converted now would keep both their option array and their
PARSE_OPT_EARLY marks, so their conversion would not have to be redone.

One consequence of staying simple is that abbreviated options are
still not matched, even though the scan is now given the command's
full option array. Resolving them the way parse_options() does would
mean duplicating the ambiguity detection that parse_long_opt()
performs. So the scan can fail to see an option that parse_options()
would accept, and its callers have to cope with that, typically by
erring on the safe side. This and the other differences with
parse_options() are documented in "parse-options.h".

In practice, despite these limitations, early scans using
early_scan_options() should still be safer and cleaner than the
ad-hoc hand-rolled scans they are meant to replace, which don't know
about option values at all and therefore disagree with
parse_options() in ways that create plain bugs.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 .../technical/api-parse-options.adoc          |   5 +
 parse-options.c                               |  81 ++++++++
 parse-options.h                               |  72 ++++++++
 t/helper/test-parse-options.c                 |  62 +++++++
 t/helper/test-tool.c                          |   1 +
 t/helper/test-tool.h                          |   1 +
 t/t0040-parse-options.sh                      | 173 ++++++++++++++++++
 7 files changed, 395 insertions(+)

diff --git a/Documentation/technical/api-parse-options.adoc b/Documentation/technical/api-parse-options.adoc
index 95b7924e84..f59d8e90a5 100644
--- a/Documentation/technical/api-parse-options.adoc
+++ b/Documentation/technical/api-parse-options.adoc
@@ -197,6 +197,11 @@ are the bitwise-or of:
 	Internal flag, set on options that were expanded from a
 	configured alias. It should not be set by callers.
 
+`PARSE_OPT_EARLY`::
+	Report this option to `early_scan_options()`, which looks at a
+	few options before parsing the command line for real. Ignored
+	by `parse_options()` itself.
+
 `PARSE_OPT_NOCOMPLETE`::
 	Do not offer this option for completion.
 
diff --git a/parse-options.c b/parse-options.c
index a132c1ea12..559dad9061 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -669,6 +669,8 @@ static void parse_options_check(const struct option *opts)
 		     opts->long_name))
 			optbug(opts, "uses feature "
 			       "not supported for dashless options");
+		if ((opts->flags & PARSE_OPT_EARLY) && !opts->long_name)
+			optbug(opts, "uses PARSE_OPT_EARLY, which needs a long name");
 		if (opts->type == OPTION_SET_INT && !opts->defval &&
 		    opts->long_name && !(opts->flags & PARSE_OPT_NONEG))
 			optbug(opts, "OPTION_SET_INT 0 should not be negatable");
@@ -706,6 +708,8 @@ static void parse_options_check(const struct option *opts)
 		case OPTION_SUBCOMMAND:
 			if (!opts->value || !opts->subcommand_fn)
 				optbug(opts, "OPTION_SUBCOMMAND needs a value and a subcommand function");
+			if (opts->flags & PARSE_OPT_EARLY)
+				optbug(opts, "OPTION_SUBCOMMAND does not support PARSE_OPT_EARLY");
 			if (!subcommand_value)
 				subcommand_value = opts->value;
 			else if (subcommand_value != opts->value)
@@ -1253,6 +1257,83 @@ int parse_options(int argc, const char **argv,
 	return parse_options_end(&ctx);
 }
 
+/*
+ * Look for `arg` among `option`. On success, return the matching option
+ * and set `value` to the value stuck to it, if any, or to NULL.
+ */
+static const struct option *find_early_scan_option(const char *arg,
+						   const struct option *option,
+						   const char **value)
+{
+	if (!skip_prefix(arg, "--", &arg))
+		return NULL;
+
+	for (const struct option *opt = option; opt->type != OPTION_END; opt++) {
+		const char *rest;
+
+		if (opt->type == OPTION_SUBCOMMAND)
+			continue;
+		if (!opt->long_name)
+			continue;
+		if (!skip_prefix(arg, opt->long_name, &rest))
+			continue;
+
+		if (!*rest) {
+			*value = NULL;
+			return opt;
+		}
+		/* Only an option that can take a value may have one stuck to it. */
+		if (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {
+			*value = rest + 1;
+			return opt;
+		}
+	}
+
+	return NULL;
+}
+
+int early_scan_options(int argc, const char **argv,
+		       const struct option *option,
+		       enum early_scan_flags flags,
+		       early_scan_fn *fn, void *data)
+{
+	for (int i = 0; i < argc; i++) {
+		const char *arg = argv[i];
+		const char *value;
+		const struct option *opt;
+		int pos = i;
+
+		/*
+		 * parse_options() always stops parsing options at these,
+		 * whatever its flags, so nothing after them is an option.
+		 */
+		if (!strcmp(arg, "--") || !strcmp(arg, "--end-of-options"))
+			return i;
+
+		opt = find_early_scan_option(arg, option, &value);
+		if (!opt) {
+			if ((flags & EARLY_SCAN_STOP_AT_NON_OPTION) &&
+			    (*arg != '-' || !arg[1]))
+				return i;
+			continue;
+		}
+
+		/*
+		 * When an option takes a value, but that value is not
+		 * stuck to it with '=', then the next argument is the
+		 * value and it has to be skipped so that it isn't
+		 * taken for an option itself.
+		 */
+		if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
+			value = argv[++i];
+
+		if (opt->flags & PARSE_OPT_EARLY && fn(opt, value, pos, data))
+			return i;
+	}
+
+	return argc;
+}
+
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
diff --git a/parse-options.h b/parse-options.h
index f29e73f85c..3ef64744a4 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -51,6 +51,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_NODASH = 1 << 5,
 	PARSE_OPT_LITERAL_ARGHELP = 1 << 6,
 	PARSE_OPT_FROM_ALIAS = 1 << 7,
+	PARSE_OPT_EARLY = 1 << 8,	/* only for early_scan_options() */
 	PARSE_OPT_NOCOMPLETE = 1 << 9,
 	PARSE_OPT_COMP_ARG = 1 << 10,
 	PARSE_OPT_CMDMODE = 1 << 11,
@@ -501,6 +502,77 @@ static inline void die_for_incompatible_opt2(int opt1, const char *opt1_name,
 		BUG("option callback expects an argument"); \
 } while(0)
 
+/*----- Early scan: scanning argv before the actual option parsing -----*/
+
+/*
+ * Some commands need to look at a few options before they can parse
+ * their command line for real, for example because the result decides
+ * whether a repository is needed at all.
+ *
+ * Such an early scan has to know which options take their value as a
+ * separate argument, or it could mistake such a value for an
+ * option. The functions below allow performing such early scans
+ * without being fooled by option values.
+ */
+
+/*
+ * Called by early_scan_options() for each argument matching a
+ * `struct option` with PARSE_OPT_EARLY set.
+ *
+ * `option` is the matching option, `value` its value or NULL if it
+ * doesn't take one, and `pos` the index of the option in argv.
+ *
+ * Returning a non-zero value stops the scan.
+ */
+typedef int early_scan_fn(const struct option *option, const char *value,
+			  int pos, void *data);
+
+enum early_scan_flags {
+	EARLY_SCAN_STOP_AT_NON_OPTION = 1 << 0, /* Stop at any non option */
+};
+
+/*
+ * Scan `argv` for the options described by `option`, calling `fn` for
+ * each of those that have PARSE_OPT_EARLY set. `argv` is not
+ * modified.
+ *
+ * `fn` may be NULL when no option has PARSE_OPT_EARLY set, which is
+ * useful to only find out where the scan stops.
+ *
+ * The scan always stops at "--" and at "--end-of-options", as
+ * parse_options() always stops parsing options there too, whatever its
+ * flags. PARSE_OPT_KEEP_DASHDASH and PARSE_OPT_KEEP_UNKNOWN_OPT only
+ * decide if the terminator is left in argv, not if it terminates.
+ *
+ * Returns the index at which the scan stopped, which is `argc` when the
+ * whole array was scanned.
+ *
+ * This scan is for now deliberately much simpler than
+ * parse_options(), so it differs from it in the following ways:
+ *
+ *  - Only the long form of an option is matched, and it has to be
+ *    spelled in full: short options and abbreviations are ignored.
+ *
+ *  - Negated forms ("--no-<name>") are not matched. This is harmless,
+ *    as they never take a value to skip.
+ *
+ *  - Options with PARSE_OPT_OPTARG or PARSE_OPT_LASTARG_DEFAULT are
+ *    treated as not taking a separate value.
+ *
+ *  - OPTION_SUBCOMMAND entries are skipped.
+ *
+ *  - OPTION_ALIAS entries are not resolved to the option they stand
+ *    for.
+ *
+ * So the scan can fail to see an option that parse_options() would
+ * accept, and callers have to cope with that, typically by erring on
+ * the safe side.
+ */
+int early_scan_options(int argc, const char **argv,
+		       const struct option *option,
+		       enum early_scan_flags flags,
+		       early_scan_fn *fn, void *data);
+
 /*----- incremental advanced APIs -----*/
 
 struct parse_opt_cmdmode_list;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index f181f0c02d..83522714c8 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -383,3 +383,65 @@ int cmd__parse_subcommand(int argc, const char **argv)
 
 	return parse_subcommand__cmd(argc, argv, test_flags);
 }
+
+static int show_early_option(const struct option *opt, const char *value,
+			     int pos, void *data UNUSED)
+{
+	printf("found: %s at %d", opt->long_name, pos);
+	if (value)
+		printf(" value: %s", value);
+	putchar('\n');
+	return 0;
+}
+
+int cmd__early_scan_options(int argc, const char **argv)
+{
+	char *a_string = NULL;
+	int an_int = 0, a_bool = 0, a_short = 0;
+
+	const struct option option[] = {
+		OPT_GROUP("early scan test options"),
+		OPT_BOOL_F(0, "wanted", &a_bool,
+			   "wanted option taking no value",
+			   PARSE_OPT_EARLY),
+		OPT_STRING_F(0, "wanted-value", &a_string, "str",
+			     "wanted option taking a value",
+			     PARSE_OPT_EARLY),
+		OPT_STRING(0, "skipped-value", &a_string, "str",
+			   "option whose value has to be skipped"),
+		OPT_INTEGER(0, "number", &an_int,
+			    "option taking an integer value"),
+		OPT_STRING_F(0, "optarg", &a_string, "str",
+			     "option with an optional value",
+			     PARSE_OPT_OPTARG),
+		OPT_STRING_F(0, "lastarg", &a_string, "str",
+			     "option with a last argument default",
+			     PARSE_OPT_LASTARG_DEFAULT),
+		OPT_STRING_F(0, "early-optarg", &a_string, "str",
+			     "early option with an optional value",
+			     PARSE_OPT_EARLY | PARSE_OPT_OPTARG),
+		OPT_STRING_F(0, "early-lastarg", &a_string, "str",
+			     "early option with a last argument default",
+			     PARSE_OPT_EARLY | PARSE_OPT_LASTARG_DEFAULT),
+		OPT_BOOL('s', NULL, &a_short, "short only option"),
+		OPT_END()
+	};
+
+	enum early_scan_flags flags = 0;
+	int stopped;
+
+	while (argc > 1 && *argv[1] == '-') {
+		if (!strcmp(argv[1], "--stop-at-non-option"))
+			flags |= EARLY_SCAN_STOP_AT_NON_OPTION;
+		else
+			break;
+		argc--;
+		argv++;
+	}
+
+	stopped = early_scan_options(argc - 1, argv + 1, option, flags,
+				     show_early_option, NULL);
+	printf("stopped at: %d of %d\n", stopped, argc - 1);
+
+	return 0;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b71a22b43b..5d2f5877d9 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -50,6 +50,7 @@ static struct test_cmd cmds[] = {
 	{ "pack-mtimes", cmd__pack_mtimes },
 	{ "parse-options", cmd__parse_options },
 	{ "parse-options-flags", cmd__parse_options_flags },
+	{ "early-scan-options", cmd__early_scan_options },
 	{ "parse-pathspec-file", cmd__parse_pathspec_file },
 	{ "parse-subcommand", cmd__parse_subcommand },
 	{ "partial-clone", cmd__partial_clone },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index f2885b33d5..071306d52d 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -43,6 +43,7 @@ int cmd__pack_deltas(int argc, const char **argv);
 int cmd__pack_mtimes(int argc, const char **argv);
 int cmd__parse_options(int argc, const char **argv);
 int cmd__parse_options_flags(int argc, const char **argv);
+int cmd__early_scan_options(int argc, const char **argv);
 int cmd__parse_pathspec_file(int argc, const char** argv);
 int cmd__parse_subcommand(int argc, const char **argv);
 int cmd__partial_clone(int argc, const char **argv);
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 449fff4d34..b796d96b9a 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -845,4 +845,177 @@ test_expect_success 'u16 limits range' '
 	test_grep "value 65536 for option .u16. not in range \[0,65535\]" err
 '
 
+test_expect_success 'early_scan_options() finds a wanted option' '
+	test-tool early-scan-options --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 0
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() reads a stuck or separate value' '
+	test-tool early-scan-options --wanted-value=one >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted-value at 0 value: one
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --wanted-value two >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted-value at 0 value: two
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() skips the value of other options' '
+	test-tool early-scan-options --skipped-value --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --skipped-value one --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() always stops at "--"' '
+	test-tool early-scan-options -- --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --stop-at-non-option -- --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --skipped-value -- --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() always stops at "--end-of-options"' '
+	test-tool early-scan-options --end-of-options --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --stop-at-non-option \
+		--end-of-options --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() can stop at a non-option' '
+	test-tool early-scan-options --stop-at-non-option \
+		arg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 0 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --stop-at-non-option \
+		--skipped-value arg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 2
+	stopped at: 3 of 3
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() ignores abbreviated options' '
+	test-tool early-scan-options --want >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() takes values from struct option' '
+	test-tool early-scan-options --number --wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --number=5 --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() does not skip an optional value' '
+	test-tool early-scan-options --optarg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --lastarg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() matches a stuck optional value' '
+	test-tool early-scan-options --early-optarg=one >actual &&
+	cat >expect <<-\EOF &&
+	found: early-optarg at 0 value: one
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --early-lastarg=two >actual &&
+	cat >expect <<-\EOF &&
+	found: early-lastarg at 0 value: two
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() does not take a separate optional value' '
+	test-tool early-scan-options --early-optarg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: early-optarg at 0
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual &&
+	test-tool early-scan-options --early-lastarg --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: early-lastarg at 0
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() ignores options without a long name' '
+	test-tool early-scan-options -s --wanted >actual &&
+	cat >expect <<-\EOF &&
+	found: wanted at 1
+	stopped at: 2 of 2
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'early_scan_options() ignores negated options' '
+	test-tool early-scan-options --no-wanted >actual &&
+	cat >expect <<-\EOF &&
+	stopped at: 1 of 1
+	EOF
+	test_cmp expect actual
+'
+
 test_done
-- 
2.56.0.rc2

[PATCH v2 3/3] fast-import: use early_scan_options() for --allow-unsafe-features

[Christian Couder]

The "feature" lines at the start of the stream are processed before the
command line options are parsed, so cmd_fast_import() scans its
arguments early to find out if `--allow-unsafe-features` was given.

That scan doesn't know which options take their value as a separate
argument, and it stops at the first argument that doesn't start with a
dash. So it disagrees with parse_options(), which accepts values
separated from their option by a space, for a command line like
"--depth 5 --allow-unsafe-features": the scan stops at "5" and never
sees the option, so unsafe "feature" commands from the stream are
refused even though the option was given.

Let's fix this by using early_scan_options(), which scans the very
same `struct option` array that parse_options() uses, so that both
agree on which options take a value, and by marking
`--allow-unsafe-features` with PARSE_OPT_EARLY so that the scan
reports it.

Note that the scan still only matches the exact option spelling, while
parse_options() also accepts unambiguous abbreviations, so the two still
disagree for a command line like "--allow-unsafe". This errs on the safe
side, and is now documented as a restriction.

Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 Documentation/git-fast-import.adoc | 10 ++++----
 builtin/fast-import.c              | 38 +++++++++++++++++-------------
 t/t9300-fast-import.sh             | 14 +++++++++++
 3 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
index fd165e11d2..c04b8fe502 100644
--- a/Documentation/git-fast-import.adoc
+++ b/Documentation/git-fast-import.adoc
@@ -66,12 +66,10 @@ fast-import stream! This option is enabled automatically for
 remote-helpers that use the `import` capability, as they are
 already trusted to run their own code.
 +
-Note that this option has to be spelled in full, and has to appear
-before any option whose value is separated from it by a space, for
-the unsafe `feature` commands in the stream to be allowed. So
-`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse
-them, while `--allow-unsafe-features --depth 5` and
-`--depth=5 --allow-unsafe-features` allow them.
+Note that this option has to be spelled in full for the unsafe
+`feature` commands in the stream to be allowed. So `--allow-unsafe`
+is accepted as an unambiguous abbreviation of this option, but the
+unsafe `feature` commands are still refused.
 
 `--signed-tags=<mode>`::
 	Specify how to handle signed tags. Behaves in the same way as
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index fbd919982c..7f36b828ce 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -4120,6 +4120,17 @@ static int option_parse_quiet(const struct option *opt UNUSED,
 	return 0;
 }
 
+static int option_parse_early_allow_unsafe(const struct option *option,
+					   const char *value UNUSED,
+					   int pos UNUSED, void *data)
+{
+	struct fast_import_state *state = data;
+
+	if (!strcmp(option->long_name, "allow-unsafe-features"))
+		state->allow_unsafe_features = 1;
+	return 0;
+}
+
 int cmd_fast_import(int argc,
 		    const char **argv,
 		    const char *prefix,
@@ -4184,7 +4195,7 @@ int cmd_fast_import(int argc,
 		OPT_HIDDEN_GROUP(N_("Advanced")),
 		OPT_BOOL_F(0, "allow-unsafe-features", &state.allow_unsafe_features,
 			   N_("allow unsafe mark commands from the stream"),
-			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
+			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY),
 		OPT_CALLBACK_F(0, "export-pack-edges", &state, N_("file"),
 			       N_("dump edge commits to <file>"),
 			       PARSE_OPT_HIDDEN | PARSE_OPT_NONEG,
@@ -4218,23 +4229,16 @@ int cmd_fast_import(int argc,
 	 * line to override stream data). But we must do an early parse of any
 	 * command-line options that impact how we interpret the feature lines.
 	 *
-	 * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features"
-	 * spelling and stops at the first argument that doesn't start with a
-	 * dash. As parse_options() below also accepts unambiguous abbreviations
-	 * and values separated by a space from their option, the two disagree
-	 * for command lines like "--allow-unsafe" or "--depth 5
-	 * --allow-unsafe-features": parse_options() accepts the option, but
-	 * this scan doesn't see it, so unsafe features from the stream are
-	 * still refused. This errs on the safe side, but should be fixed by
-	 * teaching this scan about the options that take a value.
+	 * NEEDSWORK: This scan only matches the exact
+	 * "--allow-unsafe-features" spelling, while parse_options() below
+	 * also accepts unambiguous abbreviations, so the two disagree for
+	 * a command line like "--allow-unsafe": parse_options() accepts
+	 * the option, but this scan doesn't see it, so unsafe features
+	 * from the stream are still refused. This errs on the safe side.
 	 */
-	for (int i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (*arg != '-' || !strcmp(arg, "--"))
-			break;
-		if (!strcmp(arg, "--allow-unsafe-features"))
-			state.allow_unsafe_features = 1;
-	}
+	early_scan_options(argc - 1, argv + 1, fast_import_options,
+			   EARLY_SCAN_STOP_AT_NON_OPTION,
+			   option_parse_early_allow_unsafe, &state);
 
 	rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
 	for (unsigned int i = 0; i < (cmd_save - 1); i++)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index d9de2ef0d8..1a37f2b8e6 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2344,6 +2344,20 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
 	test_path_is_missing feature-sub
 '
 
+test_expect_success 'R: --allow-unsafe-features found after a value' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	git fast-import --allow-unsafe-features <input &&
+	git fast-import --depth=5 --allow-unsafe-features <input &&
+	git fast-import --depth 5 --allow-unsafe-features <input &&
+	git fast-import --date-format raw --allow-unsafe-features <input
+'
+
+test_expect_success 'R: --allow-unsafe-features has to be spelled in full' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	test_must_fail git fast-import --allow-unsafe <input 2>err &&
+	test_grep "forbidden in input without --allow-unsafe-features" err
+'
+
 test_expect_success 'R: catch typo in marks file name' '
 	test_must_fail git fast-import --import-marks=nonexistent.marks </dev/null &&
 	echo "feature import-marks=nonexistent.marks" |
-- 
2.56.0.rc2

Re: [PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

[Christian Couder]

On Wed, Sep 2, 2026 at 8:52 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > To allow these commands to safely skip option values during their
> > early scans, this series introduces a new "early-scan" sub-API into
> > the existing "parse-options" API.
>
> Yay.
>
> > This is deliberately implemented as a new simple and fast scan, which
> > has some limitations, instead of a full refactor and reuse of the
> > parse_options() code,
>
> Sigh.  In other words, we hate these ad-hoc prescan that are buggy
> badly enough to replace them all with yet another ad-hoc prescan
> that is know to behave differently from the real thing?

Yes, because the limitations of the new scan are not very significant
in practice while refactoring the real thing (so that it can perform
an early scan without side effects) would be much more complex.

> >  - `git bisect start --term-good -- <not-a-rev>` mistook the term name
> >    `--` for the revision/path separator, so <not-a-rev> was rejected
> >    as an invalid revision instead of being treated as a path.
>
> Sorry, I fail to see much practical value in this.
>
> >  - `git rev-parse --default -- <not-a-rev>` did the same, reporting
> >    "bad revision <notarev>" while any other default value gives the
> >    usual more helpful "ambiguous argument" error.
>
> Neither in this one.

I have removed those from the series in the v2 I just sent.

> >  - `git fast-import --depth 5 --allow-unsafe-features` silently
> >    ignored `--allow-unsafe-features`, refusing unsafe features from
> >    the stream.
>
> On the other hand, this may be a very good thing.
>
> Is the reason why the ad-hoc pre-scan failed to see it was because
> it did not realize 5 is a value to the --depth option?

Yes.

> > All of these commands call parse_options(), but for `git bisect` and
> > `git rev-parse`, the specific functions doing the early scan
> > (bisect_start() and cmd_rev_parse()'s main loop) parse their own
> > options by hand after the early scan and have no `struct option` array
> > for those options.
> >
> > If bisect_start() and cmd_rev_parse() were converted to use
> > `struct option`, they could use early_scan_options_from_options() and
> > would not be affected by limitations 1), 2) and 3) above, as both use
> > the early scan only to locate `--`.
>
> I imagine that in the long term we would rather see a properly
> refactored parse-options machinery perform the prescan (perhaps with
> some kind of "dry-run" option given to the machinery) than yet
> another ad-hoc parser like this topic introduces.  It would be very
> good if this interim solution at least took the same 'options[]'
> array so that when we have the real thing in the future we do not
> have to redo the conversion effort.

This is what is implemented in the v2 I just sent. So yeah, when a
refactored parse-options machinery will be able to perform the
prescan, we will be able to use it to replace the early-scan parser
without changing or converting the callers.

> By the way, how does this interact with your other topic that has
> been stalled for quite some time?  Would moving this one forward
> help the other, or do they not have much relevance to each other?  I
> would rather not see two topics of non-trivial size stalled on a
> single author at the same time, so ...

They are separate topics and I alternate between them. I was recently
busy with travelling to the Git Merge and was a bit sick before that,
but hopefully I should be able to spend more time on them in the next
weeks. Also it seems to me that both topics have advanced to a point
where not a lot of big changes are needed. So they should move forward
quite fast now.

Re: [PATCH 1/6] parse-options: add early_scan_options()

[Christian Couder]

On Thu, Sep 3, 2026 at 12:11 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > So users must spell these specific options in full. This restriction
> > could be lifted in the future though, once the scanner is adapted to
> > accept a command's full option array, as this would give it the
> > complete context needed for safe abbreviation matching.
>
> It is unfortunate that end-users cannot tell if they are dealing
> with a system before of after "once the scanner is adapted"
> happened, so they must be trained to always spell the options in
> full to make use of the commands that use this feature.  It at least
> does not regress relative to the ad-hoc early scanners these selected
> commands have that do not even understand what they are parsing, so
> it may not be too bad.
>
> Stepping back a bit, the burden on programmers to use this would be
> to write in a separate notation what options there are in addition
> to what they feed the real parse_options(), which cuts both ways in
> the sense that because this does not take parse_options(), commands
> that do not use parse_options() can still use it, but those that do
> already use parse_options() need additional work to use eary_scan.
>
> And then once the scanner is adapted to accept the full option array,
> the programmers only need to discard the struct early_scan_option[]
> they wrote and replace it with the struct option[] they already have?
> Or would the calling convention to the scanner also change when it
> happens (oother than replacing the pointer to struct early_scan_option[]
> with another pointer to struct option[])?

I agree that what was implemented in v1 (to be able to accommodate
early scans that do not use parse_options()) didn't bring much
practical value, was a bit complex and required some churn when the
early scan would have been converted to use parse_options(). So, in
the v2 I just sent, it addresses only the early scan where
parse_options() is used, which simplifies a lot of things.

> > +static const struct early_scan_option *
> > +find_early_scan_option(const char *arg,
> > +                    const struct early_scan_option *options,
> > +                    const char **value)
>
> Because you return one single element from the incoming array of
> options, it is mildly misleading to call the variable/parameter
> "options" here and everywhere else.  Let's stick to "arrays are
> named singular, so that option[4] names 4th option" convention.

Right, I have changed the argument to `const struct option *option`.

> > +{
> > +     if (!skip_prefix(arg, "--", &arg))
> > +             return NULL;
> > +
> > +     for (; options->name; options++) {
> > +             const char *rest;
> > +
> > +             if (!skip_prefix(arg, options->name, &rest))
> > +                     continue;
>
> "--option" on the command line, after getting stripped the leading
> "--", may begin with "option", and that name may be in the option[]
> table, in which case ...
>
> > +             if (!*rest) {
> > +                     *value = NULL;
> > +                     return options;
> > +             }
>
> ... we found a hit.  But shouldn't option->takes_value be consulted
> before we return to signal the caller that the next arg is an option
> value before we return from here?  It looks a bit uneven as we do
> that for stuck form "--option=value" here.

Yeah, we found that `arg` exactly matches this option whether or not
it takes a value, but the value is not here.

Whether the next argument has to be skipped is decided by the caller:

  if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
      value = argv[++i];

find_early_scan_option() cannot do that itself, as it has neither
argv, argc nor the current index.

So signalling to the caller would be redundant, because the caller
already holds the matched option and can ask directly.

But maybe I should add a comment on the line before `if (!*rest) {`
saying that skipping a separate value is the caller's job?

> > +             /* Only an option taking a value can be stuck to one. */
> > +             if (*rest == '=' && options->takes_value) {
> > +                     *value = rest + 1;
> > +                     return options;
> > +             }
>
> And if the option[] table had "opt", then "--option" on the command
> line may begin with "--opt" but "ion" is an excess that is not a
> stuck value, so we do not consider it as a match.  OK.

Now using `takes_value` in the `*rest == '='` case wasn't quite right,
as parse_options_takes_argument() returns 0 for PARSE_OPT_OPTARG and
PARSE_OPT_LASTARG_DEFAULT, but parse_options() does accept a stuck
value for both.

So in v2 we use the same condition parse_options() uses:

  /* Only an option that can take a value may have one stuck to it. */
  if (*rest == '=' && !(opt->flags & PARSE_OPT_NOARG)) {
      *value = rest + 1;
      return opt;
  }

> > +     }
> > +     return NULL;
> > +}
>
> If we are to write a separate function anyway, I wonder how much
> more work to write a early_scan_option() parser that does take a
> real "struct option[]" array.  Its elements already know if they
> take a value or not.  For expediency, it may be OK to start by
> simplified parser that does not handle unique prefix and other
> complexities like callback functions of the real parser, but at
> least it would reduce the burden on the programmers quite a bit if
> we used the real struct option[] array, I suspect.

This is what v2 does, and I agree that it simplifies things.

Re: [PATCH 2/6] bisect: fix "--" detection when a term name is "--"

[Christian Couder]

On Thu, Sep 3, 2026 at 12:30 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > `bisect_start()` walks its arguments twice. The second loop actually
> > parses the options, and it knows that `--term-good`, `--term-old`,
> > `--term-bad` and `--term-new` take their value as a separate argument,
> > so it skips that value.
> >
> > The first loop, which only looks for the "--" separating revisions from
> > paths, doesn't know about these options. So when such an option is given
> > "--" as its value, that "--" is mistaken for the separator and
> > `has_double_dash` is wrongly set.
>
> It may be theoretically true, but I wonder how much practical value
> it has to correctly parse "--term-good --" as "Ah, the user wants to
> mark good revisions as '--' instead of 'good' or 'old'"?  Even
> though "refs/bisect/--" is *not* forbidden, how likely is it for
> users to do that?
>
> This is not like "git grep -e --" which does have much more pracical
> value.

Right, this patch and the next one have been removed from v2.

In the future we can still convert bisect_start() to the parse-options
API, and then use the early-scan API to look for "--" in a bit cleaner
way.

Re: [PATCH 1/6] parse-options: add early_scan_options()

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> Whether the next argument has to be skipped is decided by the caller:
>
>   if (parse_options_takes_argument(opt) && !value && i + 1 < argc)
>       value = argv[++i];
>
> find_early_scan_option() cannot do that itself, as it has neither
> argv, argc nor the current index.
>
> So signalling to the caller would be redundant, because the caller
> already holds the matched option and can ask directly.
>
> But maybe I should add a comment on the line before `if (!*rest) {`
> saying that skipping a separate value is the caller's job?

Not really. I was hinting if it is cleaner to have the callee do the
skipping so that caller does not have to worry about it.  After all,
the job of the early-scan machinery is to scan the options reliably
to find something later in the command line argument array.  The
less the caller needs to do, the easier the machinery is to use.

>> > +             /* Only an option taking a value can be stuck to one. */
>> > +             if (*rest == '=' && options->takes_value) {
>> > +                     *value = rest + 1;
>> > +                     return options;
>> > +             }
>>
>> And if the option[] table had "opt", then "--option" on the command
>> line may begin with "--opt" but "ion" is an excess that is not a
>> stuck value, so we do not consider it as a match.  OK.
>
> Now using `takes_value` in the `*rest == '='` case wasn't quite right,
> as parse_options_takes_argument() returns 0 for PARSE_OPT_OPTARG and
> PARSE_OPT_LASTARG_DEFAULT, but parse_options() does accept a stuck
> value for both.
>
> So in v2 we use the same condition parse_options() uses:

My giving an opaque hint pays off sometimes ;-)

Thanks.

Re: [PATCH 2/6] bisect: fix "--" detection when a term name is "--"

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> In the future we can still convert bisect_start() to the parse-options
> API, and then use the early-scan API to look for "--" in a bit cleaner
> way.

Yeah, when that happens, I can imagine that we can make detection of
"--" to come for free as a side effect of using parse_options().

Thanks.