Git development
 help / color / mirror / Atom feed
* [Outreachy] Proposal draft v1: Improve how command arguments and options are scanned and parsed
@ 2026-11-05 19:57 Tanga Joris
  2026-11-08 17:50 ` [Outreachy] Proposal draft v2: " Tanga Joris
  0 siblings, 1 reply; 4+ messages in thread
From: Tanga Joris @ 2026-11-05 19:57 UTC (permalink / raw)
  To: git
  Cc: christian.couder, siddharthasthana31, kaartic.sivaraam,
	usmanakinyemi202, Tanga Joris

Hi everyone,

This is the first draft of my proposal for the project "Improve how
command arguments and options are scanned and parsed" for the
December 2026 Outreachy round. I would be grateful for feedback,
especially on sections 4 and 5.

In short: before a command does any real work, it has to answer
requests like "-h", "--help-all" and "--git-completion-helper". Today
that happens in three places that do not agree. On master, all 120
builtins that use parse-options answer "-h" outside a repository,
but only 31 of them answer "--git-completion-helper" there. I would
like to make these entry points behave the same for every command,
guard that with tests, and then move the early scanning of options
towards parse-options itself.


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

I also meet all the Outreachy eligibility rules.

I am a curious developer who loves playing around with systems.
Identifying and solving technical problems is my jam, because it
gives me the sense that I am making an impact somewhere. I am about
to graduate from the United States International University - Africa,
where I study Applied Computer Technology. I've had the luxury of
playing a lot with C during my time learning at ALX, where it was my
introduction to programming.

I first met this kind of parsing problem in my own project, kiln
(https://github.com/tmjoris/kiln), which is a Go CLI and daemon that
runs code in Firecracker or gVisor sandboxes. Its main() parses
the global flags with flag.Parse(), which stops at the subcommand,
and each subcommand parses the rest with its own flag.FlagSet. So
"kiln -addr host:7070 run" works, but "kiln run -addr host:7070"
fails with "flag provided but not defined: -addr". 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
  https://lore.kernel.org/git/20261022004834.246892-1-tangajoris@gmail.com/
  Topic tj/checkout-orphan-more-plural, merged to master in
  a04194e134. v2 reworded the log message as Junio suggested and
  also converted "only one reference expected, %d given." after
  Tuomas's comment.

Series: drop NO_PARSEOPT from builtins that use parse-options
  v1: https://lore.kernel.org/git/20261024052449.95869-1-tangajoris@gmail.com/
  v4: https://lore.kernel.org/git/20261105003914.708836-1-tangajoris@gmail.com/
  Topic tj/drop-no-parseopt.

  patch-id, diff-pairs, fast-import and var use parse-options but
  were still marked NO_PARSEOPT, so bash completion never asked them
  for their options. René noticed that once fast-import was
  unmarked, "git fast-import --<TAB>" would wait for input, because
  fast-import reads its "feature" lines before it parses options. My
  v2 special-cased that in fast-import. Junio found it "way too ugly
  to target this so specifically" and suggested teaching
  show_usage_with_options_if_asked() to answer the completion helper,
  which v3 does. In v4, Kaartic's review added a pointer to the next
  commit, a comment in parse-options.h, and a t0012 test that catches
  builtins that use parse-options but are still marked NO_PARSEOPT.
  Cynthia showed that my test did not cover the "-all" form. When I
  tried her test I found it still passed with the fix reverted, so I
  added the leftover tmp_pack_* check to it.

  The lesson I took from Junio's comment is the one this proposal is
  built on: when a command has to answer something before it starts
  working, the answer belongs in the shared place every command
  already goes through, not in one command.

Patch: annotate: pass the arguments to blame unchanged
  https://lore.kernel.org/git/20261105073101.1449292-1-tangajoris@gmail.com/
  cmd_annotate() put "-c" in front of the user's arguments, so
  "--git-completion-helper" was no longer the lone argument and the
  diff option parser answered it instead. "git annotate --<TAB>"
  offered the options of "git diff", without --porcelain. I found it
  while trying to make the completion helper work outside a
  repository (section 4.A): with only that git.c change,
  "git annotate --git-completion-helper" segfaults there.
  Junio: "Nice simplification." ... "Looking good. Thanks."

Bug triage: "Git crashed on `git apply --check`"
  https://lore.kernel.org/git/CALdwsC1cMKZgRPZA+pBaJe4PPs9q9W0K9CQLmM2bgUCyysTB-w@mail.gmail.com/
  I reproduced the assertion failure and found the trigger in the
  reporter's file: a blank line after the "diff --git" line of a
  new-file section. An indented "new file mode" line and a
  "--- /dev/null" before "new file mode" crash it too. All three still
  crash on master; the fix is my next patch.

Reviews:
  rev-list: skip --grep argument in early scan (Azuama), v3
  https://lore.kernel.org/git/CAKrRMNrnd2+HxTxadKaha+K9ASWi_oN4h7gdjVYAGmyODwmi=w@mail.gmail.com/
  The tests fail without the fix, but --author, --committer and
  --grep-reflog still have the same problem, and the NEEDSWORK
  example above the loop becomes stale.

  promisor-remote: honor promisor.fetchOrder when trying remotes (Nelly)
  https://lore.kernel.org/git/CAKrRMNpAW+f2XXMFqpB8ffZsTdyPsaFtHe8dku=JVXACp1M3Hw@mail.gmail.com/
  Setting the new variable makes "git show" in a partial clone die
  with a BUG() in string-list.c, because an in-place split is used on
  a list that duplicates its strings.

  hash: handle invalid algorithms when formatting object IDs
  (shrimech, Rust)
  https://lore.kernel.org/git/CAKrRMNrLvPck5psPpKX1eWe26To5srfFSLFObZ4zTX09RE551w@mail.gmail.com/
  "cargo test" passes and the new test panics without the fix. I
  asked whether algo 0 should be printed the way the C code does it.

  config: move pack size limit into repo config (Joy), v3
  https://lore.kernel.org/git/CAKrRMNqzOX=vUUMrMf5P_Atar8_qCSbGdtBktfT+iL4hJfFX-Q@mail.gmail.com/
  2/2 adds a member to struct repo_config_values, which Junio's
  jc/freeze-repo-config-values asks not to do for now, and v3
  conflicts with cc/pack-index-v3 in 'seen'.

Blog: https://tmjoris.github.io ("From git config to git
send-email", about my first weeks on the list).


3. Where commands look at their arguments today
-----------------------------------------------
Before a builtin's real option parsing runs, its arguments can be
looked at in three places:

  1. git.c, run_builtin(): decides whether to set up the repository.
     A lone "-h" or "--help-all" turns RUN_SETUP into
     RUN_SETUP_GENTLY, so "git cmd -h" works outside a repository.
  2. show_usage_with_options_if_asked(): called by 15 builtins that
     must answer before doing any work (fast-import reads its input,
     others need a repository). With my series it answers -h,
     --help-all and both completion helpers.
  3. parse_options() itself answers them for everyone else, but only
     when the request is the lone argument.

Then there are the early scans: commands that read a few options
before the real parse, such as fast-import's
--allow-unsafe-features, column's --command, and the hand-written
loop in rev-list (the one Azuama's patch touches, whose NEEDSWORK
comment calls it "utterly broken"). Christian's cc/early-scan-options
gives them early_scan_options(), which reads the same struct option
array as parse_options(). Junio's view on its first round is the
direction I want to follow: "in the long term we would rather see a
properly refactored parse-options machinery perform the prescan
(perhaps with some kind of 'dry-run' option given to the machinery)
than yet another ad-hoc parser".

Junio was also clear, on the diff-files conversion, that converting
a parser is not worth it on its own for plumbing "meant for use by
scripts". So in this proposal every change has to fix something a
user can see, or remove a place where two parsers can disagree.


4. Plan
-------
A. Make the entry points agree (weeks 1-4)
   - Teach git.c to set up gently for --git-completion-helper and
     --git-completion-helper-all, as 129b3632f3 did for --help-all,
     so that completion works outside a repository. This needs my
     NO_PARSEOPT series and the annotate fix first: with the git.c
     change alone, annotate, am, ls-files and checkout-index
     segfault outside a repository, and branch, commit, merge,
     rebase, status and update-index hit a BUG(). With all three,
     all 124 parse-options builtins answer there. Add the case to
     the per-builtin loop in t1517-outside-repo.sh.
   - Look for other commands that rewrite their argv before handing
     it to another cmd_*(), the way annotate did.
   - Turn the checks I ran by hand into tests: t1517 for "outside a
     repository", t0012 for "parse-options users are not marked
     NO_PARSEOPT", and a check that no helper reads stdin.

B. Conversions only where there is a concrete reason (weeks 4-8)
   - merge-index: -o and -q are only recognised in that order. On
     master:
       git merge-index -o -q cat -a   -> works
       git merge-index -q -o cat -a   -> fatal: git merge-index: cat
                                         not in the cache
     Parse the leading options with parse-options and
     PARSE_OPT_STOP_AT_NON_OPTION, and drop its NO_PARSEOPT mark.
     Tests in t6060.
   - Early scans that disagree with the real parse, like the
     rev-list loop above, but only with a reproduced user-facing bug.
     column, bisect and check-ref-format are in Cynthia's plan, so I
     would not touch them.

C. Towards parse-options doing the prescan (weeks 8-13, RFC first)
   Christian's series already lets the scan and the real parser look
   options up the same way. The scan still stops at short options and
   at options with low-level callbacks, because it cannot run them.
   I would like to study what a "dry run" mode of parse_options()
   would need (no writes through opt->value, no callbacks, no die()),
   send that to the list as an RFC, and implement the first step only
   if the design is agreed. Otherwise it becomes the final report's
   list of open problems.

Every change is tested first: the test must fail on master, every new
branch must make a test fail when I break it on purpose, and output
and exit codes are compared with master. That is how I found the
annotate bug.


5. Timeline
-----------
Now to 6 Dec   Land tj/drop-no-parseopt and the annotate fix. Send
               the git apply fix. Keep reviewing parse-options patches.
Weeks 1-2      git.c gentle setup for the completion helper, with the
               t1517 test. First blog post.
Weeks 3-4      Turn the outside-repository checks into tests; look
               for other argv-rewriting wrappers like annotate.
Weeks 5-8      merge-index; early-scan fixes with a reproduced bug.
Weeks 8-11     "dry run" study and RFC on the list.
Weeks 11-13    First step of the RFC if agreed, otherwise the open
               problems report. Rerolls, final blog post.
Every week     A blog post and a written update to the mentors.

If I fall behind, I drop C's implementation first (keeping the RFC),
then B's early-scan items.


6. Communication
----------------
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. I will blog every week at https://tmjoris.github.io,
which anyone can read without an account, and link each post on the
list.


7. After the internship
-----------------------
I want to keep working on whatever is left in the final report,
especially the parse-options dry run, keep the entry-point tests up
to date as commands change, and keep reviewing parse-options patches.
I would also like to help the next group of applicants the way
Kaartic, René, Junio and Cynthia helped me.


8. References
-------------
[1] Microproject: https://lore.kernel.org/git/20261022004834.246892-1-tangajoris@gmail.com/
[2] tj/drop-no-parseopt v1 and René's review:
    https://lore.kernel.org/git/20261024052449.95869-1-tangajoris@gmail.com/
    https://lore.kernel.org/git/8b60c7f6-4a1c-495a-a594-aaf322279539@web.de/
[3] v2 1/2 and Junio's suggestion:
    https://lore.kernel.org/git/20261029012616.139783-1-tangajoris@gmail.com/
    https://lore.kernel.org/git/xmqqy0bg2xj5.fsf@gitster.g/
[4] v3, Kaartic's and Cynthia's reviews, and v4:
    https://lore.kernel.org/git/20261029231621.48958-1-tangajoris@gmail.com/
    https://lore.kernel.org/git/123de4e4-3387-4eef-8a05-9ed1ad1fce30@gmail.com/
    https://lore.kernel.org/git/d026d6a0-5b1b-4bbf-bd1f-5f41c9c2a4e4@gmail.com/
    https://lore.kernel.org/git/20261103092503.14872-1-herroyalpriestess@gmail.com/
    https://lore.kernel.org/git/20261105003914.708836-1-tangajoris@gmail.com/
[5] Junio on the early-scan v1 cover (long-term direction):
    https://lore.kernel.org/git/xmqqpkyviizc.fsf@gitster.g/
[6] Junio on diff-files v6 (value of converting plumbing):
    https://lore.kernel.org/git/xmqq8q3blfh1.fsf@gitster.g/
[7] 129b3632f3 (builtin: also setup gently for --help-all, 2025-08-03)
[8] cc/early-scan-options v4:
    https://lore.kernel.org/git/20261102151447.734680-6-christian.couder@gmail.com/

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [Outreachy] Proposal draft v2: Improve how command arguments and options are scanned and parsed
  2026-11-05 19:57 [Outreachy] Proposal draft v1: Improve how command arguments and options are scanned and parsed Tanga Joris
@ 2026-11-08 17:50 ` Tanga Joris
  2026-11-09 15:09   ` [Outreachy] Proposal draft v3: " Tanga Joris
  0 siblings, 1 reply; 4+ 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] 4+ messages in thread

* [Outreachy] Proposal draft v3: Improve how command arguments and options are scanned and parsed
  2026-11-04  2:53 [Outreachy] Proposal draft v2: " Cynthia Uche
@ 2026-11-08 22:06 ` Cynthia Uche
  0 siblings, 0 replies; 4+ messages in thread
