impl 3-way diff and add tests

and some other review changes

Author: 2bndy5

cpp-linter/src/clang_tools/clang_format.rs

@@ -8,6 +8,7 @@ use std::{
     sync::{Arc, Mutex, MutexGuard},
 };
 
+use gix_imara_diff::Diff;
 use log::Level;
 
 // project-specific crates/modules
@@ -145,41 +146,20 @@ pub fn run_clang_format(
     if let Some(patched_path) = &file.patched_path
         && patched_path.exists()
     {
-        let tidy_patch_contents =
-            fs::read_to_string(patched_path).map_err(|e| ClangCaptureError::ReadFileFailed {
-                file_name: patched_path.to_string_lossy().to_string(),
-                source: e,
-            })?;
-        let (tidy_diff, _) = make_patch(&tidy_patch_contents, &original_contents);
         let mut cmd = Command::new(cmd_path);
         cmd.current_dir(&cache_path);
         // edit the clang-tody patched file in-place (`-i`)
         cmd.args(["--style", &clang_params.style, "-i"]);
         // if ranges is empty, then we're just formatting the entire file.
         if !ranges.is_empty() {
-            // We're concerned about formatting what clang-tidy changed (tidy_diff.hunks().before),
-            // but we also want to include any clang-format changes that do not overlap clang-tidy fixes.
-            let mut joint_ranges = tidy_diff
-                .hunks()
-                // hunk is partially inclusive: [start, end),
-                // but clang-format expects fully inclusive line ranges.
-                // subtract 1 from hunk.before.end
-                .map(|hunk| {
-                    RangeInclusive::new(hunk.before.start, hunk.before.end.saturating_sub(1))
-                })
-                .collect::<Vec<_>>();
-            for range in &ranges {
-                let mut contained = false;
-                for hunk in tidy_diff.hunks() {
-                    if hunk.before.contains(range.start()) && hunk.before.contains(range.end()) {
-                        contained = true;
-                        break;
-                    }
-                }
-                if !contained {
-                    joint_ranges.push(range.clone());
+            let tidy_patch_contents = fs::read_to_string(patched_path).map_err(|e| {
+                ClangCaptureError::ReadFileFailed {
+                    file_name: patched_path.to_string_lossy().to_string(),
+                    source: e,
                 }
-            }
+            })?;
+            let (tidy_diff, _) = make_patch(&tidy_patch_contents, &original_contents);
+            let joint_ranges = three_way_diff(&ranges, tidy_diff);
             for range in &joint_ranges {
                 cmd.arg(format!("--lines={}:{}", range.start(), range.end()).as_str());
             }
@@ -223,11 +203,119 @@ pub fn run_clang_format(
     Ok(logs)
 }
 
+/// Essentially does a three way diff without the original source that generated the given `ranges` (simplified hunks).
+///
+/// The returned list of ranges are lines that need formatting in the clang-tidy patched file,
+/// provided by the `tidy_diff`. The given `ranges` are the line numbers in the original file
+/// that clang-tidy patched.
+fn three_way_diff(ranges: &[RangeInclusive<u32>], tidy_diff: Diff) -> Vec<RangeInclusive<u32>> {
+    // We're concerned about the formatting cases:
+    //
+    // 1. changes that clang-tidy made: `tidy_diff.hunks().after`
+    // 2. changes in the current CI event's diff (`ranges`)
+    //    that clang-tidy did not touch (`tidy_diff.hunks().before`)
+    // 3. changes that do not overlap clang-tidy fixes: `ranges` - `tidy_diff.hunks().before`
+    // 4. changes that overlap with clang-tidy fixes. This one is complex because
+    //    - tidy fixes can prefix an og range
+    //    - tidy fixes can suffix an og range
+    //    - tidy fixes can be contained within an og range
+    //    - multiple tidy fixes can (in order) suffix, be contained within, and prefix an og range
+    let mut joint_ranges = vec![];
+    let mut tidy_iter = tidy_diff.hunks().peekable();
+    let mut line_shift = 0i32;
+
+    /// Prevent pure removals from causing invalid inclusive ranges.
+    fn maybe_push_range(joint_ranges: &mut Vec<RangeInclusive<u32>>, start: u32, end: u32) {
+        if start <= end {
+            joint_ranges.push(RangeInclusive::new(start, end));
+        }
+    }
+
+    for og_range in ranges {
+        let og_start = *og_range.start();
+        let og_end = *og_range.end();
+
+        // track the start and end of a merged range that gets pushed into joint_ranges.
+        let mut merged_start = (og_start as i32 + line_shift) as u32;
+        let mut merged_end = (og_end as i32 + line_shift) as u32;
+
+        while let Some(tidy_hunk) = tidy_iter.peek() {
+            // alias for readability and prevent some repeated calculations
+            let before_start = tidy_hunk.before.start;
+            let before_end = tidy_hunk.before.end.saturating_sub(1);
+            let after_start = tidy_hunk.after.start;
+            let after_end = tidy_hunk.after.end.saturating_sub(1);
+            let delta = tidy_hunk.after.len() as i32 - tidy_hunk.before.len() as i32;
+
+            // The tidy hunk is a pure removal that exactly matches the og range.
+            if tidy_hunk.is_pure_removal() && before_start == og_start && before_end == 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;
+                merged_end = 0; // causes invalid inclusive range which does not get pushed.
+                tidy_iter.next(); // skip this tidy hunk
+                break; // skip og range and iterate to the next one.
+            }
+
+            // tidy hunk is before the og range.
+            if before_end < og_start {
+                maybe_push_range(&mut joint_ranges, after_start, after_end);
+                line_shift += delta;
+                tidy_iter.next();
+                continue;
+            }
+
+            // tidy hunk is after the og range.
+            if before_start > og_end {
+                // handle the og range before iterating the next tidy hunk
+                break;
+            }
+
+            // tidy hunk overlaps with the og range in some way (case 4).
+            if tidy_hunk.before.contains(&og_start) {
+                merged_start = after_start;
+            }
+
+            // commit the line shift now that the tidy hunk start is checked.
+            line_shift += delta;
+
+            // tidy hunk suffixes the og range.
+            if tidy_hunk.before.contains(&og_end) {
+                merged_end = after_end;
+                tidy_iter.next(); // this tidy hunk is handled.
+                break; // break from loop to push the merged range into joint_ranges.
+            }
+
+            // tidy hunk is contained within the og range.
+            // so adjust the og range end accordingly and continue iterating tidy hunks
+            merged_end = (og_end as i32 + line_shift) as u32;
+            tidy_iter.next();
+        }
+
+        maybe_push_range(&mut joint_ranges, merged_start, merged_end);
+    }
+
+    // handle any remaining tidy hunks that are after all og ranges.
+    for tidy_hunk in tidy_iter {
+        maybe_push_range(
+            &mut joint_ranges,
+            tidy_hunk.after.start,
+            tidy_hunk.after.end.saturating_sub(1),
+        );
+    }
+
+    joint_ranges
+}
+
 #[cfg(test)]
 mod tests {
     #![allow(clippy::unwrap_used)]
 
-    use super::summarize_style;
+    use std::ops::RangeInclusive;
+
+    use gix_imara_diff::{Diff, InternedInput};
+
+    use super::{summarize_style, three_way_diff};
 
     fn formalize_style(style: &str, expected: &str) {
         assert_eq!(summarize_style(style), expected);
@@ -247,4 +335,42 @@ mod tests {
     fn formalize_custom_style() {
         formalize_style("file", "Custom");
     }
+
+    #[test]
+    fn three_way_diff_mixed() {
+        const OG_SRC: &str =
+            "line1\nline2\nline3\nline4\nline5\nline6\nline7\nline8\nline9\nline10\nline11";
+        // TIDY_SRC replaces line3->StringA (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.
+        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)];
+        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]);
+    }
+
+    #[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 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];
+        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]);
+    }
 }

