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

[Christian Couder <christian.couder@gmail.com>]

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

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

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

As early_scan_options() looks up long options the same way as
parse_options(), the two also agree on abbreviations now. So
`--allow-unsafe` allows unsafe "feature" commands, while
"--quiet --export-pack --allow-unsafe-features" refuses them, as
parse_options() takes `--allow-unsafe-features` there as the value of
`--export-pack-edges`. (The old scan wrongly allowed them in this
case, as all the arguments start with a dash.)

The note about having to spell the option in full in the
documentation is therefore removed, and so is the NEEDSWORK comment
about it.

Helped-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Signed-off-by: Christian Couder <christian.couder@gmail.com>
---
 Documentation/git-fast-import.adoc |  7 ------
 builtin/fast-import.c              | 37 ++++++++++++++++--------------
 t/t9300-fast-import.sh             | 29 +++++++++++++++++++++++
 3 files changed, 49 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
index fd165e11d2..7c5900e048 100644
--- a/Documentation/git-fast-import.adoc
+++ b/Documentation/git-fast-import.adoc
@@ -65,13 +65,6 @@ Only enable this option if you trust the program generating the
 fast-import stream! This option is enabled automatically for
 remote-helpers that use the `import` capability, as they are
 already trusted to run their own code.
-+
-Note that this option has to be spelled in full, and has to appear
-before any option whose value is separated from it by a space, for
-the unsafe `feature` commands in the stream to be allowed. So
-`--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse
-them, while `--allow-unsafe-features --depth 5` and
-`--depth=5 --allow-unsafe-features` allow them.
 
 `--signed-tags=<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..2e667af349 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -4120,6 +4120,18 @@ static int option_parse_quiet(const struct option *opt UNUSED,
 	return 0;
 }
 
+static int option_parse_early_allow_unsafe(const struct option *option,
+					   const char *value UNUSED,
+					   bool unset, int pos UNUSED,
+					   void *data)
+{
+	struct fast_import_state *state = data;
+
+	if (!strcmp(option->long_name, "allow-unsafe-features"))
+		state->allow_unsafe_features = !unset;
+	return 0;
+}
+
 int cmd_fast_import(int argc,
 		    const char **argv,
 		    const char *prefix,
@@ -4184,7 +4196,7 @@ int cmd_fast_import(int argc,
 		OPT_HIDDEN_GROUP(N_("Advanced")),
 		OPT_BOOL_F(0, "allow-unsafe-features", &state.allow_unsafe_features,
 			   N_("allow unsafe mark commands from the stream"),
-			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG),
+			   PARSE_OPT_HIDDEN | PARSE_OPT_NONEG | PARSE_OPT_EARLY),
 		OPT_CALLBACK_F(0, "export-pack-edges", &state, N_("file"),
 			       N_("dump edge commits to <file>"),
 			       PARSE_OPT_HIDDEN | PARSE_OPT_NONEG,
@@ -4218,23 +4230,14 @@ int cmd_fast_import(int argc,
 	 * line to override stream data). But we must do an early parse of any
 	 * command-line options that impact how we interpret the feature lines.
 	 *
-	 * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features"
-	 * spelling and stops at the first argument that doesn't start with a
-	 * dash. As parse_options() below also accepts unambiguous abbreviations
-	 * and values separated by a space from their option, the two disagree
-	 * for command lines like "--allow-unsafe" or "--depth 5
-	 * --allow-unsafe-features": parse_options() accepts the option, but
-	 * this scan doesn't see it, so unsafe features from the stream are
-	 * still refused. This errs on the safe side, but should be fixed by
-	 * teaching this scan about the options that take a value.
+	 * The scan uses the same options as parse_options() below, so the
+	 * two agree on which options take a value and on abbreviations. In
+	 * case of doubt, the scan stops early, so unsafe features are then
+	 * refused rather than wrongly allowed.
 	 */
-	for (int i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (*arg != '-' || !strcmp(arg, "--"))
-			break;
-		if (!strcmp(arg, "--allow-unsafe-features"))
-			state.allow_unsafe_features = 1;
-	}
+	early_scan_options(argc - 1, argv + 1, fast_import_options,
+			   EARLY_SCAN_STOP_AT_NON_OPTION,
+			   option_parse_early_allow_unsafe, &state);
 
 	rc_free = mem_pool_alloc(&fi_mem_pool, cmd_save * sizeof(*rc_free));
 	for (unsigned int i = 0; i < (cmd_save - 1); i++)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index d9de2ef0d8..85059a5385 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2344,6 +2344,35 @@ test_expect_success 'R: export-marks options can be overridden by commandline op
 	test_path_is_missing feature-sub
 '
 
+test_expect_success 'R: --allow-unsafe-features found after a value' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	git fast-import --allow-unsafe-features <input &&
+	git 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 can be abbreviated' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+	git fast-import --allow-unsafe <input
+'
+
+test_expect_success 'R: disallowed abbreviation of --allow-unsafe-features' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=true \
+		git fast-import --allow-unsafe <input 2>err &&
+	test_grep "forbidden in input without --allow-unsafe-features" err
+'
+
+test_expect_success 'R: --allow-unsafe-features as the value of an abbreviation' '
+	echo "feature import-marks-if-exists=nonexistent.marks" >input &&
+	test_must_fail env GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS=false \
+		git fast-import --quiet --export-pack \
+		--allow-unsafe-features <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.116.g6de20f6092