From: Cynthia Uche @ 2026-11-08 22:06 UTC (permalink / raw)
  To: herroyalpriestess; +Cc: christian.couder, git, siddharthasthana31

Hi team,

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

Changes since v2
================
 - A short summary at the top and a new "How I work" section.
 - Section 1: updated, including how I have been preparing.
 - Section 2: the outcomes of my reviews so far (including
   Shashank's v3), my review of Karthik's patch and a doc fix.
 - Sections 3.4 to 3.6: v4 of the early-scan series, and a
   corrected account of what needs a decision in a conversion.
 - Sections 4 and 5: updated after Christian's review of v2 [18].
   column is deliberate and is no longer a target, check-ref-format
   is already being converted, and bisect is described correctly and
   becomes the main target.

In short
--------
Some commands read a few options before parse_options() runs, and the
two can disagree: fast-import's hand-written scan could allow unsafe
features on a command line that parse_options() reads differently.
Christian's early-scan series fixes that, and this project extends the
same idea to the other places that parse arguments by hand. When I
counted on origin/master, 29 entries of the command table in git.c
were still marked NO_PARSEOPT. I would start with the sub-commands of
git bisect: cmd_bisect() uses parse_options() only to dispatch them,
and bisect_start(), bisect_run() and bisect_terms() parse their
arguments by hand. I studied other candidates too; column turned out
to be deliberate and check-ref-format is already being converted, so
section 4 explains why I left them. So far: a merged microproject,
four reviews and a doc fix.


1. About me and logistics
-------------------------
Name:     Cynthia Uche. I'm also called Priestess.
Pronouns: she/her
Email:    herroyalpriestess@gmail.com
IRC:      Priestess
GitHub:   https://github.com/herr-priestess
Blog:     https://herr-priestess.github.io/
LinkedIn: https://linkedin.com/in/thecynthiauche
Location: 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, and contributing to the Git project is a good
opportunity to revisit and relearn it.

Earlier Outreachy: I have a merged patch in OpenStack Shared File
Systems Service (Manila)
(https://review.opendev.org/c/openstack/manila/+/982431), made during
the contribution period of the summer 2026 Outreachy round. That was
my first time using IRC, Gerrit and Launchpad bug tracking, so working
outside GitHub was not new to me when I started with Git's mailing
list. Git's list is different, and I have learned it in public over
the last few weeks.

Availability: 30 hours per week from 7 December 2026 to 8 March 2027.

How I have been preparing: the microproject page asks applicants not to
start the internship project itself, so I have worked on the code
around it: reviews, a doc fix and tests. I have read parse-options.c,
parse-options-cb.c, api-parse-options.adoc, the early-scan series v1
to v4, fast-import.c, column.c, bisect.c, check-ref-format.c, t0040,
t1402, t9300 and t9002.

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 'next' on 2026-10-24 and graduated to master
          (merge commit 84bea4324a); What's cooking Nov 2026 #01
          [6b] lists it under "Graduated to 'master'".
  Patrick reviewed it and found it good, so it needed no second
  version.

Review of the early-scan series:
  (a) [PATCH v3 4/4] fast-import: use early_scan_options() ... [8]
  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.
  Outcome: Christian replied on 2 Nov and sent v4 [12]. In v4 the
  call site comment explains why the flag is passed, the header
  comment of early_scan_options() says when callers should pass it
  and when they should not use the scan, and a new t9300 test, "R:
  --allow-unsafe-features after a non-option", covers that command
  line. Kaartic then wrote that v4 looks good to him, and Junio
  that he would mark the topic for 'next' [13].

  I tested v4 (tip c2b4099f84 in origin/seen): t0040 (118 tests) and
  t9300 (262 tests) pass, and the command lines from my review and
  the new test behave as described.

Other reviews:
  (b) [PATCH v2] test-chmtime: use parse_options() ... [14]
  With the PARSE_OPT_KEEP_UNKNOWN_OPT argument set to 0, the new
  t0082 still passed 3 of 3, while "test-tool chmtime --get -86400"
  failed with "unknown switch `8'". I suggested a test for negative
  offsets and listed three behaviour changes that the commit message
  did not mention: flags after the timespec are now accepted,
  "-g -v" now dies, and usage errors exit with 129 instead of 1.
  Outcome: Shashank answered that all three are intended, and sent
  v3 on 2 Nov [14]. I checked v3 on top of master (0ffa3e35c7):
  t0082 passes 4 of 4 with the negative-offset test, that test fails
  when I set the PARSE_OPT_KEEP_UNKNOWN_OPT argument to 0, the commit
  message now lists the three behaviour changes, and the output
  matches a build of master for the command lines I compared.

  (c) [Outreachy PATCH v3 1/2] parse-options: answer
  --git-completion-helper before command runs [15]
  On master (ebdc087dc9) with the series applied, t9300, t0012 and
  t9902 pass, and "git --list-cmds=parseopt" lists the four builtins
  that the series unmarks. I compared "git <cmd> --git-completion-
  helper" and the "-all" form for every builtin, with builds of
  master and of the series, and the output was identical. The new
  test only covers the plain form: with the "-all" branch changed to
  show_gitcomp(opts, 0) it still passed. I suggested a test for that
  branch.

  (d) [PATCH] api-parse-options: add missing flags and macros
  (Karthik Nayak) [20]
  I built the page and checked the behaviour text against
  parse-options.c and parse-options-cb.c. The commit message listed
  OPT_NO_WITH although the macro is OPT_WITHOUT, and did not list five
  macros that the diff adds; OPT_ALIAS was undocumented. Karthik
  fixed the message and added OPT_ALIAS.

Doc fix: api-parse-options.adoc, restoring a dropped word [21]
  The description of PARSE_OPT_KEEP_UNKNOWN_OPT lost the word
  "stopping" in 0d260f9a09 (2009). Junio: "Makes sense. Thanks for
  finding and fixing."

How these relate to the project: its tasks are to find hand-rolled
argument parsing, move it to parse-options or the early scan, and
discuss the patches with reviewers. The four reviews are in that code:
the early-scan series itself, a test helper moved to parse_options(),
the completion code in parse-options.c, and the parse-options API
document. Each time I started from running something (a mutation of
one argument, a comparison with a base build, a loop over all
builtins) and not from reading alone. In (a) that led to changes in
v4. Junio told another applicant that topics move forward through
positive reviews [16], and that is how I want to work during the
internship: start from the failing command line, make it a test, then
change the code.

(b) is also a real case of the flag that the v4 header tells callers
of the early scan to avoid: test-chmtime keeps
PARSE_OPT_KEEP_UNKNOWN_OPT so that a negative offset like "-86400" is
not rejected as an unknown switch, so a command like that could not
use early_scan_options() as it is today. So before choosing a
candidate in section 4, I check whether its parse_options() call uses
that flag.

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 v4 with its reviews; reproduced the behaviour
of the candidates in section 4; reviewed 4/4 of v3 and tested v4, and
reviewed three other patches (above).

How I work
----------
For every change: (1) a command line that shows the current behaviour,
with its exit status, on a base build; (2) a test that fails on that
base; (3) the change; (4) the same test with the fix broken on purpose,
and with the whole fix reverted; (5) a comparison of output and exit
status with the base build over the commands I touched. Examples are
the mutation runs in (b) and (c).


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. (This example is from the 4/4 commit
   message [4]; I did not run it on an old build.)

3.2 The API in v3 and v4

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. As of v4 the
header also says that passing it in other cases is safe, and useful
when the caller rejects non-option arguments itself, and that the
scan should not be used when parse_options() is called with
PARSE_OPT_KEEP_UNKNOWN_OPT [12]. Since v4, early_scan_options() also
calls parse_options_check() on the options, so a bad PARSE_OPT_EARLY
mark is caught before the scan relies on it.

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.
Christian noted that improving the early scan can lead to
refactorings like the one in 2/4, which could make a dry run easier
to implement [18].

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.
 - v4 (2 Nov, 4 patches): a switch in parse_long_opt() (2/4);
   parse_options_check() called from early_scan_options() and clearer
   header comments (3/4); a clearer comment at the call site and a
   new t9300 test (4/4).

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]. Junio then said that "--"
detection could come for free as a side effect of using
parse_options() [17], and Christian wrote on 6 Nov that an early scan
may not be needed at all for it [18].

3.5 Current state and open follow-ups

v4 was sent on 2 Nov and addresses Kaartic's points on v3 [5]: the
switch in 2/4, the parse_options_check() call and clearer comments in
3/4. Kaartic said v4 looks good, and Junio replied that he would mark
the topic for 'next' [13]. It was merged to 'next' on 6 Nov.

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.

3.6 Where a conversion is not mechanical
----------------------------------------
Studying the candidates, I found three places where converting to
parse-options needs a decision, not only a rewrite:

 (a) Arguments that start with "-". test-chmtime must accept
     "-86400". With the PARSE_OPT_KEEP_UNKNOWN_OPT argument set to 0,
     "--get -86400" failed with "unknown switch `8'" (exit 129) [14],
     and the v4 header of early_scan_options() says the scan should
     not be used with that flag [12]. I check this for every
     candidate before choosing it.
 (b) check-ref-format --branch must come first and take exactly one
     argument (argc == 3 in the code). parse-options has helpers for
     options that exclude each other (die_for_incompatible_opt2() to
     4()), but I did not find one for "this option excludes all
     others", so a conversion needs an explicit check. Hussain's v2
     [19] keeps the argc == 3 check before calling parse_options().
 (c) bisect: the sub-command functions receive what the dispatcher
     left unprocessed. I still need to check which flags a second
     parse_options() call needs there, for example for the revisions
     that follow the options and for "--" in bisect_start(). That is
     the first thing I would study in week 1.

For each, I would ask on the list and use the least invasive fix (a
check in the caller) before proposing any change to parse-options.


4. Candidate inventory and risks
--------------------------------
Method: git grep -n NO_PARSEOPT -- git.c lists the builtins marked as
not using parse-options, and I use it as the backlog. When I counted
on origin/master, 29 entries were marked (26 distinct functions, since
the four merge-recursive entries share one). The marks can be stale:
fast-import is marked, but it calls parse_options() in parse_argv(),
so I check each file before choosing it. 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). Observations in this section were made on origin/master
at v2.56.0-307-g50fe5cc63f, unless stated otherwise. What's cooking
lists only the topics that Junio has picked up, so I also search lore
for patches in flight before choosing a target; Christian's review
[18] is how I learned that check-ref-format was taken.

 - bisect (builtin/bisect.c): proposed main target
   cmd_bisect() uses parse_options() only to dispatch the sub-commands
   (OPT_SUBCOMMAND with PARSE_OPT_SUBCOMMAND_OPTIONAL), and the
   sub-command functions do not call it [18]. 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. A conversion would give each
   of them its own parse_options() call.
   Fits: in the order bisect_terms(), bisect_run(), bisect_start().
   Junio said "--" detection could come for free from parse_options()
   [17], so these conversions may not need the early scan.
   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.
   Before the internship I will search lore again, to make sure nobody
   else is converting it.

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

