Git development
 help / color / mirror / Atom feed
* [Outreachy] Proposal draft v1: Improve how command arguments and options are scanned and parsed
@ 2026-11-01 21:06 Cynthia Uche
  2026-11-04  2:53 ` [Outreachy] Proposal draft v2: " Cynthia Uche
  0 siblings, 1 reply; 8+ messages in thread
From: Cynthia Uche @ 2026-11-01 21:06 UTC (permalink / raw)
  To: git; +Cc: christian.couder, siddharthasthana31, cynthianuchepro

Hi team,

This is the first draft of my proposal for "Improve how command
arguments and options are scanned and parsed" for the December 2026
Outreachy round. I would appreciate feedback, especially on the
candidate inventory (section 4) and the timeline (section 5).

1. About me and logistics
-------------------------
Name:     Cynthia Uche. I'm also called Priestess.
Pronouns: she/her
Email:    herroyalpriestess@gmail.com
GitHub:   https://github.com/herrpriestess
Blog:     https://medium.com/@thegirlSynth
Location: Abia, Nigeria (WAT, UTC+1)

I have read the Outreachy eligibility rules and I meet all the
requirements.

I am a software engineer, mostly working on Python and backend
projects. C was the first language I learned, so I am comfortable
reading and writing it, although I have not used it much day to day
until now.

I have a merged patch in OpenStack Shared File Systems Service (Manila)
(https://review.opendev.org/c/openstack/manila/+/982431), which went
through Gerrit review, so reworking a change after review comments
is not new to me.

Availability: 30 hours per week

2. My Git work
--------------
Microproject: t2104: avoid suppressing git's exit code
  Thread: https://lore.kernel.org/git/20261021033723.5265-1-herroyalpriestess@gmail.com/
  Topic:  cu/t2104-avoid-pipes
  Status: merged to master (merge commit 84bea4324a)
  Patrick reviewed it and found it good, so it needed no second
  version.

Review of the early-scan series:
  [PATCH v3 4/4] fast-import: use early_scan_options() ...
  https://lore.kernel.org/git/20261031142255.4941-1-herroyalpriestess@gmail.com/
  I ran t9300 (261 tests) and t0040 (118 tests) on the topic as in
  origin/seen (tip dabb68a20d); all pass. I noticed that
  EARLY_SCAN_STOP_AT_NON_OPTION is passed although the real parse
  does not use PARSE_OPT_STOP_AT_NON_OPTION. With that argument set
  to 0 locally, "--depth 5 extra --allow-unsafe-features" reached
  the usage error (exit 129) instead of the "forbidden" error
  (exit 128), so the unsafe feature line had been accepted first.
  I suggested documenting this in the comment at the call site.

Steps I have taken: read the microproject and application pages,
SubmittingPatches and MyFirstContribution; built Git and ran the
relevant tests; sent my microproject by git send-email; read the
early-scan series v1 to v3 with its reviews; reproduced the
git column behaviour (section 4); ran the new fast-import
tests in t9300 and tried two command lines by hand (section 2).

3. Project understanding
------------------------
3.1 The problem: two parses that can disagree

Some commands must look at a few options before they call
parse_options(), because the result changes how they read their input.
fast-import is the example: the "feature" lines at the start of the
stream are handled before the command line is parsed, so
cmd_fast_import() scans argv early for --allow-unsafe-features.

The old scan was a hand-rolled loop. It compared each argument with
the exact option name and stopped at the first argument that did not
start with a dash. parse_options() is more permissive: it accepts
unambiguous abbreviations and values separated from their option by a
space. So the two disagree:

 - "--depth 5 --allow-unsafe-features": the scan stops at "5" and
   never sees the flag, although parse_options() accepts it. Unsafe
   features are refused. That is the safe direction, but still wrong.
 - "--quiet --export-pack --allow-unsafe-features": every argument
   starts with a dash, so the old scan saw the flag and allowed
   unsafe features. But parse_options() takes it as the value of
   --export-pack-edges, which --export-pack abbreviates. That is the
   dangerous direction.

3.2 The API in v3

early_scan_options(argc, argv, option, flags, fn, data) walks argv
using the same struct option array that parse_options() uses. Options
the caller cares about are marked PARSE_OPT_EARLY, and the callback
receives (option, value, unset, pos, data); a non-zero return stops
the scan. It returns the index of the first argument not consumed.
Two helpers make it agree with parse_options():
parse_options_takes_argument() (1/4, which also counts
PARSE_OPT_LASTARG_DEFAULT options as taking a value) and
lookup_long_opt(), extracted from parse_long_opt() (2/4), so negated
forms, abbreviations and aliases are looked up the same way without
side effects. The scan always stops at "--" and "--end-of-options";
EARLY_SCAN_STOP_AT_NON_OPTION is for callers whose parse_options()
call uses PARSE_OPT_STOP_AT_NON_OPTION or subcommands. It should not
be used with PARSE_OPT_KEEP_UNKNOWN_OPT.

3.3 Why "stop when unsure" is safe

The scan can miss an option that parse_options() would accept, but it
never reports one that parse_options() would not. It stops at anything
it cannot reliably skip: short options, unknown options, ambiguous or
disallowed abbreviations, low-level callbacks (which may consume any
number of arguments), a value stuck to a negated or no-argument
option, and a value-taking option given as the last argument. Missing
an option only makes the caller behave as if it was not given (for
fast-import, unsafe features are refused). Mistaking an option's value
for an option is the dangerous failure.

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.

3.4 History of the series

 - v1 (2 Sep, 6 patches): early_scan_options() with its own option
   table, a bisect fix for "--" detection when a term is "--", a
   rev-parse fix for "--" when it is an option value,
   parse_options_takes_argument(), building the scan from a struct
   option array, and the fast-import conversion.
 - v2 (23 Sep, 3 patches): the scan reads the real struct option
   array and PARSE_OPT_EARLY marks the wanted options. The bisect and
   rev-parse patches are gone.
 - v3 (12 Oct, 4 patches, rebased on Git 2.56, a018953688): adds the
   lookup_long_opt() extraction, handles LASTARG_DEFAULT, stops at
   more cases, adds the "unset" callback argument, and removes the
   NEEDSWORK comment in fast-import.

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].

3.5 Current state and open follow-ups

As of Nov 01, cc/early-scan-options is in seen and not in next
(early_scan_options() is not in origin/next). Junio's Oct 28 "What's
cooking" listed it as "Needs review". Kaartic reviewed v3 on
30-31 Oct and found it in much better shape. His points:
 - consume the lookup_long_opt() result in parse_long_opt() with a
   switch instead of an if chain (2/4);
 - whether the PARSE_OPT_EARLY checks in parse_options_check() should
   also be in early_scan_options(), in case it is called before
   parse_options_start() (3/4);
 - document that the callback must not keep the "option" pointer,
   because it is freed after the scan (3/4).

Not covered by the series: commands that do not use parse-options
at all, short options (the scan stops at them) and low-level
callbacks. Those are what section 4 is about.

4. Candidate inventory and risks
--------------------------------
Method: I counted hand-written option checks per builtin with
  git grep -c 'strcmp(arg' -- builtin/ | sort -t: -k2 -n
  git grep -n 'skip_prefix(arg, "--' -- builtin/
and listed builtins that never call parse_options with
  git grep -L parse_options -- 'builtin/*.c'
The counts include small value converters (am, config, log, revert),
which I treat as out of scope. The densest real parsing is in
rev-parse (43), rev-list (18), index-pack and diff (13 each) and
bisect (12).

 - 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.

 - 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.

 - 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 has a patch adding tests
   for the precedence of --allow-onelevel and --no-allow-onelevel [10];
   Junio said he would queue it, and it would guard that part of a
   conversion.
   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?

 - unpack-objects (builtin/unpack-objects.c) [if time allows]
   -n, -q, -r, --strict[=<msg-types>], --pack_header=,
   --max-input-size=. Risk: other Git programs run it with fixed
   arguments, so those must keep working; I will list the callers
   before choosing it. index-pack and fetch-pack are larger,
   same issue.

 - Not proposed: rerere (Patrick's ps/rerere-subcommands (v2, 29 Oct),
   would convert it to OPT_SUBCOMMAND [9]), rev-parse (options are
   actions run in order and mixed with revisions, and its main option
   handling is not table-driven), rev-list, diff and setup_revisions()
   in revision.c (Christian thinks it should be converted to struct
   option first).

Common risks: behaviour change for scripts; abbreviations
(GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS); short options and low-level
callbacks that stop the scan; PARSE_OPT_KEEP_UNKNOWN_OPT; test
coverage before the change. parse-options.c is changed by several
experienced contributors, so I will coordinate on the list before
any API change.

5. Plan and timeline
--------------------
Internship dates: 7 Dec 2026 to 8 Mar 2027.

Now to Dec 6:  Follow cc/early-scan-options, help it get to next
               with reviews, and discuss the targets on the list,
               including check-ref-format.
Weeks 1-2:     Agree on the inventory and a first target with the
               mentors. Write tests that show the current
               behaviour. Start the weekly blog.
Weeks 2-5:     First series: bisect_terms() and bisect_run(), tests
               first, then the conversion.
Weeks 4-8:     Second series: check-ref-format if it is still free,
               otherwise bisect_start(), which is then a main item and
               not a stretch.
Weeks 6-10:    unpack-objects, if time allows; index-pack and
               fetch-pack only if time remains.
Weeks 8-11:    column on early_scan_options(), once the series is in
               next and the "must be first" question is settled.
               Tests first, then the change.
Weeks 11-13:   Rerolls, documentation, final report, and a list of
               remaining candidates for the next intern.
Every week:    Weekly blog post.

The ranges overlap on purpose: each series usually waits days for
review, so I can work on the next one meanwhile. If I fall behind
I drop unpack-objects first, then the bisect_start() when it is only a
stretch. If the early-scan series changes shape or stalls, the column
step becomes another conversion from section 4 that does not depend on
it. I have not planned any time off, except possibly 1-3 January, which
I will arrange with the mentors beforehand.

6. Communication
----------------
A weekly blog post (Christian asks for one), linked on the list;
weekly written updates to Siddharth; calls with Christian planned in
advance; questions asked on the list first; replies to review within
1-2 days.

I will publish the blog on Medium and link each post on the list.
I love sharing about what I'm learning, and I trust that I'll be
learning a lot in the next few weeks.

7. After the internship
-----------------------
After my internship I intend to keep reviewing patches, help
newcomers learn the ropes, just like I did, and keep working on the
follow-ups that did not fit the internship. I believe in paying it
forward, so if I ever get the chance to mentor someone else, I will
consider it a privilege.

8. References
-------------
[1] v1 cover:
    https://lore.kernel.org/git/20260902161047.476753-1-christian.couder@gmail.com/
[2] Junio on the v1 cover and Christian's answer:
    https://lore.kernel.org/git/xmqqpkyviizc.fsf@gitster.g/
    https://lore.kernel.org/git/CAP8UFD3qUpjUayhkMumZ41iMut=1=Pcmzx1YYcEV9NMG18OPsw@mail.gmail.com/
[3] Junio on v1 2/6 (bisect) and Christian's answer:
    https://lore.kernel.org/git/xmqqse3rffr4.fsf@gitster.g/
    https://lore.kernel.org/git/CAP8UFD3sh9Ejfgv7CB33LRU_z3i662_+tjdW7JqwRrzexWX_Ow@mail.gmail.com/
[4] v3 cover and 4/4:
    https://lore.kernel.org/git/20261012074413.337183-1-christian.couder@gmail.com/
    https://lore.kernel.org/git/20261012074413.337183-5-christian.couder@gmail.com/
[5] Kaartic on 2/4, 3/4 and the v3 cover:
    https://lore.kernel.org/git/02ba1b9d-b598-454b-897f-cb33ba190b13@gmail.com
    https://lore.kernel.org/git/6782fa63-1e46-4ae9-b129-a157986c2556@gmail.com
    https://lore.kernel.org/git/1e194ea7-2e28-4142-b678-d8cb8bca8e35@gmail.com
[6] "What's cooking" Oct 2026 #10:
    https://lore.kernel.org/git/xmqqo6cd5hnt.fsf@gitster.g/
[7] My microproject thread and merge commit:
    https://lore.kernel.org/git/20261021033723.5265-1-herroyalpriestess@gmail.com/
    84bea4324a
[8] My review of 4/4:
    https://lore.kernel.org/git/20261031142255.4941-1-herroyalpriestess@gmail.com/
[9] Patrick's ps/rerere-subcommands:
    https://lore.kernel.org/git/20261029-pks-rerere-subcommands-v2-0-07d5ba18e882@pks.im/
[10] Fien Dora's t1402 patch (v3) and Junio's reply:
     https://lore.kernel.org/git/20261029121715.3845-1-fiendora5@gmail.com/
     https://lore.kernel.org/git/xmqqqzh81goi.fsf@gitster.g/
[11] Earlier drafts: none, this is v1.

9. Closing remarks
------------------
Git has a VAST codebase, and it can seem quite intimidating at first
to a newcomer, but the support in this community has made it much
easier to start. Resilience is one of my core attributes, and I also
know to ask for help when I need it, so I won't be the first to give
up should things get more demanding than I expect. I would be glad to
be mentored by experienced contributors and to keep contributing after
the internship.

Thanks,
Cynthia Uche

^ permalink raw reply	[flat|nested] 8+ messages in thread
* [Outreachy] Proposal draft v2: Improve how command arguments and options are scanned and parsed
@ 2026-11-08 17:50 Tanga Joris
  2026-11-09 15:09 ` [Outreachy] Proposal draft v3: " Tanga Joris
  0 siblings, 1 reply; 8+ messages in thread
