From 1afc62c28fe9cd046d85365afcb14702ff9326f0 Mon Sep 17 00:00:00 2001 From: Matthew Suozzo Date: Tue, 4 Aug 2026 13:27:55 -0400 Subject: [PATCH] diff: match a side that appears verbatim inside the other collect_unchanged_words() gives up on a region when no word occurs the same number of times on both sides and the ends of the region both differ. The whole region is then reported as changed, even when one side appears verbatim within the other and the region is really an insertion (or a deletion) around that run. Materialized conflicts hit this shape. Under the "diff" conflict marker style, one side of the conflict is repeated with a "+" prefix on every line, so every word count in the region is unbalanced. A line with no word bytes, like "\t\t},", then has no anchor at all. In a color-words diff of a conflicted file this breaks the "+" column on the last repeated line: 48 51: + { 49 52: + "github.com/org/project", 50 53: },+ }, 54: +++++++ ... Rows "48 51" and "49 52" show the correct output, a lone inserted "+". Row "50 53" instead shows the whole line as removed and then re-added with the "+" attached. This fix searches for the shorter side of the region as a contiguous run of tokens within the longer side, after the leading and trailing trims have matched what they can. Knuth-Morris-Pratt keeps the cost linear in the region size. The run must be at least two tokens long as a single shared token is no evidence of common structure, and matching it would arbitrarily pick one of its occurrences on the other side (notably, a fefe07b3c34d "diff: consider uncommon words to match only if they have the same count" removed a similar heuristic). Requiring one whole side to match contiguously and taking the leftmost occurrence on ties keeps the choice deterministic. Fixes #9914 --- CHANGELOG.md | 5 + cli/tests/test_diff_command.rs | 4 +- lib/src/diff.rs | 203 +++++++++++++++++++++++++++++++-- 3 files changed, 200 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a3fe5b07f3b..d9ae1233e28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Fixed bugs +* Diffs no longer report a region as entirely replaced when one side of it + appears verbatim within the other side. This fixes an issue in color-words + diffs of conflicted files, where the last line was previously shown as a + removal followed by a `+`-prefixed addition, corrupting the `+` column. + * Recursive alias definitions are detected more precisely. jj can now expand aliases that are simply repeated. For example, with the alias `jj = []`, the command `jj jj jj` will resolve to `jj`. Aliases can also fall back to the diff --git a/cli/tests/test_diff_command.rs b/cli/tests/test_diff_command.rs index 95e55cb732b..3deef4fecb2 100644 --- a/cli/tests/test_diff_command.rs +++ b/cli/tests/test_diff_command.rs @@ -1624,8 +1624,8 @@ fn test_diff_color_words_omit_blank_right_line() { .normalize_stdout_with(strip_ansi_escape_codes); insta::assert_snapshot!(output, @" Modified regular file file1: - 1 1: a x - 2: y + 1 1: a + 1 2: xy 2 3: z b Modified regular file file2: 1 1: a x diff --git a/lib/src/diff.rs b/lib/src/diff.rs index dc72fbd8b6d..d1994bf5b96 100644 --- a/lib/src/diff.rs +++ b/lib/src/diff.rs @@ -447,20 +447,119 @@ fn collect_unchanged_words( .take_while(|&(l, r)| comp.eq_hashed(l, r)) .count(); - found_positions.extend(itertools::chain( - (0..common_leading_len).map(|i| { + found_positions.extend((0..common_leading_len).map(|i| { + ( + left.map_to_global(LocalWordPosition(i)), + right.map_to_global(LocalWordPosition(i)), + ) + })); + let left_middle_len = left.ranges.len() - common_leading_len - common_trailing_len; + let right_middle_len = right.ranges.len() - common_leading_len - common_trailing_len; + // Match one middle occurring verbatim inside the other. Needs a strictly + // shorter side of at least two tokens. + if left_middle_len != right_middle_len && left_middle_len.min(right_middle_len) >= 2 { + collect_unchanged_embedded_words( + found_positions, + &left.narrowed( + LocalWordPosition(common_leading_len) + ..LocalWordPosition(left.ranges.len() - common_trailing_len), + ), + &right.narrowed( + LocalWordPosition(common_leading_len) + ..LocalWordPosition(right.ranges.len() - common_trailing_len), + ), + comp, + ); + } + found_positions.extend((1..=common_trailing_len).rev().map(|i| { + ( + left.map_to_global(LocalWordPosition(left.ranges.len() - i)), + right.map_to_global(LocalWordPosition(right.ranges.len() - i)), + ) + })); +} + +/// Matches the shorter side against the run where it appears verbatim inside +/// the longer side, if one exists. The whole region then diffs as an insertion +/// (or deletion) around that run instead of as a complete replacement. +/// +/// This only handles regions where no other same-word-occurrence anchor exists +/// on both sides (so [`collect_unchanged_words_lcs()`] found nothing). +/// +/// Runs of fewer than two tokens are not matched (caller-enforced) since a lone +/// shared token is weak evidence, and picking one of its occurrences would be +/// arbitrary (see `test_unchanged_ranges_non_unique_removed`). +fn collect_unchanged_embedded_words( + found_positions: &mut Vec<(WordPosition, WordPosition)>, + left: &LocalDiffSource, + right: &LocalDiffSource, + comp: &WordComparator, +) { + let swapped = left.ranges.len() > right.ranges.len(); + let (pattern, text) = if swapped { + (right, left) + } else { + (left, right) + }; + let Some(offset) = find_embedded_run(pattern, text, comp) else { + return; + }; + found_positions.extend((0..pattern.ranges.len()).map(|i| { + let pattern_pos = LocalWordPosition(i); + let text_pos = LocalWordPosition(offset + i); + if swapped { ( - left.map_to_global(LocalWordPosition(i)), - right.map_to_global(LocalWordPosition(i)), + left.map_to_global(text_pos), + right.map_to_global(pattern_pos), ) - }), - (1..=common_trailing_len).rev().map(|i| { + } else { ( - left.map_to_global(LocalWordPosition(left.ranges.len() - i)), - right.map_to_global(LocalWordPosition(right.ranges.len() - i)), + left.map_to_global(pattern_pos), + right.map_to_global(text_pos), ) - }), - )); + } + })); +} + +/// Finds the leftmost occurrence of `pattern`'s tokens as a contiguous run +/// within `text`, in O(pattern + text) time (Knuth-Morris-Pratt). +fn find_embedded_run( + pattern: &LocalDiffSource, + text: &LocalDiffSource, + comp: &WordComparator, +) -> Option { + let pattern_len = pattern.ranges.len(); + let pattern_word = |i: usize| HashedWord { + hash: pattern.hashes[i], + text: &pattern.text[pattern.ranges[i].clone()], + }; + // suffix_prefix_len[i]: length of the longest proper prefix of + // pattern[..=i] that is also a suffix of it + let mut suffix_prefix_len: SmallVec<[usize; 16]> = smallvec![0; pattern_len]; + let mut len = 0; + for i in 1..pattern_len { + let word = pattern_word(i); + while len > 0 && !comp.eq_hashed(word, pattern_word(len)) { + len = suffix_prefix_len[len - 1]; + } + if comp.eq_hashed(word, pattern_word(len)) { + len += 1; + } + suffix_prefix_len[i] = len; + } + let mut matched = 0; + for (pos, word) in text.hashed_words().enumerate() { + while matched > 0 && !comp.eq_hashed(word, pattern_word(matched)) { + matched = suffix_prefix_len[matched - 1]; + } + if comp.eq_hashed(word, pattern_word(matched)) { + matched += 1; + } + if matched == pattern_len { + return Some(pos + 1 - matched); + } + } + None } fn collect_unchanged_words_lcs( @@ -1286,6 +1385,52 @@ mod tests { ); } + #[test] + fn test_unchanged_ranges_embedded_run() { + // No word count is balanced and neither end matches, but the left side + // occurs verbatim within the right side and is matched as a run. + assert_eq!( + unchanged_ranges( + (b"x y", &[0..1, 2..3]), + (b"y x y x y x", &[0..1, 2..3, 4..5, 6..7, 8..9, 10..11]), + ), + vec![(0..1, 2..3), (2..3, 4..5)] + ); + // The same in the other direction. + assert_eq!( + unchanged_ranges( + (b"y x y x y x", &[0..1, 2..3, 4..5, 6..7, 8..9, 10..11]), + (b"x y", &[0..1, 2..3]), + ), + vec![(2..3, 0..1), (4..5, 2..3)] + ); + // Of several occurrences, the leftmost is matched. + assert_eq!( + unchanged_ranges( + (b"x x y", &[0..1, 2..3, 4..5]), + (b"y x x x y x", &[0..1, 2..3, 4..5, 6..7, 8..9, 10..11]), + ), + vec![(0..1, 4..5), (2..3, 6..7), (4..5, 8..9)] + ); + // A single shared token is not a run. Either occurrence would be an + // arbitrary guess. + assert_eq!( + unchanged_ranges( + (b"x", std::slice::from_ref(&(0..1))), + (b"y x x y", &[0..1, 2..3, 4..5, 6..7]), + ), + vec![] + ); + // A non-contiguous occurrence is not a run. + assert_eq!( + unchanged_ranges( + (b"x y", &[0..1, 2..3]), + (b"y x q y x", &[0..1, 2..3, 4..5, 6..7, 8..9]), + ), + vec![] + ); + } + #[test] fn test_unchanged_ranges_recursion_needed() { // "|" matches first, then "b" matches within the left/right range. @@ -1335,6 +1480,44 @@ mod tests { ); } + #[test] + fn test_diff_materialized_conflict_line_prefix() { + // The "diff" conflict marker style repeats one side of the conflict with a + // "+" prefix on every line. Neither the repeated block nor the rest of the + // conflict lines up line by line, so all of it reaches the word and then + // the non-word pass. The last repeated line used to come out as a + // whole-line replacement, because " }," has no word bytes and each of + // its non-word bytes occurs a different number of times on each side. + // rustfmt would wrap the long literals with backslash continuations, + // which swallow the significant leading spaces, so they stay on one line. + #[rustfmt::skip] + assert_eq!( + diff([ + " {\n one,\n },\n {\n two,\n },\n", + "+ {\n+ one,\n+ },\n+ {\n+ two,\n+ },\n+++++++ side #2\n {\n three,\n },\n {\n four,\n },\n {\n five,\n },\n>>>>>>> conflict 1 of 1 ends\n", + ]), + vec![ + DiffHunk::different(["", "+"]), + DiffHunk::matching([" {\n"].repeat(2)), + DiffHunk::different(["", "+"]), + DiffHunk::matching([" one,\n"].repeat(2)), + DiffHunk::different(["", "+"]), + DiffHunk::matching([" },\n"].repeat(2)), + DiffHunk::different(["", "+"]), + DiffHunk::matching([" {\n"].repeat(2)), + DiffHunk::different(["", "+"]), + DiffHunk::matching([" two,\n"].repeat(2)), + DiffHunk::different(["", "+"]), + DiffHunk::matching([" },"].repeat(2)), + DiffHunk::different([ + "", + "\n+++++++ side #2\n {\n three,\n },\n {\n four,\n },\n {\n five,\n },\n>>>>>>> conflict 1 of 1 ends", + ]), + DiffHunk::matching(["\n"].repeat(2)), + ] + ); + } + #[test] fn test_diff_single_input() { assert_eq!(diff(["abc"]), vec![DiffHunk::matching(["abc"])]);