Studied and not proposed:

 - check-ref-format (builtin/check-ref-format.c): already being
   converted. Hussain Sitamouwala sent "check-ref-format: use
   parse-options API" (v1 on 29 Oct, v2 on 3 Nov) [19], as Christian
   pointed out [18], and Fien Dora plans the same [10]. I will not
   start it. What I found on master: 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. Observed:
     --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
   Hussain's v2 commit message states three of the differences that I
   listed as risks: abbreviations such as --norm and "--" before
   <refname> are now accepted, and options after <refname> are still
   rejected (PARSE_OPT_STOP_AT_NON_OPTION). It also sets
   PARSE_OPT_NONEG on --normalize, --print and --refspec-pattern,
   which covers the negated forms. v2 changes only
   builtin/check-ref-format.c, so the newly accepted forms have no
   tests yet; I leave that to the review in progress. Tests today:
   t1402 covers --normalize, --allow-onelevel and --refspec-pattern,
   alone and combined, and --branch (also from outside a repository,
   and with the name -nain). Fien Dora's tests for the precedence of
   --allow-onelevel and --no-allow-onelevel were merged to 'next' on
   1 Nov and are marked "Will merge to 'master'" in What's cooking
   #01 [6b] [10].

 - git column (builtin/column.c): deliberate, not a target
   It already uses parse_options(). Only the early look at argv[1] for
   "--command=" is by hand, and the code says "This one is special and
   must be the first one". Christian said the rule looks deliberate,
   and that git tag, the only internal user, does not use --command
   [18]. 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)
   I do not plan to change it.

 - rerere (Patrick's ps/rerere-subcommands, v2 of 29 Oct, would
   convert it to OPT_SUBCOMMAND [9]; "Needs review" in What's cooking
   #01 [6b]).
 - 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 (a much larger
   change).

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 until it reaches
               master, review related patches, and discuss the
               target with the mentors (my proposal is bisect).
Weeks 1-2:     Agree on the first target with the mentors. Study how
               a second parse_options() call behaves after the
               sub-command dispatch (3.6 (c)). Write tests that show
               the current behaviour of bisect_terms() and
               bisect_run(). Start the weekly blog.
Weeks 2-6:     First series: bisect_terms() and bisect_run(), tests
               first, then the conversion.
Weeks 5-9:     Second series: bisect_start(). It is a main item,
               with tests for replay and the BISECT_LOG contents
               first.
Weeks 8-11:    Third series: unpack-objects (after listing its
               callers), or another command from the NO_PARSEOPT
               list that the mentors prefer; index-pack and
               fetch-pack only if time remains.
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 the third series first. If someone else takes bisect, or the
mentors prefer another command, I will pick the next candidate from
the NO_PARSEOPT list with them. 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 GitHub Pages 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
-----------------------
I like to give back to communities that have helped me grow. I have
volunteered as a backend technical coordinator for the Nexascale
community, where I helped new developers learn the ropes in backend
development. I have also helped to organize tech programs in my local
community (DevFest under GDG, Women Techmakers, SheCodeAfrica).

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/
[6b] "What's cooking" Nov 2026 #01:
    https://lore.kernel.org/git/xmqqcxsmejd9.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, v1 and v2:
     https://lore.kernel.org/git/20261101210623.622-1-herroyalpriestess@gmail.com/
     https://lore.kernel.org/git/20261104025340.7799-1-herroyalpriestess@gmail.com/
[12] v4 cover and Christian's reply to my review:
     https://lore.kernel.org/git/20261102151447.734680-6-christian.couder@gmail.com/
     https://lore.kernel.org/git/CAP8UFD2m+wpcgqB2SasWLxyekY+5khrYyPkOuLoYwANqZws8fw@mail.gmail.com/
[13] Kaartic on v4 and Junio's reply:
     https://lore.kernel.org/git/faf0b5e6-1c63-4c68-9f23-42a452879854@gmail.com/
     https://lore.kernel.org/git/xmqqv76dal2l.fsf@gitster.g/
[14] My review of test-chmtime v2, Shashank's answer and his v3:
     https://lore.kernel.org/git/20261102024623.24897-1-herroyalpriestess@gmail.com/
     https://lore.kernel.org/git/CANHmUQS15GGDOXm-WZ9M-UyKf2N0K+VixowL_heNKW0vjE0vJw@mail.gmail.com/
     https://lore.kernel.org/git/pull.2280.v3.git.1793639275978.gitgitgadget@gmail.com/
[15] My review of Tanga Joris's v3 1/2:
     https://lore.kernel.org/git/20261103092503.14872-1-herroyalpriestess@gmail.com/
[16] Junio to Tanga Joris, 26 Oct:
     https://lore.kernel.org/git/xmqq33ttvz0q.fsf@gitster.g/
[17] Junio on "--" detection coming for free from parse_options():
     https://lore.kernel.org/git/xmqqmrt7c1zf.fsf@gitster.g/
[18] Christian's review of my v2:
     https://lore.kernel.org/git/CAP8UFD0uA+zF4x8WBRA6sGxSGX52Nw=GFBznzdFnX3SptHDUcw@mail.gmail.com/
[19] Hussain Sitamouwala's check-ref-format patches (GitGitGadget
     pull request; the thread is on the list under the same subject):
     https://github.com/git/git/pull/2470
