From: Christian Couder Date: Mon, 02 Nov 2026 15:36:32 GMT Subject: Re: [PATCH v3 4/4] fast-import: use early_scan_options() for --allow-unsafe-features Message-ID: In-Reply-To: <20261031142255.4941-1-herroyalpriestess@gmail.com> Hi Cynthia, On Sat, Oct 31, 2026 at 3:25 PM Cynthia Uche 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. > > > + * 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 > which succeeds (exit=0), and > > ./git fast-import --depth 5 extra --allow-unsafe-features > 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.