Refactor BlameRanges to support multiple range formats as an enum

This modification introduces changes to the `BlameRanges` struct, converting it into an enum to support both `OneBasedInclusive` and `ZeroBasedExclusive` range formats. The `FullFile` variant, denoting that the entire file is to be blamed, serves as a more explicit substitute for a previously used "empty" range.

Author: holodorum

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 line range was given, line range is expected to be a 0-based inclusive range in the format '<start>,<end>'")]
+    InvalidZeroBasedLineRange,
     #[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

@@ -10,7 +10,7 @@ use gix_traverse::commit::find as find_commit;
 use smallvec::SmallVec;
 
 use super::{process_changes, Change, UnblamedHunk};
-use crate::{BlameEntry, Error, Options, Outcome, Statistics};
+use crate::{BlameEntry, BlameRanges, Error, Options, Outcome, Statistics};
 
 /// Produce a list of consecutive [`BlameEntry`] instances to indicate in which commits the ranges of the file
 /// at `suspect:<file_path>` originated in.
@@ -94,15 +94,14 @@ 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 zero_based_ranges = options.ranges.to_zero_based_exclusive(num_lines_in_blamed)?;
+    let mut hunks_to_blame = match zero_based_ranges {
+        BlameRanges::ZeroBasedExclusive(ranges) => ranges
+            .into_iter()
+            .map(|range| UnblamedHunk::new(range, suspect))
+            .collect::<Vec<_>>(),
+        _ => return Err(Error::InvalidZeroBasedLineRange),
+    };
 
     let (mut buf, mut buf2) = (Vec::new(), Vec::new());
     let commit = find_commit(cache.as_ref(), &odb, &suspect, &mut buf)?;

gix-blame/src/lib.rs

@@ -20,4 +20,6 @@ mod types;
 pub use types::{BlameEntry, BlameRanges, Options, Outcome, Statistics};
 
 mod file;
+mod tests;
+
 pub use file::function::file;

gix-blame/src/tests.rs

