Hi Cynthia,
On Sat, Oct 31, 2026 at 3:25 PM Cynthia Uche
[off-list ref] wrote:
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.
quoted
+ * 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.
Yeah, the end of that header comment wasn't as helpful as it could
have been. I have reworked and improved it. In the v4 I just sent, it
looks like this:
* This requires the scan to be told about the way the later
* parse_options() call treats its arguments though:
*
* - when parse_options() is called with PARSE_OPT_STOP_AT_NON_OPTION
* or with subcommands, callers should pass
* EARLY_SCAN_STOP_AT_NON_OPTION,
*
* - when parse_options() is called with PARSE_OPT_KEEP_UNKNOWN_OPT,
* which disables abbreviations there but not here, callers should
* not use the scan.
*
* Passing EARLY_SCAN_STOP_AT_NON_OPTION in other cases is safe, and
* useful when the caller rejects non-option arguments itself."
*/
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.
Right, in v4, I have improved the comment to explain why we pass the flag:
/*
* We don't parse most options until after we've seen the set of
* "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.
*
* 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_STOP_AT_NON_OPTION is passed even though parse_options()
* is not called with PARSE_OPT_STOP_AT_NON_OPTION, as any non-option
* argument makes parse_argv() reject the command line, and the stream
* should not have been processed with unsafe features by then.
*/
I also added a small test for `git fast-import --depth 5 extra
--allow-unsafe-features` as it looks like an interesting thing to
test.
I did not find other issues in this patch :)
Thank you for your suggestions and your review.