[20] Karthik Nayak's api-parse-options patch, my review and his
     answer:
     https://lore.kernel.org/git/20261105-kn-parse-options-doc-v1-1-403edda5947a@gmail.com/t/#u
[21] My doc fix and Junio's reply:
     https://lore.kernel.org/git/20261104224559.4197-1-herroyalpriestess@gmail.com/

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] 4+ messages in thread

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

Hi everyone,

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

Changes since v2:
 - Moved the git daemon conversion into the plan, so the plan does
   not depend on the early-scan work.
 - Tried early_scan_options() in "git diff", and added what happened
   to 3B.
 - Removed the fallback of fixing the loop alone from the minimum
   outcome.
 - Updated the timeline.

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
(https://github.com/tmjoris/kiln), 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.

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

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].
  Both topics merge cleanly on top of 'next', which has Christian's
  early-scan series, and t9300, t0040, t0012 and t9902 pass.
- 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. Phillip asked for an error instead of accepting
  these broken patches, and I agreed. v2 will follow once we settle
  the details.
- A fix for the splitIndex bug that Andy Koppe reported on the list
  [7]. With core.splitIndex and index.skipHash both set, index
  entries are lost when a new shared index is written.
- Reviews:
  - Edson's credential conversion [8]. I suggested a helper for the
    code that was repeated in four functions, and a test for the
    check on extra arguments, which no test covered.
  - Azuama's rev-list early scan fix [9]. 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
    [10]. Junio then asked for a different approach than a list of
    options in rev-list.c.
  - Joy's pack size limit patch [11]. 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 [12].
  - Nelly's promisor.fetchOrder patch [13]. 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 [14]. "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" [15]. 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.

  3. git daemon. It reads all its options by hand, in one loop of
     about 150 lines. --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
     I would convert the loop to parse-options, with
     PARSE_OPT_STOP_AT_NON_OPTION for the list of directories. Other
     programs outside builtin/, like imap-send and scalar, already
     use parse_options(). Existing inetd and start-up scripts must
     keep working, so every argument form stays, and --no- forms that
     do not exist today stay rejected. The daemon re-runs itself with
     its original arguments and --serve, and parse_options() changes
     argv, so I have to keep a copy. Tests in t5570.

     Junio's question about plumbing [15] may apply here too. No
     other Git program runs git daemon, which is why I picked it over
     index-pack and fetch-pack. I would ask on the list first. If the
     list does not want it, I would send a range check for --port and
     pick another command with the mentors.

  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" [16]. Few people type --no-index after -G. But "git diff -G
  x ../a ../b" does not need --no-index at all, and it fails too.

  Junio asked in Azuama's rev-list thread whether a separate early
  scan is a good direction at all [17]. For "git diff" I think one
  is still needed, because --no-index decides which parser reads the
  rest of the command line. So it has to be found before either
  parser runs.

  The early scan has one rule that I find useful. It may miss an
  option that parse_options() would accept, but it must never report
  one that parse_options() would not. So when it is unsure, it stops.
  Missing an option is the safe failure.

  To see how far the scan gets today, I replaced the loop with
  early_scan_options() on top of 'next', with add_diff_options() and
  --no-index as its table. "git diff -S --no-index HEAD~1 HEAD" then
  works. But "git diff -G x --no-index a b" still fails, and these
  stop working:
       git diff -p --no-index a b
       git diff -U1 --no-index a b
       git diff -Gx --no-index a b
       git diff -w ../a ../b
  The scan stops at every short option, so it never reaches
  --no-index. t4053 still passes with this change, because it has no
  case with a short option before --no-index. So the scan has to
  step over short options first. 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. I will add tests for
  these cases first.

  Question: should I teach the scan to step over short options? Or
  would a smaller fix be better, where the loop looks options up in
  the add_diff_options() table? Junio called a hand-kept list of
  options "inherently fragile" [18], so I don't plan on adding one.


