Hi Cynthia,
On Wed, Nov 4, 2026 at 3:53 AM Cynthia Uche [off-list ref] wrote:
Hi team,
This is the second draft of my proposal for "Improve how command
arguments and options are scanned and parsed" for the December 2026
Outreachy round. v1 is here: [11]. I would appreciate
feedback, especially on the candidate inventory (section 4) and the
timeline (section 5).
Thank you for your interest in Git and this project.
[...]
Why not make parse_options() do a dry run? Because it would be a much
larger refactoring. According to the 3/4 commit message, a dry run
would have to avoid writing through opt->value, calling callbacks,
dying on invalid values, handling --help and tracking command mode
conflicts. Junio has said he would prefer this in the long term [2].
The scan is simpler, and it keeps the option array and the
PARSE_OPT_EARLY marks, so it does not block a dry run later.
Yeah, improving the early scan can result in refactorings like the one
in patch 2/4, which could make it easier to implement a dry run in
parse_options().
[...]
Why bisect and rev-parse were dropped: Junio said he saw little
practical value in them, since he doubted users would name a bisect
term or a --default value "--", unlike fast-import, where the bug
affects a safety check. Christian removed both patches in v2 and said
that bisect_start() could use the early scan once it has been converted
to parse-options [2], [3].
In https://lore.kernel.org/git/xmqqmrt7c1zf.fsf@gitster.g/ Junio also
said detection of "--" could eventually come for free as a side effect
of using parse_options(). So not sure an early scan will be needed at
all.
[...]
- git column (builtin/column.c, cmd_column())
It already uses parse_options(). Only the early step is by hand:
if argv[1] starts with "--command=", config is read with that
name before parse_options() runs, and a die() follows if the
parsed and scanned values differ. Observed:
git column --command=foo works (exit 0)
git column --command foo dies (exit 128)
git column --width=40 --command=foo dies (exit 128)
git column --com=foo dies (exit 128)
The three failures all print "fatal: --command must be the
first argument", even when "--command foo" really is first. I found
this rule only in the die() message: searching Documentation/ and
t/ for "must be the first" finds nothing related, and t9002 does not
test --command.
Fits: replacing the scan with early_scan_options() and
PARSE_OPT_EARLY on --command, once the series is in next. I have not
yet checked whether any option in column's table would
make the scan stop.
Risk: user-visible change (errors become success). Question for
the list: is the "must be first" rule intentional? The callback
receives pos, so it could be kept (pos == 0) or dropped.
`git column` is "used internally by other git commands to format
output into columns", but `git tag`, the only internal user, doesn't
use `--command`. So I guess not much effort has been made to make it
user friendly (which include allowing --command to not be first and
allowing `--command foo`) because it's not supposed to be used a lot,
nor by regular users.
The "must be first" rule also looks deliberate rather than an
oversight. "builtin/column.c" has the comment "This one is special and
must be the first one" above the scan.
- bisect (builtin/bisect.c)
bisect_start() has two hand-written loops: one looks for "--"
(the bug of the dropped v1 patch) and one parses --no-checkout,
--first-parent, --reset-when-found[=...] and the four --term-*
options, each in a "--opt value" and a "--opt=value" branch.
bisect_run() checks only argv[0] for --reset-when-found, and
bisect_terms() compares one string with --term-good/old/bad/new.
Fits: convert to parse-options first, order: bisect_terms(),
bisect_run(), then bisect_start() as a stretch.
Risk: bisect_start() is called from "bisect start", from
bisect_autostart() and from "bisect replay"; the original
arguments are written to BISECT_LOG and replayed, so they must be
kept. Abbreviations and "--no-" forms would start to be accepted.
It looks like bisect already uses parse_options() but only to dispatch
sub-commands (OPT_SUBCOMMAND() with PARSE_OPT_SUBCOMMAND_OPTIONAL).
The sub-command functions indeed don't call it
- check-ref-format (builtin/check-ref-format.c)
One loop over leading dash arguments handles --normalize/--print,
--allow-onelevel, --no-allow-onelevel and --refspec-pattern;
--branch is special-cased as exactly argc == 3; anything else
calls usage(). git.c lists it as NO_PARSEOPT.
Tests: t1402 covers --normalize, --allow-onelevel and
--refspec-pattern, alone and combined, and --branch (also from
outside a repository, and with the name -nain). It does not test
--print, abbreviations or "--". Fien Dora's patch adding tests for
the precedence of --allow-onelevel and --no-allow-onelevel [10]
was merged to 'next' on 1 Nov and is marked "Will merge to
'master'" in What's cooking #01 [6b]. Her commit message says she
plans to explore converting this parser in a separate series [10].
Observed on origin/master (v2.56.0-307-g50fe5cc63f):
--norm refs/heads/x usage text, exit=129
-- refs/heads/x usage text, exit=129
--normalize -- refs/heads/x usage text, exit=129
refs/heads/x --normalize usage text, exit=129
--branch -nain "fatal: '-nain' is not a valid
branch name", exit=128
Fits: parse-options: OPT_BIT for the flags (--no-allow-onelevel
then comes for free), OPT_ALIAS for --print, and dropping
NO_PARSEOPT in git.c.
Risks: commands that fail today would succeed: abbreviations, "--"
and negated forms such as --no-normalize would be accepted, and
so would options after <refname> unless I pass
PARSE_OPT_STOP_AT_NON_OPTION. --branch needs an explicit check
that it is not combined with other options, and "--branch -nain"
must keep failing with the invalid branch name error (exit 128),
not become a usage error from short option parsing.
Question for the list: is accepting "--" and abbreviations
welcome, should options after <refname> keep being rejected?
I haven't taken a look at this, but I saw that Hussain Sitamouwala
sent "check-ref-format: use parse-options API" patches.
Thanks.