Skip to content

Commit e1db1b7

Browse files
committed
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.
1 parent 4661a7b commit e1db1b7

File tree

6 files changed

+199
-85
lines changed

6 files changed

+199
-85
lines changed

gix-blame/src/error.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,9 @@ pub enum Error {
2828
#[error(transparent)]
2929
DiffTree(#[from] gix_diff::tree::Error),
3030
#[error("Invalid line range was given, line range is expected to be a 1-based inclusive range in the format '<start>,<end>'")]
31-
InvalidLineRange,
31+
InvalidOneBasedLineRange,
32+
#[error("Invalid line range was given, line range is expected to be a 0-based inclusive range in the format '<start>,<end>'")]
33+
InvalidZeroBasedLineRange,
3234
#[error("Failure to decode commit during traversal")]
3335
DecodeCommit(#[from] gix_object::decode::Error),
3436
#[error("Failed to get parent from commitgraph during traversal")]

gix-blame/src/file/function.rs

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,11 @@ pub fn file(
9494
return Ok(Outcome::default());
9595
}
9696

97-
let ranges = options.range.to_zero_based_exclusive(num_lines_in_blamed)?;
98-
let mut hunks_to_blame = Vec::with_capacity(ranges.len());
99-
100-
for range in ranges {
101-
hunks_to_blame.push(UnblamedHunk {
102-
range_in_blamed_file: range.clone(),
103-
suspects: [(suspect, range)].into(),
104-
});
105-
}
97+
let ranges_to_blame = options.ranges.to_ranges(num_lines_in_blamed);
98+
let mut hunks_to_blame = ranges_to_blame
99+
.into_iter()
100+
.map(|range| UnblamedHunk::new(range, suspect))
101+
.collect::<Vec<_>>();
106102

107103
let (mut buf, mut buf2) = (Vec::new(), Vec::new());
108104
let commit = find_commit(cache.as_ref(), &odb, &suspect, &mut buf)?;

gix-blame/src/file/tests.rs

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1338,3 +1338,100 @@ mod process_changes {
13381338
);
13391339
}
13401340
}
1341+
1342+
mod blame_ranges {
1343+
use crate::BlameRanges;
1344+
1345+
#[test]
1346+
fn create_from_single_range() {
1347+
let range = BlameRanges::from_one_based_inclusive_range(20..=40);
1348+
match range {
1349+
BlameRanges::PartialFile(ranges) => {
1350+
assert_eq!(ranges.len(), 1);
1351+
assert_eq!(ranges[0], 19..40);
1352+
}
1353+
_ => panic!("Expected PartialFile variant"),
1354+
}
1355+
}
1356+
1357+
#[test]
1358+
fn create_from_multiple_ranges() {
1359+
let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=4, 10..=14]);
1360+
match ranges {
1361+
BlameRanges::PartialFile(ranges) => {
1362+
assert_eq!(ranges.len(), 2);
1363+
assert_eq!(ranges[0], 0..4);
1364+
assert_eq!(ranges[1], 9..14);
1365+
}
1366+
_ => panic!("Expected PartialFile variant"),
1367+
}
1368+
}
1369+
1370+
#[test]
1371+
fn add_range_merges_overlapping() {
1372+
let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
1373+
ranges.add_range(3..=7).unwrap();
1374+
1375+
match ranges {
1376+
BlameRanges::PartialFile(ranges) => {
1377+
assert_eq!(ranges.len(), 1);
1378+
assert_eq!(ranges[0], 0..7);
1379+
}
1380+
_ => panic!("Expected PartialFile variant"),
1381+
}
1382+
}
1383+
1384+
#[test]
1385+
fn add_range_merges_adjacent() {
1386+
let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
1387+
ranges.add_range(6..=10).unwrap();
1388+
1389+
match ranges {
1390+
BlameRanges::PartialFile(ranges) => {
1391+
assert_eq!(ranges.len(), 1);
1392+
assert_eq!(ranges[0], 0..10);
1393+
}
1394+
_ => panic!("Expected PartialFile variant"),
1395+
}
1396+
}
1397+
1398+
#[test]
1399+
fn add_range_keeps_separate() {
1400+
let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5);
1401+
ranges.add_range(6..=10).unwrap();
1402+
1403+
match ranges {
1404+
BlameRanges::PartialFile(ranges) => {
1405+
assert_eq!(ranges.len(), 1);
1406+
assert_eq!(ranges[0], 0..10);
1407+
}
1408+
_ => panic!("Expected PartialFile variant"),
1409+
}
1410+
}
1411+
1412+
#[test]
1413+
fn convert_to_zero_based_exclusive() {
1414+
let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=5, 10..=15]);
1415+
let ranges = ranges.to_ranges(20);
1416+
assert_eq!(ranges.len(), 2);
1417+
assert_eq!(ranges[0], 0..5);
1418+
assert_eq!(ranges[1], 9..15);
1419+
}
1420+
1421+
#[test]
1422+
fn convert_full_file_to_zero_based() {
1423+
let ranges = BlameRanges::WholeFile;
1424+
let ranges = ranges.to_ranges(100);
1425+
assert_eq!(ranges.len(), 1);
1426+
assert_eq!(ranges[0], 0..100);
1427+
}
1428+
1429+
#[test]
1430+
fn default_is_full_file() {
1431+
let ranges = BlameRanges::default();
1432+
match ranges {
1433+
BlameRanges::WholeFile => (),
1434+
_ => panic!("Expected WholeFile variant"),
1435+
}
1436+
}
1437+
}

