[PATCH 0/6] Standardize early option scanning to fix argument parsing bugs

[Christian Couder <christian.couder@gmail.com>]

A number of commands perform an early scan of their arguments to look
for specific flags or structural separators (like `--`).

These hand-rolled early scans are often fragile. They especially fail
to account for options that take their value as a separate
argument. This leads to disagreements between the early scan and the
actual parse_options() pass. For example, the early scanner might miss
a special option entirely, or mistakenly treat an option's value as
the `--` path separator.

To allow these commands to safely skip option values during their
early scans, this series introduces a new "early-scan" sub-API into
the existing "parse-options" API.

This is deliberately implemented as a new simple and fast scan, which
has some limitations, instead of a full refactor and reuse of the
parse_options() code, because the limitations are not very significant
in practice, while a full refactor and reuse of the parse_options()
code would be much more complex.

The current limitations of the new early scan code are:

 1. short options are ignored,

 2. options with PARSE_OPT_LASTARG_DEFAULT or PARSE_OPT_OPTARG are
 treated as not taking a separate value,

 3. negated options ("--no-...") are not automatically generated,

 4. abbreviated options will not be matched.

Note that while the others could be real issues for some commands,
"3. negated options" is not a practical issue because negated options
never consume a separate argument.

The early scan is performed by a new early_scan_options() function
which takes a `const struct early_scan_option *options` array as
argument. That array can be built either by hand or by a new
early_scan_options_from_options() function, which takes a
`const struct option *options` array, when the command already uses
`struct option`.

This allows us to use the new early-scan API even for commands that
don't use the parse-options API yet, and which are the majority of
commands performing an early scan.

In this series, only `git bisect`, `git rev-parse` and `git
fast-import` are converted to the early-scan API, which fixes bugs in
those commands:

 - `git bisect start --term-good -- <not-a-rev>` mistook the term name
   `--` for the revision/path separator, so <not-a-rev> was rejected
   as an invalid revision instead of being treated as a path.

 - `git rev-parse --default -- <not-a-rev>` did the same, reporting
   "bad revision <notarev>" while any other default value gives the
   usual more helpful "ambiguous argument" error.

 - `git fast-import --depth 5 --allow-unsafe-features` silently
   ignored `--allow-unsafe-features`, refusing unsafe features from
   the stream.

All of these commands call parse_options(), but for `git bisect` and
`git rev-parse`, the specific functions doing the early scan
(bisect_start() and cmd_rev_parse()'s main loop) parse their own
options by hand after the early scan and have no `struct option` array
for those options.

If bisect_start() and cmd_rev_parse() were converted to use
`struct option`, they could use early_scan_options_from_options() and
would not be affected by limitations 1), 2) and 3) above, as both use
the early scan only to locate `--`.

Note that using early_scan_options_from_options() rather than a
hand-written table does not change how abbreviations are handled: the
scan matches long names exactly either way. Limitation 4) would
nevertheless become relevant to those commands, because such a
conversion would also make parse_options() the parser for the options
after the early scan has first inspected them, and parse_options()
resolves abbreviations while their current hand-rolled loops do not.

`git diff`, `git column`, `git rev-list` and setup_revisions() in
"revision.c" could also be converted to the early-scan API but aren't
in this series for different reasons:

 - `git diff` has a number of short options like `-S`, `-G`, `-O`
   taking separate values.

 - `git column` scans `argv[1]` for `--command=` before reading the
   configuration. Because `--command` is an OPT_STRING,
   parse_options() also accepts `--command <name>` and abbreviations,
   so the two passes disagree. Converting it would fix that, but it
   changes user-visible behaviour in a command this series does not
   otherwise touch.

 - `git rev-list` and "revision.c" are about converting
   setup_revisions(), but converting it to `struct option` first is
   likely the better way forward.

Overview of the patches:
========================

 - Patch 1/6 introduces early_scan_options(), the early scanner that
   will be used instead of hand-rolled ones, along with its
   infrastructure.

 - Patches 2/6 and 3/6 use this scanner to fix bugs in `git bisect`
   and `git rev-parse` respectively.

 - Patch 4/6 refactors some existing code into a new
   parse_options_takes_argument() helper that will be used in the next
   patch.

 - Patch 5/6 introduces the new early_scan_options_from_options() as a
   bridge between the parse-options API and the early-scan API.

 - Patch 6/6 uses early_scan_options_from_options() to fix the early
   scan for `--allow-unsafe-features` in `git fast-import`.

CI tests:
=========

They all pass, see:

https://github.com/chriscool/git/actions/runs/33612974808


Christian Couder (6):
  parse-options: add early_scan_options()
  bisect: fix "--" detection when a term name is "--"
  rev-parse: fix "--" detection when it is an option value
  parse-options: add parse_options_takes_argument()
  parse-options: build early scan options from a struct option array
  fast-import: use early_scan_options() for --allow-unsafe-features

 Documentation/git-fast-import.adoc |  10 +-
 builtin/bisect.c                   |  27 ++++--
 builtin/fast-import.c              |  46 +++++----
 builtin/rev-parse.c                |  26 ++++--
 parse-options.c                    | 144 ++++++++++++++++++++++++++---
 parse-options.h                    |  92 ++++++++++++++++++
 t/helper/test-parse-options.c      |  71 ++++++++++++++
 t/helper/test-tool.c               |   2 +
 t/helper/test-tool.h               |   2 +
 t/t0040-parse-options.sh           | 103 +++++++++++++++++++++
 t/t1500-rev-parse.sh               |   5 +
 t/t6030-bisect-porcelain.sh        |   8 ++
 t/t9300-fast-import.sh             |  14 +++
 13 files changed, 503 insertions(+), 47 deletions(-)

-- 
2.55.0.787.g3f9e2241eb.dirty