@@ -0,0 +1,128 @@
+use crate::types::BlameRanges;
+
+mod blame_ranges {
+    #[test]
+    fn create_from_single_range() {
+        let range = BlameRanges::from_range(20..=40);
+        match range {
+            BlameRanges::OneBasedInclusive(ranges) => {
+                assert_eq!(ranges.len(), 1);
+                assert_eq!(ranges[0], 20..=40);
+            }
+            _ => panic!("Expected OneBasedInclusive variant"),
+        }
+    }
+
+    #[test]
+    fn create_from_multiple_ranges() {
+        let ranges = BlameRanges::from_ranges(vec![1..=4, 10..=14]);
+        match ranges {
+            BlameRanges::OneBasedInclusive(ranges) => {
+                assert_eq!(ranges.len(), 2);
+                assert_eq!(ranges[0], 1..=4);
+                assert_eq!(ranges[1], 10..=14);
+            }
+            _ => panic!("Expected OneBasedInclusive variant"),
+        }
+    }
+
+    #[test]
+    fn add_range_merges_overlapping() {
+        let mut ranges = BlameRanges::from_range(1..=5);
+        ranges.add_range(3..=7).unwrap();
+
+        match ranges {
+            BlameRanges::OneBasedInclusive(ranges) => {
+                assert_eq!(ranges.len(), 1);
+                assert_eq!(ranges[0], 1..=7);
+            }
+            _ => panic!("Expected OneBasedInclusive variant"),
+        }
+    }
+
+    #[test]
+    fn add_range_merges_adjacent() {
+        let mut ranges = BlameRanges::from_range(1..=5);
+        ranges.add_range(6..=10).unwrap();
+
+        match ranges {
+            BlameRanges::OneBasedInclusive(ranges) => {
+                assert_eq!(ranges.len(), 1);
+                assert_eq!(ranges[0], 1..=10);
+            }
+            _ => panic!("Expected OneBasedInclusive variant"),
+        }
+    }
+
+    #[test]
+    fn add_range_keeps_separate() {
+        let mut ranges = BlameRanges::from_range(1..=5);
+        ranges.add_range(7..=10).unwrap();
+
+        match ranges {
+            BlameRanges::OneBasedInclusive(ranges) => {
+                assert_eq!(ranges.len(), 2);
+                assert_eq!(ranges[0], 1..=5);
+                assert_eq!(ranges[1], 7..=10);
+            }
+            _ => panic!("Expected OneBasedInclusive variant"),
+        }
+    }
+
+    #[test]
+    fn convert_to_zero_based_exclusive() {
+        let ranges = BlameRanges::from_ranges(vec![1..=5, 10..=15]);
+        let result = ranges.to_zero_based_exclusive(20).unwrap();
+        match result {
+            BlameRanges::ZeroBasedExclusive(ranges) => {
+                assert_eq!(ranges.len(), 2);
+                assert_eq!(ranges[0], 0..5);
+                assert_eq!(ranges[1], 9..15);
+            }
+            _ => panic!("Expected ZeroBasedExclusive variant"),
+        }
+    }
+
+    #[test]
+    fn convert_full_file_to_zero_based() {
+        let ranges = BlameRanges::FullFile;
+        let result = ranges.to_zero_based_exclusive(100).unwrap();
+        match result {
+            BlameRanges::ZeroBasedExclusive(ranges) => {
+                assert_eq!(ranges.len(), 1);
+                assert_eq!(ranges[0], 0..100);
+            }
+            _ => panic!("Expected ZeroBasedExclusive variant"),
+        }
+    }
+
+    #[test]
+    fn convert_zero_based_to_zero_based() {
+        let ranges = BlameRanges::ZeroBasedExclusive(vec![0..5, 10..15]);
+        let result = ranges.to_zero_based_exclusive(20).unwrap();
+        match result {
+            BlameRanges::ZeroBasedExclusive(ranges) => {
+                assert_eq!(ranges.len(), 2);
+                assert_eq!(ranges[0], 0..5);
+                assert_eq!(ranges[1], 10..15);
+            }
+            _ => panic!("Expected ZeroBasedExclusive variant"),
+        }
+    }
+
+    #[test]
+    fn error_on_range_beyond_max_lines() {
+        let ranges = BlameRanges::from_range(1..=15);
+        let result = ranges.to_zero_based_exclusive(10);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn default_is_full_file() {
+        let ranges = BlameRanges::default();
+        match ranges {
+            BlameRanges::FullFile => (),
+            _ => panic!("Expected FullFile variant"),
+        }
+    }
+}

gix-blame/src/types.rs

@@ -24,9 +24,11 @@ use crate::Error;
 /// let range = BlameRanges::from_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_ranges(vec![
+///     1..=4, // Lines 1-4
+///    10..=14, // Lines 10-14
+/// ]
+/// );
 /// ```
 ///
 /// # Line Number Representation
@@ -40,36 +42,33 @@ use crate::Error;
 /// 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.
 #[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]
+    FullFile,
+    /// Blame ranges in 1-based inclusive format.
+    OneBasedInclusive(Vec<RangeInclusive<u32>>),
+    /// Blame ranges in 0-based exclusive format.
+    ZeroBasedExclusive(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] }
+        Self::OneBasedInclusive(vec![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();
+        let mut result = Self::OneBasedInclusive(vec![]);
         for range in ranges {
-            result.merge_range(range);
+            let _ = result.merge_range(range);
         }
         result
     }
@@ -81,23 +80,32 @@ 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_range(&mut self, new_range: RangeInclusive<u32>) -> Result<(), Error> {
+        match self {
+            Self::OneBasedInclusive(_) => self.merge_range(new_range),
+            _ => Err(Error::InvalidOneBasedLineRange),
+        }
     }
 
     /// 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;
+    fn merge_range(&mut self, new_range: RangeInclusive<u32>) -> Result<(), Error> {
+        match self {
+            Self::OneBasedInclusive(ref mut ranges) => {
+                // Check if this range can be merged with any existing range
+                for range in &mut *ranges {
+                    // Check if ranges overlap or are adjacent
+                    if new_range.start() <= &(range.end() + 1) && range.start() <= &(new_range.end() + 1) {
+                        *range = *range.start().min(new_range.start())..=*range.end().max(new_range.end());
+                        return Ok(());
+                    }
+                }
+                // If no overlap found, add it as a new range
+                ranges.push(new_range);
+                Ok(())
             }
+            _ => Err(Error::InvalidOneBasedLineRange),
         }
-        // If no overlap found, add it as a new range
-        self.ranges.push(new_range);
     }
 
     /// Convert the 1-based inclusive ranges to 0-based exclusive ranges.
@@ -111,30 +119,26 @@ impl BlameRanges {
     /// - 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]);
-        }
-
-        let mut result = Vec::with_capacity(self.ranges.len());
-        for range in &self.ranges {
-            if *range.start() == 0 {
-                return Err(Error::InvalidLineRange);
+    pub fn to_zero_based_exclusive(&self, max_lines: u32) -> Result<BlameRanges, Error> {
+        match self {
+            Self::FullFile => {
+                let range = 0..max_lines;
+                Ok(BlameRanges::ZeroBasedExclusive(vec![range]))
             }
-            let start = range.start() - 1;
-            let end = *range.end();
-            if start >= max_lines || end > max_lines || start == end {
-                return Err(Error::InvalidLineRange);
+            Self::OneBasedInclusive(ranges) => {
+                let mut result = Vec::with_capacity(ranges.len());
+                for range in ranges {
+                    let start = range.start() - 1;
+                    let end = *range.end();
+                    if start >= max_lines || end > max_lines || start == end {
+                        return Err(Error::InvalidOneBasedLineRange);
+                    }
+                    result.push(start..end);
+                }
+                Ok(BlameRanges::ZeroBasedExclusive(result))
             }
-            result.push(start..end);
+            Self::ZeroBasedExclusive(_) => Ok(self.clone()),
         }
-        Ok(result)
-    }
-
-    /// Returns true if no specific ranges are set (meaning blame entire file)
-    pub fn is_empty(&self) -> bool {
-        self.ranges.is_empty()
     }
 }
 
@@ -334,6 +338,17 @@ 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;
+
+        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)
     }

gix-blame/tests/blame.rs

@@ -355,10 +355,11 @@ mod blame_ranges {
             suspect,
         } = Fixture::new().unwrap();
 
-        let mut ranges = BlameRanges::new();
-        ranges.add_range(1..=2); // Lines 1-2
-        ranges.add_range(1..=1); // Duplicate range, should be ignored
-        ranges.add_range(4..=4); // Line 4
+        let ranges = BlameRanges::from_ranges(vec![
+            1..=2, // Lines 1-2
+            1..=1, // Duplicate range, should be ignored
+            4..=4, // Line 4
+        ]);
 
         let lines_blamed = gix_blame::file(
             &odb,