From: Tanga Joris @ 2026-11-08 17:50 UTC (permalink / raw)
  To: git
  Cc: christian.couder, siddharthasthana31, kaartic.sivaraam,
	usmanakinyemi202, Tanga Joris

Hi everyone,

This is the second draft of my proposal for "Improve how command
arguments and options are scanned and parsed" [1].

Changes since v1:
 - v1 was mostly about making the entry points agree. The project
   page asks for converting hand-written parsing and improving the
   API where needed, so I rebuilt the plan around that.
 - Added the commands I plan to work on, with what I saw when I ran
   them on master.
 - Moved the completion helper work to before the internship.
 - The plan has three items: stash show, the number fixes, and "git
   diff". For "git diff" (3B), I ask the mentors which way to go.
 - Added section 4 with other places I found, like git daemon and
   the revision options.
 - Shortened everything else.

I would be grateful for feedback on section 3, and on the questions
in section 7. I plan to start with stash show. If someone is already
working on one of these commands, please tell me and I will pick
another one.


1. About me
-----------
Name: Tanga Joris
Pronouns: he/him
Email: tangajoris@gmail.com
GitHub: https://github.com/tmjoris
Blog: https://tmjoris.github.io
Location: Nairobi, Kenya (EAT, UTC+3)

I meet all the Outreachy eligibility rules.

