diff --git a/sample_files/compare.expected b/sample_files/compare.expected index a5e634c12b..fbf9541077 100644 --- a/sample_files/compare.expected +++ b/sample_files/compare.expected @@ -58,6 +58,12 @@ f44f0dffe3617ddafca3d65e94a21613 - sample_files/devicetree_1.dts sample_files/devicetree_2.dts 64b33c6d65dbc85777ee30cf3893fbdb - +sample_files/displaypanic_a_1.txt sample_files/displaypanic_a_2.txt +34442b044ab320973fc989efda46e9d6 - + +sample_files/displaypanic_b_1.nix sample_files/displaypanic_b_2.nix +474dcbe72b3c62d6acc5eb8ed80da52a - + sample_files/elisp_1.el sample_files/elisp_2.el 98571b62f7407e90e9441e4f47ea85ae - diff --git a/sample_files/displaypanic_a_1.txt b/sample_files/displaypanic_a_1.txt new file mode 100644 index 0000000000..0db4625e39 --- /dev/null +++ b/sample_files/displaypanic_a_1.txt @@ -0,0 +1,14 @@ + + +exit: + mov x0, #0 + mov x8, #93 + svc #0 + + + + + + + + diff --git a/sample_files/displaypanic_a_2.txt b/sample_files/displaypanic_a_2.txt new file mode 100644 index 0000000000..6c93583df1 --- /dev/null +++ b/sample_files/displaypanic_a_2.txt @@ -0,0 +1,13 @@ + + + + +exit_success: + mov x0, #0 + mov x8, #93 + svc #0 + +exit_error: + mov x0, #1 + mov x8, #93 + svc #0 diff --git a/sample_files/displaypanic_b_1.nix b/sample_files/displaypanic_b_1.nix new file mode 100644 index 0000000000..f77250eeb2 --- /dev/null +++ b/sample_files/displaypanic_b_1.nix @@ -0,0 +1,40 @@ +{ + flake.darwinModules.communication = { + homebrew.casks = [ + "signal" + "vesktop" + "whatsapp" + "zulip" + ]; + }; + + flake.homeModules.communication = + { + osConfig, + lib, + pkgs, + ... + }: + let + inherit (lib.lists) optionals; + isLinux = osConfig.nixpkgs.hostPlatform.isLinux; + in + { + packages = [ + pkgs.cinny-desktop + pkgs.thunderbird + ] + ++ optionals isLinux [ + pkgs.signal-desktop + ( + pkgs.discord.overrideAttrs + (old: { + postFixup = '' + wrapProgram $out/opt/Discord/Discord \ + --add-flags "x" + ''; + }) + ) + ]; + }; +} diff --git a/sample_files/displaypanic_b_2.nix b/sample_files/displaypanic_b_2.nix new file mode 100644 index 0000000000..c7285b117f --- /dev/null +++ b/sample_files/displaypanic_b_2.nix @@ -0,0 +1,39 @@ +{ + flake.darwinModules.discord = { + homebrew.casks = [ "vesktop" ]; + }; + + flake.darwinModules.signal-desktop = { + homebrew.casks = [ "signal" ]; + }; + + flake.darwinModules.whatsapp = { + homebrew.casks = [ "whatsapp" ]; + }; + + flake.darwinModules.zulip = { + homebrew.casks = [ "zulip" ]; + }; + + flake.homeModules.discord = + { + osConfig, + lib, + pkgs, + ... + }: + let + inherit (lib.lists) optional; + in + { + packages = optional osConfig.nixpkgs.hostPlatform.isLinux ( + pkgs.discord.overrideAttrs + (old: { + postFixup = '' + wrapProgram $out/opt/Discord/Discord \ + --add-flags "x" + ''; + }) + ); + }; +} diff --git a/src/display/hunks.rs b/src/display/hunks.rs index 6e8f3688c1..d14d83e769 100644 --- a/src/display/hunks.rs +++ b/src/display/hunks.rs @@ -577,11 +577,16 @@ fn matched_novel_lines( true }; if should_append { - lines.push(( - Some(self_line), - next_opposite(self_line, &opposite_to_lhs, highest_rhs), - )); + let opposite = next_opposite(self_line, &opposite_to_lhs, highest_rhs); + lines.push((Some(self_line), opposite)); highest_lhs = Some(self_line); + // Also update highest_rhs if we used an RHS line as opposite, + // to prevent that RHS line from being added again when processed. + if let Some(opp) = opposite { + if highest_rhs.map_or(true, |h| opp > h) { + highest_rhs = Some(opp); + } + } } } Side::Right => { @@ -591,11 +596,16 @@ fn matched_novel_lines( true }; if should_append { - lines.push(( - next_opposite(self_line, &opposite_to_rhs, highest_rhs), - Some(self_line), - )); + let opposite = next_opposite(self_line, &opposite_to_rhs, highest_lhs); + lines.push((opposite, Some(self_line))); highest_rhs = Some(self_line); + // Also update highest_lhs if we used an LHS line as opposite, + // to prevent that LHS line from being added again when processed. + if let Some(opp) = opposite { + if highest_lhs.map_or(true, |h| opp > h) { + highest_lhs = Some(opp); + } + } } } } @@ -694,6 +704,7 @@ mod tests { use super::*; use crate::{ + display::test_util::{novel, unchanged}, hash::DftHashMap, syntax::{MatchKind, TokenKind}, }; @@ -868,4 +879,31 @@ mod tests { ] ); } + + #[test] + fn test_matched_novel_lines_opposite_threshold() { + // LHS novels at lines 3, 6; RHS novel at line 2, then at line 10 + // RHS line 10 has LHS opposites {4, 8} + // Should pick LHS 8 (first > highest_lhs=6), not LHS 4 + let lhs_mps = [novel(3), novel(6), unchanged(4, 10), unchanged(8, 10)]; + let rhs_mps = [novel(2), unchanged(10, 4), unchanged(10, 8), novel(10)]; + + let result = matched_novel_lines(&lhs_mps, &rhs_mps); + let (lhs, _) = result.iter().find(|(_, r)| *r == Some(10.into())).unwrap(); + assert_eq!(*lhs, Some(8.into())); + } + + #[test] + fn test_matched_novel_lines_no_duplicate_lines() { + // LHS novel at line 1 pairs with RHS line 2 as opposite. + // RHS novel at line 2 should not create another entry. + let lhs_mps = [novel(1), unchanged(1, 2)]; + let rhs_mps = [novel(2), unchanged(2, 1)]; + + let result = matched_novel_lines(&lhs_mps, &rhs_mps); + + // RHS line 2 should appear exactly once + let rhs_2_count = result.iter().filter(|(_, r)| *r == Some(2.into())).count(); + assert_eq!(rhs_2_count, 1); + } } diff --git a/src/display/mod.rs b/src/display/mod.rs index 8604d52d9b..dfbff355ae 100644 --- a/src/display/mod.rs +++ b/src/display/mod.rs @@ -4,3 +4,6 @@ pub(crate) mod inline; pub(crate) mod json; pub(crate) mod side_by_side; pub(crate) mod style; + +#[cfg(test)] +pub(crate) mod test_util; diff --git a/src/display/side_by_side.rs b/src/display/side_by_side.rs index 362db43bfd..c3a07a4460 100644 --- a/src/display/side_by_side.rs +++ b/src/display/side_by_side.rs @@ -545,7 +545,7 @@ pub(crate) fn print( } let matched_lines = all_matched_lines_filled(lhs_mps, rhs_mps, &lhs_lines, &rhs_lines); - let mut matched_lines_to_print = &matched_lines[..]; + let matched_lines_to_print = &matched_lines[..]; let mut lhs_max_visible_line = 1.into(); let mut rhs_max_visible_line = 1.into(); @@ -612,12 +612,11 @@ pub(crate) fn print( display_options.num_context_lines as usize, ); let aligned_lines = &matched_lines_to_print[start_i..end_i]; - // We iterate through hunks in order, so we know the next hunk - // must appear after start_i. This makes - // `matched_lines_indexes_for_hunk` faster on later - // iterations, and this function is hot on large textual - // diffs. - matched_lines_to_print = &matched_lines_to_print[start_i..]; + // Note: We previously sliced matched_lines_to_print here as an + // optimization, assuming hunks appear in order. However, hunks + // from matched_novel_lines may not be strictly ordered by their + // position in matched_lines when there are interleaved LHS-only + // and RHS-only novel sections. See issue #770. let no_lhs_changes = hunk.novel_lhs.is_empty(); let no_rhs_changes = hunk.novel_rhs.is_empty(); @@ -782,6 +781,10 @@ mod tests { use super::*; use crate::{ + display::{ + hunks::matched_pos_to_hunks, + test_util::{novel, unchanged}, + }, options::DEFAULT_TERMINAL_WIDTH, parse::guess_language::Language, syntax::{AtomKind, MatchKind, TokenKind}, @@ -907,4 +910,39 @@ mod tests { &rhs_mps, ); } + + /// Regression test for issue #770: slicing optimization assumed hunks appear + /// in matched_lines order, but LHS-only hunks may appear before RHS-only hunks + /// even when RHS lines come first. Would panic before fix. + #[test] + fn test_display_hunks_out_of_order_issue_770() { + // LHS novels at 10-12, RHS novels at 3-5. Hunks: [LHS, RHS] but + // matched_lines has RHS first. Slicing after LHS hunk would remove RHS lines. + let lhs_src = (0..15) + .map(|i| format!("L{i}")) + .collect::>() + .join("\n"); + let rhs_src = (0..10) + .map(|i| format!("R{i}")) + .collect::>() + .join("\n"); + + let lhs_mps = vec![unchanged(0, 0), novel(10), novel(11), novel(12)]; + let rhs_mps = vec![unchanged(0, 0), novel(3), novel(4), novel(5)]; + + let hunks = matched_pos_to_hunks(&lhs_mps, &rhs_mps); + + // Would panic before fix with "Hunk lines should be present in matched lines" + print( + &hunks, + &DisplayOptions::default(), + "t.txt", + None, + &FileFormat::PlainText, + &lhs_src, + &rhs_src, + &lhs_mps, + &rhs_mps, + ); + } } diff --git a/src/display/test_util.rs b/src/display/test_util.rs new file mode 100644 index 0000000000..1f182f0b8f --- /dev/null +++ b/src/display/test_util.rs @@ -0,0 +1,36 @@ +//! Test helpers for creating [`MatchedPos`] values with minimal boilerplate. + +use line_numbers::SingleLineSpan; + +use crate::parse::syntax::{MatchKind, MatchedPos, TokenKind}; + +/// Creates a [`SingleLineSpan`] at the given line with column range 0..1. +pub(crate) fn span(line: u32) -> SingleLineSpan { + SingleLineSpan { + line: line.into(), + start_col: 0, + end_col: 1, + } +} + +/// Creates a novel [`MatchedPos`] at the given line. +pub(crate) fn novel(line: u32) -> MatchedPos { + MatchedPos { + kind: MatchKind::Novel { + highlight: TokenKind::Delimiter, + }, + pos: span(line), + } +} + +/// Creates an unchanged [`MatchedPos`] linking `self_line` to `opposite_line`. +pub(crate) fn unchanged(self_line: u32, opposite_line: u32) -> MatchedPos { + MatchedPos { + kind: MatchKind::UnchangedToken { + highlight: TokenKind::Delimiter, + self_pos: vec![span(self_line)], + opposite_pos: vec![span(opposite_line)], + }, + pos: span(self_line), + } +}