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

From: Christian Couder <christian.couder@gmail.com>
Date: Nov 2, 2026, 15:36 UTC
Message-ID: <CAP8UFD2m+wpcgqB2SasWLxyekY+5khrYyPkOuLoYwANqZws8fw@mail.gmail.com>
In-Reply-To: <20261031142255.4941-1-herroyalpriestess@gmail.com>

Hi Cynthia,

On Sat, Oct 31, 2026 at 3:25 PM Cynthia Uche <herroyalpriestess@gmail.com> wrote:

Show 20 quoted lines

>
> 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."
 */

Show 32 quoted lines

> 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.