gix-blame/src/types.rs

Lines changed: 86 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -21,12 +21,14 @@ use crate::Error;
2121
/// use gix_blame::BlameRanges;
2222
///
2323
/// // Blame lines 20 through 40 (inclusive)
24-
/// let range = BlameRanges::from_range(20..=40);
24+
/// let range = BlameRanges::from_one_based_inclusive_range(20..=40);
2525
///
2626
/// // Blame multiple ranges
27-
/// let mut ranges = BlameRanges::new();
28-
/// ranges.add_range(1..=4); // Lines 1-4
29-
/// ranges.add_range(10..=14); // Lines 10-14
27+
/// let mut ranges = BlameRanges::from_one_based_inclusive_ranges(vec![
28+
/// 1..=4, // Lines 1-4
29+
/// 10..=14, // Lines 10-14
30+
/// ]
31+
/// );
3032
/// ```
3133
///
3234
/// # Line Number Representation
@@ -40,39 +42,53 @@ use crate::Error;
4042
/// An empty `BlameRanges` (created via `BlameRanges::new()` or `BlameRanges::default()`) means
4143
/// to blame the entire file, similar to running `git blame` without line number arguments.
4244
#[derive(Debug, Clone, Default)]
43-
pub struct BlameRanges {
44-
/// The ranges to blame, stored as 1-based inclusive ranges
45-
/// An empty Vec means blame the entire file
46-
ranges: Vec<RangeInclusive<u32>>,
45+
pub enum BlameRanges {
46+
/// Blame the entire file.
47+
#[default]
48+
WholeFile,
49+
/// Blame ranges in 0-based exclusive format.
50+
PartialFile(Vec<Range<u32>>),
4751
}
4852

4953
/// Lifecycle
5054
impl BlameRanges {
51-
/// Create a new empty BlameRanges instance.
52-
///
53-
/// An empty instance means to blame the entire file.
54-
pub fn new() -> Self {
55-
Self::default()
56-
}
57-
5855
/// Create from a single range.
5956
///
60-
/// The range is 1-based, similar to git's line number format.
61-
pub fn from_range(range: RangeInclusive<u32>) -> Self {
62-
Self { ranges: vec![range] }
57+
/// Note that the input range is 1-based inclusive, as used by git, and
58+
/// the output is zero-based `BlameRanges` instance.
59+
///
60+
/// @param range: A 1-based inclusive range.
61+
/// @return: A `BlameRanges` instance representing the range.
62+
pub fn from_one_based_inclusive_range(range: RangeInclusive<u32>) -> Self {
63+
let zero_based_range = Self::inclusive_to_zero_based_exclusive(range);
64+
Self::PartialFile(vec![zero_based_range])
6365
}
6466

6567
/// Create from multiple ranges.
6668
///
67-
/// All ranges are 1-based.
68-
/// Overlapping or adjacent ranges will be merged.
69-
pub fn from_ranges(ranges: Vec<RangeInclusive<u32>>) -> Self {
70-
let mut result = Self::new();
71-
for range in ranges {
72-
result.merge_range(range);
69+
/// Note that the input ranges are 1-based inclusive, as used by git, and
70+
/// the output is zero-based `BlameRanges` instance.
71+
///
72+
/// @param ranges: A vec of 1-based inclusive range.
73+
/// @return: A `BlameRanges` instance representing the range.
74+
pub fn from_one_based_inclusive_ranges(ranges: Vec<RangeInclusive<u32>>) -> Self {
75+
let zero_based_ranges = ranges
76+
.into_iter()
77+
.map(Self::inclusive_to_zero_based_exclusive)
78+
.collect::<Vec<_>>();
79+
let mut result = Self::PartialFile(vec![]);
80+
for range in zero_based_ranges {
81+
let _ = result.merge_range(range);
7382
}
7483
result
7584
}
85+
86+
/// Convert a 1-based inclusive range to a 0-based exclusive range.
87+
fn inclusive_to_zero_based_exclusive(range: RangeInclusive<u32>) -> Range<u32> {
88+
let start = range.start() - 1;
89+
let end = *range.end();
90+
start..end
91+
}
7692
}
7793

7894
impl BlameRanges {
@@ -81,60 +97,51 @@ impl BlameRanges {
8197
/// The range should be 1-based inclusive.
8298
/// If the new range overlaps with or is adjacent to an existing range,
8399
/// they will be merged into a single range.
84-
pub fn add_range(&mut self, new_range: RangeInclusive<u32>) {
85-
self.merge_range(new_range);
100+
pub fn add_range(&mut self, new_range: RangeInclusive<u32>) -> Result<(), Error> {
101+
match self {
102+
Self::PartialFile(_) => {
103+
let zero_based_range = Self::inclusive_to_zero_based_exclusive(new_range);
104+
self.merge_range(zero_based_range)
105+
}
106+
_ => Err(Error::InvalidOneBasedLineRange),
107+
}
86108
}
87109

88110
/// Attempts to merge the new range with any existing ranges.
89111
/// If no merge is possible, add it as a new range.
90-
fn merge_range(&mut self, new_range: RangeInclusive<u32>) {
91-
// Check if this range can be merged with any existing range
92-
for range in &mut self.ranges {
93-
// Check if ranges overlap or are adjacent
94-
if new_range.start() <= range.end() && range.start() <= new_range.end() {
95-
*range = *range.start().min(new_range.start())..=*range.end().max(new_range.end());
96-
return;
112+
fn merge_range(&mut self, new_range: Range<u32>) -> Result<(), Error> {
113+
match self {
114+
Self::PartialFile(ref mut ranges) => {
115+
// Check if this range can be merged with any existing range
116+
for range in &mut *ranges {
117+
// Check if ranges overlap
118+
if new_range.start <= range.end && range.start <= new_range.end {
119+
*range = range.start.min(new_range.start)..range.end.max(new_range.end);
120+
return Ok(());
121+
}
122+
// Check if ranges are adjacent
123+
if new_range.start == range.end || range.start == new_range.end {
124+
*range = range.start.min(new_range.start)..range.end.max(new_range.end);
125+
return Ok(());
126+
}
127+
}
128+
// If no overlap or adjacency found, add it as a new range
129+
ranges.push(new_range);
130+
Ok(())
97131
}
132+
_ => Err(Error::InvalidOneBasedLineRange),
98133
}
99-
// If no overlap found, add it as a new range
100-
self.ranges.push(new_range);
101134
}
102135

103-
/// Convert the 1-based inclusive ranges to 0-based exclusive ranges.
104-
///
105-
/// This is used internally by the blame algorithm to convert from git's line number format
106-
/// to the internal format used for processing.
107-
///
108-
/// # Errors
109-
///
110-
/// Returns `Error::InvalidLineRange` if:
111-
/// - Any range starts at 0 (must be 1-based)
112-
/// - Any range extends beyond the file's length
113-
/// - Any range has the same start and end
114-
pub fn to_zero_based_exclusive(&self, max_lines: u32) -> Result<Vec<Range<u32>>, Error> {
115-
if self.ranges.is_empty() {
116-
let range = 0..max_lines;
117-
return Ok(vec![range]);
118-
}
119-
120-
let mut result = Vec::with_capacity(self.ranges.len());
121-
for range in &self.ranges {
122-
if *range.start() == 0 {
123-
return Err(Error::InvalidLineRange);
124-
}
125-
let start = range.start() - 1;
126-
let end = *range.end();
127-
if start >= max_lines || end > max_lines || start == end {
128-
return Err(Error::InvalidLineRange);
136+
/// Convert the ranges to a vector of `Range<u32>`.
137+
pub fn to_ranges(&self, max_lines: u32) -> Vec<Range<u32>> {
138+
match self {
139+
Self::WholeFile => {
140+
let full_range = 0..max_lines;
141+
vec![full_range]
129142
}
130-
result.push(start..end);
143+
Self::PartialFile(ranges) => ranges.clone(),
131144
}
132-
Ok(result)
133-
}
134-
135-
/// Returns true if no specific ranges are set (meaning blame entire file)
136-
pub fn is_empty(&self) -> bool {
137-
self.ranges.is_empty()
138145
}
139146
}
140147

@@ -334,6 +341,17 @@ pub struct UnblamedHunk {
334341
}
335342

336343
impl UnblamedHunk {
344+
/// Create a new instance
345+
pub fn new(range: Range<u32>, suspect: ObjectId) -> Self {
346+
let range_start = range.start;
347+
let range_end = range.end;
348+
349+
UnblamedHunk {
350+
range_in_blamed_file: range_start..range_end,
351+
suspects: [(suspect, range_start..range_end)].into(),
352+
}
353+
}
354+
337355
pub(crate) fn has_suspect(&self, suspect: &ObjectId) -> bool {
338356
self.suspects.iter().any(|entry| entry.0 == *suspect)
339357
}

gix-blame/tests/blame.rs

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,7 @@ mod blame_ranges {
332332
"simple.txt".into(),
333333
gix_blame::Options {
334334
diff_algorithm: gix_diff::blob::Algorithm::Histogram,
335-
ranges: BlameRanges::from_range(1..=2),
335+
ranges: BlameRanges::from_one_based_inclusive_range(1..=2),
336336
since: None,
337337
},
338338
)
@@ -355,10 +355,11 @@ mod blame_ranges {
355355
suspect,
356356
} = Fixture::new().unwrap();
357357

358-
let mut ranges = BlameRanges::new();
359-
ranges.add_range(1..=2); // Lines 1-2
360-
ranges.add_range(1..=1); // Duplicate range, should be ignored
361-
ranges.add_range(4..=4); // Line 4
358+
let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![
359+
1..=2, // Lines 1-2
360+
1..=1, // Duplicate range, should be ignored
361+
4..=4, // Line 4
362+
]);
362363

363364
let lines_blamed = gix_blame::file(
364365
&odb,
@@ -391,7 +392,7 @@ mod blame_ranges {
391392
suspect,
392393
} = Fixture::new().unwrap();
393394

394-
let ranges = BlameRanges::from_ranges(vec![1..=2, 1..=1, 4..=4]);
395+
let ranges = BlameRanges::from_one_based_inclusive_ranges(vec![1..=2, 1..=1, 4..=4]);
395396

396397
let lines_blamed = gix_blame::file(
397398
&odb,

src/plumbing/main.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1578,7 +1578,7 @@ pub fn main() -> Result<()> {
15781578
&file,
15791579
gix::blame::Options {
15801580
diff_algorithm,
1581-
ranges: gix::blame::BlameRanges::from_ranges(ranges),
1581+
ranges: gix::blame::BlameRanges::from_one_based_inclusive_ranges(ranges),
15821582
since,
15831583
},
15841584
out,

0 commit comments

Comments
 (0)