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

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.

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?


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