From 782b3a11fbd03e1508cf7d0e6df5a561174aece4 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 27 Jun 2026 01:22:34 -0700 Subject: [PATCH 1/4] fix: use diff line index as 1-based line numbers After doing some experiments, I found that the `Diff` API (from `gix-imara-diff` crate) uses 0-based line indexes instead of 1-based line numbers. Furthermore the `UnifiedDiff` API (again from `gix-imara-diff` crate) uses 1-based line numbers when printing hunk headers (`@@ -3, 5 +3,6 @@`). Thus, printing the unified diff into a patch file remains the same as it was already accurate. So this patch gracefully increments the line indexes by 1 so they can be compared to ranges of 1-based line numbers. I left the `print_diff()` function in the test module as helper/proof when analyzing future regressions. --- cpp-linter/src/clang_tools/clang_format.rs | 90 +++++++++++++++------- cpp-linter/src/common_fs.rs | 10 +-- 2 files changed, 67 insertions(+), 33 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index 65716f78..c72bde66 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -119,10 +119,13 @@ pub fn run_clang_format( replacements: diff .hunks() .filter_map(|hunk| { + let altered_start = hunk.after.start.saturating_add(1); // convert to 1-based line numbers + let og_start = hunk.before.start.saturating_add(1); // convert to 1-based line numbers + let og_end = hunk.before.end; // exclusive end is inclusive for 1-based line numbers let replacement = if hunk.is_pure_insertion() { - RangeInclusive::new(hunk.after.start, hunk.after.start) + RangeInclusive::new(altered_start, altered_start) } else { - RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1)) + RangeInclusive::new(og_start, og_end) }; if ranges.is_empty() { Some(replacement) @@ -241,14 +244,14 @@ fn three_way_diff(ranges: &[RangeInclusive], tidy_diff: Diff) -> Vec= og_end { // Skip the og range and tidy hunk entirely. // The line shift must still be adjusted for the pure removal though line_shift += delta; @@ -272,7 +275,7 @@ fn three_way_diff(ranges: &[RangeInclusive], tidy_diff: Diff) -> Vec], tidy_diff: Diff) -> VecStringA (hunk before=2..3) and - // line8+line9+line10->StringB+StringC (hunk before=7..10), then appends StringE. - // The second hunk's before=7..10 contains og_end=9 but not og_start=6, - // which exercises the "tidy hunk suffixes og range" branch. + "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\nline11\nline12"; + // The first hunk line2-3->StringA-B (hunk 2..4 prefixes og range[3..=5]). + // The second hunk line8-11->StringC-D\nline11\n (hunk 8..12 suffixes og range[7..=9]). const TIDY_SRC: &str = - "line1\nline2\nStringA\nline4\nline5\nline6\nline7\nStringB\nStringC\nline11\nStringE"; - let input = InternedInput::new(OG_SRC, TIDY_SRC); - let mut tidy_diff = Diff::compute(gix_imara_diff::Algorithm::Histogram, &input); - tidy_diff.postprocess_lines(&input); - let ranges = vec![RangeInclusive::new(2, 4), RangeInclusive::new(6, 9)]; + "line1\nStringA\nStringB\nline4\nline5\nline6\nline7\nline8\nStringC\nStringD\nline11"; + let (tidy_diff, _input) = make_patch(TIDY_SRC, OG_SRC); + #[cfg(feature = "bin")] + print_diff(OG_SRC, TIDY_SRC, &tidy_diff, &_input); + let ranges = vec![RangeInclusive::new(3, 5), RangeInclusive::new(7, 9)]; println!("tidy diff: {tidy_diff:#?}\ncompared to og ranges: {ranges:?}"); let joint_ranges = three_way_diff(&ranges, tidy_diff); println!("joint ranges: {joint_ranges:#?}"); - assert_eq!(joint_ranges, vec![2..=4, 6..=10]); + assert_eq!(joint_ranges, vec![2..=5, 7..=11]); } #[test] fn three_way_diff_separated() { const OG_SRC: &str = "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\nline11"; - // TIDY_SRC removes "line3" (index 2) which decrements offsets in ranges[5,8] and removes ranges[2,2]. + // TIDY_SRC removes "line3" which decrements offsets in ranges[5,8] and removes ranges[3,3]. // TIDY_SRC appends StringE, which handles remaining tidy hunks after done iterating ranges const TIDY_SRC: &str = "line1\nline2\nline4\nline5\nline6\nline7\nline8\nline9\nline10\nline11\nStringE"; - let input = InternedInput::new(OG_SRC, TIDY_SRC); - let mut tidy_diff = Diff::compute(gix_imara_diff::Algorithm::Histogram, &input); - tidy_diff.postprocess_lines(&input); - let ranges = vec![2..=2, 5..=8]; + let (tidy_diff, _input) = make_patch(TIDY_SRC, OG_SRC); + #[cfg(feature = "bin")] + print_diff(OG_SRC, TIDY_SRC, &tidy_diff, &_input); + let ranges = vec![3..=3, 5..=8]; println!("tidy diff: {tidy_diff:#?}\ncompared to og ranges: {ranges:?}"); let joint_ranges = three_way_diff(&ranges, tidy_diff); println!("joint ranges: {joint_ranges:#?}"); assert_eq!(joint_ranges, vec![4..=7, 9..=10]); } + + #[cfg(feature = "bin")] + fn print_diff( + og: &str, + altered: &str, + diff: &gix_imara_diff::Diff, + input: &gix_imara_diff::InternedInput<&str>, + ) { + use clap::builder::styling::{AnsiColor, Color, Style}; + use gix_imara_diff::{BasicLineDiffPrinter, UnifiedDiffConfig}; + + println!("---\nOG SRC:"); + for (i, l) in og.lines().enumerate() { + println!("{i:>2}|{l}"); + } + println!("---\nALTERED SRC:"); + for (i, l) in altered.lines().enumerate() { + println!("{i:>2}|{l}"); + } + let printer = BasicLineDiffPrinter(&input.interner); + let mut config = UnifiedDiffConfig::default(); + config.context_len(0); + let unified = diff.unified_diff(&printer, config, input).to_string(); + for l in unified.lines() { + let style = if l.starts_with('+') { + Style::new().fg_color(Some(Color::Ansi(AnsiColor::Green))) + } else if l.starts_with('-') { + Style::new().fg_color(Some(Color::Ansi(AnsiColor::Red))) + } else { + Style::new() + }; + println!("{style}{l}{style:#}"); + } + } } diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index 01b01a9a..b5ed317d 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -133,11 +133,11 @@ impl FileObj { pub fn is_hunk_in_diff(&self, hunk: &Hunk) -> Option<(u32, u32)> { let (start_line, end_line) = if !hunk.before.is_empty() { // if old hunk's total lines is > 0 - let start = hunk.before.start; - (start, start + hunk.before.len() as u32 - 1) + let start = hunk.before.start.saturating_add(1); // convert to 1-based line numbers + (start, hunk.before.start + hunk.before.len() as u32) } else { // old hunk's total lines is 0, meaning changes were only added - let start = hunk.after.start; + let start = hunk.after.start.saturating_add(1); // convert to 1-based line numbers // make old hunk's range span 1 line (start, start) }; @@ -207,7 +207,7 @@ impl FileObj { .map_err(FileObjError::OpenPatchFileFailed)?; patch_file .write_all( - format!("--- a/{file_name}\n+++ b/{file_name}\n{unified_diff}",).as_bytes(), + format!("--- a/{file_name}\n+++ b/{file_name}\n{unified_diff}").as_bytes(), ) .map_err(FileObjError::WritePatchFailed)?; } @@ -332,7 +332,7 @@ impl FileObj { format!( "Please remove the line(s)\n- {}", hunk.before - .map(|l| l.to_string()) + .map(|l| l.saturating_add(1).to_string()) .collect::>() .join("\n- ") ) From fda3c086429bac38b5770e4ba42b1d25783970b1 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 27 Jun 2026 01:48:25 -0700 Subject: [PATCH 2/4] use pretty printed diff for integration test too. --- cpp-linter/tests/comments.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cpp-linter/tests/comments.rs b/cpp-linter/tests/comments.rs index 4ee3fbbb..71e62d5b 100644 --- a/cpp-linter/tests/comments.rs +++ b/cpp-linter/tests/comments.rs @@ -1,5 +1,6 @@ #![cfg(feature = "bin")] use chrono::Utc; +use clap::builder::styling::{AnsiColor, Color, Style}; use cpp_linter::rest_client::USER_OUTREACH; use cpp_linter::run::run_main; use cpp_linter::{cli::ThreadComments, rest_client::COMMENT_MARKER}; @@ -310,7 +311,17 @@ async fn setup(lib_root: &Path, tmp_dir: &TempDir, test_params: &TestParams) { ); let patch_content = std::fs::read_to_string(patch_path).expect("Failed to read generated patch file."); - println!("Generated patch content:\n{patch_content}"); + println!("Generated patch content:"); + for l in patch_content.lines() { + let style = if l.starts_with('+') { + Style::new().fg_color(Some(Color::Ansi(AnsiColor::Green))) // Green for additions + } else if l.starts_with('-') { + Style::new().fg_color(Some(Color::Ansi(AnsiColor::Red))) // Red for deletions + } else { + Style::new() // Default style for context lines + }; + println!("{style}{l}{style:#}"); + } } let summary_out_file_abs_path = if test_params.relative_summary_out_file { From 29f82f6e944dfde3fa62cd6b55c14b9ca43f79f4 Mon Sep 17 00:00:00 2001 From: Brendan <2bndy5@gmail.com> Date: Sat, 27 Jun 2026 02:01:47 -0700 Subject: [PATCH 3/4] add test to cover pure-removal PR review suggestion --- cpp-linter/src/common_fs.rs | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/cpp-linter/src/common_fs.rs b/cpp-linter/src/common_fs.rs index b5ed317d..37ded27f 100644 --- a/cpp-linter/src/common_fs.rs +++ b/cpp-linter/src/common_fs.rs @@ -401,10 +401,12 @@ pub(crate) fn mk_path_abs>(path: P) -> Result Date: Sat, 27 Jun 2026 02:45:40 -0700 Subject: [PATCH 4/4] fix remaining hunks in `tidy_iter` too --- cpp-linter/src/clang_tools/clang_format.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp-linter/src/clang_tools/clang_format.rs b/cpp-linter/src/clang_tools/clang_format.rs index c72bde66..0215e540 100644 --- a/cpp-linter/src/clang_tools/clang_format.rs +++ b/cpp-linter/src/clang_tools/clang_format.rs @@ -302,8 +302,8 @@ fn three_way_diff(ranges: &[RangeInclusive], tidy_diff: Diff) -> Vec