From: Cynthia Uche <herroyalpriestess@gmail.com>
Date: Sat, 31 Oct 2026 14:22:55 GMT
Subject: Re: [PATCH v3 4/4] fast-import: use early_scan_options() for --allow-unsafe-features
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.

> +	 * 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