Refactor BlameRanges to support multiple range formats as an enum

holodorum · holodorum · commit 84840980bb38 · 2025-05-13T19:49:00.000+02:00
This modification introduces changes to the `BlameRanges` struct, converting it into an enum to support both `PartialFile` and `WholeFile`. Internally the range in `BlameRanges` is stored as zero-based-exclusive now.
diff --git a/gix-blame/src/error.rs b/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")]
diff --git a/gix-blame/src/file/function.rs b/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_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)?;
diff --git a/gix-blame/src/file/tests.rs b/gix-blame/src/file/tests.rs
@@ -1338,3 +1338,100 @@ mod process_changes {
         );
     }
 }
+
+mod blame_ranges {
+    use crate::BlameRanges;
+
+    #[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"),
+        }
+    }
+
+    #[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"),
+        }
+    }
+
+    #[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"),
+        }
+    }
+
+    #[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"),
+        }
+    }
+
+    #[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"),
+        }
+    }
+
+    #[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);
+        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_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"),
+        }
+    }
+}
diff --git a/gix-blame/src/types.rs b/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,43 +38,61 @@ 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>) -> Self {
+        let zero_based_range = Self::inclusive_to_zero_based_exclusive(range);
+        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>>) -> Self {
+        if ranges.is_empty() {
+            return 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_range(range);
         }
         result
     }
+
+    /// Convert a 1-based inclusive range to a 0-based exclusive range.
+    fn inclusive_to_zero_based_exclusive(range: RangeInclusive<u32>) -> Range<u32> {
+        let start = range.start() - 1;
+        let end = *range.end();
+        start..end
+    }
 }
 
 impl BlameRanges {
@@ -81,60 +101,51 @@ 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::PartialFile(_) => {
+                let zero_based_range = Self::inclusive_to_zero_based_exclusive(new_range);
+                self.merge_range(zero_based_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: 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);
+                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.
-    ///
-    /// 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]);
-        }
-
-        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);
+    /// Convert the ranges to a vector of `Range<u32>`.
+    pub fn to_ranges(&self, max_lines: u32) -> Vec<Range<u32>> {
+        match self {
+            Self::WholeFile => {
+                let full_range = 0..max_lines;
+                vec![full_range]
             }
-            result.push(start..end);
+            Self::PartialFile(ranges) => ranges.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 +345,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)
     }
diff --git a/gix-blame/tests/blame.rs b/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_range(1..=2),
+                ranges: BlameRanges::from_one_based_inclusive_range(1..=2),
                 since: None,
             },
         )
@@ -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_one_based_inclusive_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,
@@ -391,7 +392,7 @@ mod blame_ranges {
             suspect,
         } = Fixture::new().unwrap();
 
-        let ranges = BlameRanges::from_ranges(vec![1..=2, 1..=1, 4..=4]);
+        let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=2, 1..=1, 4..=4]);
 
         let lines_blamed = gix_blame::file(
             &odb,
diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs
@@ -1578,7 +1578,7 @@ pub fn main() -> Result<()> {
                     &file,
                     gix::blame::Options {
                         diff_algorithm,
-                        ranges: gix::blame::BlameRanges::from_ranges(ranges),
+                        ranges: gix::blame::BlameRanges::from_one_based_inclusive_ranges(ranges),
                         since,
                     },
                     out,
