Improve support for multiple blame ranges

gix-blame/src/error.rs

@@ -29,8 +29,8 @@ pub enum Error {
     DiffTree(#[from] gix_diff::tree::Error),
     #[error("Invalid line range was given, line range is expected to be a 1-based inclusive range in the format '<start>,<end>'")]
     InvalidOneBasedLineRange,
-    #[error("Invalid line range was given, line range is expected to be a 0-based inclusive range in the format '<start>,<end>'")]
-    InvalidZeroBasedLineRange,
+    #[error("Invalid range addition was attempted. Cannot add a range to a whole file.")]
+    InvalidRangeAddition,
     #[error("Failure to decode commit during traversal")]
     DecodeCommit(#[from] gix_object::decode::Error),
     #[error("Failed to get parent from commitgraph during traversal")]

gix-blame/src/file/function.rs

@@ -94,7 +94,7 @@ pub fn file(
         return Ok(Outcome::default());
     }
 
-    let ranges_to_blame = options.ranges.to_ranges(num_lines_in_blamed);
+    let ranges_to_blame = options.ranges.to_zero_based_exclusive_ranges(num_lines_in_blamed);
     let mut hunks_to_blame = ranges_to_blame
         .into_iter()
         .map(|range| UnblamedHunk::new(range, suspect))

gix-blame/src/file/tests.rs

@@ -1340,79 +1340,74 @@ mod process_changes {
 }
 
 mod blame_ranges {
-    use crate::BlameRanges;
+    use crate::{BlameRanges, Error};
+
+    #[test]
+    fn create_with_invalid_ranges() {
+        let range = BlameRanges::from_one_based_inclusive_range(0..=10);
+
+        assert!(matches!(range, Err(Error::InvalidOneBasedLineRange)));
+    }
 
     #[test]
     fn create_from_single_range() {
-        let range = BlameRanges::from_one_based_inclusive_range(20..=40);
-        match range {
-            BlameRanges::PartialFile(ranges) => {
-                assert_eq!(ranges.len(), 1);
-                assert_eq!(ranges[0], 19..40);
-            }
-            _ => panic!("Expected PartialFile variant"),
+        let range = BlameRanges::from_one_based_inclusive_range(20..=40).unwrap();
+        if let BlameRanges::PartialFile(ranges) = range {
+            assert_eq!(ranges, vec![19..40]);
         }
     }
 
     #[test]
     fn create_from_multiple_ranges() {
-        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=4, 10..=14]);
-        match ranges {
-            BlameRanges::PartialFile(ranges) => {
-                assert_eq!(ranges.len(), 2);
-                assert_eq!(ranges[0], 0..4);
-                assert_eq!(ranges[1], 9..14);
-            }
-            _ => panic!("Expected PartialFile variant"),
+        let range = BlameRanges::from_one_based_inclusive_ranges(vec![1..=4, 10..=14]).unwrap();
+        if let BlameRanges::PartialFile(ranges) = range {
+            assert_eq!(ranges, vec![0..4, 9..14]);
         }
     }
 
     #[test]
     fn add_range_merges_overlapping() {
-        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
-        ranges.add_range(3..=7).unwrap();
-
-        match ranges {
-            BlameRanges::PartialFile(ranges) => {
-                assert_eq!(ranges.len(), 1);
-                assert_eq!(ranges[0], 0..7);
-            }
-            _ => panic!("Expected PartialFile variant"),
+        let mut range = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap();
+        range.add_one_based_inclusive_range(3..=7).unwrap();
+        if let BlameRanges::PartialFile(ranges) = range {
+            assert_eq!(ranges, vec![0..7]);
         }
     }
 
+    #[test]
+    fn add_range_merges_overlapping_both() {
+        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=3).unwrap();
+        ranges.add_one_based_inclusive_range(5..=7).unwrap();
+        ranges.add_one_based_inclusive_range(2..=6).unwrap();
+
+        if let BlameRanges::PartialFile(ranges) = ranges {
+            assert_eq!(ranges, vec![0..7]);
+        }
+    }
     #[test]
     fn add_range_merges_adjacent() {
-        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
-        ranges.add_range(6..=10).unwrap();
-
-        match ranges {
-            BlameRanges::PartialFile(ranges) => {
-                assert_eq!(ranges.len(), 1);
-                assert_eq!(ranges[0], 0..10);
-            }
-            _ => panic!("Expected PartialFile variant"),
+        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap();
+        ranges.add_one_based_inclusive_range(6..=10).unwrap();
+
+        if let BlameRanges::PartialFile(ranges) = ranges {
+            assert_eq!(ranges, vec![0..10]);
         }
     }
 
     #[test]
     fn add_range_keeps_separate() {
-        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
-        ranges.add_range(6..=10).unwrap();
-
-        match ranges {
-            BlameRanges::PartialFile(ranges) => {
-                assert_eq!(ranges.len(), 1);
-                assert_eq!(ranges[0], 0..10);
-            }
-            _ => panic!("Expected PartialFile variant"),
+        let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap();
+        ranges.add_one_based_inclusive_range(6..=10).unwrap();
+
+        if let BlameRanges::PartialFile(ranges) = ranges {
+            assert_eq!(ranges, vec![0..10]);
         }
     }
 
     #[test]
     fn convert_to_zero_based_exclusive() {
-        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=5, 10..=15]);
-        let ranges = ranges.to_ranges(20);
+        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=5, 10..=15]).unwrap();
+        let ranges = ranges.to_zero_based_exclusive_ranges(20);
         assert_eq!(ranges.len(), 2);
         assert_eq!(ranges[0], 0..5);
         assert_eq!(ranges[1], 9..15);
@@ -1421,17 +1416,14 @@ mod blame_ranges {
     #[test]
     fn convert_full_file_to_zero_based() {
         let ranges = BlameRanges::WholeFile;
-        let ranges = ranges.to_ranges(100);
+        let ranges = ranges.to_zero_based_exclusive_ranges(100);
         assert_eq!(ranges.len(), 1);
         assert_eq!(ranges[0], 0..100);
     }
 
     #[test]
     fn default_is_full_file() {
         let ranges = BlameRanges::default();
-        match ranges {
-            BlameRanges::WholeFile => (),
-            _ => panic!("Expected WholeFile variant"),
-        }
+        assert!(matches!(ranges, BlameRanges::WholeFile));
     }
 }

gix-blame/src/types.rs

@@ -57,9 +57,9 @@ impl BlameRanges {
     ///
     /// @param range: A 1-based inclusive range.
     /// @return: A `BlameRanges` instance representing the range.
-    pub fn from_one_based_inclusive_range(range: RangeInclusive<u32>) -> Self {
-        let zero_based_range = Self::inclusive_to_zero_based_exclusive(range);
-        Self::PartialFile(vec![zero_based_range])
+    pub fn from_one_based_inclusive_range(range: RangeInclusive<u32>) -> Result<Self, Error> {
+        let zero_based_range = Self::inclusive_to_zero_based_exclusive(range)?;
+        Ok(Self::PartialFile(vec![zero_based_range]))
     }
 
     /// Create from multiple ranges.
@@ -71,9 +71,9 @@ impl BlameRanges {
     ///
     /// @param ranges: A vec of 1-based inclusive range.
     /// @return: A `BlameRanges` instance representing the range.
-    pub fn from_one_based_inclusive_ranges(ranges: Vec<RangeInclusive<u32>>) -> Self {
+    pub fn from_one_based_inclusive_ranges(ranges: Vec<RangeInclusive<u32>>) -> Result<Self, Error> {
         if ranges.is_empty() {
-            return Self::WholeFile;
+            return Ok(Self::WholeFile);
         }
 
         let zero_based_ranges = ranges
@@ -82,16 +82,19 @@ impl BlameRanges {
             .collect::<Vec<_>>();
         let mut result = Self::PartialFile(vec![]);
         for range in zero_based_ranges {
-            let _ = result.merge_range(range);
+            let _ = result.merge_zero_based_exclusive_range(range?);
         }
-        result
+        Ok(result)
     }
 
     /// Convert a 1-based inclusive range to a 0-based exclusive range.
-    fn inclusive_to_zero_based_exclusive(range: RangeInclusive<u32>) -> Range<u32> {
+    fn inclusive_to_zero_based_exclusive(range: RangeInclusive<u32>) -> Result<Range<u32>, Error> {
+        if range.start() == &0 {
+            return Err(Error::InvalidOneBasedLineRange);
+        }
         let start = range.start() - 1;
         let end = *range.end();
-        start..end
+        Ok(start..end)
     }
 }
 
@@ -101,44 +104,43 @@ impl BlameRanges {
     /// The range should be 1-based inclusive.
     /// If the new range overlaps with or is adjacent to an existing range,
     /// they will be merged into a single range.
-    pub fn add_range(&mut self, new_range: RangeInclusive<u32>) -> Result<(), Error> {
+    pub fn add_one_based_inclusive_range(&mut self, new_range: RangeInclusive<u32>) -> Result<(), Error> {
         match self {
             Self::PartialFile(_) => {
-                let zero_based_range = Self::inclusive_to_zero_based_exclusive(new_range);
-                self.merge_range(zero_based_range)
+                let zero_based_range = Self::inclusive_to_zero_based_exclusive(new_range)?;
+                self.merge_zero_based_exclusive_range(zero_based_range)
             }
-            _ => Err(Error::InvalidOneBasedLineRange),
+            Self::WholeFile => Err(Error::InvalidRangeAddition),
         }
     }
 
     /// Attempts to merge the new range with any existing ranges.
     /// If no merge is possible, add it as a new range.
-    fn merge_range(&mut self, new_range: Range<u32>) -> Result<(), Error> {
+    fn merge_zero_based_exclusive_range(&mut self, new_range: Range<u32>) -> Result<(), Error> {
         match self {
             Self::PartialFile(ref mut ranges) => {
-                // Check if this range can be merged with any existing range
-                for range in &mut *ranges {
-                    // Check if ranges overlap
-                    if new_range.start <= range.end && range.start <= new_range.end {
-                        *range = range.start.min(new_range.start)..range.end.max(new_range.end);
-                        return Ok(());
-                    }
-                    // Check if ranges are adjacent
-                    if new_range.start == range.end || range.start == new_range.end {
-                        *range = range.start.min(new_range.start)..range.end.max(new_range.end);
-                        return Ok(());
-                    }
-                }
-                // If no overlap or adjacency found, add it as a new range
-                ranges.push(new_range);
+                // Partition ranges into those that don't overlap/aren't adjacent and those that do
+                let (mut non_overlapping, overlapping): (Vec<_>, Vec<_>) = ranges.drain(..).partition(|range| {
+                    // Check if ranges DON'T overlap and are NOT adjacent
+                    new_range.end < range.start || range.end < new_range.start
+                });
+
+                // Fold all overlapping ranges together with the new range
+                let merged_range = overlapping.into_iter().fold(new_range, |acc, range| {
+                    acc.start.min(range.start)..acc.end.max(range.end)
+                });
+
+                // Add back non-overlapping ranges and the merged range
+                non_overlapping.push(merged_range);
+                *ranges = non_overlapping;
                 Ok(())
             }
-            _ => Err(Error::InvalidOneBasedLineRange),
+            Self::WholeFile => Err(Error::InvalidRangeAddition),
         }
     }
 
     /// Convert the ranges to a vector of `Range<u32>`.
-    pub fn to_ranges(&self, max_lines: u32) -> Vec<Range<u32>> {
+    pub fn to_zero_based_exclusive_ranges(&self, max_lines: u32) -> Vec<Range<u32>> {
         match self {
             Self::WholeFile => {
                 let full_range = 0..max_lines;
@@ -345,10 +347,9 @@ pub struct UnblamedHunk {
 }
 
 impl UnblamedHunk {
-    /// Create a new instance
-    pub fn new(range: Range<u32>, suspect: ObjectId) -> Self {
-        let range_start = range.start;
-        let range_end = range.end;
+    pub fn new(from_range_in_blamed_file: Range<u32>, suspect: ObjectId) -> Self {
+        let range_start = from_range_in_blamed_file.start;
+        let range_end = from_range_in_blamed_file.end;
 
         UnblamedHunk {
             range_in_blamed_file: range_start..range_end,

gix-blame/tests/blame.rs

@@ -332,7 +332,7 @@ mod blame_ranges {
             "simple.txt".into(),
             gix_blame::Options {
                 diff_algorithm: gix_diff::blob::Algorithm::Histogram,
-                ranges: BlameRanges::from_one_based_inclusive_range(1..=2),
+                ranges: BlameRanges::from_one_based_inclusive_range(1..=2).unwrap(),
                 since: None,
             },
         )
@@ -359,7 +359,8 @@ mod blame_ranges {
             1..=2, // Lines 1-2
             1..=1, // Duplicate range, should be ignored
             4..=4, // Line 4
-        ]);
+        ])
+        .unwrap();
 
         let lines_blamed = gix_blame::file(
             &odb,
@@ -392,7 +393,7 @@ mod blame_ranges {
             suspect,
         } = Fixture::new().unwrap();
 
-        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=2, 1..=1, 4..=4]);
+        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=2, 1..=1, 4..=4]).unwrap();
 
         let lines_blamed = gix_blame::file(
             &odb,

src/plumbing/main.rs

@@ -1578,7 +1578,7 @@ pub fn main() -> Result<()> {
                     &file,
                     gix::blame::Options {
                         diff_algorithm,
-                        ranges: gix::blame::BlameRanges::from_one_based_inclusive_ranges(ranges),
+                        ranges: gix::blame::BlameRanges::from_one_based_inclusive_ranges(ranges)?,
                         since,
                     },
                     out,