diff --git a/gix-blame/src/error.rs b/gix-blame/src/error.rs index 979cc8cd6ef..c31bcfc42fa 100644 --- 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 ','")] - InvalidLineRange, + InvalidOneBasedLineRange, + #[error("Invalid line range was given, line range is expected to be a 0-based inclusive range in the format ','")] + 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 index 76e8f68e4fe..0794d58171d 100644 --- 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::>(); 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 index 02e7d89ebd1..a287765ef7d 100644 --- 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 index d60b723e598..a01c0448810 100644 --- 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>, +pub enum BlameRanges { + /// Blame the entire file. + #[default] + WholeFile, + /// Blame ranges in 0-based exclusive format. + PartialFile(Vec>), } /// 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) -> 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) -> 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>) -> 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>) -> Self { + if ranges.is_empty() { + return Self::WholeFile; + } + + let zero_based_ranges = ranges + .into_iter() + .map(Self::inclusive_to_zero_based_exclusive) + .collect::>(); + 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) -> Range { + 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) { - self.merge_range(new_range); + pub fn add_range(&mut self, new_range: RangeInclusive) -> 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) { - // 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) -> 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>, 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`. + pub fn to_ranges(&self, max_lines: u32) -> Vec> { + 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() } } @@ -144,7 +155,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, } @@ -334,6 +345,17 @@ pub struct UnblamedHunk { } impl UnblamedHunk { + /// Create a new instance + pub fn new(range: Range, 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 index 99e5105f6df..e4061bcfc1e 100644 --- a/gix-blame/tests/blame.rs +++ b/gix-blame/tests/blame.rs @@ -194,7 +194,7 @@ macro_rules! mktest { format!("{}.txt", $case).as_str().into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: BlameRanges::default(), + ranges: BlameRanges::default(), since: None, }, )? @@ -265,7 +265,7 @@ fn diff_disparity() { format!("{case}.txt").as_str().into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: BlameRanges::default(), + ranges: BlameRanges::default(), since: None, }, ) @@ -297,7 +297,7 @@ fn since() { "simple.txt".into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: BlameRanges::default(), + ranges: BlameRanges::default(), since: Some(gix_date::parse("2025-01-31", None).unwrap()), }, ) @@ -332,7 +332,7 @@ mod blame_ranges { "simple.txt".into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: 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, @@ -368,7 +369,7 @@ mod blame_ranges { "simple.txt".into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: ranges, + ranges, since: None, }, ) @@ -384,14 +385,14 @@ mod blame_ranges { } #[test] - fn multiple_ranges_usingfrom_ranges() { + fn multiple_ranges_using_from_ranges() { let Fixture { odb, mut resource_cache, 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, @@ -401,7 +402,7 @@ mod blame_ranges { "simple.txt".into(), gix_blame::Options { diff_algorithm: gix_diff::blob::Algorithm::Histogram, - range: ranges, + ranges, since: None, }, ) diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 894b4e2031a..cc4f9cc3d31 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -1578,7 +1578,7 @@ pub fn main() -> Result<()> { &file, gix::blame::Options { diff_algorithm, - range: gix::blame::BlameRanges::from_ranges(ranges), + ranges: gix::blame::BlameRanges::from_one_based_inclusive_ranges(ranges), since, }, out,