Re: [PATCH v2 11/12] fast-import: use parse_options() for command line options

On Sat, Aug 8, 2026 at 9:25 AM Elijah Newren <[email protected]> wrote:
>
> On Tue, Aug 4, 2026 at 3:04 AM Christian Couder
> <[email protected]> wrote:
> >
> > Previous commits have started to use the parse-options API to display
> > output from `git fast-import -h` and `git fast-import --help-all` and
> > to prepare for parsing the command line options using this API.
> >
> > Let's now actually use the API to parse command line options.
> >
> > This brings a number of changes that are mostly beneficial:
> >
> >   - The `--alias`, `--get-mark`, `--cat-blob`, `--ls` and `--notes`
> >     options are no longer accepted on the command line. They were
> >     previously accepted as no-ops because parse_argv() fell through to
> >     parse_one_feature(). They are not documented in the OPTIONS section
> >     and are only meaningful as in-stream feature assertions, so
> >     accepting them on the command line was an accident of code sharing
> >     dating back to 9c8398f0c9 (fast-import: add option command,
> >     2009-12-04).
> >
> >   - Abbreviated options like `--dep=5` now work since parse_options()
> >     allows unambiguous prefixes.
> >
> >   - As `--cat-blob` is an abbreviation of `--cat-blob-fd`, using the
> >     former on the command line will fail with "option `cat-blob-fd'
> >     requires a value" unlike the other four options that are not
> >     accepted anymore on the command line (see above).
> >
> >   - The error messages for some options might differ a bit.
> >
> >   - The code is shorter and more standard.
>
> I think the list might be missing three behavioral changes:
>
> 1) A bare "--" (or a trailing "--") is now accepted and the command
> reads the stream normally, whereas the base treated it as a usage
> error:
>
>         printf '' | git fast-import --   # before: 129 (usage), after: 0
>
> The old parse_argv() broke on "--" and then did "if (i != state->argc)
> usage_with_options(...)"; parse_options() instead consumes "--" as the
> end-of-options marker and returns just argv0.  Harmless /
> conventional, just unlisted.
>
> 2) Value-taking options now also accept the space-separated "--opt
> value" form (e.g. "--depth 5", "--max-pack-size 1m", "--date-format
> raw"), not just "--opt=value".   Also expected parse_options()
> behavior and a nice improvement.

Yeah, in v3 I have added the following to the commit message:

      - Value-taking options now also accept the space-separated
        `--opt value` form, like `--depth 5`, in addition to the
        `--opt=value` form.

      - A bare or trailing `--` is now accepted and the stream is read
        normally, while it used to be a usage error.

> 3) The handling of "--allow-unsafe-features" has changed and might
> trip users up.
>
> Because the "feature" lines at the top of the stream are processed
> before parse_argv() runs, cmd_fast_import() does an early scan just to
> learn whether unsafe features are permitted:
>
>         for (int i = 1; i < argc; i++) {
>                 const char *arg = argv[i];
>                 if (*arg != '-' || !strcmp(arg, "--"))
>                         break;
>                 if (!strcmp(arg, "--allow-unsafe-features"))
>                         state.allow_unsafe_features = 1;
>         }
>
> That scan (a) matches only the exact spelling and (b) stops at the
> first token not starting with '-'.  In the base that was fine, because
> the old parse_argv loop was equally strict (exact spelling,
> "--opt=value" only).
> But now that parse_options() also accepts unambiguous abbreviations
> and space-separated values, the two passes disagree.  With an unsafe
> feature line in the stream:
>
>     # (A) old-style spelling, still fine:
>     printf 'feature import-marks-if-exists=/nope\n' |
>       git fast-import --depth=5 --allow-unsafe-features
>     # -> exit 0
>
>     # (B) space-separated value, newly accepted by parse_options():
>     printf 'feature import-marks-if-exists=/nope\n' |
>       git fast-import --depth 5 --allow-unsafe-features
>     # -> fatal: feature 'import-marks-if-exists' forbidden ... (128)
>
>     # (C) abbreviation, newly accepted by parse_options():
>     printf 'feature import-marks-if-exists=/nope\n' |
>       git fast-import --allow-unsafe
>     # -> fatal: feature 'import-marks-if-exists' forbidden ... (128)
>
> In (B) the early scan breaks on the bare "5" (it can't tell "5" is
> --depth's argument) and never reaches --allow-unsafe-features; in (C)
> the abbreviation isn't recognized by the strcmp().  Yet in both cases
> parse_options() itself accepts the option ("git fast-import
> --allow-unsafe" alone exits 0), so it's only the in-stream feature
> that gets rejected.
>
> This errs on the safe side (it refuses an unsafe feature rather than
> allowing one), and it's a minor inconsistency, but it might surprise
> users.  At a minimum, it should probably be documented as a
> shortcoming or TODO or something.  One possible solution is a
> dedicated parse_options() pass for just --allow-unsafe-features;
> another might be just requiring --allow-unsafe-features to be the
> *first* argument.  Thoughts?

Right, I prefer to document it for now while I plan to work on fixing
this in a more general way soon, as there are other places that have
similar problems.

In v3, I have added the following to the `--allow-unsafe-features`
option documentation in "Documentation/git-fast-import.adoc":

    Note that this option has to be spelled in full, and has to appear
    before any option whose value is separated from it by a space, for
    the unsafe `feature` commands in the stream to be allowed. So
    `--allow-unsafe` or `--depth 5 --allow-unsafe-features` still refuse
    them, while `--allow-unsafe-features --depth 5` and
    `--depth=5 --allow-unsafe-features` allow them.

and this NEEDSWORK in the code:

   * NEEDSWORK: This scan only matches the exact "--allow-unsafe-features"
   * spelling and stops at the first argument that doesn't start with a
   * dash. As parse_options() below also accepts unambiguous abbreviations
   * and values separated by a space from their option, the two disagree
   * for command lines like "--allow-unsafe" or "--depth 5
   * --allow-unsafe-features": parse_options() accepts the option, but
   * this scan doesn't see it, so unsafe features from the stream are
   * still refused. This errs on the safe side, but should be fixed by
   * teaching this scan about the options that take a value.

> Anyway, other than the above list of three additional behavioral
> changes, the rest of the patch looks right.  Thanks for tackling
> modernizing this command.

Thank you for the review.