[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.