refactor(git): tokenize git show's objects instead of walking clusters - #4001
Merged
Merged
Conversation
📊 Automated PR Analysis
SummaryRefactors git show's argument scanning to reuse the arg_tokenizer's Attachment/ValueSpec grammar instead of a hand-rolled short-cluster walk, resolving two TODOs left pending on the prior ValueSpec factorization. It removes the now-unnecessary probe_is_blob pre-filter TODO deliberately, and adds differential and regression testing showing behavior parity with real git. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
`show_positionals` hand-rolled git's short-flag cluster grammar to find which
`git show` arguments are objects: a per-char walk that re-queried the flag table
through a `format!("-{c}")` allocation, plus its own `--`-boundary slice. The
tokenizer's `ValueSpec` table models exactly that — attached value, separate
value, and the solo-only cluster rule — so the walk is now `tokenize_grammar` +
`before_dashdash` + `Token::is_free_positional`.
It reads `git show` under diff's grammar rather than log's, matching the route
classifier beside it: `git show -wl 100` consumes the 100 as the rename limit,
where log's `-l` is solo-only and would leave it looking like a second object and
silently drop the blob window.
Verified byte-identical (stdout, stderr and exit code) against a binary built
from the merge-base over 94 `git show` invocations: blob and commit shows, the
`-wG a:b HEAD:blob` cluster, `-pS a:b`, `-S 'url:1' HEAD`, `-L 1,2:file`,
`-- <pathspec>`, `--textconv`/`--filters`/`--ext-diff`, the `:(exclude)`/`:!`/
`:^`/`:/` magic pathspecs, and every short flag whose log and diff spellings
disagree.
The `cat-file` probe keeps its subprocess: it only runs once a free positional
already looks like `rev:path`, which is the one case where nothing but git can
answer, so no flag pre-filter can skip it without risking the blob path.
Also drops a doc link on `log_wants_raw_shape` pointing at a `#[cfg(test)]`
helper, which rustdoc cannot resolve outside a test build.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KuSh
force-pushed
the
fix/show-arg-tokenizer
branch
from
September 17, 2026 22:37
624cdd4 to
a6f170b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What the TODOs were
src/cmds/git/git.rscarried twoTODO(after #3681)markers left by the author of theblob-show path (#3265), both waiting on the
ValueSpecfactorization that landed with #3681.1.
flag_token_consumes_next— the short-cluster walk.show_positionalsfound agit show's object arguments with a hand-rolled scan:position(|a| a == "--")for theboundary,
starts_with('-')for flags, and a per-char walk of each short cluster thatre-queried the flag table by rebuilding
format!("-{c}")for every character. The semanticsit encoded — the first value-taking short flag in a cluster takes the remainder as an inline
value, or the next argument when it is the cluster's last char — is exactly
Attachmentplusthe
solo_onlyrule the tokenizer already models. It is now:Three functions and a per-char allocation go away.
One grammar per subcommand. This reads
git showunderdiff_takes_value, notlog_takes_value, matchingcommit_or_stat_route's existingtokenize_git_diff_argsfor thesame subcommand. Measured against real git, not assumed:
git show -wl 100 HEAD-lis the rename limit and clustersgit show -wl HEADerror: switch `l' expects an integer valuegit show -n 1 HEADgit show -wn 1 HEAD/-pn 1 HEADfatal: ambiguous argument '1'So
-lisvalue()and-nissolo_only()— the diff table. Under log's table-lissolo-only, which would leave the
100looking like a second object and silently drop theblob window.
2.
probe_is_blob— dropped, deliberately. The suggestion was a flag pre-filter to avoidthe
cat-filesubprocess on the common path. There is nothing conservative left to skip: theprobe only runs once a free positional already looks like
rev:path, and after this changethat set is strictly smaller than before. An ordinary commit show never reaches it, and on the
blob path
rtk git show HEAD:file.txtmeasures 3.7 ms ± 0.4 ms end to end — probe included,well inside the 10 ms budget and unchanged from the merge-base. Any pre-filter would trade a
non-problem for a risk to the one decision only git can make, so the marker is removed rather
than acted on.
Also fixes the doc link on
log_wants_raw_shape, which pointed atrequests_raw_log_output—a
#[cfg(test)]helper rustdoc cannot resolve outside a test build. Other broken links in thetree are pre-existing and left alone.
Differential verification
A binary built from the merge-base and one from this branch, run over a matrix of
git showinvocations in a fixture repo under
LC_ALL=C, with stdout, stderr and exit code comparedbyte for byte.
rev:pathblob show,:0:pathindex blob,-wG a:b HEAD:blob,-pS a:b,-S 'url:1' HEAD,-L 1,2:file,-- <pathspec>,--textconv,--filters,--ext-diff,--no-textconv, the magic pathspecs:(exclude)/:!/:^/:/text,::/:,--stat/--numstat/--name-only/--pretty/--format,--word-diff/--color-words/--diff-merges,-s,--grep -p,-20, bare-, and multi-object shows.colon operand next to a real blob so grammar drift shows up as a routing change —
-wl a:b,-pn a:b,-cl a:b,-wG/-wS/-wI/-wO/-wL,-U 3vs-U3,-M50,-C/-B,-Gfoo,-wGa:b,-lw 100,-wnl 1, a--as a flag's would-be value, and--combined with each.Result: 94/94 identical, 0 differences. The runner was sanity-checked against itself first
(same binary twice) to confirm it is deterministic.
tests/git_show_blob_differential_test.rs, the seeded 600-iteration fuzzer that already guardsthis path against
git cat-file -tground truth, also stays green.Regression tests
Behavioural, CI-run (not
#[ignore]), against the real binary and real git, intests/guard_integration_test.rs:git_show_cluster_flag_value_is_not_mistaken_for_the_blob_object—-wG a:b HEAD:big.txtmust still window the blob. Red when
show_positionalsis reduced to astarts_with('-')scan.git_show_rename_limit_clusters_under_diffs_grammar_not_logs—-wl 100 HEAD:big.txtmuststill window the blob. Red under
log_takes_value, green underdiff_takes_value. Thisis the test that pins the one-grammar-per-subcommand choice.
git_show_blob_spec_after_double_dash_is_a_pathspec_not_an_object— a blob spec past--must print the commit, not the file. This one is a pin rather than a fail-before test: the
cat-fileprobe and thecan_windowgate already make it unobservable on their own, whichis the defence-in-depth working as designed.
Both fail-before results were confirmed by temporarily reverting, running red, and restoring.
Performance
hyperfine --warmup 5 -N, merge-base vs this branch:git show HEADgit show HEAD:file.txtgit show -wG a:b HEAD:file.txtWithin noise, inside the <10 ms budget.
Out of scope
The output-shape routing refactor proposed in #3910 —
log_wants_raw_shape,diff_wants_raw_shape,show_wants_raw_shapeandbody_is_suppressedare untouched here sothat work can land cleanly.
Related issue
Refs #3954 — that issue targets
flag_token_consumes_next's hand-maintained value-taking table,which this PR deletes in favour of the shared
diff_takes_valuegrammar, so the two-tables-driftroot cause it describes is closed here. Not
Fixes, because its specific claims no longer hold:its one reproducing example (
--ignore-matching-lines a:b HEAD:big.txt) already windows ondevelop (36090 B raw -> 8243 B), and git 2.53.0 rejects the three flags it names in separated
form (
git show --notes x HEAD:big.txt->fatal: ambiguous argument 'x'; same for--conflict-marker-sizeand--submodule), so adding them to any table would make RTK consumea token git treats as a revision.