cpp-linter/src/common_fs.rs

@@ -187,9 +187,6 @@ impl FileObj {
 
         self.get_suggestions(review_comments, &diff, &input, summary_only)
             .map_err(FileObjError::DisplayStringFailed)?;
-        if summary_only {
-            return Ok(());
-        }
         if let Some(advice) = &self.tidy_advice {
             // now check for clang-tidy warnings with no fixes applied
             let file_ext = self
@@ -203,6 +200,10 @@ impl FileObj {
             for note in &advice.notes {
                 if note.fixed_lines.is_empty() && self.is_line_in_diff(&note.line) {
                     // notification had no suggestion applied in `patched`
+                    total += 1;
+                    if summary_only {
+                        continue;
+                    }
                     let mut suggestion = format!(
                         "### clang-tidy diagnostic\n**{file_name}:{}:{}** {}: [{}]\n\n> {}\n",
                         &note.line,
@@ -217,7 +218,6 @@ impl FileObj {
                                 .as_str(),
                         );
                     }
-                    total += 1;
                     let mut is_merged = false;
                     for s in &mut review_comments.comments {
                         if s.path == file_name
@@ -306,6 +306,13 @@ impl FileObj {
                     }
                 }
                 _ => {
+                    printer.display_header(
+                        &mut patch_buff,
+                        hunk.before.start,
+                        hunk.after.start,
+                        hunk.before.len() as u32,
+                        hunk.after.len() as u32,
+                    )?;
                     printer.display_hunk(
                         &mut patch_buff,
                         &input.before[hunk.before.start as usize..hunk.before.end as usize],