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 hunk ↗ jump to hunk
+ * 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