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

[Junio C Hamano <gitster@pobox.com>]

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.