Improve support for multiple blame ranges

gix-blame/src/error.rs

@@ -28,7 +28,9 @@ pub enum Error {
     #[error(transparent)]
     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>'")]
-    InvalidLineRange,
+    InvalidOneBasedLineRange,
+    #[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,15 +94,11 @@ pub fn file(
         return Ok(Outcome::default());
     }
 
-    let ranges = options.range.to_zero_based_exclusive(num_lines_in_blamed)?;
-    let mut hunks_to_blame = Vec::with_capacity(ranges.len());
-
-    for range in ranges {
-        hunks_to_blame.push(UnblamedHunk {
-            range_in_blamed_file: range.clone(),
-            suspects: [(suspect, range)].into(),
-        });
-    }
+    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))
+        .collect::<Vec<_>>();
 
     let (mut buf, mut buf2) = (Vec::new(), Vec::new());
     let commit = find_commit(cache.as_ref(), &odb, &suspect, &mut buf)?;

gix-blame/src/file/tests.rs

@@ -1338,3 +1338,92 @@ mod process_changes {
         );
     }
 }
+
+mod blame_ranges {
+    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).unwrap();
+        if let BlameRanges::PartialFile(ranges) = range {
+            assert_eq!(ranges, vec![19..40]);
+        }
+    }
+
+    #[test]
+    fn create_from_multiple_ranges() {
+        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 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).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).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]).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);
+    }
+
+    #[test]
+    fn convert_full_file_to_zero_based() {
+        let ranges = BlameRanges::WholeFile;
+        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();
+        assert!(matches!(ranges, BlameRanges::WholeFile));
+    }
+}

gix-blame/src/types.rs

@@ -21,12 +21,14 @@ use crate::Error;
 /// use gix_blame::BlameRanges;
 ///
 /// // Blame lines 20 through 40 (inclusive)
-/// let range = BlameRanges::from_range(20..=40);
+/// let range = BlameRanges::from_one_based_inclusive_range(20..=40);
 ///
 /// // Blame multiple ranges
-/// let mut ranges = BlameRanges::new();
-/// ranges.add_range(1..=4);   // Lines 1-4
-/// ranges.add_range(10..=14); // Lines 10-14
+/// let mut ranges = BlameRanges::from_one_based_inclusive_ranges(vec![
+///     1..=4, // Lines 1-4
+///    10..=14, // Lines 10-14
+/// ]
+/// );
 /// ```
 ///
 /// # Line Number Representation
@@ -36,42 +38,63 @@ use crate::Error;
 /// - This will be converted to `19..40` internally as the algorithm uses 0-based ranges that are exclusive at the end
 ///
 /// # Empty Ranges
-///
-/// An empty `BlameRanges` (created via `BlameRanges::new()` or `BlameRanges::default()`) means
-/// to blame the entire file, similar to running `git blame` without line number arguments.
+/// You can blame the entire file by calling `BlameRanges::default()`, or by passing an empty vector to `from_one_based_inclusive_ranges`.
 #[derive(Debug, Clone, Default)]
