Repository navigation
fix(git): stop show reporting on HEAD and log capping in silence - #4117
Conversation
`git show --oneline -p <sha>` printed the requested commit's header and then HEAD's stat and full patch. The stat step drops `--oneline`, which shifts every later argument down one index, then strips patch-shape flags using the token indices of the unshifted args: it deleted `<sha>` and forwarded a bare `-p`, which outranked RTK's `--no-patch`. Re-tokenize the shifted args. Same path for `--patch`, `-U<n>` and `-W`. `git log -p` caps the walk at 10 commits, and the notice that says so was driven by counting `commit <hex>` headers in RTK's own output. `--oneline` emits no such header, `log.decorate` and `--graph` disfigure the one there is, `-z` runs the walk onto one line, `--line-prefix` puts something in front of it, colour puts an escape there, and a SHA-256 name is not 40 characters -- on each of those the cap took 15 of 25 commits with nothing on stderr. Ask git for the one commit just past the limit instead and read only whether anything came back, which no output shape can disfigure. What that probe forwards is pruned to what bounds the walk: the format flags, because RTK's own is written first and would lose the arbitration; `--skip`, which RTK's has to absorb; `--exit-code`, which would report failure on a walk git was happy with; `--output`, a redirect that would truncate the file the command just wrote; and the patch-shape flags, which are work with no answer in them and one extra run of the user's `diff.external` program. `--patch-with-stat` and `--patch-with-raw` now count as patch requests, which they always were -- `git show`'s stat header was carrying a full patch under both spellings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📊 Automated PR Analysis
SummaryFixes two regressions introduced by #3681 in RTK's git wrapper: Review Checklist
Linked issues: #3979 Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
There was a problem hiding this comment.
Reviewed by building the branch and reproducing both blockers on develop (0924356b) vs this PR's binary, in a 2000-commit repo.
Verified
- Blocker 1:
rtk git show --oneline -p <root-sha>on develop prints HEAD's stat and patch (1200 lines,src/cmds/git/git_cmd.rs | 213 ++…first). On this PR it prints the root commit's (524 lines,.github/workflows/release.yml | 145 +++first). Same for-U2. Swapped order-p --oneline <sha>and--stat <sha>were already correct and stay identical. - Blocker 3:
rtk git log --oneline -p,--format=%s -p,--graph -pall capped at 10 with empty stderr on develop; on this PR each prints[rtk] capped at 10 commits; pass -n <count> for more. Diff-selected walk-p -S LazyLock(17 matches) fires the notice,-S 'fn run_log'(2 matches) correctly stays silent.-n 30,-5,HEAD~3..HEADunchanged. - Exit codes preserved (
--pretty=bogus,nosuchref→ 128). Probe overhead ~8 ms on a debug build.
Follow-ups, none blocking
src/cmds/git/git_cmd.rswalk_exceeds_limitcounting branch: when the walk is diff-selected and the user also passes-z, the probe forwards-z, git emits the whole%Hwalk NUL-separated on one line,.lines()sees one non-hex line and the notice is silent again. Repro on this branch:rtk git log -p -S LazyLock -z→ 17 selected, 10 printed, no notice. Droppingzinlog_probe_args(or splitting on['\n', '\0']) closes it.- The notice is now printed even when git refused the main command, if the probe succeeds after pruning:
rtk git log -p --pretty=bogusprints the fatal line then the cap notice (exit 128 kept). Cosmetic; gating onresult.success()would do. - Not from this PR: the filtered path for plain
rtk git log --oneline(no raw-shape flag) still caps at 50 silently (git_cmd.rs:1815). Worth a separate ticket since Blocker 3 was phrased as covering--oneline.
--patch-with-stat / --patch-with-raw in requests_patch_output only matter for the log probe: for show they already route to raw passthrough via show_wants_raw_shape, so that part of the description is a no-op for show.
Approving. CI green on all three OSes; local fmt and clippy clean.
Fixes two of the three blockers from the release review on #3979, both introduced by #3681.
Part of a set of six, one per originating PR: #3681 (this), #3552, #3265, #3772, #3857, #3941.
Blocker 1 —
git showreturns another commit's diffrtk git show --oneline -p <sha>printed the requested commit's header, then HEAD's stat and HEAD's full patch. Exit 0, no warning.The stat step calls
args_without_oneline, which drops--onelineand so shifts every later argument down one index. The result was then handed toshow_cmdtogether with the original tokens, whosesource_indexvalues still address the unshifted args.args_without_patch_shapetherefore deleted the argument that had inherited the patch flag's index — the<sha>— and forwarded a bare-p, which outranks RTK's--no-patchby git's last-flag-wins.Fix: re-tokenize the shifted args. Same path for
--patch,-U<n>and-W.While there:
--patch-with-statand--patch-with-rawnow count as patch requests, which they always were —git show's stat header was carrying a full patch under both spellings.Blocker 3 —
git logcapped to 10 commits, silently for most formatsIn the raw-shape path RTK injects
-10, and the notice that says so was gated on countingcommit <hex>headers in RTK's own output. That only ever worked for the default format. On a 25-commit repo the cap took 15 commits with nothing on stderr under every one of:--oneline— no commit header at alllog.decorate/--graph— the header is decorated or behind a rail-z— the whole walk on one line--line-prefix=<s>— something in front of every linecolor.ui=always— an escape at column 0--name-only/--name-status— no commit header in that shape eitherFix: stop reading RTK's own output and ask git instead. Which question to ask depends on how the walk is narrowed, because git applies its filters in a fixed order:
--max-count=1 --skip=<limit>→ did anything come back?-S,-G,--diff-filter,--find-object)--max-count=<limit+1>→ how many came back?--skipwhere the walk starts and applies these filters after it, so skipping past the cap reports on commits the filter would have dropped —rtk git log -p -S needlematching twice claimed a cap of ten--max-countis applied last, after every filter, which is what makes the second form answerable. Its count is taken by line shape afterstrip_ansi, with no upper bound on the object name so SHA-256 still counts.What the probe forwards is pruned to what bounds the walk. Dropped:
--pretty/--format/--oneline--pretty=format:prints a commit as no bytes at all--skip--exit-codegit logexit 1, which would read as "git refused"--outputdiff.externalprogramRTK's own flags go in front of every user argument, since git refuses an option that follows a positional — without that a bare pathspec made the probe fail outright and the notice went silent again.
Verification
tests/git_log_cap_notice_test.rsis new: 12 tests over real repositories covering every shape above, the SHA-256 case,--skiparithmetic, the--outputfile, aGIT_EXTERNAL_DIFFinvocation count, and the diff-selected walks, where the notice is asserted to fire if and only ifgit log --oneline <filter>itself selects more than the cap. The negative side is covered too — a walk shorter than the cap, an explicit-n/--max-count/revision range, and a command git refuses must all stay silent.Each new assertion was confirmed to fail when its own fix is reverted, and the two "announces the cap" tests fail against
developwithstderr was "".cargo fmt --all,cargo clippy --all-targetsandcargo test --allare green, rebased on currentdevelop.🤖 Generated with Claude Code