From 9390c8abed102bed3d6ca8bfb02f2c84cdee7860 Mon Sep 17 00:00:00 2001 From: Ivan Zuzak Date: Sun, 6 Sep 2026 09:48:43 +0200 Subject: [PATCH 1/3] fix(gix-blame): reject reversed one-based line ranges. `BlameRanges::from_one_based_inclusive_range()`, `BlameRanges::from_one_based_inclusive_ranges()` and `BlameRanges::add_one_based_inclusive_range()` only rejected ranges starting at `0`, and silently turned a reversed range like `2..=1` into the empty 0-based range `1..1`. Empty ranges are not representable further down: `gix_blame::file()` eventually builds a `BlameEntry` whose `len` is a `NonZeroU32`, so an empty hunk trips `expect("BUG: hunks are never empty")` in `force_non_zero()`. This commit makes it impossible to construct a `BlameRanges` value with empty internal ranges using the methods mentioned above. Directly constructed `BlameRanges::PartialFile` values can still hold empty ranges and that gap is addressed in a separate commit. This matches the `gix blame -L ,` CLI command which already rejects reversed ranges. The `git blame` CLI instead swaps `` and `` if they were provided in reverse order, but its own test suite labels that as undocumented behaviour, and `git` does reject other malformed line numbers (`git blame -L 0,3` fails with `fatal: -L invalid line number: 0`), so rejecting is the more predictable choice for a library. --- gix-blame/src/file/tests.rs | 38 +++++++++++++++++++++++++ gix-blame/src/types.rs | 56 +++++++++++++++++++++++++++---------- 2 files changed, 80 insertions(+), 14 deletions(-) diff --git a/gix-blame/src/file/tests.rs b/gix-blame/src/file/tests.rs index 2b60af1d4a4..79345dc2f50 100644 --- a/gix-blame/src/file/tests.rs +++ b/gix-blame/src/file/tests.rs @@ -995,6 +995,44 @@ mod blame_ranges { assert!(matches!(ranges, Err(Error::InvalidOneBasedLineRange))); } + #[test] + #[expect(clippy::reversed_empty_ranges)] + fn constructors_reject_reversed_ranges() { + assert!( + matches!( + BlameRanges::from_one_based_inclusive_range(2..=1), + Err(Error::InvalidOneBasedLineRange) + ), + "a reversed range cannot be turned into a non-empty 0-based range, so it must be rejected" + ); + assert!( + matches!( + BlameRanges::from_one_based_inclusive_ranges(vec![1..=2, 4..=3]), + Err(Error::InvalidOneBasedLineRange) + ), + "a single reversed range invalidates the whole set, even if other ranges are fine" + ); + } + + #[test] + #[expect(clippy::reversed_empty_ranges)] + fn adding_a_reversed_range_is_rejected_and_leaves_the_selection_untouched() { + let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=3).expect("valid range"); + + assert!( + matches!( + ranges.add_one_based_inclusive_range(5..=4), + Err(Error::InvalidOneBasedLineRange) + ), + "adding a reversed range fails just like constructing from one" + ); + assert_eq!( + ranges.to_zero_based_exclusive_ranges(100), + vec![0..3], + "a rejected range must not be merged into the existing selection" + ); + } + #[test] fn create_from_single_range() { let ranges = BlameRanges::from_one_based_inclusive_range(20..=40).unwrap(); diff --git a/gix-blame/src/types.rs b/gix-blame/src/types.rs index c82cf6d103c..71ec79505c3 100644 --- a/gix-blame/src/types.rs +++ b/gix-blame/src/types.rs @@ -24,11 +24,10 @@ use crate::file::function::tokens_for_diffing; /// let range = BlameRanges::from_one_based_inclusive_range(20..=40); /// /// // Blame multiple ranges -/// let mut ranges = BlameRanges::from_one_based_inclusive_ranges(vec![ -/// 1..=4, // Lines 1-4 +/// let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![ +/// 1..=4, // Lines 1-4 /// 10..=14, // Lines 10-14 -/// ] -/// ); +/// ]); /// ``` /// /// # Line Number Representation @@ -37,8 +36,18 @@ use crate::file::function::tokens_for_diffing; /// - A range of `20..=40` represents 21 lines, spanning from line 20 up to and including line 40 /// - This will be converted to `19..40` internally as the algorithm uses 0-based ranges that are exclusive at the end /// -/// # Empty Ranges -/// You can blame the entire file by calling `BlameRanges::default()`, or by passing an empty vector to `from_one_based_inclusive_ranges`. +/// Ranges are always non-empty, so `` may never exceed ``. This mirrors the `gix blame -L ,` +/// command-line interface, but differs from `git blame -L ,` which silently swaps reversed ranges. +/// That swapping is [explicitly documented as undocumented behaviour][swap] in `git`'s own test suite, so we +/// prefer to reject what we cannot unambiguously interpret. +/// +/// # Blaming the Whole File +/// +/// You can blame the entire file by calling [`BlameRanges::default()`], or by passing an empty vector to +/// [`BlameRanges::from_one_based_inclusive_ranges()`]. Note that this is about an empty collection of ranges; +/// an individual range may never be empty. +/// +/// [swap]: https://github.com/git/git/blob/3cb9185f65410273787f74333cc027d2ea5daada/t/annotate-tests.sh#L271-L273 #[derive(Debug, Clone, Default)] pub enum BlameRanges { /// Blame the entire file. @@ -50,21 +59,31 @@ pub enum BlameRanges { /// Lifecycle impl BlameRanges { - /// Create from a single 0-based range. + /// Create from a single 1-based inclusive range. /// /// Note that the input range is 1-based inclusive, as used by git, and - /// the output is a zero-based `BlameRanges` instance. + /// the output is a 0-based exclusive `BlameRanges` instance. + /// + /// # Errors + /// + /// Returns [`Error::InvalidOneBasedLineRange`] if `range` starts at `0`, or if it is reversed, + /// i.e. if its start exceeds its end. pub fn from_one_based_inclusive_range(range: RangeInclusive) -> Result { let zero_based_range = Self::inclusive_to_zero_based_exclusive(range)?; Ok(Self::PartialFile(vec![zero_based_range])) } - /// Create from multiple 0-based ranges. + /// Create from multiple 1-based inclusive ranges. /// /// Note that the input ranges are 1-based inclusive, as used by git, and - /// the output is a zero-based `BlameRanges` instance. + /// the output is a 0-based exclusive `BlameRanges` instance. /// /// If the input vector is empty, the result will be `WholeFile`. + /// + /// # Errors + /// + /// Returns [`Error::InvalidOneBasedLineRange`] if any range starts at `0`, or if any range is + /// reversed, i.e. if its start exceeds its end. pub fn from_one_based_inclusive_ranges(ranges: Vec>) -> Result { if ranges.is_empty() { return Ok(Self::WholeFile); @@ -82,13 +101,16 @@ impl BlameRanges { } /// Convert a 1-based inclusive range to a 0-based exclusive range. + /// + /// Reversed ranges are rejected rather than turned into empty 0-based ranges, as the blame + /// algorithm cannot represent a hunk without lines. fn inclusive_to_zero_based_exclusive(range: RangeInclusive) -> Result, Error> { - if range.start() == &0 { + let (start, end) = (*range.start(), *range.end()); + // Not `RangeInclusive::is_empty()`, which is also `true` for a range iterated to exhaustion. + if start == 0 || start > end { return Err(Error::InvalidOneBasedLineRange); } - let start = range.start() - 1; - let end = *range.end(); - Ok(start..end) + Ok(start - 1..end) } } @@ -96,6 +118,12 @@ impl BlameRanges { /// Add a single range to blame. /// /// The new range will be merged with any overlapping existing ranges. + /// + /// # Errors + /// + /// Returns [`Error::InvalidOneBasedLineRange`] if `new_range` starts at `0`, or if it is + /// reversed, i.e. if its start exceeds its end. The existing selection is left untouched in + /// that case. pub fn add_one_based_inclusive_range(&mut self, new_range: RangeInclusive) -> Result<(), Error> { let zero_based_range = Self::inclusive_to_zero_based_exclusive(new_range)?; self.merge_zero_based_exclusive_range(zero_based_range); From fd1aad388c44bd36b116e285061c4ca9eed4b3fc Mon Sep 17 00:00:00 2001 From: Ivan Zuzak Date: Sun, 6 Sep 2026 09:54:53 +0200 Subject: [PATCH 2/3] change(gix-blame)!: make invalid `BlameRanges` unrepresentable. `BlameRanges` was a public enum whose `PartialFile(Vec>)` variant could be constructed and mutated directly, bypassing the constructors that establish its invariants. It is now an opaque newtype over a private `Selection` enum, so every value is built by `merge_zero_based_exclusive_range()` and is valid by construction. The constructors already validated and normalized their input, but nothing stopped a caller from writing the invalid state out by hand, and every such state misbehaved differently: * `PartialFile(vec![2..1])` panics with `BUG: hunks are never empty`, * `PartialFile(vec![0..3, 1..4])` succeeds, blaming lines 2 and 3 twice, * `PartialFile(vec![0..2, 0..2])` succeeds, duplicating every entry, * `PartialFile(vec![])` succeeds and blames nothing, while `from_one_based_inclusive_ranges(vec![])` blames everything. Validating at the point of use would only catch the cases we remember to check, in the call sites we remember to check them in. Making the state unrepresentable retires all four at once, and means no new public error variant is needed to describe a state that can no longer exist. `BlameRanges::WholeFile` and `BlameRanges::PartialFile` are no longer public. Callers can use the following migration paths: * to select the whole file, use `BlameRanges::default()`, * to select ranges, use the existing `from_one_based_inclusive_range()`, `from_one_based_inclusive_ranges()` and `add_one_based_inclusive_range()` constructors, * to inspect a selection, use the new `is_whole_file()` and `selected_ranges()` accessors, * callers holding 0-based exclusive ranges can convert each `start..end` to the 1-based inclusive `(start + 1)..=end`, which is exact for every non-empty range. `to_zero_based_exclusive_ranges()` changes from `pub` to `pub(crate)` in the same move. It resolves the selection against a particular file's line count, rather than reporting the selection as stored. Callers can compute that count from the same content and line-counting rules, but `file()` already does so internally. `is_whole_file()` and `selected_ranges()` provide inspection without requiring a line count. Making this method `pub(crate)` is an API-surface choice, not a requirement for ensuring the stored-range invariants -- it avoids maintaining a separate public API for file-specific resolution. --- gix-blame/src/file/tests.rs | 38 +++++++++++++++++- gix-blame/src/types.rs | 80 ++++++++++++++++++++++++++----------- 2 files changed, 92 insertions(+), 26 deletions(-) diff --git a/gix-blame/src/file/tests.rs b/gix-blame/src/file/tests.rs index 79345dc2f50..701f5395816 100644 --- a/gix-blame/src/file/tests.rs +++ b/gix-blame/src/file/tests.rs @@ -1103,7 +1103,7 @@ mod blame_ranges { #[test] fn convert_full_file_to_zero_based() { - let ranges = BlameRanges::WholeFile; + let ranges = BlameRanges::default(); assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..100]); } @@ -1145,6 +1145,40 @@ mod blame_ranges { fn default_is_full_file() { let ranges = BlameRanges::default(); - assert!(matches!(ranges, BlameRanges::WholeFile)); + assert!( + ranges.is_whole_file(), + "not selecting anything in particular blames the whole file" + ); + assert_eq!( + ranges.selected_ranges(), + None, + "there are no individual ranges to inspect when the whole file is selected" + ); + } + + #[test] + fn selected_ranges_are_non_empty_sorted_and_disjoint() { + let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![10..=15, 1..=5, 3..=4, 6..=7]) + .expect("all ranges are valid"); + + assert!( + !ranges.is_whole_file(), + "selecting individual ranges is not the same as selecting the whole file" + ); + assert_eq!( + ranges.selected_ranges(), + Some([0..7, 9..15].as_slice()), + "overlapping and adjacent ranges are merged, and the result is sorted, so no line is blamed twice" + ); + } + + #[test] + fn an_empty_set_of_ranges_is_the_whole_file() { + let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![]).expect("an empty selection is valid"); + + assert!( + ranges.is_whole_file(), + "selecting no ranges at all cannot be distinguished from selecting everything" + ); } } diff --git a/gix-blame/src/types.rs b/gix-blame/src/types.rs index 71ec79505c3..a824d0d0d2c 100644 --- a/gix-blame/src/types.rs +++ b/gix-blame/src/types.rs @@ -15,6 +15,10 @@ use crate::file::function::tokens_for_diffing; /// It handles the conversion between git's 1-based inclusive ranges and the internal /// 0-based exclusive ranges used by the blame algorithm. /// +/// Values of this type are always valid: they either select the whole file, or a non-empty set of +/// non-empty, sorted and pairwise disjoint ranges. Construction therefore goes through the +/// constructors below, which validate and normalize their input. +/// /// # Examples /// /// ```rust @@ -47,13 +51,26 @@ use crate::file::function::tokens_for_diffing; /// [`BlameRanges::from_one_based_inclusive_ranges()`]. Note that this is about an empty collection of ranges; /// an individual range may never be empty. /// +/// If you already hold 0-based exclusive ranges, convert each `start..end` to the 1-based inclusive +/// `(start + 1)..=end` before passing it in. That conversion is exact for every non-empty range. +/// /// [swap]: https://github.com/git/git/blob/3cb9185f65410273787f74333cc027d2ea5daada/t/annotate-tests.sh#L271-L273 #[derive(Debug, Clone, Default)] -pub enum BlameRanges { +pub struct BlameRanges(Selection); + +#[derive(Debug, Clone, Default)] +enum Selection { /// Blame the entire file. #[default] WholeFile, - /// Blame ranges in 0-based exclusive format. + /// Blame the given ranges, in 0-based exclusive format. + /// + /// Upheld invariants, all established by [`BlameRanges::merge_zero_based_exclusive_range()`]: + /// + /// * the `Vec` is never empty - that state is spelled [`Selection::WholeFile`], + /// * every range is non-empty, so it can become a [`BlameEntry`] with a [`NonZeroU32`] length, + /// * the ranges are sorted by `start` and pairwise disjoint and non-adjacent, so no line is + /// ever attributed twice. PartialFile(Vec>), } @@ -69,8 +86,9 @@ impl BlameRanges { /// Returns [`Error::InvalidOneBasedLineRange`] if `range` starts at `0`, or if it is reversed, /// i.e. if its start exceeds its end. pub fn from_one_based_inclusive_range(range: RangeInclusive) -> Result { - let zero_based_range = Self::inclusive_to_zero_based_exclusive(range)?; - Ok(Self::PartialFile(vec![zero_based_range])) + let mut result = Self::default(); + result.merge_zero_based_exclusive_range(Self::inclusive_to_zero_based_exclusive(range)?); + Ok(result) } /// Create from multiple 1-based inclusive ranges. @@ -78,24 +96,16 @@ impl BlameRanges { /// Note that the input ranges are 1-based inclusive, as used by git, and /// the output is a 0-based exclusive `BlameRanges` instance. /// - /// If the input vector is empty, the result will be `WholeFile`. + /// If the input vector is empty, the result selects the whole file. /// /// # Errors /// /// Returns [`Error::InvalidOneBasedLineRange`] if any range starts at `0`, or if any range is /// reversed, i.e. if its start exceeds its end. pub fn from_one_based_inclusive_ranges(ranges: Vec>) -> Result { - if ranges.is_empty() { - return Ok(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 { - result.merge_zero_based_exclusive_range(range?); + let mut result = Self::default(); + for range in ranges { + result.merge_zero_based_exclusive_range(Self::inclusive_to_zero_based_exclusive(range)?); } Ok(result) } @@ -114,6 +124,24 @@ impl BlameRanges { } } +/// Access +impl BlameRanges { + /// Return `true` if the entire file is selected, which is the default. + pub fn is_whole_file(&self) -> bool { + matches!(self.0, Selection::WholeFile) + } + + /// Return the selected 0-based exclusive ranges, or `None` if the whole file is selected. + /// + /// The ranges are non-empty, sorted and pairwise disjoint. + pub fn selected_ranges(&self) -> Option<&[Range]> { + match &self.0 { + Selection::WholeFile => None, + Selection::PartialFile(ranges) => Some(ranges), + } + } +} + impl BlameRanges { /// Add a single range to blame. /// @@ -131,10 +159,14 @@ impl BlameRanges { Ok(()) } - /// Adds a new ranges, merging it with any existing overlapping ranges. + /// Add `new_range`, merging it with any existing overlapping or adjacent ranges. + /// + /// This is the only place that creates [`Selection::PartialFile`], and is what upholds its + /// invariants. `new_range` must be non-empty. fn merge_zero_based_exclusive_range(&mut self, new_range: Range) { - match self { - Self::PartialFile(ranges) => { + debug_assert!(!new_range.is_empty(), "BUG: an empty range must never be selected"); + match &mut self.0 { + Selection::PartialFile(ranges) => { // Partition ranges into those that don't overlap and those that do. let (mut non_overlapping, overlapping): (Vec<_>, Vec<_>) = ranges .drain(..) @@ -149,18 +181,18 @@ impl BlameRanges { *ranges = non_overlapping; ranges.sort_by_key(|a| a.start); } - Self::WholeFile => *self = Self::PartialFile(vec![new_range]), + Selection::WholeFile => self.0 = Selection::PartialFile(vec![new_range]), } } /// Gets zero-based exclusive ranges. - pub fn to_zero_based_exclusive_ranges(&self, max_lines: u32) -> Vec> { - match self { - Self::WholeFile => { + pub(crate) fn to_zero_based_exclusive_ranges(&self, max_lines: u32) -> Vec> { + match &self.0 { + Selection::WholeFile => { let full_range = 0..max_lines; vec![full_range] } - Self::PartialFile(ranges) => ranges + Selection::PartialFile(ranges) => ranges .iter() .filter_map(|range| { if range.end < max_lines { From 3dac5d63d46688a2f18e4fe4c9898f669ca72a0f Mon Sep 17 00:00:00 2001 From: Ivan Zuzak Date: Sun, 6 Sep 2026 09:49:21 +0200 Subject: [PATCH 3/3] Resolve blame ranges against a `NonZeroU32` line count `BlameRanges::to_zero_based_exclusive_ranges()` returned `vec![0..0]` for a whole-file selection when asked to resolve against a file of `0` lines. That is an empty range, which the blame algorithm cannot represent as a hunk -- the `NonZeroU32` length of a `BlameEntry` would panic on it. Rather than special-case `0` inside the method, the `max_lines` parameter is now a `NonZeroU32`, so a file without lines cannot be described in the first place. This lets the method handle every permitted input without needing a special case for zero lines and restores the invariant that a whole-file selection always resolves to exactly one non-empty range. `gix_blame::file()` never hit the bad case, but only because `initial_state()` happens to return early when the blamed content has no lines, three functions away from where the invariant is needed. That check was load-bearing by coincidence. It is now a `let ... else` on `NonZeroU32::new()`, so the compiler enforces what was previously a convention. The method became `pub(crate)` in the previous commit, so none of this reaches users -- what changes is an internal invariant, not the API. The clamping and dropping behaviour for ranges that reach past the end of the file is unchanged, and is now documented. Note that this differs from `git blame -L 100,200`, which fails with `fatal: file has only lines` where we return an empty result. --- gix-blame/src/file/function.rs | 15 ++++++---- gix-blame/src/file/tests.rs | 50 ++++++++++++++++++++++++---------- gix-blame/src/types.rs | 26 +++++++++++++++--- gix-blame/tests/blame.rs | 31 +++++++++++++++++++++ 4 files changed, 97 insertions(+), 25 deletions(-) diff --git a/gix-blame/src/file/function.rs b/gix-blame/src/file/function.rs index 047ed5a3384..17550c4570c 100644 --- a/gix-blame/src/file/function.rs +++ b/gix-blame/src/file/function.rs @@ -926,17 +926,18 @@ fn initial_state( commit_id: suspect, })?; let blamed_file_blob = odb.find_blob(&blamed_file_entry_id, buf)?.data.to_vec(); - let num_lines_in_blamed = tokens_for_diffing(&blamed_file_blob).tokenize().count() as u32; // Binary or otherwise empty? - if num_lines_in_blamed == 0 { + let Some(num_lines_in_blamed) = + NonZeroU32::new(tokens_for_diffing(&blamed_file_blob).tokenize().count() as u32) + else { return Ok(InitialState { blamed_file_blob, hunks_to_blame: Vec::new(), out: Vec::new(), first_suspect: None, }); - } + }; let ranges_to_blame = options.ranges.to_zero_based_exclusive_ranges(num_lines_in_blamed); let hunks_to_blame = ranges_to_blame @@ -957,16 +958,18 @@ fn initial_state( } => { let null_id = first_suspect.kind().null(); let blamed_file_blob = contents.into_owned(); - let num_lines_in_blamed = tokens_for_diffing(&blamed_file_blob).tokenize().count() as u32; - if num_lines_in_blamed == 0 { + // Binary or otherwise empty? + let Some(num_lines_in_blamed) = + NonZeroU32::new(tokens_for_diffing(&blamed_file_blob).tokenize().count() as u32) + else { return Ok(InitialState { blamed_file_blob, hunks_to_blame: Vec::new(), out: Vec::new(), first_suspect: None, }); - } + }; let ranges_to_blame = options.ranges.to_zero_based_exclusive_ranges(num_lines_in_blamed); let mut hunks_to_blame = ranges_to_blame diff --git a/gix-blame/src/file/tests.rs b/gix-blame/src/file/tests.rs index 701f5395816..c4465264da9 100644 --- a/gix-blame/src/file/tests.rs +++ b/gix-blame/src/file/tests.rs @@ -986,8 +986,17 @@ mod process_changes { } mod blame_ranges { + use std::num::NonZeroU32; + use crate::{BlameRanges, Error}; + /// A file of `n` lines, for resolving a selection against. + /// + /// A file without lines cannot be expressed, which is the point: it has nothing to blame. + fn num_lines(n: u32) -> NonZeroU32 { + NonZeroU32::new(n).expect("these tests never describe a file without lines") + } + #[test] fn create_with_invalid_range() { let ranges = BlameRanges::from_one_based_inclusive_range(0..=10); @@ -1027,7 +1036,7 @@ mod blame_ranges { "adding a reversed range fails just like constructing from one" ); assert_eq!( - ranges.to_zero_based_exclusive_ranges(100), + ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..3], "a rejected range must not be merged into the existing selection" ); @@ -1037,21 +1046,21 @@ mod blame_ranges { fn create_from_single_range() { let ranges = BlameRanges::from_one_based_inclusive_range(20..=40).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![19..40]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![19..40]); } #[test] fn create_from_multiple_ranges() { let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=4, 10..=14]).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..4, 9..14]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..4, 9..14]); } #[test] fn create_with_empty_ranges() { let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![]).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..100]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..100]); } #[test] @@ -1059,7 +1068,7 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap(); ranges.add_one_based_inclusive_range(3..=7).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..7]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..7]); } #[test] @@ -1068,7 +1077,7 @@ mod blame_ranges { ranges.add_one_based_inclusive_range(5..=7).unwrap(); ranges.add_one_based_inclusive_range(2..=6).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..7]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..7]); } #[test] @@ -1076,7 +1085,7 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(5..=7).unwrap(); ranges.add_one_based_inclusive_range(1..=3).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..3, 4..7]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..3, 4..7]); } #[test] @@ -1084,28 +1093,39 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap(); ranges.add_one_based_inclusive_range(6..=10).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..10]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..10]); } #[test] fn non_sorted_ranges() { let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![10..=15, 1..=5]).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..5, 9..15]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..5, 9..15]); } #[test] fn convert_to_zero_based_exclusive() { let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=5, 10..=15]).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..5, 9..15]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..5, 9..15]); } #[test] fn convert_full_file_to_zero_based() { let ranges = BlameRanges::default(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..100]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..100]); + } + + #[test] + fn the_whole_file_always_resolves_to_exactly_one_non_empty_range() { + for n in [1, 2, 100] { + assert_eq!( + BlameRanges::default().to_zero_based_exclusive_ranges(num_lines(n)), + vec![0..n], + "selecting the whole file always yields exactly one range covering all {n} lines" + ); + } } #[test] @@ -1114,7 +1134,7 @@ mod blame_ranges { ranges.add_one_based_inclusive_range(1..=10).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(100), vec![0..10]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(100)), vec![0..10]); } #[test] @@ -1122,7 +1142,7 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap(); ranges.add_one_based_inclusive_range(16..=20).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(7), vec![0..5]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(7)), vec![0..5]); } #[test] @@ -1130,7 +1150,7 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap(); ranges.add_one_based_inclusive_range(6..=10).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(7), vec![0..7]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(7)), vec![0..7]); } #[test] @@ -1138,7 +1158,7 @@ mod blame_ranges { let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=4).unwrap(); ranges.add_one_based_inclusive_range(6..=10).unwrap(); - assert_eq!(ranges.to_zero_based_exclusive_ranges(7), vec![0..4, 5..7]); + assert_eq!(ranges.to_zero_based_exclusive_ranges(num_lines(7)), vec![0..4, 5..7]); } #[test] diff --git a/gix-blame/src/types.rs b/gix-blame/src/types.rs index a824d0d0d2c..86872aad354 100644 --- a/gix-blame/src/types.rs +++ b/gix-blame/src/types.rs @@ -185,10 +185,23 @@ impl BlameRanges { } } - /// Gets zero-based exclusive ranges. - pub(crate) fn to_zero_based_exclusive_ranges(&self, max_lines: u32) -> Vec> { - match &self.0 { + /// Resolves the selection into 0-based exclusive ranges while taking into account `max_lines` - + /// the number of lines in the content that will be blamed. + /// + /// Ranges that reach past `max_lines` are clamped to it, and ranges that start past `max_lines` + /// are dropped. Consequently the result can be empty, if every selected range starts past + /// `max_lines`, but every range it does contain is guaranteed to be non-empty. + /// [`file()`](crate::file()) relies on that guarantee, as a hunk without lines cannot be turned + /// into a [`BlameEntry`]. + /// + /// Note that a file without lines cannot be expressed here, as `max_lines` is a [`NonZeroU32`]. + /// Such a file has nothing to attribute, and [`file()`](crate::file()) recognizes it before + /// any range is resolved. + pub(crate) fn to_zero_based_exclusive_ranges(&self, max_lines: NonZeroU32) -> Vec> { + let max_lines = max_lines.get(); + let ranges = match &self.0 { Selection::WholeFile => { + // Kept as a binding to avoid `clippy::single_range_in_vec_init`. let full_range = 0..max_lines; vec![full_range] } @@ -206,7 +219,12 @@ impl BlameRanges { } }) .collect(), - } + }; + debug_assert!( + ranges.iter().all(|range| !range.is_empty()), + "BUG: resolved ranges must never be empty, or creating a `BlameEntry` from them will panic" + ); + ranges } } diff --git a/gix-blame/tests/blame.rs b/gix-blame/tests/blame.rs index 99b14c9c34d..a1077583204 100644 --- a/gix-blame/tests/blame.rs +++ b/gix-blame/tests/blame.rs @@ -683,6 +683,37 @@ mod untracked_changes { Ok(()) } + // An empty buffer is the only way to express content without lines: a lone newline tokenizes to + // one line, as does newline-free binary content. Blaming it as untracked changes is what lets us + // exercise a line count of zero, which the internal range resolution no longer accepts. + #[test] + fn a_file_without_lines_has_nothing_to_blame() -> gix_testtools::Result { + let worktree_path = gix_testtools::scripted_fixture_read_only("make_blame_repo.sh")?; + + let mut fixture = Fixture::for_worktree_path(worktree_path.to_path_buf())?; + let lines_blamed = fixture + .blame_untracked_changes( + "untracked-lines.txt".into(), + Vec::new(), + gix_blame::Options { + diff_algorithm: gix_diff::blob::Algorithm::Histogram, + ranges: BlameRanges::default(), + since: None, + rewrites: Some(gix_diff::Rewrites::default()), + debug_track_path: false, + }, + )? + .entries; + + assert!( + lines_blamed.is_empty(), + "content without lines cannot be attributed, so blame stops before a line count is ever \ + needed to resolve `BlameRanges` into ranges to blame" + ); + + Ok(()) + } + #[test] fn untracked_file() -> gix_testtools::Result { let worktree_path = gix_testtools::scripted_fixture_read_only("make_blame_repo.sh")?;