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

From: Cynthia Uche <herroyalpriestess@gmail.com>
Date: Oct 31, 2026, 14:22 UTC
Message-ID: <20261031142255.4941-1-herroyalpriestess@gmail.com>
In-Reply-To: <20261012074413.337183-5-christian.couder@gmail.com>

Hi Christian,

I read through the series and ran t0040 and t9300 on the cc/early-scan-options topic, as in origin/seen (tip dabb68a20d). All 118 tests in t0040 pass, including the new early-scan tests, and all 261 tests in t9300 pass, including the new R: tests.

Show 8 quoted lines

> +	 * 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.
> +	 */
> +	early_scan_options(argc - 1, argv + 1, fast_import_options,
> +			   EARLY_SCAN_STOP_AT_NON_OPTION,
> +			   option_parse_early_allow_unsafe, &state);

The call passes EARLY_SCAN_STOP_AT_NON_OPTION, but the 3/4 header says to pass it when parse_options() is called with PARSE_OPT_STOP_AT_NON_OPTION or with subcommands. That is not the case here: parse_argv() calls parse_options() with only PARSE_OPT_KEEP_ARGV0, and the table has no subcommands. Stopping at the first non-option is the safe direction, and it matches the old loop, which also broke at the first argument not starting with a dash. But the comment only says the scan stops early "in case of doubt", and does not mention this flag.

With an input file containing "feature import-marks-if-exists=nonexistent.marks", I tried:

  ./git fast-import --depth 5 --allow-unsafe-features <input

which succeeds (exit=0), and

  ./git fast-import --depth 5 extra --allow-unsafe-features <input

which dies with "forbidden in input without --allow-unsafe-features" (exit=128). The stray "extra" would make parse_options() fail with a usage error, but that only happens after the stream's feature lines have been processed.

To see what the flag changes, I replaced EARLY_SCAN_STOP_AT_NON_OPTION with 0 locally and ran the second command again. It then failed with the usage error (exit=129) instead, so the scan had gone past "extra", seen --allow-unsafe-features, and the unsafe feature line was accepted before parse_options() rejected the command line.

I would suggest adding a sentence to the comment saying that the scan stops at the first non-option through EARLY_SCAN_STOP_AT_NON_OPTION, even though parse_options() is not called with PARSE_OPT_STOP_AT_NON_OPTION, so that readers know why the flag is passed.

I did not find other issues in this patch :)

Thanks, Cynthia