Repository navigation
fix(diff): byte-accurate identity and LCS alignment (adopt upstream #3268) - #115
Merged
Merged
Conversation
render_file_diff decided identity from `content.lines()`, which strips a trailing \r and treats the final newline as optional. So a CRLF-vs-LF or missing-trailing-newline pair collapsed to identical line vectors and rtk printed "[ok] Files are identical" and exited 0 for files that differ. Reproduced with 200 lines where 24 carry CRLF: cmp reports a difference at byte 1061 and exits 1; rtk called them identical. That is the worst shape for a filter, because `diff` is used as a gate: a false identity silently passes a broken check. rtk also never runs diff here (it reimplements the comparison and synthesizes the exit code), so there was no child status to fall back on. Gate identity on byte equality, before lines() can normalize anything. When the bytes differ but the text matches, name the cause (line endings, trailing newline) rather than rendering a change list of visually identical strings. That message deliberately avoids the word "identical": it is the signal for true identity and must not appear for files that differ.
compute_diff walked `for i in 0..max_len` comparing lines1[i] to lines2[i], so
one inserted line desynchronized everything after it: every later pair compared
unrelated lines, the whole tail rendered as changed, and the output grew large
enough that the never_worse guard threw it away and dumped both files
concatenated instead.
Measured on a 200-line file with one line inserted at the top: 403 lines of
output before (both files, concatenated), 4 after ("+1 added, -0 removed" and
the inserted line).
Trims the common prefix and suffix first, which is what keeps the LCS table
affordable on real diffs, then runs an O(n*m) table over the middle. Past
LCS_CELL_CAP cells the middles share too little for an alignment to mean
anything, so it emits a wholesale replacement rather than allocate gigabytes.
Runs of deletions pair with the insertions that follow them, so a rewritten
line still reports as one Modified rather than a Removed plus an unrelated
Added. Line numbers now come from each line's own file instead of a shared
positional index.
Review round on fork PR #115 found the cap fallback untested: the existing wholesale-replacement test uses 2x2 = 4 cells, so it exercises lcs_ops, never the cap branch. Add a 2001x2001 fixture (> 4M cells) that forces the cap path and asserts every line reports as removed + added with line numbers from each line's own file. Fork-authored amendment on top of the upstream rtk-ai#3268 cherry-picks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
Review round (pre-merge, two-axis /code-review vs develop)Standards axis — no hard violations. Two judgement calls, both deliberately left as-is for upstream byte-parity with rtk-ai#3268:
Spec axis — algorithm verified end to end (LCS backtracking hand-traced, prefix/suffix trim guards checked for overlap, CRLF counting matches the spec message, exit-1 propagation traced to
Quality gate re-run after the amendment: fmt clean, clippy zero warnings, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adopts upstream rtk-ai/rtk#3268 by @iliaal. Closes #100.
What this fixes
Two defects in
src/cmds/git/diff_cmd.rs, both reachable from a plainrtk diff a b(and from barediff, which the hook rewrites into this path):[ok] Files are identicalwith exit 0 for files whose only difference is CRLF-vs-LF or a missing final newline — identity was decided fromstr::lines(), which normalizes both away. A false identity silently passes any verification gate built ondiff.never_worsethen dumped both files concatenated (403 lines for a 1-line insertion).Commits
ce07afffix(diff): compare bytes before claiming two files are identicalc1d47d654cb3a1fix(diff): align by LCS instead of comparing line N to line Nd4411ccc7d29b0test(diff): cover the LCS_CELL_CAP wholesale-replacement branchAt the cherry-pick tip (
54cb3a1),git diff pr-3268 HEAD -- src/cmds/git/diff_cmd.rswas empty — the adopted code is byte-identical to the upstream PR branch. The amendment adds one test on top (28 insertions, test module only) and does not touch production code.Repro (real binaries, Windows)
Fixtures:
base.txt= 200 lines;base_crlf.txt= same text, lines 50–74 CRLF;inserted.txt= base with one line inserted at the top.Note: for very small differing files the
never_worseguard prints the raw concatenated content instead of the "differs, text matches" message (condensed is longer than raw at that size). The exit code — the part scripts rely on — is correct in every case. Same caveat upstream rtk-ai#3471 documents.Quality gate
cargo fmt --all(no changes) &&cargo clippy --all-targets(zero warnings) &&cargo test --all(2691 passed in the main suite, 0 failed across all suites), via the x64 host toolchain (scripts/win-dev-env.ps1).No pre-existing fork tests were modified; the two cherry-picks add 11 new unit tests and the amendment adds 1 more (LCS_CELL_CAP branch).
Deliberately not adopted
--ignore-whitespaceflag) and #3519 (byte-guard only) patch the same site. fix(diff): stop reporting differing files as identical, and align by LCS rtk-ai/rtk#3268 is the most complete — it is the only one that also fixes the positional-alignment defect — so it was adopted and the others are referenced here per the ticket. The--ignore-whitespaceconvenience flag from fix(diff): compare files byte-accurately instead of by normalized lines rtk-ai/rtk#3471 is a feature, not a bug fix, and was not adopted.