Repository navigation
fix: render a real diff instead of dumping both files for modified lines - #3048
Conversation
KuSh
left a comment
There was a problem hiding this comment.
Requesting changes — one of them blocks CI — but I want to lead with this: the core idea here is the most valuable one in the whole rtk diff PR pool, and it is under-sold by its own title.
Replacing content1 + "\n---\n" + content2 with a real classic diff doesn't just improve the fallback; it fixes the reason several other diff fixes are invisible to users. Because the old baseline is a dump of both files, never_worse prefers it constantly, so on small inputs rtk diff prints two blobs and the carefully-rendered output is thrown away. #3519's new whitespace message hits exactly this. Your PR is the structural fix underneath it.
Verification I did: built the head, fuzzed format_classic_diff over 900 random file pairs, and checked every emitted fallback with patch — all applied cleanly and reproduced file2 exactly, including tail-add, tail-delete, dissimilar-replacement, and the overlapping 1c1/1a2 anchor cases. No panics, no underflows, no non-advancing loops. That formatter is solid.
Four things to fix, three inline. The fourth: scripts/test-all.sh:532 asserts assert_contains ... "added", which never matches the new output (grep -c added = 0). It was already red on base, but this PR is the fix for that path and should update the assertion.
Also worth a look while you're here: docs/usage/FEATURES.md:249 still claims ~60% savings for rtk diff. On a 200-line file with 3 modified lines your output is 315 bytes against GNU diff's 315 — 0% reduction. The condensed render only wins past roughly 15 modified lines, because of its 2-line header. That number was wrong before this PR too, but the PR makes it easy to state the real one.
|
All four are in as of bed658d, and thanks for the fuzzing writeup on the formatter, that was more verification than the PR had.
On FEATURES.md, I took your measurement rather than picking a new round number: it now says the saving is variable, 0% on small diffs, with the condensed render winning past roughly 15 modified lines. Local run: clippy clean, 2438 tests passing. |
- Drop the needless borrow at the format_diff_changes call site; with warnings = "deny" this was failing cargo clippy --all-targets and blocking the clippy job that build, test and release depend on. - Keep the original both-files dump as the timer.track baseline. The synthesized classic diff is empty for identical files, which recorded input_tokens = 0 and reported ~0% instead of ~46% in rtk gain --history. - Un-gate render_file_diff: fold it into render_diff rather than compiling a production helper only under cfg(test). - Update the scripts/test-all.sh assertion to match the real output.
bed658d to
d1a0d9c
Compare
KuSh
left a comment
There was a problem hiding this comment.
All four points from the previous review are fixed — threads resolved.
I rebased the branch onto develop and pushed it, since it had gone conflicting in the meantime. The only conflict was docs/usage/FEATURES.md: develop removed every **Economies :** line from that file wholesale in eb3f814, so your rewrite of the rtk diff one collided with a deletion. I dropped that hunk and trimmed the matching bullet from the commit message. No code was changed — scripts/test-all.sh and src/cmds/git/diff_cmd.rs are exactly as you pushed them.
Verified locally
-
cargo fmt --all --check,cargo clippy --all-targets(withwarnings = "deny"),cargo test --all— all clean, 2858 tests passing. -
The new
test-all.shassertion, run verbatim: passes. -
Recorded metrics compared against a
developbuild on identical fixtures and the sameRTK_DB_PATH:case develop this branch identical files [ok] Files are identical, 46.2%unchanged one line changed dumps both files, 0.0% 2c2/< beta/---/> betaX, 53.8% -
I fuzzed
format_classic_diffover 900 generated file pairs (lengths 0–12 on both sides, so tail-append, tail-delete, wholesale-replacement and empty-file shapes all occur). 885 produced a non-empty patch; all 885 apply cleanly withpatchand reproduce the second file byte-for-byte, and the 15 identical pairs correctly produced an empty patch. No panics, no underflows, no non-advancing loops.
Nice work — this is the change that makes a small edit readable instead of a dump of both files.
Two notes for the record, neither blocking
-
never_worsenow guards the printed output against the classic diff, whiletimer.trackrecords against the both-files dump — two different strings, where before they were one. Sortk gaincan show a small negative forrtk diffon wholesale replacements (I measured −5% onrtk diff Cargo.toml LICENSE, where the old code clamped to 0%). It is arguably the honest number, since a faithful diff of two unrelated files genuinely isn't compressible. Flagging it so it isn't a surprise later. -
The fuzzing also showed that only 289 of the 885 patches match what GNU
diffemits. The rest are valid and exact but not minimal:compute_diffcompares positionally, so a single inserted line shifts everything after it and cascades into a run of bogusNcNpairs. Across the corpus that makes the output about 40% larger than GNU's. That is pre-existing and well outside this PR's scope — your change is strictly better than what it replaces in exactly those cases — but it does bound how good the fallback can look until the underlying comparison is smarter.
d1a0d9c to
1936103
Compare
…th files The guard picks between the classic fallback and the condensed render, so the fallback is what rtk diff has to beat -- but savings were still booked against a dump of both files, a string no command ever emits. The two had drifted apart, so rtk gain could record a loss: -5% on rtk diff Cargo.toml LICENSE, and -250% on two empty files. Measuring against the fallback makes a negative impossible rather than merely unobserved, since the guard already caps the shown output at it. Identical files, where diff prints nothing, keep the both-files baseline so their recorded ratio stays comparable; two near-empty files can make that dump cheaper than the verdict line, which would book a loss against the cheapest possible answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1936103 to
8495176
Compare
`rtk diff <f1> <f2>` decided identity from `str::lines()`, which strips a trailing `\r` and treats the final newline as optional. A CRLF-vs-LF pair printed "[ok] Files are identical" and exited 0, so a verification gate built on `diff` silently passed. Identity is now decided on the bytes, and when the bytes differ while the text matches, the render names the cause instead of listing visually identical strings. `compute_diff` compared line N to line N, so one inserted line desynchronized the whole tail: 200 lines with one insertion rendered 403 lines of output, and the `never_worse` guard threw that away and dumped both files. It now trims the common prefix and suffix and aligns the middle with Myers' greedy algorithm, bounded by a banded trace of 1,000,000 cells rather than by the edit distance. A lopsided pair keeps a window a few diagonals wide however large `d` grows, so one line against five thousand aligns instead of being refused. Every reported change carries a line number in each file. The classic renderer needs both: `NaM` anchors an append in file1 and ranges it in file2, `NdM` does the reverse, and after one insertion neither frame can be derived from the other. A dissimilar in-place rewrite stays one `Replaced` rather than becoming a `Removed` plus an `Added`, so the classic path groups it as a single change hunk without inferring the pairing from equal line numbers. All three listing paths are bounded by count and by bytes, and the aligned path spends its count budget before the script exists: `myers_ops` knows the edit distance when it reaches the end, so one line against 40,000 refuses without cloning a script it would throw away. Past the budget, equal lengths pair by position and say so, and unequal ones report a measured floor plus line bounds in each file. Rebased onto develop, which merged rtk-ai#3048 in the meantime. An empty change list is no longer the "identical" signal: `FileComparison` distinguishes byte identity from an invisible difference and from a refused listing, so an over-cap comparison cannot route through the identical branch and exit 0.
`rtk diff <f1> <f2>` decided identity from `str::lines()`, which strips a trailing `\r` and treats the final newline as optional. A CRLF-vs-LF pair printed "[ok] Files are identical" and exited 0, so a verification gate built on `diff` silently passed. Identity is now decided on the bytes, and when the bytes differ while the text matches, the render names the cause instead of listing visually identical strings. `compute_diff` compared line N to line N, so one inserted line desynchronized the whole tail: 200 lines with one insertion rendered 403 lines of output, and the `never_worse` guard threw that away and dumped both files. It now trims the common prefix and suffix and aligns the middle with Myers' greedy algorithm, bounded by a banded trace of 1,000,000 cells rather than by the edit distance. A lopsided pair keeps a window a few diagonals wide however large `d` grows, so one line against five thousand aligns instead of being refused. Every reported change carries a line number in each file. The classic renderer needs both: `NaM` anchors an append in file1 and ranges it in file2, `NdM` does the reverse, and after one insertion neither frame can be derived from the other. A dissimilar in-place rewrite stays one `Replaced` rather than becoming a `Removed` plus an `Added`, so the classic path groups it as a single change hunk without inferring the pairing from equal line numbers. All three listing paths are bounded by count and by bytes, and the aligned path spends its count budget before the script exists: `myers_ops` knows the edit distance when it reaches the end, so one line against 40,000 refuses without cloning a script it would throw away. Past the budget, equal lengths pair by position and say so, and unequal ones report a measured floor plus line bounds in each file. Rebased onto develop, which merged rtk-ai#3048 in the meantime. An empty change list is no longer the "identical" signal: `FileComparison` distinguishes byte identity from an invisible difference and from a refused listing, so an over-cap comparison cannot route through the identical branch and exit 0.
`rtk diff <f1> <f2>` decided identity from `str::lines()`, which strips a trailing `\r` and treats the final newline as optional. A CRLF-vs-LF pair printed "[ok] Files are identical" and exited 0, so a verification gate built on `diff` silently passed. Identity is now decided on the bytes, and when the bytes differ while the text matches, the render names the cause instead of listing visually identical strings. `compute_diff` compared line N to line N, so one inserted line desynchronized the whole tail: 200 lines with one insertion rendered 403 lines of output, and the `never_worse` guard threw that away and dumped both files. It now trims the common prefix and suffix and aligns the middle with Myers' greedy algorithm, bounded by a banded trace of 1,000,000 cells rather than by the edit distance. A lopsided pair keeps a window a few diagonals wide however large `d` grows, so one line against five thousand aligns instead of being refused. Every reported change carries a line number in each file. The classic renderer needs both: `NaM` anchors an append in file1 and ranges it in file2, `NdM` does the reverse, and after one insertion neither frame can be derived from the other. A dissimilar in-place rewrite stays one `Replaced` rather than becoming a `Removed` plus an `Added`, so the classic path groups it as a single change hunk without inferring the pairing from equal line numbers. All three listing paths are bounded by count and by bytes, and the aligned path spends its count budget before the script exists: `myers_ops` knows the edit distance when it reaches the end, so one line against 40,000 refuses without cloning a script it would throw away. Past the budget, equal lengths pair by position and say so, and unequal ones report a measured floor plus line bounds in each file. Rebased onto develop, which merged rtk-ai#3048 in the meantime. An empty change list is no longer the "identical" signal: `FileComparison` distinguishes byte identity from an invisible difference and from a refused listing, so an over-cap comparison cannot route through the identical branch and exit 0.
Summary
rtk diff FILE1 FILE2was printing both files in full, joined by a lone---with no markers or hunk headers, for small changes instead of a real diff. The cause is thenever_worseguard: it was handed a whole-file concatenation as its baseline, so for small files the correctly-condensed render had more tokens than that raw concatenation, lost the comparison, and the whole-file dump was shown. Because agents lean onrtk diffas an equality check, output that looks nothing like a diff is actively misleading.This derives the
never_worsebaseline from a genuine diff instead, so the result is always a valid diff, either the condensed render or a classic normal-diff fallback with realNcNhunk headers, and never a whole-file dump. The identical-files short-circuit (exit 0) and the exit-1-on-difference contract are unchanged.Test plan
cargo fmt --all && cargo clippy --all-targets && cargo testrtk diffon a one-line change now prints a1c1hunk header with the changed lines instead of both filesCloses #2811
AI was used for assistance.