4. Other places I found
-----------------------
This is not in the timeline, but I would start it with the mentors
if there is time.

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 [16]. 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-7      Convert git daemon to parse-options.
Weeks 8-12     "git diff" --no-index, in the way the list prefers.
               RFC first if it changes the scan.
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, the number fixes and the git daemon
conversion sent and reviewed, and the "git diff" tests and an RFC
sent. If I fall behind, the "git diff" work stops at the tests and
the RFC.


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. Would you like to see the revision options work from section 4?
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 v2:
     https://lore.kernel.org/git/20261108175038.946313-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] splitIndex fix:
     https://lore.kernel.org/git/20261108234235.1594316-1-tangajoris@gmail.com/
[8] My review of Edson's credential patch:
     https://lore.kernel.org/git/CAKrRMNqsh=d+_C8u+q1EBeQVwDZaywGQGZ89oYLyyvmB=8u1jw@mail.gmail.com/
[9] My review of Azuama's rev-list v3:
     https://lore.kernel.org/git/CAKrRMNrnd2+HxTxadKaha+K9ASWi_oN4h7gdjVYAGmyODwmi=w@mail.gmail.com/
[10] Azuama's rev-list v4:
     https://lore.kernel.org/git/20261105120937.10269-1-azuama69@gmail.com/