I am about to graduate from the United States International
University - Africa, where I study Applied Computer Technology. I
learned C at ALX, where it was my introduction to programming.

I had done some small parsing work before, in my own project, kiln,
and it made me curious about how much goes on behind something as
seemingly simple as parsing command-line arguments. I enjoy that kind
of work: starting with something that looks straightforward,
following the implementation, and gradually understanding the
decisions and assumptions behind it.

kiln (https://github.com/tmjoris/kiln) is a Go CLI that runs code in
sandboxes. "kiln -addr host:7070 run" works, but "kiln run -addr
host:7070" fails. This is because main() and each subcommand parse
their flags separately. Git has the same question of who looks at an
argument first, and what they do with it.

Availability: 40 hours a week from 7 December 2026 to 8 March 2027.
I have no classes, exams or other work during the internship.
Nairobi is UTC+3, so my working day overlaps with European hours.


2. My Git work so far
---------------------
- Microproject: checkout: use Q_() for messages with counts [2].
  Merged to master in a04194e134.
- tj/drop-no-parseopt, v5 [3]. Four builtins used parse-options but
  were still marked NO_PARSEOPT, so completion never asked them for
  their options. The topic removes the flag from them, and adds a
  t0012 test that catches such builtins. Junio: "OK. Let's mark the
  topic as ready for 'next'."
- tj/annotate-blame-args [4]. I found it while trying to make
  --git-completion-helper work outside a repository. cmd_annotate()
  put "-c" in front of the user's arguments, so the helper option was
  no longer the only argument. The diff option parser answered it
  instead, and "git annotate --<TAB>" offered the options of
  "git diff". The "-c" was not needed, so the fix deletes code.
  Junio: "Nice simplification."
  Both topics are marked "Will merge to 'next'?" in What's cooking
  [5].
- apply: a fix for the "git apply --check" crash reported on the
  list [6]. While looking into it, I found two more ways to trigger
  the same assertion.
- Reviews:
  - Azuama's rev-list early scan fix [7]. The tests failed without
    the fix, but --author, --committer and --grep-reflog had the
    same problem. Azuama's v4 skips their values too, and its
    NEEDSWORK comment now uses my "--author --missing-only" example
    [8]. Junio then asked for a different approach than a list of
    options in rev-list.c.
  - Joy's pack size limit patch [9]. 2/2 added a member to struct
    repo_config_values, which Junio had asked people not to do for
    now. So I asked whether 1/2 could be sent on its own, and Joy
    sent it as a separate patch [10].
  - Nelly's promisor.fetchOrder patch [11]. Setting the new variable
    made "git show" in a partial clone die with a BUG() in
    string-list.c, because an in-place split was used on a list that
    duplicates its strings.
  - shrimech's Rust patch for formatting object IDs [12]. "cargo
    test" passed and the new test failed without the fix. I asked
    whether algo 0 should be printed the way the C code does it.
    brian explained that algo 0 coming from C is meant to panic.


3. Plan
-------
Before the internship, I plan to land my two topics. If there is
time, I would also send one small git.c patch so that
--git-completion-helper works outside a repository, as was done for
--help-all last year. That patch needs both of my topics first.

To pick the commands, I started from the 29 builtins marked
NO_PARSEOPT in git.c. I also read builtins that loop over argv by
hand before or after their real parse. That is how I found stash
show and the --no-index loop in "git diff". I ran each
candidate on master (fb255012ac), and I was able to reproduce
everything below. Other places I found are in section 4.

A. Convert hand-written parsing to parse-options

  1. stash show. git stash show has a problem when somebody uses
     -G <word>, which keeps only the changes that mention <word>.
     Running "git stash show -p -Gbar" shows the diff of the patch,
     but running "git stash show -p -G bar" fails with "error: bar is
     not a valid reference". -S has the same problem.

     It fails because, after parse_options(), stash show runs a loop
     over each argument that is left over. If that argument starts
     with "-", it goes to the diff options. If it does not, it is
     treated as a stash name. That is also what happens to "bar".
     Since it is treated as a stash name, and it is not a valid one,
     the command fails.

     blame and shortlog do not have this problem. They parse their
     own options and the diff options together with
     parse_revision_opt(), so -G gets its value. I would do the same
     in stash show. What is left at the end would still be the stash
     name, so "git stash show 1" keeps working. Some options, like
     --all, are ignored today but would start to fail, so the commit
     message has to say so.

  2. Numbers in index-pack and fetch-pack. index-pack reads
     --max-input-size with strtoumax(). The issue is that it only
     reads the digits at the start, and silently ignores the rest of
     the characters:
       git index-pack --stdin --max-input-size=1k   limit of 1 byte
       git index-pack --stdin --max-input-size=abc  no limit
     So "1k" means 1 byte, and "abc" means 0, which means no limit.
     fetch-pack reads --depth with strtol() in the same way, so a bad
     depth fetches the whole history:
       git fetch-pack --depth=abc ../src HEAD       no depth limit
     The receive.maxInputSize config rejects "abc" and reads "1k" as
     1024, so --max-input-size and the config do not agree.

     These are plumbing, and Junio asked on the diff-files
     conversion whether converting is worth it for plumbing "meant
     for use by scripts" [13]. So I would send small fixes for the
     numbers, and convert the rest only if the list wants it.
     unpack-objects has the same --max-input-size code, so I would
     ask on the list whether to fix it in the same series.

  For each command I will first add tests for what works today, like
  "git stash show 1". The test for the bug must fail before the fix.
  I will also break each new branch on purpose, to check that a test
  catches it.

B. Improve the early scan where "git diff" needs it

  Before parsing anything, "git diff" runs a small loop whose job is
  to look for --no-index. The loop's answer determines which parser
  reads the rest of the command line: either the no-index one or the
  normal one. An issue arises because the loop stops at the first
  argument that doesn't start with "-". So in "git diff -G x
  --no-index a b", it stops at "x", the value of -G, and never sees
  --no-index. Since it never saw --no-index, the loop picks the
  normal parser, which then gets --no-index as an option and fails
  with "invalid option".

  The opposite also happens. In "git diff -S --no-index HEAD~1 HEAD",
  "--no-index" is the value of -S, a search for that text. But the
  loop takes it for the option, so Git switches to no-index mode and
  fails with "Could not access 'HEAD~1'". Inside a repository, with
  a and b in the work tree:
       git diff -G x --no-index a b         error: invalid option:
                                            --no-index
       git diff -S --no-index HEAD~1 HEAD   error: Could not access
                                            'HEAD~1'
  With the value stuck (-Gx, -S--no-index), both work.

  Christian's v1 cover letter lists "git diff" among the commands
  that could use the early scan, but were left out because it "has a
  number of short options like `-S`, `-G`, `-O` taking separate
  values" [14].

  Junio asked in Azuama's rev-list thread whether a separate early
  scan is a good direction at all [15]. For "git diff" I think one is
  still needed, because --no-index has to be found before either
  parser runs.

  But early_scan_options() cannot be used here yet. It needs a list
  of the options, and "git diff" does not have one in a single place.
  It also stops at every short option, like -G, so it would first
  have to learn to step over them. For long options, patch 2/4 of
  Christian's series split lookup_long_opt() out of parse_long_opt(),
  so the scan can find them. Short options have nothing like that yet.

  Where the loop stops also decides when two paths outside the
  repository are treated as --no-index. So "git diff -G x ../a ../b"
  fails too, while "git diff ../a ../b" works. t4053, the test script
  for --no-index, has no case with a short option before --no-index.
  So I will add tests for these three cases first.

  Either way, the loop should not keep its own list of the options
  that take a value. In his reply to Azuama's v4 [16], Junio called
  that approach "inherently fragile".

  Question: should I work towards using the early scan here, or would
  you rather see the loop in "git diff" fixed first?


4. Other places I found
-----------------------
These are not in the timeline, but I would pick one of them with the
mentors if there is time.

git daemon. It reads all its options by hand. --port is read with
strtoul(), and nothing checks that the number is a valid port:
       git daemon --port=70000 ...    listens on port 4464, no error
       git daemon --port=0x2329 ...   listens on port 9001
A small fix would be a range check. A full conversion to
parse-options has to keep every argument form working, because
existing inetd and start-up scripts call it with those arguments.

Revision options. handle_revision_opt() in revision.c parses the
options of log, rev-list, show and others by hand. Christian's v1
cover letter suggests converting them to struct option [14]. The
diff options were moved to parse-options in 2019, but the revision
options were not. This is core code, so I would only start it if the
mentors want it.


5. Timeline
-----------
Now to 6 Dec   Land tj/drop-no-parseopt and tj/annotate-blame-args.
Weeks 1-2      stash show: tests first, then the fix.
Weeks 3-4      The number fixes in index-pack and fetch-pack, and the
               "git diff" tests from 3B. This is the holidays, so
               small patches.
Weeks 5-10     "git diff" --no-index, in the way the list prefers.
               RFC first if it changes the scan.
Weeks 11-12    Rerolls, or the git daemon range check if there is
               time.
Week 13        Spare week for late reviews, and the final blog post.

Review on the list takes days, so the weeks overlap. While one series
waits, I work on the next one.

At minimum: stash show and the number fixes in 'next', and the "git
diff" tests and a fix sent and discussed. If the list does not want
the early scan there, the fix is the loop in "git diff" alone. If I
fall behind, I drop the section 4 item first.


6. Communication
----------------
A blog post every week for Christian, and a short written update
every week for Siddharth, by email. The update says what I sent,
what is in review, what is blocked, and what I will do next week.
Design questions go to the list before code. I answer every review
comment on the list before sending another version, as Kaartic
reminded me to.


7. Questions for mentors
------------------------
1. Should I start with stash show, or would you pick another command?
2. Which item from section 4, if any, would you like to see after
   that?
The question about "git diff" is at the end of 3B.


8. After the internship
-----------------------
I want to keep working on the early scan and the revision options,
and keep reviewing parse-options patches. I would also like to help
the next group of applicants the way reviewers on the list have
helped me.


9. References
-------------
[1] Proposal v1:
     https://lore.kernel.org/git/20261105195730.1518433-1-tangajoris@gmail.com/
[2] Microproject, checkout: use Q_():
     https://lore.kernel.org/git/20261022004834.246892-1-tangajoris@gmail.com/
[3] tj/drop-no-parseopt v5:
     https://lore.kernel.org/git/20261105223458.1534785-1-tangajoris@gmail.com/
[4] annotate: pass the arguments to blame unchanged:
     https://lore.kernel.org/git/20261105073101.1449292-1-tangajoris@gmail.com/
[5] What's cooking in git.git (Nov 2026, #03):
     https://lore.kernel.org/git/xmqqmrrlfzcd.fsf@gitster.g/
[6] apply fix for "git apply --check":
     https://lore.kernel.org/git/20261107195429.96842-1-tangajoris@gmail.com/
[7] My review of Azuama's rev-list v3:
     https://lore.kernel.org/git/CAKrRMNrnd2+HxTxadKaha+K9ASWi_oN4h7gdjVYAGmyODwmi=w@mail.gmail.com/
[8] Azuama's rev-list v4:
     https://lore.kernel.org/git/20261105120937.10269-1-azuama69@gmail.com/
[9] My review of Joy's pack size limit v3:
     https://lore.kernel.org/git/CAKrRMNqzOX=vUUMrMf5P_Atar8_qCSbGdtBktfT+iL4hJfFX-Q@mail.gmail.com/
[10] Joy's separate patch for 1/2:
     https://lore.kernel.org/git/pull.2298.git.1793966368860.gitgitgadget@gmail.com/
[11] My review of Nelly's promisor.fetchOrder patch:
     https://lore.kernel.org/git/CAKrRMNpAW+f2XXMFqpB8ffZsTdyPsaFtHe8dku=JVXACp1M3Hw@mail.gmail.com/
[12] My review of shrimech's Rust patch:
     https://lore.kernel.org/git/CAKrRMNrLvPck5psPpKX1eWe26To5srfFSLFObZ4zTX09RE551w@mail.gmail.com/
[13] Junio on the diff-files conversion:
     https://lore.kernel.org/git/xmqq8q3blfh1.fsf@gitster.g/
[14] Christian's early-scan v1 cover letter:
     https://lore.kernel.org/git/20260902161047.476753-1-christian.couder@gmail.com/
[15] Junio on whether rev-list needs an early scan:
     https://lore.kernel.org/git/xmqqa4nlkelo.fsf@gitster.g/
[16] Junio on Azuama's rev-list v4:
     https://lore.kernel.org/git/xmqqwlqqokxr.fsf@gitster.g/

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-11-09 15:10 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-01 21:06 [Outreachy] Proposal draft v1: Improve how command arguments and options are scanned and parsed Cynthia Uche
2026-11-04  2:53 ` [Outreachy] Proposal draft v2: " Cynthia Uche
2026-11-05  0:08   ` Usman Akinyemi
2026-11-05  0:55     ` Priestess
2026-11-06 20:24   ` Christian Couder
2026-11-07  8:49     ` [Outreachy] Proposal draft v2: Improve how command Cynthia Uche
2026-11-08 22:06   ` [Outreachy] Proposal draft v3: Improve how command arguments and options are scanned and parsed Cynthia Uche
  -- strict thread matches above, loose matches on Subject: below --
2026-11-08 17:50 [Outreachy] Proposal draft v2: " Tanga Joris
2026-11-09 15:09 ` [Outreachy] Proposal draft v3: " Tanga Joris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox