Skip to content

fix(diff): region parser for condense_unified_diff — budget-owned hunks, raw fallback - #3788

Merged
KuSh merged 16 commits into
rtk-ai:developfrom
kylehgc:claude/upstream-issue-comments-4pky96
Sep 7, 2026
Merged

KuSh merged 16 commits into
rtk-ai:developfrom
kylehgc:claude/upstream-issue-comments-4pky96

Conversation

@kylehgc

@kylehgc kylehgc commented Aug 31, 2026 •

Copy link
Copy Markdown
Contributor

Closes #3769.

Rebased onto current develop, so the two slices already landed there are not re-included: #2652 (the ... +N more trailer) and #3687 (markers at column 0 on the stdin path) are taken as the base, and their tests (test_condense_unified_diff_large_no_false_overflow_indicator, test_condense_unified_diff_no_false_overflow, the strengthened anti-indent assertion) are kept byte-identical. What remains is the region parser itself and the non-UTF-8 raw passthrough. The matching defect in compact_diff (src/cmds/git/git.rs) is not part of this PR: #3269 fixed it on develop with budget-owned hunks, including diff --cc / diff --combined as file boundaries, and git.rs here is byte-identical to the merge base.

What changed

condense_unified_diff (the rtk diff - path) classified lines by prefix, which is ambiguous by construction: a removed SQL -- comment is --- comment on the wire. The rewrite parses structure first — (prologue)(file-header)(hunk)* regions, hunk extent owned by the @@ -a,b +c,d @@ line budget (one old-budget per parent, so @@@ combined headers parse with the same arithmetic) — and classifies only within regions. Any structural disagreement → byte-exact raw passthrough, never partial output.

Coverage against the issue's reproducers (before-behavior documented in #3769; each row has its own test on this branch):

#3769 reproducer After
1 — removed -- comment lines kept at column 0, counted ((+0 -3))
2 — added ++ text line kept; [file] label intact
3 — format-patch signature + mbox bullets excluded by position (mbox regions), never by value
4 — deletions named /dev/null named by the --- side, (deleted) note
5 — binary / rename-only / mode-only / copy / empty-file sections note-only entries instead of no output
6 — --color input parses (structure is read through an ANSI-stripped view); content lines keep their bytes, escapes included
7 — non-UTF-8 stdin byte-exact raw passthrough (was: hard error, exit 1)
8 — CRLF-only changes bytes survive verbatim (split('\n'), not lines())
9 — \ No newline at end of file kept exactly when the line above it was emitted (in-hunk after a -/+, or right after the budget closes on one); a marker git attached to a context line is dropped rather than shown under the wrong line
10 — b/b/x.rs, timestamps in names the prefixes the diff --git line settled are stripped exactly once (--no-prefix keeps b/x.rs); timestamps come off at the last tab, only when timestamp-shaped
11 — phantom ... +N more trailer already fixed on base by #2652; its tests kept byte-identical

Detector precedence (explicit total order)