[11] My review of Joy's pack size limit v3:
     https://lore.kernel.org/git/CAKrRMNqzOX=vUUMrMf5P_Atar8_qCSbGdtBktfT+iL4hJfFX-Q@mail.gmail.com/
[12] Joy's separate patch for 1/2:
     https://lore.kernel.org/git/pull.2298.git.1793966368860.gitgitgadget@gmail.com/
[13] My review of Nelly's promisor.fetchOrder patch:
     https://lore.kernel.org/git/CAKrRMNpAW+f2XXMFqpB8ffZsTdyPsaFtHe8dku=JVXACp1M3Hw@mail.gmail.com/
[14] My review of shrimech's Rust patch:
     https://lore.kernel.org/git/CAKrRMNrLvPck5psPpKX1eWe26To5srfFSLFObZ4zTX09RE551w@mail.gmail.com/
[15] Junio on the diff-files conversion:
     https://lore.kernel.org/git/xmqq8q3blfh1.fsf@gitster.g/
[16] Christian's early-scan v1 cover letter:
     https://lore.kernel.org/git/20260902161047.476753-1-christian.couder@gmail.com/
[17] Junio on whether rev-list needs an early scan:
     https://lore.kernel.org/git/xmqqa4nlkelo.fsf@gitster.g/
[18] Junio on Azuama's rev-list v4:
     https://lore.kernel.org/git/xmqqwlqqokxr.fsf@gitster.g/

Thanks,
Tanga

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-11-09 15:10 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-11-05 19:57 [Outreachy] Proposal draft v1: Improve how command arguments and options are scanned and parsed Tanga Joris
2026-11-08 17:50 ` [Outreachy] Proposal draft v2: " Tanga Joris
2026-11-09 15:09   ` [Outreachy] Proposal draft v3: " Tanga Joris
  -- strict thread matches above, loose matches on Subject: below --
2026-11-04  2:53 [Outreachy] Proposal draft v2: " Cynthia Uche
2026-11-08 22:06 ` [Outreachy] Proposal draft v3: " Cynthia Uche

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