-pub struct BlameRanges {
-    /// The ranges to blame, stored as 1-based inclusive ranges
-    /// An empty Vec means blame the entire file
-    ranges: Vec<RangeInclusive<u32>>,
+pub enum BlameRanges {
+    /// Blame the entire file.
+    #[default]
+    WholeFile,
+    /// Blame ranges in 0-based exclusive format.
+    PartialFile(Vec<Range<u32>>),
 }
 
 /// Lifecycle
 impl BlameRanges {
-    /// Create a new empty BlameRanges instance.
-    ///
-    /// An empty instance means to blame the entire file.
-    pub fn new() -> Self {
-        Self::default()
-    }
-
     /// Create from a single range.
     ///
-    /// The range is 1-based, similar to git's line number format.
-    pub fn from_range(range: RangeInclusive<u32>) -> Self {
-        Self { ranges: vec![range] }
+    /// Note that the input range is 1-based inclusive, as used by git, and
+    /// the output is zero-based `BlameRanges` instance.
+    ///
+    /// @param range: A 1-based inclusive range.
+    /// @return: A `BlameRanges` instance representing the 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.
     ///
-    /// All ranges are 1-based.
-    /// Overlapping or adjacent ranges will be merged.
-    pub fn from_ranges(ranges: Vec<RangeInclusive<u32>>) -> Self {
-        let mut result = Self::new();
-        for range in ranges {
-            result.merge_range(range);
+    /// Note that the input ranges are 1-based inclusive, as used by git, and
+    /// the output is zero-based `BlameRanges` instance.
+    ///
+    /// If the input vector is empty, the result will be `WholeFile`.
+    ///
+    /// @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>>) -> Result<Self, Error> {
+        if ranges.is_empty() {
+            return Ok(Self::WholeFile);
+        }
+
+        let zero_based_ranges = ranges
+            .into_iter()
+            .map(Self::inclusive_to_zero_based_exclusive)
+            .collect::<Vec<_>>();
+        let mut result = Self::PartialFile(vec![]);
+        for range in zero_based_ranges {
+            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>) -> Result<Range<u32>, Error> {
+        if range.start() == &0 {
+            return Err(Error::InvalidOneBasedLineRange);
+        }
+        let start = range.start() - 1;
+        let end = *range.end();
+        Ok(start..end)
     }
 }
 
@@ -81,60 +104,50 @@ 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>) {
-        self.merge_range(new_range);
+    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_zero_based_exclusive_range(zero_based_range)
+            }
+            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: RangeInclusive<u32>) {
-        // Check if this range can be merged with any existing range
-        for range in &mut self.ranges {
-            // Check if ranges overlap or 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;
-            }
-        }
-        // If no overlap found, add it as a new range
-        self.ranges.push(new_range);
-    }
+    fn merge_zero_based_exclusive_range(&mut self, new_range: Range<u32>) -> Result<(), Error> {
+        match self {
+            Self::PartialFile(ref mut ranges) => {
+                // 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
+                });
 
-    /// Convert the 1-based inclusive ranges to 0-based exclusive ranges.
-    ///
-    /// This is used internally by the blame algorithm to convert from git's line number format
-    /// to the internal format used for processing.
-    ///
-    /// # Errors
-    ///
-    /// Returns `Error::InvalidLineRange` if:
-    /// - Any range starts at 0 (must be 1-based)
-    /// - Any range extends beyond the file's length
-    /// - Any range has the same start and end
-    pub fn to_zero_based_exclusive(&self, max_lines: u32) -> Result<Vec<Range<u32>>, Error> {
-        if self.ranges.is_empty() {
-            let range = 0..max_lines;
-            return Ok(vec![range]);
-        }
+                // 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)
+                });
 
-        let mut result = Vec::with_capacity(self.ranges.len());
-        for range in &self.ranges {
-            if *range.start() == 0 {
-                return Err(Error::InvalidLineRange);
-            }
-            let start = range.start() - 1;
-            let end = *range.end();
-            if start >= max_lines || end > max_lines || start == end {
-                return Err(Error::InvalidLineRange);
+                // Add back non-overlapping ranges and the merged range
+                non_overlapping.push(merged_range);
+                *ranges = non_overlapping;
+                Ok(())
             }
-            result.push(start..end);
+            Self::WholeFile => Err(Error::InvalidRangeAddition),
         }
-        Ok(result)
     }
 
-    /// Returns true if no specific ranges are set (meaning blame entire file)
-    pub fn is_empty(&self) -> bool {
-        self.ranges.is_empty()
+    /// Convert the ranges to a vector of `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;
+                vec![full_range]
+            }
+            Self::PartialFile(ranges) => ranges.clone(),
+        }
     }
 }
 
@@ -144,7 +157,7 @@ pub struct Options {
     /// The algorithm to use for diffing.
     pub diff_algorithm: gix_diff::blob::Algorithm,
     /// The ranges to blame in the file.
-    pub range: BlameRanges,
+    pub ranges: BlameRanges,
     /// Don't consider commits before the given date.
     pub since: Option<gix_date::Time>,
 }
@@ -334,6 +347,16 @@ pub struct UnblamedHunk {
 }
 
 impl UnblamedHunk {
+    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,
+            suspects: [(suspect, range_start..range_end)].into(),
+        }
+    }
+
     pub(crate) fn has_suspect(&self, suspect: &ObjectId) -> bool {
         self.suspects.iter().any(|entry| entry.0 == *suspect)
     }