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