[PATCH v3 11/12] fast-import: use parse_options() for command line options

Previous commits have started to use the parse-options API to display
output from `git fast-import -h` and `git fast-import --help-all` and
to prepare for parsing the command line options using this API.

Let's now actually use the API to parse command line options.

This brings a number of changes that are mostly beneficial:

  - The `--alias`, `--get-mark`, `--cat-blob`, `--ls` and `--notes`
    options are no longer accepted on the command line. They were
    previously accepted as no-ops because parse_argv() fell through to
    parse_one_feature(). They are not documented in the OPTIONS section
    and are only meaningful as in-stream feature assertions, so
    accepting them on the command line was an accident of code sharing
    dating back to 9c8398f0c9 (fast-import: add option command,
    2009-12-04).

  - Abbreviated options like `--dep=5` now work since parse_options()
    allows unambiguous prefixes.

  - As `--cat-blob` is an abbreviation of `--cat-blob-fd`, using the
    former on the command line will fail with "option `cat-blob-fd'
    requires a value" unlike the other four options that are not
    accepted anymore on the command line (see above).

  - Value-taking options now also accept the space-separated
    `--opt value` form, like `--depth 5`, in addition to the
    `--opt=value` form.

  - A bare or trailing `--` is now accepted and the stream is read
    normally, while it used to be a usage error.

  - The error messages for some options might differ a bit.

  - The code is shorter and more standard.

Note that parse_one_feature() is now always called with its
`from_stream` argument set to 1, but the code simplifications that
can be made are left for a following clean-up commit.

Signed-off-by: Christian Couder <[email protected]>
---
 Documentation/git-fast-import.adoc |  7 +++++
 builtin/fast-import.c              | 43 ++++++++++--------------------
 t/t9300-fast-import.sh             |  7 +++++
 3 files changed, 28 insertions(+), 29 deletions(-)

diff --git a/Documentation/git-fast-import.adoc b/Documentation/git-fast-import.adoc
index 7c5900e048..fd165e11d2 100644
--- a/Documentation/git-fast-import.adoc
+++ b/Documentation/git-fast-import.adoc
@@ -65,6 +65,13 @@ 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 40cc9c4a23..dd873ec433 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3975,31 +3975,11 @@ static const char *const fast_import_usage[] = {
 
 static void parse_argv(struct fast_import_state *state)
 {
-	unsigned int i;
-
-	for (i = 1; i < state->argc; i++) {
-		const char *a = state->argv[i];
-
-		if (*a != '-' || !strcmp(a, "--"))
-			break;
-
-		if (!skip_prefix(a, "--", &a))
-			die(_("unknown option %s"), a);
-
-		if (parse_one_option(state, a))
-			continue;
-
-		if (parse_one_feature(state, a, 0))
-			continue;
-
-		if (skip_prefix(a, "cat-blob-fd=", &a)) {
-			option_cat_blob_fd(state, a);
-			continue;
-		}
+	int argc = parse_options(state->argc, state->argv, state->prefix,
+				 state->option, fast_import_usage,
+				 PARSE_OPT_KEEP_ARGV0);
 
-		die(_("unknown option --%s"), a);
-	}
-	if (i != state->argc)
+	if (argc > 1)
 		usage_with_options(fast_import_usage, state->option);
 
 	state->seen_data_command = 1;
@@ -4135,11 +4115,6 @@ int cmd_fast_import(int argc,
 {
 	struct fast_import_state state;
 
-	/*
-	 * NEEDSWORK: For now this is used only to render
-	 * `-h`/`--help-all` usage messages. The actual parsing is
-	 * done by parse_one_option()/parse_one_feature().
-	 */
 	struct option fast_import_options[] = {
 		OPT_GROUP(N_("Common")),
 		OPT_CALLBACK_F(0, "date-format", NULL, N_("fmt"),
@@ -4230,6 +4205,16 @@ int cmd_fast_import(int argc,
 	 * "feature" lines at the start of the stream (which allows the command
 	 * 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.
 	 */
 	for (int i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index fe6c2617ac..d9de2ef0d8 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2827,6 +2827,13 @@ test_expect_success 'R: unknown commandline options are rejected' '\
 	test_must_fail git fast-import --non-existing-option < /dev/null
 '
 
+test_expect_success 'R: feature-only names are rejected on the command line' '
+	for opt in --alias --get-mark --ls --notes
+	do
+		test_must_fail git fast-import "$opt" </dev/null || return 1
+	done
+'
+
 test_expect_success 'R: die on invalid option argument' '
 	echo "option git active-branches=-5" |
 	test_must_fail git fast-import &&
-- 
2.55.0.530.gdb3615d990.dirty