Four findings on the section header and the word-diff guard:
- Under the default `core.quotepath` git escapes a non-ASCII path one octal
byte at a time. Unwrapping the quotes without decoding them printed
`\303\251t\303\251.txt`, so a grep for the file by name found nothing.
- A single-path `diff --cc` / `diff --combined` header was split at its
midpoint like a two-path one, reading a file called `dup dup` as `dup`.
- Prefixes were matched by name, so `--dst-prefix` fell through and the
header carried the whole pair. Matching the halves against each other
reads any prefix, named or not.
- `--word-diff=none` is the mode that turns a word diff back off, leaving an
ordinary unified diff. Treating it as a word diff passed the raw diff
through, so a defensive `--word-diff=none` lost every saving. Modes are
last-one-wins, which is what that mode is for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four more from an adversarial pass, two of them undermining the previous
commit's own rationale.
`--word-diff`, `--word-diff-regex` and `--color-words` drop the marker column
and put `[-removed-]` / `{+added+}` inline, so a word-diff body line is
arbitrary content in the position `compact_diff` reads as a marker. The byte
slicing stopped the panic, but not the misparse: a line starting `+` counts as
an addition and consumes the wrong budget, one starting `\` is dropped as a
no-newline annotation, and a real `git show --word-diff=plain` row reading
`diff --git [-a/src/main.rs...` opens a new file section. There is no faithful
compaction of a word diff, so `run_diff`, `run_show` and `git stash show -p`
pass these modes through, the way `gh pr diff --patch` already does for mbox.
`rtk pipe --filter git-diff` has no flags to read and still misparses word-diff
text piped into it; `compact_diff` reads a unified or combined diff, and that is
now what its callers give it.
A hunk header lists one range per marker column, and a mismatch was charged
against nothing. `@@@ -1 +1 @@@` left the second parent untracked, so ` -lost`
drove the tracked budgets to zero and the hunk closed with the removal
discarded. `@@@ -1 -1 -1 +0,0 @@@` declared a third parent with no column to
spend it, so that budget never reached zero, the hunk never closed by count, and
the mbox signature and version became its body. The ranges are reconciled with
the columns now: a missing one gets `usize::MAX`, which keeps the hunk open to
the next header rather than dropping content, and an extra one is dropped.
`diff_header_path` split on the first ` b/`, which is inside the path for a file
under a directory named `x b` -- `diff --git a/x b/y b/x b/y` reported `y`. And
`--no-prefix` or a custom `--src-prefix` leaves no ` b/` at all, so `diff --git
x x` reported both paths as the section header. Anything but a rename names the
same path twice, so the halves are the same length and the separating space sits
dead centre; splitting there handles all three and does not care what the
prefixes are. A rename's halves differ in length and still fall through to the
` b/` split.
Ordinary unified diffs are byte-identical to the previous head: verified on a
26,561-line real diff of this repo and on a 390-line one.
Four problems from the review, one of them a crash.
`&line[..width]` slices by byte index. `--word-diff`, `--color-words` and
`--word-diff-regex` emit body lines with no marker column, so their content
lands where the markers are read, and a leading multi-byte character split
mid-character: `rtk git diff --word-diff` aborted with exit 101 and empty stdout
on a file holding `école`. `run_diff`, `run_show` and `git stash show -p` have no
`catch_unwind`, so the panic was fatal there. The markers are ASCII by
construction, so comparing bytes removes the boundary question.
A combined diff carries one marker column per parent, and a space means
opposite things depending on the line: on a change line the line is in that
parent unchanged, on a removal line it is absent from it. Collapsing the columns
into one add/delete pair lost that, so `old` never converged on real conflict
output and the hunk never closed by count -- the declared-length termination
this commit's predecessor is built around silently did not apply to `diff --cc`,
which is what git emits for every unmerged path. The reverse case closed early
and dropped the body: `@@@ -1,2 -1,4 +1,2 @@@` over two removals from the second
parent alone rendered as a bare hunk header, with no tally and no truncation
note. `HunkHeader` now tracks a budget per parent and ends the hunk when every
one of them and the result are spent.
The leading-context ring drained only on a hunk's first change line, so a hunk
that ends without one discarded what it held and rendered as a bare header.
Flushing wherever the hunk closes -- and at end of input -- keeps it consistent
with the buffering, inside the same diff-wide budget.
`diff_header_path` mishandled git's quoted paths. Under the default
`core.quotepath`, git escapes a non-ASCII path and wraps it in `"`, which
removes the ` b/` the plain form splits on, so the fallback returned the whole
remainder -- both paths -- as the section header. The quoted form is handled
explicitly now, and the doc comment names the distinction the code makes: git
quotes non-ASCII bytes, not spaces.
A hunk ran until the next header, so any trailing text was read as its body.
`gh pr diff --patch` routes here, and GitHub's `.patch` is an mbox: each patch
carries a bare `---` before the diffstat and a `-- ` signature after the diff,
both at column 0. With hunk bodies now anchoring, a two-commit mbox with two
real deletions reported `grep -c "^-"` = 5. `@@ -a,b +c,d @@` declares its own
line counts, so the hunk now ends when they are consumed and anything after it
is outside every hunk. That covers trailing prose and truncated input too,
rather than the shapes we happen to know about, and a heuristic on `---` / `-- `
could not have worked: a deleted line whose content is `- ` renders as `-- `.
`--patch` also joins `has_non_diff_format_flag`, so an mbox passes through
whole. Someone asking for a patch wants one that applies.
Leading context keeps the last lines before the change rather than the first.
With `-U10` or `--function-context` the first three sit ten lines above it, and
emitting those tells the reader that ctx3 precedes the deletion when ctx10 does.
A three-slot ring buffer costs the same lines. The diff-wide budget is charged
when the buffer drains, so a line the ring evicted costs nothing.
Combined diffs carry one marker column per parent, so a line changed against
only one of them holds its marker in column 2. Those were counted as context:
neither in the `+N -M` tally nor reachable by an anchored grep. The tally reads
both columns now. The grep gap is real and cannot be closed without rewriting
the line, so FEATURES.md states it alongside the per-hunk cap. Against
`git diff --numstat` on three complete commit ranges the tally is now exact,
where master under-counted one of them by +2 -1.
`diff_header_path` keeps paths containing spaces: git does not quote them, so
`diff --cc my file.txt` needs the whole remainder, not the last token.
FEATURES.md said 15 lines for a 24-line block and showed the stat's first line
with a leading space it does not have. It now also carries a measured example
with real compression, since the two-file one saves 11% in a document
advertising 60-90%.
Dropping the `+++` / `---` guards assumed those headers only appear before the
first `@@`. That holds for `diff --git` sections. It does not hold for the
`diff --cc` sections git emits for unmerged paths: `diff --cc` never matched
the `diff --git` branch, so `in_hunk` stayed set from the previous section and
the `--- a/z.txt` / `+++ b/z.txt` headers of every later file were printed at
column 0 and counted in the tally. On a two-file conflict with no deletions,
`rtk git diff | grep -c "^-"` returned 1. The state reset now fires on any
`diff --` prefix, and the path falls back to the trailing token for the
`diff --cc <path>` shape, so the headers land before the first `@@` again.
Same repo now returns 0.
Leading context no longer counts against `max_lines`, so it cannot displace
change lines. Its previous justification was wrong: before this branch, leading
context was never emitted, so it could not have crowded anything out of
`max_hunk_lines`. The cost it does carry is on the global budget, and on a
five-commit diff of this repo it cost 57 change lines (144 shown, against 201
with the exemption). A diff-wide cap of `max_lines / 10` bounds what the
exemption gives back: without it, a diff of many small hunks would spend three
exempt lines on every one of them. The same diff comes out at 71.7% byte
reduction against 71.8% on master, which showed zero anchorable change lines.
`docs/usage/FEATURES.md` showed change lines indented under a
`src/main.rs (+5/-2)` header, which no version of rtk emits. It now carries
real captured output, plus the two limits of the anchored audit: the 100-line
per-hunk cap, and that the output is not an applyable patch.
The `+++` and `---` guards ran inside the `in_hunk` branch, where they can only
match content: git emits the file headers before the first `@@`, and `in_hunk`
resets to false on every `diff --git`. So an added `++i;` or a removed
`-- sql comment` was dropped from the body and left out of the `+N -M` tally.
A diff doing both rendered as a bare hunk header with no body and no tally, and
`grep -c '^-'` answered 0 for a file that really did lose a line.
Leading context now has its own budget of three lines. The `hunk_shown > 0`
guard silenced every context line before a hunk's first change, and giving it
`max_hunk_lines` instead would let a long run of context crowd out the change
it is there to frame.
The truncation note splits by sign: `... (30 deletions, 30 additions
truncated)`. It stays indented so the anchored greps this output is meant to
support never count it, which means a reader who trusts `grep -c '^-'` past the
100-line hunk cap needs the note to say how many deletions it did not see. One
merged line count could not.
The comment on the column-0 change narrated the bug's history and sat on the
`@@` branch while describing the branch below it. It now states the invariant,
once, where the code it governs lives.
compact_diff indented every hunk-body line two spaces to nest it under the
filename. The content was all there, but the indent moved it off column 0, so
`git diff | grep '^-'` matched nothing and an audit asking "was anything
removed?" got a confident, wrong "no" while --stat reported deletions.
Worth being precise about the failure, because the earlier write-up was wrong
on both counts: nothing was ever dropped, and added lines were broken
identically (`^+` also matched nothing). It is not a deletions-only defect and
not content loss. It is one indent breaking every anchored grep.
Emit hunk bodies in git's own unified shape instead. Context lines already
carry git's leading space, so they stay distinct from additions. rtk's own
annotations (the `+N -M` tally, `... (N lines truncated)`) keep the indent
precisely so these same greps never count them as diff lines.
The output still cannot be mistaken for an applyable patch: rtk drops the
`diff --git`/`---`/`+++` headers, so a patch parser rejects it either way.
The indent was never what made that safe.
Every other run_* function that inspects args for "--" (run_diff,
run_checkout) calls args_utils::restore_double_dash() first to
re-insert the "--" that clap's trailing_var_arg strips out. run_log
never did, so requests_raw_log_output()/log_arg_tokens()'s own
take_while(|arg| *arg != "--") check was dead code in production:
by the time run_log saw args, the literal "--" was already gone.
Concretely, `rtk git log -- -p` lost its "--" and -p (a file
literally named -p, a valid pathspec) was misdetected as the real
patch flag, routing to raw passthrough instead of RTK's filtered
log. Added an end-to-end regression test (real rtk binary, real git
repo) alongside the existing git_log_patch_output_matches_raw_git
test, since restore_double_dash reads live process args and can't
be exercised by calling run_log's internals directly.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
requests_raw_log_output only recognized -p/-u/--patch variants as needing
the raw path. --stat, --numstat, --name-only, --name-status, --raw,
--shortstat, --dirstat, and --summary change git's raw output shape the
same way -p does, but weren't listed — RTK's injected --pretty=format +
---END--- markers can't coexist with them, so the diffstat/name-list block
got misparsed as the start of the next commit, mangling output silently
instead of just leaving it unfiltered.
Also share one log_arg_tokens() pass across run_log's flag/limit checks
instead of retokenizing per check, and drop a redundant "-n" match arm
already covered by consumes_next_token_as_value.
has_limit_flag, has_format_flag, wants_merges, and parse_user_limit scanned
args positionally with no awareness that a value belonging to --grep,
--author, etc. can itself look like a flag (-5, --pretty, --merges),
reproducing the same misdetection class this branch already fixed for -p.
Unify these into a single log_arg_tokens tokenizer shared by
requests_raw_log_output, real_flag_args, and parse_user_limit, which also
stops at the -- pathspec separator so a literal path like -5 after -- isn't
misread as a flag.
Cross-checked git-log(1)'s full option list against
consumes_next_token_as_value() (using git log <opt> <token> -1 against
real git 2.53.0 to see whether <token> gets swallowed as the option's
value or leaks through as a positional arg). --diff-algorithm and
--diff-filter both take a required, separate-token value
(`git log --diff-algorithm -p` rejects -p as an invalid algorithm
rather than treating it as the patch flag) but were missing from the
list, so a genuine -p right after either was misdetected as the
option's own value and the raw-patch request went undetected.
Every other bracket/optional-value option checked (--format, --pretty,
--stat, -M, -C, -B, etc.) is correctly attached-value-only and stays
out of the list.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Audited every option in consumes_next_token_as_value() against real
git 2.53.0 behavior (git log <opt> <token> -1, checking whether the
token gets swallowed as the option's value or leaks through as a
positional arg). --max-parents and --min-parents behave like -U:
`git log --max-parents 2` fails with "ambiguous argument '2'", so a
real -p right after them was being misread as their value.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These options only accept an attached value (-U3, --unified=3,
--expand-tabs=4) — verified against git 2.53.0, where
`git log --expand-tabs 4` fails with "fatal: ambiguous argument '4'"
instead of treating 4 as the value. consumes_next_token_as_value()
was swallowing the next token for them anyway, so a real -p right
after one of these was misread as their value and the raw patch
request went undetected.
Addresses review feedback from @aeppling on #3575.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Addresses the inline review on #2717.
decode_process_output
- Decode a line at a time instead of reinterpreting the whole buffer at
the first bad byte. Valid UTF-8 lines keep their bytes; only lines that
fail UTF-8 validation go through the code page, so one stray byte no
longer mangles output that was almost entirely UTF-8. The line is the
unit because a byte run is not one: GB18030's four-byte sequences embed
bytes in the ASCII digit range, so any rule that ends a run below 0x80
splits them. \n cannot appear as a trail byte in any encoding handled
here, and a process does not switch encoding mid-line.
- A code page result is only accepted when it decodes cleanly, so a UTF-8
line with a corrupt byte falls back to lossy UTF-8 rather than mojibake.
- Replace the hand-written code page table with the codepage crate, as
suggested. That also fixes 54936, which was mapped to GBK and now
correctly resolves to gb18030.
- Add oem_cp for the legacy OEM/DOS pages (437, 850, 852, …) that plain
cmd.exe still defaults to in many locales. encoding_rs implements only
WHATWG encodings, so codepage alone returns None for them.
- Fall back to GetACP when GetConsoleOutputCP reports no console, which
is the piped case rtk normally runs in, and warn once instead of
falling back to lossy silently.
- Cache the code page lookup in a OnceLock.
- The mapping and the walk take the code page as a parameter, so they are
compiled and unit-tested on every platform rather than only Windows.
Call sites
- Route the remaining production sites through stream::exec_capture and
exec_capture_stdin rather than decoding at each one, so future callers
inherit decoding. git commit keeps inherited stdin via the _stdin
variant. Test-only sites go back to from_utf8_lossy: they assert on
rtk's own UTF-8 output, where a console code page has no meaning.
- Decode the streamed path (read_lines_lossy) too — the OEM/ANSI lines
its comment describes were still going straight to U+FFFD.
- curl keeps its body on from_utf8_lossy: a response body is a network
payload whose encoding comes from the HTTP charset, not the local
console, and non-UTF-8 bodies already take the binary passthrough for
#1087. Only curl's own stderr is code page decoded.
git commit summary parsing
- parse_commit_output sliced from byte 1, which panics when the first
line starts with a multi-byte character — git prints hook output before
its summary, and a lossily decoded line starts with a multi-byte
U+FFFD. Locate the bracket pair with find instead, so both indices are
character boundaries.
Verified: unit tests for the walk, GBK, gb18030, CP437/850, mixed lines,
truncated input and every byte value; a test pinning that output without
a code page stays byte-identical to from_utf8_lossy; and the Windows-only
lookup cross-compiled for x86_64-pc-windows-msvc.
requests_raw_log_output() matched bare -p/-u/--patch tokens anywhere in
the args, without checking whether the previous token was an option like
--grep or -S that consumes the next token as its value. `git log --grep
-p` searches commit messages for the literal string "-p" (verified
against real git 2.53.0: no diff output, identical to --grep=-p) but RTK
treated it as a patch request and skipped its own filtering/limit,
dumping raw uncapped git log output for what is actually a plain grep
search.
Skip the value token after any known value-taking git log/diff option
before checking for the patch flags.
requests_raw_log_output() scanned the full args slice for patch flags
like -p, so `git log -- -p` (a literal pathspec named -p) was
misdetected as a raw patch request and skipped RTK's normal filtering.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review feedback (KuSh): the helper existed but most filter modules still
called String::from_utf8_lossy directly, so non-UTF-8 console output
(e.g. GBK on Chinese-locale Windows) was still mangled for most commands.
Wire decode_process_output through the remaining child-output call sites:
git, go, aws, curl (non-binary paths only — raw binary passthrough for
#1087 is untouched), system read, and discover registry. File-content
decoding (dotnet TRX XML, hook trust snippets) intentionally keeps
from_utf8_lossy since console code pages don't apply there.
RTK could emit more tokens than the underlying command on small inputs:
filters that add headers, summaries, re-indentation, or a tee hint, plus
synthetic no-result messages ("0 matches", "No stashes", "[docker] 0
containers") printed where the raw command emitted nothing. Both break the
Transparency principle and inflate tokens instead of saving them.
- core::guard::never_worse(raw, filtered) returns raw when the filtered form
has more tokens (reuses tracking::estimate_tokens), so RTK output is never
larger than the real command.
- runner::emit_guarded(filtered, hint, raw) composes body + tee hint, guards
the whole, prints, and returns what was shown so printed == tracked.
- run_captured_filter guards the run_filtered* family centrally; per-site
guards cover the remaining single-string filters.
- On empty raw, emit empty and preserve the exit code instead of a synthetic
no-result message (the messages were cosmetic with no dependents; #2461
reports the grep one as actively harmful).
- git stash show now propagates its exit code instead of masking a real
failure as Ok(0).
Resolves#2551.
run_worktree list mode never checked result.success() — a failed
`git worktree list` (e.g. run outside a repo) was flattened to empty
output + exit 0. The has_action branch already guards on success; apply
the same guard to list mode and surface git's error with its exit code.
Part of #2497
The compact status path only caught the "not a git repository" error and
let every other failure (corrupt index, lock contention, broken refs)
fall through to a formatted "Clean working tree" + Ok(0), masking a real
git error. The non-compact path already guards on result.success(); apply
the same guard to the compact path, keeping the friendly not-a-repo
message and propagating git's real exit code otherwise.
Part of #2497
run_commit had a branch that printed 'ok (nothing to commit)' and fell
through to Ok(0) whenever git exited non-zero with a 'nothing to commit'
message — so a no-op or hook-aborted commit was reported as success with
exit 0. Gate strictly on output.status.success() via a CommitOutcome
helper: any failure surfaces git's message and propagates the real exit
code, matching native git. Same bug class as #1581 (push) and #1535
(stash).
Fixes#2494
Separator lines (═══, ---) and an emoji status marker cost tokens
without adding signal for the LLM — RTK output must never add noise
over raw. Semantic labels are kept; the emoji is swapped for plain
monochrome unicode.
The compact `git status` path ran `git status --porcelain -b -uall`. The
`-uall` flag expands fully-untracked directories into every file, while raw
`git status` collapses them (e.g. `node_modules/`). This made rtk output
larger than raw — measured ~29x on a 200-file untracked dir (5500B vs 191B) —
violating RTK's compress-or-match-raw invariant and inflating tokens.
Remove `-uall` so untracked directories collapse exactly like raw. This keeps
#991's actual fix intact: all modified/staged/renamed/conflict paths are still
shown with no grouped summaries or `... +N more` overflow markers (`-uall`
never affected those lines). Untracked files in partially-tracked dirs and any
paths git itself expands are still preserved by the formatter.
Measured (rtk vs raw git status):
- node_modules/ (200 files): 5500B (-2780%) -> 25B (+87% savings)
- normal (8 mod + 2 untracked): +71% -> +76%
- 17 modified (#991 case): unchanged at +58%, all 17 shown, 0 overflow markers
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
When user runs 'git log -1 --format='%H' HEAD' where HEAD is a merge
commit, rtk was adding --no-merges which filtered out the merge commit
itself and returned the second parent instead. This made 'git log -1'
return wrong SHAs for merge commits.
Fix: don't add --no-merges when user explicitly passes -n N or
--max-count=N. When a user specifies an exact count they expect exactly
that many commits, not filtered results. Also skip --no-merges if user
already passed --merges or --no-merges explicitly.
Fixesrtk-ai/rtk#2009.
Nicolas Le Cam