Earlier rules own the line; this is the order condense_unified_diff_strict applies, verbatim from its doc comment, so the implementation can be checked against it rather than reconstructed. Every rule reads the line through a structural view (ANSI- and CR-stripped); content is pushed from the raw line.

  1. Inside a hunk, the @@ budget owns every line. Invalid body prefix, over-consumed budget, or EOF/next-header with budget still owed → fall back raw. \ No newline at end of file consumes no budget and is kept exactly when the line above it was emitted — it is the only witness of a newline-only change. A hunk that closes with no -/+ line at all is not a shape any unified-diff producer emits (whitespace-ignoring modes suppress the hunk): it is --word-diff / --color-words output whose inline markers sit behind an indent → raw.
    • 1b. The new-side marker lands on the line right after its hunk's budget closed and is kept by the same test; a \ line anywhere else is prose (its text is localized, so position decides).
  2. mbox From <sha> separator (40- and 64-hex), an hg export changeset header (# HG changeset patch) or an hg log -p changeset: line resets to the prose prologue. The hg region differs in two ways: rule 6's fact arms stay live in it, and hg's own diff -r echo closes it and counts as reaching its body (every hg file gets one, binary files included).
  3. diff --git / diff --cc opens a file section; git extended headers (rename, copy, Binary files, GIT binary patch, mode lines) annotate it; index / similarity index lines are structural. diff --git X Y names one path under two prefixes — a//b/ by default, i//w//c//o/ under diff.mnemonicPrefix, anything under --src-prefix/--dst-prefix, none under --no-prefix (X == Y) — so the sides are split where they share the longest tail at a / boundary (character-aligned: 😀 and 🙀 share bytes, not a tail), the first path component before the tail on each side is that side's prefix (what the sides do not share beyond it is path: git diff --no-index d1/f d2/f names d2/f), and the header pair strips exactly those; a path containing b/ is not cut. diff --cc / --combined carry one unprefixed path, kept whole. A diff --git section that closes with nothing behind it — no hunk, no note — is a stream cut off right after its header (git never emits one) → raw.
    • 3b. A diff -<opts> X Y line that is not git's — GNU diff's per-file echo when it compares directories, or hg's echo — marks a diff -r stream (rule 9); keyed on diff - because every echo carries an option and a --format=%B body line starting with diff does not. After a GNU echo the roots diff was given are directories, so header pairs keep them whole; hg prefixes a//b/ like git.
    • 3c. svn's Index: <path> opens a file section; Cannot display: file marked as a binary type. is its binary note, and the header pair or diff --git line that follows names it again. An Index: section that closes with nothing parsed is a copy/move target svn has nothing to show for — (no content) — unless a column-0 line was dropped inside it, which is svn describing that file in words the parser did not read → raw.
  4. --- X immediately followed by +++ Y whose next line opens a hunk (every real producer's does) is a file header: names a still-hunkless section in place — unless git's rename to/copy to already named it exactly, or the section is a binary one (a pair after it is another producer's next file) — or opens a new section. A pair with no hunk behind it is never consumed, open section or not: it falls through to rules 7–9, so stray marked lines are never swallowed as a phantom header. Timestamps come off first (the last tab, and only in the shapes real producers write — GNU's ISO date, hg's Mon Sep 07 …, svn's (revision 1), git's bare tab — so a tab inside a name survives even under hg diff --nodates), then names are dequoted, then the section's prefixes are stripped — or, for a pair on its own, a//b/ when the two names differ and nothing when they repeat or the pair follows a GNU echo. This positional rule (not line values) is what ends the prologue.
  5. @@ after a file header opens a hunk; malformed @@ there → fall back. Before any file section (a hunk quoted in commit prose) it stays prose.
  6. File-level facts outside hunks become note-only entries: Only in <dir>: <file> and standalone Binary files X and Y differ / Files X and Y are identical / Files X and Y differ (GNU diff -r, -s, -q; roots kept whole, the X and Y boundary is the longest shared tail at a / boundary; a bare diffutils 3.10 Only in holding more than one : splits at the root the stream already named on its echo, header pairs or Binary files lines), * Unmerged path <file> (folded into the section git emits for the same path right after it, also when git's unquoted fact line was cut at a newline in the name), Submodule <path> <a>..<b> (<how>) and Submodule <path> contains … content (a submodule both dirty and moved gets one entry), hg's Binary file <path> has changed; Common subdirectories: X and Y is informational and dropped. The GNU arms read the English spelling only — GNU diff translates its lines; git translates none — and a translated one is caught by rule 9, never dropped; any GNU arm firing marks the stream as GNU's. Suppressed inside mbox message regions, where column-0 prose is indistinguishable by value — except git's own Submodule <path> <hex>..<hex>[ (<how>)]: in that strict shape, which format-patch --submodule=log puts where a file section would go.
  7. --/-- outside a hunk, only in streams that carried an mbox separator, is the format-patch signature: prose. The single value-keyed exclusion.
  8. Any other marked line outside a hunk is evidence of a stale or under-declared budget → fall back raw, with two exemptions. Inside an mbox message region, up to format-patch's diffstat ( <path> | N +-, or the Bin form), column-0 marked lines are commit prose; from the first diffstat line on, a marked line is lost content and falls back raw, which covers a stat-ful patch whichever of its file sections lost its headers. A --no-stat patch has no diffstat to end the tolerance, so its region is judged by shape at its close: exempted lines that are not prose-shaped in a region that never reached a file header → raw. Two positional exceptions: a bare --- directly followed by a diffstat line is the separator git log --stat -p / git show --stat -p put between the message and the stat, and a -<TAB>-<TAB><path> row is git log --numstat -p's binary-file count; lost content never looks like either.
  9. Everything else is prose and is dropped — except in a GNU diff -r stream, which carries no prose: a column-0 line no arm read there (a translated Only in/Binary files, File X is a fifo …, Symbolic links … differ) is a fact the parser cannot read → raw, wherever in the stream it was dropped (GNU sorts its output, so an entry may well parse before the fact it could not read, and the echo or a fact arm may settle the question only afterwards). hg's echo does not count a prologue drop against the stream: hg log -p --template prose is not a fact.

Names

Every [file] name is meant to be byte-equal to the real filename under the producer's own root, decoded per producer from real captures of each: git C-quotes ("caf\303\251.txt") on every line that names a file, and the escapes are decoded — café.txt, quo"te.txt, a tab. GNU diff C-quotes its echo and header lines the same way but shell-quotes Only in / Binary files names from diffutils 3.11 on ('only '$'\303\251''.txt', and the "Kyle's notes.txt" form it picks for a name holding a '; a quoted directory may itself hold : , so the Only in split follows the quoting) and prints them bare before; both generations are in the corpus. * Unmerged path and Submodule lines are never quoted and are not decoded. A decoded name that would split a [file] line (a newline) or is not UTF-8 keeps the producer's own quoted spelling with the prefix stripped inside the quotes, so rename to, diff --cc and the header pair agree on one spelling and two Latin-1 files stay two spellings. git prefixes are stripped exactly as the diff --git line settled them; GNU roots (diff -ru a b → b/c.txt, diff -ru . ../new → ../new/x) are directories and are kept.

Documented bounds (noise, never loss)

Every one of these is an executable test (documented_bounds_hold and the raw-corpus test). mbox prose quoting an unindented well-formed header-plus-hunk block fabricates a phantom entry; a stale-budget leftover of exactly -- in a malformed mbox stream is swallowed as a signature; a message whose own prose imitates a diffstat line ( name | 3) ends the exemption early, and only a marked line between it and format-patch's own --- + diffstat sends the stream raw; a --no-stat patch whose first section is mangled while a later one survives is lost content the shape check cannot see; every localized GNU diff -r stream and every localized svn binary notice falls back raw; an svn property change (a header pair with no hunk followed by a ## -a,b +c,d ## block) sends the stream raw; a commit message that starts a column-0 line with Only in , Submodule or Index: outside an mbox region gets a phantom entry; prose shaped diff -u old new at column 0 outside a message region reads as a GNU echo and sends a git stream raw; a templated multi-changeset hg log -p goes raw at its second changeset header; a bare two-file diff -u a/f b/f with no echo cannot be told from git's a//b/ prefixes and is stripped; a slash-less or multi-component --src-prefix/--dst-prefix is cut at the first /; git show <tag> / --format=%B bodies with column-0 bullets fall back raw (rule 8's prologue has no exemption).

Not included: the rtk diff - exit-code question (run_stdin always exits 0) — the fix lives in main.rs's handling rather than the parser; deferred to its own issue.

Fixture corpus (all real producers)

81 fixtures in tests/fixtures/diff/, every one captured from a real tool:

  • git 2.47/2.54: multi-file, -U0, -W, --no-prefix (with and without a rename), --src-prefix/--dst-prefix, diff.mnemonicPrefix (worktree and --ours during a conflict, with * Unmerged path facts), --no-index on directories and on two binaries under --no-prefix, rename/delete/binary, an emoji-to-emoji rename under core.quotepath=false, copy, mode-only, empty new+deleted, typechange (symlink → file) with a binary under a directory named x b/, core.quotepath names (non-ASCII, ", space, tab, rename), two Latin-1 names, a log -p over renames of Latin-1, newline, "-leading and backslash names, a conflict with diff --cc on binaries under b/ and on quoted names, --ours on a newline-named conflict, no-eol, no-newline marker after a context line and after a - line, log -p, log --stat -p, log --numstat -p, show --format=%B with diff prose, show --cc (real conflicted merge, and a three-parent octopus via --cc and diff-tree -c), --color=always, format-patch single/series/cover-letter/--no-stat --cover-letter/SHA-256/--submodule=log/--interdiff/a message quoting a hunk, unmerged (git diff --ours), submodule (--submodule=log, --submodule=diff, dirty, dirty and moved), --word-diff (raw), a -w | head truncation (raw), a PowerShell-written stream with a UTF-8 BOM and CRLF.
  • GNU diffutils 3.10 and 3.12: diff -u, -ru, -rN, -rus, -rq, --label, -u on directories (Common subdirectories), CRLF, French locale (raw, and the localized \ Pas de fin de ligne … marker kept by position), quoted names under C and UTF-8 locales on both generations (apostrophes, tabs, "…", quoted directories holding : , and the same directories bare under 3.10), roots ./../new and literal a/b, fifo and symlink facts (raw), a dir-vs-file fact sorted first (raw), -rq with a fifo first (raw), a translated fact after a text section (raw).
  • Mercurial 7.0: hg export text-only, binary-first, and binary-only with a marked message line; hg diff (single--r echo, quoted-character names, tabs) and hg diff --nodates on a tab name; hg log -p over two changesets, with and without --template.
  • Subversion 1.14: svn diff, svn diff --git, a move (empty Index: target), a property change (raw).

Property tests over the corpus: (a) every marked hunk-body line survives verbatim at column 0, verified by replaying the hunk budgets against git's own header counts (over- and under-consumption both fail); (b) every [file] counter equals its rendered lines; (c) the raw fallback fires on zero condensing corpus fixtures, and the seven raw-asserting captures are pinned byte-exact separately; (d) output is never larger than input. Table-driven round trips assert that every rendered [file] name is byte-equal to the real filename under the producer's own root for git and for diffutils 3.10 / 3.12 (C and UTF-8), with entry counts asserted so a spurious decode fails too.

Savings

This is a fidelity filter: it keeps every content line by design, so savings are 50–90 % on metadata-heavy streams but single digits on content-heavy single-file ones. Per CONTRIBUTING.md, the applicable bar is the 20 % admission criterion, asserted per-fixture on the metadata-heavy corpus fixtures; the enforced invariant is guard::never_worse(raw, filtered) — output never larger than input — which the savings test asserts on every fixture. The old filter's headline savings were partly counterfeit, reached by dropping and miscounting content (reproducers 1, 2, 8 in #3769).

Verification

  • Differential against the previous head on every round: 400 real git show streams and 300 git format-patch series from this repo's history, byte-identical except the intended fixture changes.
  • Structural fuzz: every fixture mutated 120 ways (deleted, duplicated, truncated, swapped, inserted structural lines) through the binary — no panics, no fabricated content line.
  • Two independent review passes (six lenses each — producers, parser state machine, name fidelity, upstream code-review prompt, test realism, output contract — every finding re-run against the binary by two adversarial verifiers) before this round was sent; everything they confirmed is fixed and fixtured here.
  • Key fixes proven load-bearing by revert: each pinning test fails against the previous version of the parser.
  • Quality gate on this branch: cargo fmt clean, cargo clippy --all-targets zero warnings, cargo test --all green.
  • Two regexes (LazyLock, touched only inside an mbox message region), no dependencies, no async; the parser is a single pass over split('\n'), allocation-free per line unless the line carries an escape or a quote. The largest real stream in this repo's history (136 KB) parses in under 100 ms on a debug build.

…nks, raw fallback

Parse the stream into (prologue)(file-header)(hunk)* regions before
classifying: hunk extent comes from the @@ line budget (one old-budget per
parent, so @@@ combined headers parse too), content is classified only
inside hunks, and any structural disagreement (invalid body prefix, budget
over/under-run, budget owed at EOF, malformed @@) falls back to raw
passthrough. Detector precedence is a documented total order on
condense_unified_diff_strict.

Fixes, each with its reproducer from
claudedocs/diff-classifier-review-2026-08-29.md as a test:
- SQL/Lua '-- comment' removals (--- on the wire) no longer dropped
- added '++ text' lines no longer parsed as file headers
- format-patch mbox prose and the '-- ' signature no longer counted
- +++ /dev/null deletions named by their --- side
- binary / rename-only / mode-only sections reported instead of vanishing
- --color input stripped (was: silently empty output), non-UTF-8 stdin
  read lossily (was: hard error, no fallback)
- CRLF content bytes survive verbatim (split('\n'), not lines())
- b/ prefix stripped exactly once; timestamps tab-split from header names
- the '... +N more' trailer removed: it claimed truncation that never
  happened (all lines were printed above it)

Fixture corpus: 13 real-producer captures (git 2.54, GNU diff) in
tests/fixtures/diff -- multi-file, -U0, -W, rename/delete/binary,
log -p, show --cc, format-patch single/series/cover, diff -u/-ru/-rN,
CRLF. Property tests: every marked body line survives to the output,
counters equal rendered lines, the fallback fires on zero corpus fixtures.
…fixture hygiene

- Structural fallback now emits the caller's exact bytes (a lossy UTF-8
  decode or ANSI-stripped 'raw' is not raw); the decode-strip-parse-guard
  pipeline moved into condense_stdin so it is unit-testable.
- Dropped the mailer-padding tolerance: a line shorter than the hunk's
  prefix width now falls back instead of being guessed into context.
- Plain-diff fixtures recaptured with relative paths (no local usernames
  or session ids); copy-only and mode-only sections get real fixtures and
  a vanish-class test.
…ighten guards

- new file mode / deleted file mode sections (an empty file added or
  removed) get their own notes; previously they vanished silently whenever
  another file in the stream parsed cleanly. Real fixture captured, fix
  proven load-bearing by revert.
- Signature tolerance (rule 6) now requires the stream to have carried an
  mbox From separator; a bare '--' in a plain diff falls back raw.
- never_worse discrimination by token estimate instead of pointer
  identity; stdin decoded once (Cow) instead of twice; hunk-body prefix
  checked on the byte view without a per-line allocation.
- File-level facts outside hunks become note-only entries instead of
  vanishing while the stream condenses: 'Only in <dir>: <file>' and
  standalone 'Binary files ... differ' (GNU diff -r), '* Unmerged path'
  (git diff --ours during a merge), and 'Submodule <a>..<b>' headers.
  Suppressed inside mbox message regions, where column-0 prose is
  indistinguishable by value. Real fixtures captured for all four shapes;
  fix proven load-bearing by revert.
- is_mbox_from accepts 64-hex separators: SHA-256 repos' format-patch
  streams no longer fall back raw wholesale.
- .context() on the stdin read and raw-fallback write paths.
- Savings comment corrected to the measured corpus numbers (52-87%
  metadata-heavy, single digits content-heavy); exemption escalation
  unchanged.
…-8 gate

- '\ No newline at end of file' survives to the output (in-hunk and the
  common new-side position just after the budget closes): it is the only
  witness of a trailing-newline-only change, which otherwise renders as
  two byte-identical -/+ lines. Real fixture; proven load-bearing by
  revert.
- Non-UTF-8 stdin now always takes the raw-bytes branch, even when the
  stream parses: condensing would rewrite content bytes to U+FFFD, and
  byte fidelity outranks savings. (Base hard-errored here; raw
  passthrough is strictly better on both counts.)
- Doc-comment bounds recorded for the mbox header-pair spoof and the
  raw-fallback newline append.
- A ---/+++ pair opening a NEW section must be followed by a hunk header
  (every real producer's is); a stray marked pair left over from a lying
  budget now falls through to the marked-line detector and raw fallback
  instead of being consumed as a phantom header, losing both lines.
- never_worse guard in condense_stdin compares against the original
  input, not the ANSI-stripped intermediate.
- HUNKLESS_CORPUS pulls the four note-only fixtures into the no-fallback
  and never-larger property tests; savings comment names its metric.
@CLAassistant

CLAassistant commented Aug 31, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on one commit only — and the defect in it came from my advice, not yours, so let me put that first.

On #3769 I told you:

file headers only appear between diff --git (which resets in_hunk) and the first @@, so inside a hunk a +/- line is always content

That is wrong, and you implemented it faithfully. in_hunk is reset by a section header but never at hunk end, so a --- at column 0 after a hunk is still read as content. Two real stream shapes break it, both detailed inline.

The diff_cmd.rs region parser is excellent and I found nothing in it

Verified against the built binary at 789fdc6:

  • 0 / 155 real commits from this repo's history hit the raw passthrough.
  • Tally accuracy checked against git show --numstat on 120 commits and a 143k-line git diff HEAD~500 HEAD — zero mismatches.
  • Producers exercised live: git log -p, git show --cc/-c, git diff -W, -U0, --binary, --color=always, --word-diff (correctly falls back), format-patch series, GNU diff -u/-ru, non-UTF-8 stdin.
  • 400 fuzzed mutated real diffs through rtk diff -: no panics, no non-zero exits.
  • Checked and correct: parse_hunk_header (omitted counts, @@@ arity, closing-marker validation, no slicing panics), per-parent budget arithmetic including the ' '-is-filler rule, \ No newline on both sides, CR-stripped structural decisions with raw byte push, header_name single-prefix strip and tab split, non-UTF-8 byte-exact passthrough, ANSI strip on the stdin path, the never_worse reimplementation, from_utf8_lossy returning Cow::Borrowed on the success path, and the tests/fixtures/** -text gitattributes protecting the CRLF fixture.

The budget-owned-hunk design is exactly right, and the fixture corpus is what made this reviewable.

My recommendation: drop the git.rs commit entirely

Not because the problem isn't real — it is — but because #3269 already fixes it, and fixes it better. That PR rewrote compact_diff around the same budget-based approach your parser uses (Option<HunkHeader> + parse_hunk_header) instead of the two-line guard removal I suggested. I measured it against both shapes that break this commit:

mbox format-patch series  →  +1 -1 per patch   (matches git show --numstat)
diff -ru A B              →  all four change lines at column 0, nothing miscounted
git show --cc             →  names f.txt, renders  - main / -side / ++resolved

That last line matters for you specifically: #3269 matches on line.starts_with("diff --"), so it already covers the diff --cc / diff --combined header handling you added here — without the miscount.

#3269 is approved and mergeable, so it will very likely land first. Rebasing this PR on top and dropping the git.rs commit would remove the finding below, shrink the diff, and leave the region parser — the part with unique value — as the whole of the change.

Sorry for sending you down a path I had not checked properly.

Comment thread src/cmds/git/git.rs Outdated
Comment thread src/cmds/git/git.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Review round 2 (KuSh): no maintainer decision is pending on the savings
floor — CONTRIBUTING.md is authoritative at 20% as an admission
criterion. Drop the stale escalation sentence and cite that bar plus the
never-worse invariant directly.
…ar asserted

Rule 8's exemption narrows from the whole prologue to the mbox message
region only. A stream whose beginning was cut mid-hunk (head-clipped
capture, pipe that lost its first chunk) puts real hunk-body lines
before the first file header; the prologue exemption silently dropped
them while the intact tail condensed into a complete-looking result.
Now they force raw passthrough, honoring the parser's disagree->raw
contract. mbox commit prose (the format-patch --- separator, bullet
lists, quoted hunks) keeps its tolerance; no corpus fixture has
stream-start column-0 marked lines, so nothing real regresses.

Also pins the CONTRIBUTING.md 20% admission bar as a per-fixture
assertion on the metadata-heavy corpus streams, so the percentages
cited in the fidelity comment cannot rot.
@kylehgc
kylehgc force-pushed the claude/upstream-issue-comments-4pky96 branch from 789fdc6 to f4495eb Compare September 1, 2026 01:16
@kylehgc

kylehgc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Round 2 pushed (head f4495eb). Taking your recommendation:

Dropped the git.rs commit entirely — src/cmds/git/git.rs is byte-identical to the merge base again. #3269's budget rewrite is the right fix for compact_diff, and there's no point porting the same logic twice. That also moots the combined-diff test-comment note. No apology needed — the premise was plausible and the empirical check that disproved it was yours.

diff_cmd.rs fidelity comment — the escalation sentence is gone; the comment now cites CONTRIBUTING.md's 20% admission bar and the never-worse invariant directly. To keep the cited percentages from rotting, the bar is now asserted per-fixture on the six metadata-heavy corpus fixtures (metadata_heavy_fixtures_clear_the_admission_bar); content-heavy fidelity shapes stay bound by the never-larger property only.

One hardening from a cold re-review pass (round 5): rule 8's marked-line exemption covered the whole stream-start prologue, so a stream whose beginning was cut mid-hunk (a head-clipped capture, a pipe that lost its first chunk) had its orphaned hunk-body lines silently dropped as prologue prose while the intact tail condensed into a complete-looking result:

printf -- '-lost removal\n+lost addition\ndiff --git a/f b/f\n--- a/f\n+++ b/f\n@@ -1 +1 @@\n-x\n+y\n' | rtk diff -

The base classifier loses the same two lines (its changes.clear() at the first +++), so this is inherited, not introduced — but this parser's contract is disagree → raw, and that was the one remaining marked-line drop not honoring it. The exemption is now narrowed to the mbox message region only (format-patch --- separator, bullet lists, and quoted hunks stay prose); a stream-start marked line forces raw passthrough. No corpus fixture has stream-start column-0 marked lines, so nothing real regresses. Pinned by truncated_prologue_marked_lines_fall_back_to_raw, which fails against the previous guard.

A probe worth running once #3269 lands: the prefix-collision class has one more face in develop's compact_diff — a removed line whose content starts with -- (SQL/Lua/Haskell comment: -- users table → wire --- users table) fails the !starts_with("---") guard and falls to the context branch. Mid-hunk it's uncounted (+1 -1 where --numstat says +1 -2); as the hunk's first line it's dropped entirely (hunk_shown == 0 gate — the file and hunk header render with no content and no tally). Symmetric for added ++-content (++counter;) on the +++ guard. Both reproduce with a plain git diff of a two-line SQL file. #3269's budget-owned hunks should fix this by construction — one probe of that shape after it merges would confirm. The region parser here already handles both (+0 -1 / +1 -2, line kept).

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 2. All three findings from the last round are fixed and resolved — the git.rs commit is gone, and d57c7544 replaced the escalation comment. You went past what I asked on that last one: asserting the 20% bar per-fixture, with an honest note on why content-heavy fixtures are excluded, is a better answer than the citation I suggested.

I also ran the #3769 reproducers against the built binary using real producer output, and every one is fixed with exact counters (checked against git show --numstat, not eyeballed):

# before now
1 -- comment removals dropped, counter under-reports schema.sql (+0 -3), all three kept at column 0
2 ++ text parsed as a file header, label destroyed [file] notes.md (+1 -0), line kept
3 mbox -- signature counted as a removal +1 -1 per file, matches numstat
4 deletions labelled /dev/null [file] del.txt (deleted) (+0 -2)
5 rename-only / mode-only produce no output (renamed from ...), (mode changed)
9 --color → empty output identical to the uncoloured run
10 non-UTF-8 → hard error byte-exact raw passthrough
11 CRLF stripped cat -A shows -l2^M$ — \r survives

Worth recording: my first hand-written fixture had an arithmetically impossible @@ -1,4 +1,1 @@ and the parser correctly fell back to raw. The strict-budget design does what your write-up says it does — I just walked into the trap the issue warns about.

At scale the parser holds: 1167 non-merge commits of this repo through rtk diff - gave 4 raw fallbacks (all genuinely empty commits) and 0 content losses; 300 commits each of highlight.js and DefinitelyTyped gave 2 fallbacks, 0 losses; every (+A -B) label matched its rendered line count across all 1167; 7000 structured-fuzz inputs produced 0 panics and 0 fabricated lines.

Requesting changes for two cases where the byte-fidelity contract does not hold — both verified with repros, both inline. Neither is a design problem; they are two gaps in a net that otherwise catches everything.

Three smaller things are also inline.

On CI: the clippy=FAILURE badge is not this PR. Clippy is clean on f4495eb locally, and clean again after merging current develop (tests green, merge auto-resolves). The check ran while develop was still broken by the lazy_static / RtkRule breakage. A re-run or rebase turns it green.

On the PR description: it still documents the companion compact_diff commit, which is no longer here — I asked you to drop it, so this is my mess to flag. Please trim that section rather than restoring the commit: #3269 has since merged, and it fixed the same defect in compact_diff with budget-owned hunks, including diff --cc / diff --combined as file boundaries. I verified the old starts_with("+++") / starts_with("---") guards are gone from develop. As the description reads now, a reviewer could approve a fix that isn't in the diff.

Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
… --no-prefix names

Structure is read through a per-line ANSI/CR-stripped view and content is
pushed raw, so escapes that are part of the user's content survive (the
file path was already byte-faithful on the same input). Rule 8's mbox
tolerance now ends at the first diffstat line: an orphaned hunk body in a
hand-edited series falls back raw instead of being dropped as prose, while
version notes between `---` and the diffstat stay prose. `diff --git X Y`
with X == Y marks a --no-prefix stream whose header names keep their
leading a/ b/ (creations and deletions included); the check is confined
to the two-path form. The new-side `\ No newline` marker is kept only on
the line right after its hunk closes. The dead `!in_prologue` conjunct
and the variable behind it are gone.

Real `git diff --cached --no-prefix` fixture added to the corpus.
@kylehgc

kylehgc commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Round 3 pushed (fix in 120450b, then current develop merged in as 9106d48 — clean, git.rs untouched) so the clippy badge re-runs against a fixed base. All five findings taken; each has a pinning test that fails against the round-2 parser.

ANSI in content (blocking) — took your first option: the parser now reads every line through a structural() view (ANSI- and CR-stripped, allocation-free unless the line carries an escape) and pushes content from the raw line, the same split the CR handling already used. condense_stdin no longer pre-strips anything. Your repro now gives -^[[31merror^[[0m old / +^[[31merror^[[0m new under cat -v, byte-identical to the file path. One consequence worth naming: on a --color stream the body lines are the input's own bytes, so they start with git's colour escape rather than the marker — the parser cannot tell git's decoration from escapes that are content, and fidelity wins. The label is clean and the counters are right; stdin_colored_stream_parses_with_content_bytes_intact pins the new contract, ansi_inside_content_lines_survives_verbatim pins your repro.

Marked lines in an mbox region past the diffstat (blocking) — I read your suggested gate ("an mbox region that has already seen a file header") and could not make it fit the mangled stream: patch 2's region never sees a file header, that is what was stripped. Gating on the stream having seen one would fall back on every multi-patch series, since patch N's message carries its --- separator and bullets before its own diff --git. What does separate the two is position inside the message region: once the diffstat starts, nothing but diffstat sits between it and the file header. So rule 8's exemption now ends at the first diffstat line ( <path> | N +-) — a column-0 marked line after it and before the next file header is lost content and falls back raw, exactly like the stream-start prologue. Keyed on the diffstat rather than on --- deliberately: version notes (Changes in v2: + bullets) conventionally sit between --- and the diffstat, and keying on --- sent that canonical shape to raw. Keyed on the per-file | line rather than the N files changed summary because the summary is localized. Your mangled two-patch series now passes through byte-exact (-p/+q present, nothing rendered as complete); bullets, quoted hunks, version notes and the signature keep their tolerance; the corpus still has zero fallbacks. Documented bounds: a --no-stat patch has no diffstat, so its tolerance runs to the file header; a message whose own prose has a space-indented | line followed by column-0 marked lines falls back raw — noise, not loss. Pinned by mbox_marked_lines_past_the_diffstat_fall_back_to_raw, which also asserts that the intact series and the version-notes shape still parse.

--no-prefix names — the same-path-twice signal, read from the diff --git X Y line rather than the header pair: the pair cannot decide creations and deletions (one side is /dev/null), the diff --git line can (--no-prefix repeats one path there too). FileEntry carries prefixed: Option<bool> — Some from rule 3, None for sections opened by a header pair alone, where minus != plus decides. That also fixes the rfind(" b/") fallback name you pointed at. The check is confined to diff --git (a diff --cc a a is one path named a a, not a twice). New real fixture git_diff_no_prefix_raw.txt (git diff --cached --no-prefix: modify and create under b/, delete under a/, a path with a space — which git follows with a tab) joins the corpus for the property tests; [file] b/x.rs, [file] a/y.rs (deleted), [file] b/z.rs (new file), [file] b/has space.txt; b/b/x.rs still strips once. Pinned by no_prefix_streams_keep_a_and_b_directories.

Rule 1b proximity — gated on the line immediately after the budget closed (closed_at == Some(i - 1)), and a kept marker extends the window by one so a run of markers is kept whole; a \ line anywhere else outside a hunk is prose. Position rather than value because GNU diff localizes the marker text. Pinned by backslash_line_away_from_a_hunk_is_prose.

Also from a cold re-review pass: the coloured test now covers GNU diff -u --color=always (bare header pair with a coloured @@, the shape where rule 4's lookahead has to read through the stripped view), and the output-format comment says outright that the column-0 guarantee holds for uncoloured input.

Dead !in_prologue conjunct — dropped, and with its only reader gone in_prologue itself is removed; the rule-8 comment no longer claims the implication.

PR description trimmed as you asked: the companion-commit section is gone, the intro says outright that git.rs is untouched here because #3269 fixed compact_diff on develop, and rows 6/9/10 plus rules 1/3/8 reflect this round.

Gate: cargo fmt clean, cargo clippy --all-targets zero warnings, cargo test --all green (2872 unit, all integration). Your six repros re-run against the built binary, output as above.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 3. All five findings from my last review are fixed — the git.rs commit is gone, d57c7544 replaced the escalation comment, and you went past what I asked by asserting the 20% bar per fixture rather than merely citing it. Threads resolved.

I then spent a long pass on the one thing you deferred — the mbox prose exemption. Your "call it a day" was defensible: eight designs later, every attempt to close gap 1 fully either introduces a false-positive class or breaks something the current code handles. But gap 2 turns out to fire on this repo's own history, so it is worth fixing, and there is a small patch that does it. Details inline; the two blocks I would change are :891 and :878.

What I verified about the current head

  • The #3769 reproducers, using real producer output (my first hand-written fixture had an arithmetically impossible @@ -1,4 +1,1 @@ and correctly fell back to raw — the strict-budget design caught me, exactly as your write-up warns). All eight fixed, with counters checked against git show --numstat.
  • A 135-stream format-patch corpus (singles, 2/3/5 series, --no-stat, --cover-letter, --no-prefix, plus mangled variants) and a 300-commit git show corpus, run differentially against every candidate.
  • CI's clippy=FAILURE on an earlier head was not this PR — clippy is clean locally and clean after merging current develop. It ran while develop was broken by the lazy_static / RtkRule breakage. Now green.

The proposal: ~24 production lines

Two mechanisms, layered, both inside condense_unified_diff_strict:

static MBOX_DIFFSTAT_RE: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"^ .+\| *(?:\d+(?: [-+]+)?|Bin\b.*) *$").unwrap());

static MARKED_PROSE_RE: LazyLock<Regex> =
    LazyLock::new(|| Regex::new(r"^(?:[-+] \S|-+\s*$|--[A-Za-z])").unwrap());
  1. MBOX_DIFFSTAT_RE replaces contains(" | ") as the past_diffstat trigger. past_diffstat is kept — see :878, this turns out to be load-bearing for a shape neither of us modelled.
  2. Two bools, region_reached_body / region_exempt_nonprose, set at both file-header sites and checked at the From <sha> arm and after the loop.

Full branch if useful: KuSh:pr3788-lead5.

Seven gates, all green: the 6-case mbox harness; a 4-case discriminator; the first-file-mangle adversarial case; real bulleted cover letters with and without a diffstat; the 135-stream differential (130 identical, 3 new fallbacks all on *_mangled, 0 content-diff); 300/300 on git show; and flag-shaped prose in bodyless regions. fmt, clippy and cargo test --all clean.

The honest cost — this is not a free win

Tightening the trigger gives up ~5-7 streams that the current code rescues by accident. Measured over 60 real multi-file commits, --no-stat with the first section mangled: current head 8 RAW / 52 lost, patched 1 RAW / 59 lost. Those 8 rescues happen because a loose " | " matched a space-indented context line inside the orphan body — shell pipes, markdown table rows, a doc-comment pipe — and accidentally forced a fallback. They are a side effect of gap 2 itself, so any fix for gap 2 surrenders them.

I do not think that changes the answer, since the same shape already loses 52/60 today, but you should decide that rather than discover it.

What I tried and rejected, so you do not have to

  • Replacing past_diffstat with a region-close check, a Subject: [PATCH 0/N] cover-letter test, or a look-ahead for a patch body. Five independent designs; all five lose content on the shape at :878. The reason is structural, not tuning.
  • A both-polarities discriminator. 17% of real diffs are single-polarity (78 plus-only, 26 minus-only of 599), so it misses a large slice, and it false-positives on empty commits and on cover letters whose blurb has bullets.
  • Surrendering orphaned lines as a labelled entry instead of falling back raw. Tempting — it recovers 17% of bytes on mangled streams. But the entry loses the filename ([PATCH 4/4] docs(config): … where the intact parse gives docs/guide/getting-started/configuration.md) and its counts are wrong (+13 -8 against a true +13 -5, because commit-message bullets get counted as removals). An agent reading that has to re-fetch, which costs more than the raw fallback it replaced.

Still open after this patch

A --no-stat patch whose first section is mangled while a later one survives. No diffstat for past_diffstat to key on, and the region reaches a later file header so every region-close check is blind. Current code loses 52/60 here; the patch loses 59/60.

One unexplored lead, if you ever want it: format-patch --no-stat always leaves exactly one blank line before diff --git, whereas a mangled first section leaves the orphan body immediately adjacent. That is a positional discriminator rather than a shape heuristic, so it would not reintroduce the false positives that sank the other designs. I did not build it.

Five smaller findings on the file-level-fact arms are inline — all low, all demonstrable from fixtures this PR itself ships.

Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
KuSh and others added 2 commits September 1, 2026 22:07
…less region

`condense_unified_diff_strict` ends rule 8's mbox prose exemption at
format-patch's diffstat, detected by `contains(" | ")`. That matches
ordinary commit prose. It fires on this repo's own history: a bullet
reading "- The FEATURES.md `--line-numbers | -n` row was STALE" ends the
exemption, the bullets after it read as lost content, and the stream
falls back raw — 4097 bytes where 1634 condensed would do. Documenting
an option as `long | short` is standard notation, not an edge case.

Anchor the trigger on the count-and-signs right-hand side instead. Nine
real diffstat shapes still end the tolerance, including `Bin`, a bare
count for a pure rename, brace renames and paths containing a pipe.

`past_diffstat` is kept, not replaced. It is load-bearing beyond the
diffstat: it is the only mid-region check, so it is what catches a
stat-ful patch whose *first* section lost its headers while a later one
survived. Five designs that swapped it for a region-close check all lost
that shape — 60 real multi-file commits, 60 raw fallbacks before, 60
silent losses after.

A `--no-stat` patch has no diffstat, so there the exemption is settled
when the region closes, and only for a region that never reached a file
header. Such a region is bodyless by construction — a cover letter, an
`--always` empty commit — or a patch whose body lost its headers, and
only the second is loss. Shape tells them apart: over 111k marked lines
from 698 real diffs, no whole diff is entirely prose-shaped. Measured on
49 real two-patch series, 8 raw fallbacks before, 49 after.

The cost is honest: tightening the trigger gives up the streams the
loose match rescued by accident, when a pipe in a context line inside an
orphan body happened to force a fallback. Those rescues were a side
effect of the bug being fixed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ames, quoted paths

Review round 3 on the file-level-fact arms, plus a cold pass on top:

- `Only in <dir>: <file>` splits on the FIRST `: ` (GNU diff's separator
  follows the directory; a filename may carry its own).
- Standalone `Binary files X and Y differ` splits at the ` and ` where
  both sides agree past their first component, so a filename containing
  ` and ` is not cut, and is named through `header_name` so the `a/`/`b/`
  strip follows the same decision as a header pair.
- `* Unmerged path X` folds into the section git emits for X right after
  it: one `[file] X (unmerged) (+A -B)` entry instead of two.
- `Submodule <path> <a>..<b>` keeps only the path in the name slot (the
  tokens before the LAST `..` token, so a path containing `..` survives),
  and the sha-less `Submodule <path> contains ... content` dirty line is a
  fact too -- it used to be dropped silently beside a condensed sibling.
  Real fixture `git_diff_submodule_dirty_raw.txt` joins the corpus.
- `rename to` / `copy to` is git's exact path; the `+++` pair no longer
  re-derives it, which stripped a real `b/` directory under `--no-prefix`.
- `core.quotepath` wraps non-ASCII paths in quotes on every header line;
  names are dequoted before the prefix strip.
- The header-pair hunk gate applies to open hunkless sections as well:
  git only emits `---`/`+++` before the first `@@`, so a stray pair there
  is lost content and falls back raw instead of renaming the section.
- `diff --cc X` carries one path and leaves `prefixed` to its header pair.
- `past_diffstat` comment states the first-section-mangled shape it alone
  catches; the ` name | 3` bound says it fires always (the `---` separator
  is the marked line that follows); the stdin doc says the never-worse
  check is inlined and why.

A/B against 9106d48: 400 `git show` streams byte-identical; 290
format-patch series identical except 12 that fell back raw through a
prose pipe and now condense, zero new fallbacks.
@kylehgc

kylehgc commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Round 4 pushed (your pr3788-lead5 commit as 43b3184, then 613e785).

Took your commit as-is, your authorship kept — MBOX_DIFFSTAT_RE plus the shape check for stat-less regions. The ~5-7 accidental rescues it gives up on mangled --no-stat streams are a fair trade against a false positive that fires on this repo's own history; decided, not discovered. A/B against 9106d48 over 290 real format-patch series from this repo (3-commit windows, with and without --no-stat): 12 series that fell back raw through a prose pipe now condense, zero new fallbacks; 400 git show streams byte-identical.

All five fact-arm findings taken, each pinned by a test that fails against 9106d48:

  • Only in splits on the first : — [file] docs/notes: draft.md (only in one side).
  • Binary files X and Y differ finds the and where both sides agree past their first component, so black and white.png is not cut, and is named through header_name so the b/ strip follows the same decision as a header pair (img.bin now matches its siblings f1.txt / sub/deep.txt in diff_ru_raw.txt). Only in deliberately keeps the producer's root — the root is the fact — and the rule-6 doc now says so.
  • * Unmerged path folds into the section git emits right after it: [file] cfile.txt (unmerged) (+4 -0), one entry.
  • Submodule takes the tokens before the last .. token — [file] sub (submodule), and a path containing .. keeps its name.
  • The past_diffstat comment states the first-section-mangled shape it alone catches.

A cold review pass on top surfaced four more, also pinned:

  • git's sha-less Submodule <path> contains modified content line was dropped silently whenever a sibling condensed — a real git diff capture is now in the corpus and renders [file] sub (submodule, modified content) (untracked, untracked and modified likewise).
  • --no-prefix renames lost their real b/: the +++ pair re-derived a name rename to had already given exactly. The pair no longer overrides a rename/copy note — [file] b/dst.txt (renamed from b/src.txt).
  • core.quotepath (git's default) quotes any non-ASCII path on every header line, so the prefix strip never fired: names are dequoted first — [file] caf\303\251.txt.
  • The header-pair hunk gate now applies to an open hunkless section too, since git only emits ---/+++ before the first @@; a stray pair there falls back raw instead of renaming the section and dropping two lines. Relatedly, diff --cc X carries one path and cannot settle the prefix question, so it leaves that to its pair (diff --cc b/x under --no-prefix now names b/x).

Doc honesty: the name | 3 residual falls back raw always, not only when marked lines follow — format-patch's own --- separator is that marked line. The bound now says so.

Gate: fmt clean, clippy zero warnings, cargo test --all green (2967 unit + integration). Your b3ae7be repro: 4108 → 1633 bytes, [file] scripts/test-all.sh (+17 -5).

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 4. All seven findings from my last review are fixed — threads resolved. The label arms all check out: [file] cfile.txt (unmerged) (+4 -0) folded into one entry, [file] sub (submodule) with a recoverable path, black and white.png split correctly, docs/notes: draft.md splitting on the first ": ", the never-worse doc claim gone, and b/x.bin respecting the prefix decision in a --no-prefix stream.

Thanks for taking the diffstat/region patch. I re-ran the seven gates on 613e785 and they all hold: 6/6 on the mbox harness, RAW/RAW/condensed/condensed on the discriminator, RAW (content preserved) on the first-file-mangle adversarial case, and clean on real bulleted cover letters and on flag-shaped prose in bodyless regions.

One correction to my last round: I flagged [file] img.bin (binary) in the diff -ru fixture as still wrong. It wasn't — in a diff -ru a b stream a/b are the comparison roots, so img.bin is the right name. My isolated probe was the wrong instrument; embedded in a real --no-prefix git stream it gives b/x.bin correctly.

Requesting changes for one new issue, which is the same silent-loss class this PR exists to eliminate — it just arrives through a door nobody was watching. Details inline at :915 / :930, plus a small asymmetry at :985.

Verification on this head: cargo test --all (2976+83), clippy --all-targets and fmt --check all clean. An independent differential over 400 git show and 300 git format-patch streams from this repo's history — reimplementing the @@ budget separately and comparing true in-hunk body lines against rtk's output — found 0 mismatches and 0 content loss, the only fallback being the intentional non-UTF-8 Latin-1 fixture. Over-declared, under-declared and truncated budgets all reach return None correctly. The parser itself is in very good shape.

Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
…ule range kept

GNU diff translates `Only in` and `Binary files … differ` (git translates
none of its own lines). Rule 6 read the English spelling only, so on a
French machine `diff -ru g1 g2 | rtk diff -` dropped the three translated
fact lines as prose and, because a text section still parsed, returned
`Some` -- four files became one, silently.

Rule 3b now reads GNU diff's per-file `diff <opts> X Y` echo (printed
whenever it compares directories) as the mark of a `diff -r` stream. Such
a stream carries no prose, so a column-0 line no arm read there is a fact
in a language the parser does not speak, and rule 9 returns `None`
instead of dropping it. A line dropped before the echo settled the
question counts the same way once it does -- `diff -r` may list an
`Only in` first. Real fixture `diff_ru_fr_raw.txt` (`LC_ALL=fr_FR.UTF-8`,
diffutils 3.10) pins the raw fallback; the English `diff_ru_raw.txt`
still parses in full, as do `git log -p` streams whose prologue is prose.

The `Submodule <path> <a>..<b>` note now carries the range --
`[file] sub (submodule e139196..b0ac9b1)` -- for parity with the dirty
arm's `(submodule, modified content)`.
@kylehgc

kylehgc commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Round 5 pushed (4ccb5aa).

Translated GNU fact lines (blocking) — took your second option, the cheap one: raw, never loss. The signal I keyed on is the one GNU diff gives for free: its per-file command echo (diff -ru X Y), printed whenever it compares directories, which git never emits. Rule 3b reads that echo as "this is a diff -r stream"; such a stream carries no prose, so a column-0 line no arm understood there is a fact in a language the parser does not speak, and rule 9 returns None. One wrinkle your repro does not show but diff -r's merged directory listing produces: an Only in can come first, before any echo. A column-0 line dropped as prologue prose is remembered, and the echo, when it arrives, turns that drop into None as well.

Pinned with a real capture rather than your pasted lines: tests/fixtures/diff/diff_ru_fr_raw.txt is LC_ALL=fr_FR.UTF-8 diff -ru g1 g2 from diffutils 3.10 (Debian, locales-all; Git for Windows ships no diffutils.mo). Through 4ccb5aa it comes back byte-identical; through 613e785 it gave g2/f.txt and g2/sub/deep.txt and lost the other three. The English diff_ru_raw.txt still parses in full (echo consumed as a context marker, four facts labelled), and a git log -p stream whose prologue is commit/Author: prose is untouched because it has no echo. LC_ALL=C was never on the table for the reason you gave — nothing is spawned on this path — and the rule-6/9 docs now say the two GNU arms are English-only by value with rule 9 as the net.

Cost, stated: every localized diff -r stream now falls back raw, always. On your machine that is diff -ru by default. Positional parsing of the translated lines would recover the savings, but I could not find a shape that survives Only in lines whose directory is the last thing before : in every language (Seulement dans g1: x and Nur in g1: x do, but the binary line puts the two paths at different positions per language), so I did not fabricate one.

Submodule range — kept in the note: [file] sub (submodule e139196..b0ac9b1), and a..b as a path still names itself. The indented > subject lines stay prose.

Differential against 613e785: 400 git show streams identical; 300 format-patch series identical (0 new fallbacks); every fixture identical except the two intended (diff_ru_fr → raw, submodule note). fmt / clippy / cargo test --all clean.

Benchmark CI red is the develop-side flake you have seen before, not this head.

@KuSh KuSh left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 5. All three findings from my last review are fixed — threads resolved, and the GNU-fact-line fix took the option I argued for: an unrecognised localized marker is now a structural disagreement that falls back raw rather than a line dropped as prose. Verified on this machine's French locale:

$ diff -ru g1 g2 | rtk diff -            # fr — was: 3 of 4 files silently dropped
Les fichiers binaires g1/bin.dat et g2/bin.dat sont différents
diff -ru g1/f.txt g2/f.txt
...                                       # byte-exact passthrough, nothing lost

$ LC_ALL=C diff -ru g1 g2 | rtk diff -   # unchanged, still condenses
[file] g2/bin.dat (binary)
[file] g2/f.txt (+1 -1)
[file] g1/onlyleft.txt (only in one side)
[file] g2/onlyright.txt (only in one side)

The submodule arm now carries the range ([file] sub (submodule 0aa534a..2bf16f5)), matching its sibling. I also re-checked that earlier rounds did not regress: the gap-2 trigger still condenses, and the first-file-mangle adversarial case still falls back raw with content preserved.

One blocking ask, at :787. Everything else below is a note — take them or leave them.

What I verified about this head

  • 995 file sections across 300 real commits: every [file] X (+A -R) counter matches git --numstat exactly, 0 missing sections, 0 fallbacks.
  • Option matrix (-U0, -U10, --no-prefix, --color=always, -M -C, --patience, --histogram, --ignore-all-space, -W, --binary, --stat -p, --summary -p) — counters correct throughout.
  • Content-loss sweep over 400 real commits and 4000 mutated diffs: zero dropped hunk-body lines, with 3144/3990 malformed inputs correctly taking the raw path. 3000 byte-level fuzz mutations: no panics.
  • Savings A/B over 200 commits: 35.0% against the base's 35.1% — no regression. 3 MB diff in 14 ms.
  • CI is green on fmt, clippy and all three OS jobs.

The parser is materially more correct than what it replaces, and the content-preservation thesis survived everything I threw at it.

Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
…dequoted renames

`\ No newline at end of file` describes the line directly above it. The
parser never renders context lines, so a marker git attached to an
unchanged last line landed under the last `-`/`+` line rendered and
claimed THAT line lacked a newline -- the one place the output stated
something untrue about the diff. The marker is now kept exactly when the
line above it was emitted (in-hunk after a `-`/`+`, or on the line right
after the budget closed on one), which keeps the witness of a
newline-only change and drops a marker that would describe the wrong
line. `emitted_at` replaces `closed_at`: the position test is the same
for both rules. Real fixture `git_diff_no_newline_raw.txt` carries both
shapes.

`# HG changeset patch` opens a message region like an mbox `From`, so
`hg export`'s column-0 headers and message no longer latch
`dropped_prologue` and send the whole changeset raw through its own
`diff -r <a> -r <b> <file>` echo. Real fixture `hg_export_raw.txt`
(Mercurial 7.0.1) joins the corpus.

`rename to`/`copy to` (and their `from`) go through `dequote`, so a
`core.quotepath` name has one spelling across a stream. The submodule
note keeps the range's direction qualifier:
`[file] sub (submodule e139196..b0ac9b1 rewind)`.

The body-line replay in `corpus_every_marked_body_line_survives` shares
the parser's per-parent presence rule (there is no second formula for
combined-diff columns); its doc now says so and names the oracle -- git's
header counts -- and it asserts a budget is never still owed at a
non-body line or at EOF, so under-consumption fails there too.
@kylehgc

kylehgc commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

Round 7 pushed (9634a9a, on top of the d12acfc merge you reviewed).

\ No newline placement (blocking) — took your first shape, because it makes the marker correct rather than merely true: it is kept exactly when the line directly above it was emitted, in-hunk after a -/+ or on the line right after the budget closed on one. A marker git attached to a context line is dropped — the output never carries that line, so the marker would describe the wrong one, and it witnesses no change anyway. The note form would have needed an old/new qualifier to say which file lacks the newline, and the newline-only case (-c, marker, +c) is exactly where that distinction matters. emitted_at replaces closed_at; the same position test now serves both rules. Pinned on a real git diff with both shapes in one stream: your a\nb\nc → A\nb\nc gives -a / +A and nothing under it; x\ny gaining its newline gives -y / marker / +y.

hg export — the latch was doing its job on the wrong producer. # HG changeset patch now opens a message region the way an mbox From does, so hg's column-0 headers and message are exempt prose, and its diff -r <a> -r <b> <file> echo lands after the region closes. Real hg export tip (Mercurial 7.0.1) is in the corpus: two files condense, a translated fact line appended after the second echo still forces raw. The diff -r context stays as it was for GNU streams; I did not weaken the latch, since your two suggested relaxations would both reopen the Only in-first French case.

Quoted renames — rename from/to and copy from/to go through dequote; pinned with [file] na\303\257ve.txt (renamed from caf\303\251.txt).

Submodule qualifier — [file] sub (submodule e139196..b0ac9b1 rewind); the log body lines stay prose.

Replay independence — you are right that there is no second formula for the per-parent rule, so I stopped claiming one. The test's doc now says the walk shares the rule and names the oracle it is checked against: git's own header counts, which must be consumed exactly. Over-consumption already underflowed; under-consumption is now asserted too — a budget still owed when a non-body line or EOF arrives fails there, not only through property (c). The drift you describe (rule wrong, both walks agreeing) cannot consume git's counts exactly on the @@@ fixture, which is what that assertion pins.

Differential against d12acfc: 400 git show streams and 300 format-patch series byte-identical; the only fixture changes are the three intended (no-newline, hg, submodule note). fmt / clippy / cargo test --all clean.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified round 7 against 9634a9a8. The \ No newline at end of file handling is right — the marker is kept and correctly positioned when it describes an emitted line, suppressed when it describes a context line, with a fixture covering both shapes. hg export condenses, rename/copy dequote, submodule carries (rewind). Four threads resolved.

Two blocking items below, both silent content loss with no raw fallback. :813 is new this round; :1102 is pre-existing but reproduces in plain English with no locale involved, so it seems worth closing here rather than carrying it forward.

On the dequote question, and why I'd stop fixing it arm by arm

I went through every arm that builds a name and checked what its producer actually emits, rather than what the parser assumes:

arm producer emits needs
diff --git, ---/+++, rename to, copy to "caf\303\251.txt" — git C-quoting strip wrapper and decode escapes
Binary files … differ (git) "a/caf\303\251.bin" same
Binary files … differ (GNU) 'gb/d1/caf'$'\303\251''.bin' — shell quoting a different decoder
Only in (GNU) 'caf'$'\303\251''.txt' — shell quoting a different decoder
* Unmerged path café.txt — never quoted, even for a path containing " nothing
Submodule … café — never quoted nothing

So the three arms without dequote are correct as written — adding it there would corrupt a path that legitimately contains a quote. I had them down as missing-dequote bugs before I checked the producers; they aren't, and I'd rather say so than have you patch them.

What the table does show is that the shared helper is doing the wrong job in two places, and that is the part worth factoring:

  • dequote strips the wrapper but leaves the escapes, as its doc says. So every arm that has dequote still renders [file] caf\303\251.txt — a name the agent downstream can't cat, can't grep for, can't hand back to git. core.quotepath is on by default, so that is every non-ASCII path, on the main code path.
  • the GNU arms need a shell-quote decoder that doesn't exist, and :1013 currently routes GNU input through the git one.

The factorisation the table argues for isn't a universal DiffPath newtype — that would break the two arms that must not decode. It's one decoder per producer, selected where the producer is already known, so an arm can't inherit the wrong one by default.

On testing it

A "no \" survives into a rendered name" invariant is what I reached for first, and it was actively misleading: it flagged all three of the correct arms and stayed silent on both real defects, because it tested the parser against inputs no producer emits.

What would have caught these is a round trip against the real producers — create a file whose name needs quoting (non-ASCII, an embedded ", a space), run real git diff / real diff -ru, feed the output in, assert the rendered name is byte-equal to the actual filename. One table-driven test, covers both directions (a missing decode and a spurious one), and it keeps working for arms added later. The fixtures here are already captured from real producers, so most of the machinery exists.

Notes inline. Everything other than :813 and :1102 is non-blocking.

Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs Outdated
Comment thread src/cmds/git/diff_cmd.rs
Comment thread src/cmds/git/diff_cmd.rs Outdated
… GNU diff, hg and svn

Review round 7 asked for two things; a producer enumeration and two
adversarial review passes answered for thirty. Every item below has a
real capture in the corpus (81 fixtures now: git 2.47/2.54, diffutils
3.10 and 3.12, Mercurial 7.0, Subversion 1.14, PowerShell) and a test
that fails without the fix.

Silent loss (a file or content line missing while the output looked
complete) -- each now returns `None`, byte-exact raw passthrough:

- GNU sorts its output, so a fact the parser cannot read (`File X is a
  fifo ...`, `Symbolic links ... differ`, a translated `Only in`) can
  come before the first `Only in` / `Binary files` / `Files` line, and
  `-q` streams carry no echo at all. The latch is now `dropped_line` --
  any column-0 line dropped as prose outside a message region -- and is
  judged once after the loop against `gnu_stream`, which GNU's fact arms
  set as well as its echo (hg's echo does not: an `hg log -p --template`
  prologue is prose, not a fact).
- `hg export`'s region stayed open until the first header pair and
  swallowed `Binary file bin.dat has changed` on a binary-first
  changeset. hg's `diff -r` echo (one `-r` from `hg diff`, two from
  `hg export` / `hg log -p`) now closes the region, counts as reaching
  its body, and the line is a binary fact; rule 6 stays live inside an
  hg region. `hg log -p`'s `changeset:` line opens the region like
  `# HG changeset patch` does.
- `git diff --word-diff` / `--color-words` hunks whose lines are all
  indented carry their `[-x-]{+y+}` markers behind the indent, so the
  hunk was booked as context and the file vanished. A hunk that closes
  with no `-`/`+` line is not a unified-diff shape (whitespace-ignoring
  modes suppress the hunk) -> `None`.
- `git format-patch --submodule=log` puts `Submodule sub a..b:` where a
  file section would go, inside the message region where rule 6 is
  suppressed. The strict `<hex>..<hex>[ (<how>)]:` shape is admitted
  there (`is_submodule_range`); prose never ends a line that way.
- A stream cut off right after `diff --git` / `index` dropped the file
  it named: git never emits a header-only section, so one that closes
  with nothing behind it -> `None` (`FileEntry::from_git`; `flush`
  returns `Option`). git's `index` / `similarity index` lines are
  structural and no longer count as dropped prose.
- A plain `svn diff` names a binary only as `Index: b.dat` + `Cannot
  display: file marked as a binary type.`, both prose before: the
  binary vanished beside its text siblings. `Index: <path>` opens a
  section (rule 3c); the notice is its binary note; an `Index:` section
  that closes empty is `(no content)` (a copy/move target) unless a
  column-0 line was dropped inside it (a localized notice) -> `None`.
- A binary section followed by another producer's header pair was
  renamed in place and its `binary` note landed on the next file; a
  pair after a binary section opens its own section.

A crash: the shared-tail scan behind `diff --git X Y` and `Binary files
X and Y` compared bytes, and two different multi-byte characters can
share trailing bytes (`😀`/`🙀`, `é`/`©`): the slice landed mid-character
and the process aborted with no output at all. The scan backs off to a
character boundary on both sides.

Names, byte-equal to the real filename under the producer's own root:

- `dequote` decodes git's C quoting (`"caf\303\251.txt"` -> `café.txt`)
  instead of stripping the wrapper; a name that would split a `[file]`
  line (newline) or is not UTF-8 keeps git's own quoted spelling, with
  the prefix stripped inside the quotes, so `rename to`, `diff --cc` and
  the header pair agree on one spelling and two Latin-1 names stay two.
- `unquote_shell` decodes diffutils >= 3.11's shell quoting on `Only in`
  / `Binary files` names -- `'…'`, `$'…'` and the `"…"` form it picks
  for a name holding a `'`; a quoted directory may itself hold `: `, so
  the `Only in` split follows the quoting (`shell_word_len`). 3.10
  prints those names bare; a bare `Only in` with more than one `: `
  splits at the root the stream already named on its echo, header pairs
  or `Binary files` lines. Both generations are in the corpus.
- `diff --git X Y` is split where the sides share the longest tail at a
  `/` boundary (`shared_tail`); the first path component before the
  tail on each side is that side's prefix (`FileEntry::prefixes`
  replaces `prefixed`): `a/`/`b/`, `i/`/`w/` under
  `diff.mnemonicPrefix`, `--src-prefix`/`--dst-prefix`, none under
  `--no-prefix`. The header pair strips exactly those, so `* Unmerged
  path b/inb.txt` folds into its `i/b/inb.txt w/b/inb.txt` section, a
  binary under a directory named `x b/` keeps its path, and `git diff
  --no-index d1/f d2/f` names `d2/f`. `diff --cc <path>` is kept whole;
  `--no-index --no-prefix x.bin y.bin` names `y.bin`.
- GNU diff names files under the roots it was given, never prefixes:
  header pairs after a GNU echo and the standalone `Binary files` /
  `Files` arms keep them (`diff -ru a b` -> `b/c.txt`, `diff -ru .
  ../new` -> `../new/x`), splitting `X and Y` by the same shared tail.
- Timestamps are stripped from `---`/`+++` lines only, at the LAST tab
  and only when shaped like one of the real producers' (GNU ISO date,
  hg `Mon Sep 07 …`, svn `(revision 1)`, git's bare tab), so a name
  containing a tab survives even under `hg diff --nodates`.
- An `* Unmerged path` fact git printed unquoted and cut at a newline
  folds into its quoted section instead of a phantom entry.

Spurious raw fallbacks on well-formed streams, now condensed: `git log
--stat -p` / `git show --stat -p` (bare `---` followed by a diffstat
line is the separator -- which also drops the old ` name | 3` bound),
`git log --numstat -p` (`-<TAB>-<TAB>` binary rows), `hg log -p` with
and without `--template`, an hg binary-only changeset whose message has
a marked non-prose line, `--format=%B` prose starting with `diff ` (the
GNU echo arm keys on `diff -`), `diff -u` on directories (`Common
subdirectories:` dropped), an svn copy/move target.

Also: GNU `-s` / `-q` facts are entries; a PowerShell-written stream's
UTF-8 BOM is stripped; a submodule both dirty and moved is one entry;
the budget replay in property (a) asserts under-consumption; the docs
list rule 1b and every bound is executable in `documented_bounds_hold`.
@kylehgc

kylehgc commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Round 8 pushed (27abf4a).

Your two blockers are fixed, and rather than wait for the next pair I enumerated producers the way you have been — real captures of git 2.47/2.54, diffutils 3.10 and 3.12, Mercurial 7.0, Subversion 1.14 and PowerShell across the option matrix and quoting styles — and put two adversarial review passes over the result. The corpus is 81 real fixtures now; every item below has one and a test that fails without the fix. The PR description is rewritten to match (rules 1b, 3b, 3c, the quoting table, the corpus).

GNU facts after an entry (blocking) — the latch is now "a column-0 line was dropped at all", judged once after the loop against "this is a GNU diff stream", which GNU's fact arms establish as well as its echo. That also closes the ordering you did not write up: GNU sorts, so File X is a directory … can precede the first Only in, and -q streams never carry an echo. Your fifo/symlink capture, a dir-vs-file capture and a -rq fifo-first capture all come back byte-exact.

hg region (blocking) — hg emits its diff -r echo for every file, binary ones included (real 7.0.1 capture), so the echo closes the region and counts as reaching its body; Binary file X has changed is a fact, and rule 6 stays live inside an hg region so your echo-less example works too. While there: hg diff echoes a single -r (the detector only knew two), hg log -p opens with changeset: at column 0 and always went raw, and a templated hg log -p has a prose prologue with no header at all — all three condense now.

Names — took the table's argument and went further, against real captures. git's C quoting is decoded (café.txt, quo"te.txt, a tab); diffutils 3.11+ shell quoting is decoded including the "…" form it picks for a name holding a ' ("Kyle's notes.txt") and a quoted directory that itself holds : ; 3.10 prints those names bare, and a bare Only in e: y: only.txt now splits at the root the stream already named on its echo and header pairs rather than at the first : . A name a [file] line cannot carry (a newline) or that is not UTF-8 keeps git's own quoted spelling with the prefix stripped inside the quotes, so rename to, diff --cc and the header pair agree on one spelling and two Latin-1 files stay two. The diff --git X Y split records the prefix it finds as one path component (a//b/, i//w/ under diff.mnemonicPrefix, --src-prefix/--dst-prefix, none under --no-prefix) and the pair strips exactly that, so * Unmerged path b/inb.txt folds into its mnemonic section, a binary under x b/ keeps its path, git diff --no-index d1/f d2/f names d2/f, and diff --cc b/blob.bin is kept whole. GNU roots are never prefixes: after a GNU echo, header pairs and the Binary files / Files arms keep diff -ru a b's b/c.txt and diff -ru . ../new's ../new/x, splitting X and Y by the longest shared tail at a / boundary. That reverses the img.bin reading you settled on in round 4, deliberately: with literal a/b roots the old output mixed f1.txt and img.bin with b/newfile.txt in one stream, while b/f1.txt, b/img.bin, b/newfile.txt are all paths that exist under the cwd diff was run from, and match what git diff --no-index a b names. If you would rather strip literal a/b roots, it is one flag. Timestamps come off ---/+++ lines only, at the last tab and only in the shapes real producers write, so a tab inside a name survives even under hg diff --nodates.

A crash the review found: the shared-tail scan compared bytes, and two different multi-byte characters can share trailing bytes (😀 and 🙀, é and ©) — the slice landed mid-character and rtk diff - aborted with no output at all on a rename between emoji names under core.quotepath=false. It backs off to a character boundary now; real capture in the corpus.

More silent-loss shapes, found and fixed: git diff --word-diff / --color-words where every hunk line is indented (the markers sit behind the indent, the hunk was context, the file vanished) — a hunk that closes with no marked line is not a unified-diff shape, raw; format-patch --submodule=log puts Submodule sub a..b: inside the message region — the strict <hex>..<hex>: shape is admitted there; a stream cut off right after diff --git / index dropped the file it named — git never emits a header-only section, raw; a plain svn diff binary (Index: + Cannot display…, both prose before) vanished beside its text siblings — Index: opens a section, the notice is its binary note, an Index: that closes empty is (no content) unless words were dropped inside it, then raw; a binary section followed by another producer's header pair was renamed in place and lost.

Spurious raw fallbacks, now condensed: git log --stat -p / show --stat -p (the bare --- before the diffstat — which also retires the name | 3 bound from round 4), git log --numstat -p, hg log -p, an hg binary-only changeset with a marked non-prose message line, --format=%B prose starting with diff (the GNU echo arm keys on diff -), diff -u on directories (Common subdirectories:), an svn copy/move target. GNU -s/-q facts are entries; a PowerShell-written stream's BOM is stripped (real Out-File capture, BOM + CRLF, binary first); the localized \ Pas de fin de ligne … marker is in the corpus, kept by position.

Verification: fmt / clippy clean, cargo test --all green; differential against 9634a9a over 400 git show streams and 300 format-patch series byte-identical; structural fuzz over the 81 fixtures with no panic and no fabricated line; every documented bound is now an executable test.

@KuSh KuSh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round 7 (landing) — APPROVED

Claim: replace condense_unified_diff with a region parser that never silently drops content, falling back byte-exact to raw when the structure disagrees.
Scope: accept. (Stated now — rounds 1–6 predate the current template; frozen from here.) Closes #3769.
Ran: 400 real git show streams × develop/9634a9a8/27abf4ad; 1041 mutated fixture variants; 7 thread repros; merge into b0d471eb. Savings 35.5% vs the 20% floor (CONTRIBUTING.md).

Blocks merge (0 — both cleared)

Both blockers from the last round are fixed, verified by their own repros:

  • :813 hg region — Binary file bin.dat has changed before the first text header now condenses to [file] bin.dat (binary) instead of vanishing. Better than the raw fallback I asked for.
  • :1102 GNU facts after an entry — the fifo/symlink capture comes back byte-exact; b_fifo and b_link are preserved.

Checked and correct — no need to re-verify

  • No regression on ordinary streams. 400 real git show streams from this repo's history: 27abf4ad is byte-identical to 9634a9a8 on 400/400. The 1841 new source lines are additive — they pick up producers that previously went raw or were lost, and leave the git path untouched.
  • No content loss. Same 400 streams, checking every path named in a diff --git header survives into the output: 0 losses on develop, 0 on this head (324 condensed, 76 raw fallbacks).
  • No panics. 1041 mutated fixture variants (deletions, duplications, truncations, mid-stream cuts, line swaps): 0 panics, 0 timeouts, 0 content loss.
  • Savings. develop 36.6%, this head 35.5% — 1.1pt for the fidelity, well over the 20% floor.
  • Merges clean. Into b0d471eb: no conflicts, compiles clean, 3322 tests pass.
  • Gates. cargo fmt --all --check and clippy --all-targets clean; full CI matrix green on ubuntu/macos/windows; CLA signed.
  • The five non-blocking notes from the last round are all fixed: C-escape decoding (café.txt, not caf\303\251.txt), GNU shell-quote decoding, the g1// double slash, and the dirty-and-moved submodule folding into one entry.

Threads

The :2580 thread is answered. At round 5 the doc called this walk "independent"; it was not,
for the per-parent presence rule. You dropped that word and documented the real oracle — git's
counts, consumed exactly, with over-consumption underflowing and under-consumption asserted.

I checked it by mutation rather than by reading: dropping the shared-context term from the
implementation's rule, leaving the replay untouched. It is caught — by
corpus_never_falls_back_to_raw and the combined-diff parse tests. Worth knowing that
corpus_every_marked_body_line_survives, the test the replay lives in, still passes under that
mutation: with the rule wrong the budget never zeroes, the stream falls back to raw, and every
marked line then survives trivially. So the rule is pinned by the suite, just not by the walk
its own doc sits above.

Not blocking and no follow-up: the doc no longer overclaims and the behaviour is pinned.
Resolving.

Your questions

  • "If you would rather strip literal a/b roots, it is one flag." → Keep what you have; no flag. You are right and my round-4 reading was wrong. The deciding evidence is that the Only in arm has one side and can never strip, so any strip policy mixes rooted and unrooted names inside a single stream — which is exactly what the old output did:

    9634a9a8 :  [file] f1.txt        [file] img.bin        [file] b/newfile.txt
    27abf4ad :  [file] b/f1.txt      [file] b/img.bin      [file] b/newfile.txt
    

    b/f1.txt, b/img.bin and b/newfile.txt all resolve from the cwd diff ran in; f1.txt and img.bin do not. Consistency has to win here, and it agrees with what git diff --no-index a b names.

Next

Nothing — approving, no follow-ups. Seven rounds is well past what this should have cost, and that is on the review side, not yours: the producer-by-producer enumeration you did in this push is the work that should have framed round 1.

Rounds: landing (7 posted; budget is 3). Threads: 0 open, 7 resolved this round.

@KuSh
KuSh merged commit ab0cf40 into rtk-ai:develop Sep 7, 2026
12 checks passed
@rtk-release-bot rtk-release-bot Bot mentioned this pull request Sep 7, 2026
KuSh added a commit to iliaal/rtk that referenced this pull request Sep 8, 2026
rtk-ai#3788 rewrote the piped-stream half of diff_cmd.rs (condense_stdin,
condense_unified_diff_strict) while this branch rewrote the file-versus-file
half (Hunk, compute_diff, the renderers). The two conflicts are positional:
both sides add code at the same offset. Resolved as a union -- develop's
imports plus this branch's, and both blocks kept whole.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

condense_unified_diff misclassifies ---/+++ content — silent drops, renamed file labels, wrong counters (rtk diff -)

3 participants