diff --git a/Cargo.lock b/Cargo.lock index 2df79331bb8..365fb927fa5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1551,6 +1551,7 @@ dependencies = [ "gix-trace 0.1.11", "gix-traverse 0.43.1", "gix-worktree 0.38.0", + "thiserror 2.0.3", ] [[package]] diff --git a/gix-blame/Cargo.toml b/gix-blame/Cargo.toml index 039f4788bfd..fc4baf3fe48 100644 --- a/gix-blame/Cargo.toml +++ b/gix-blame/Cargo.toml @@ -21,6 +21,8 @@ gix-hash = { version = "^0.15.0", path = "../gix-hash" } gix-worktree = { version = "^0.38.0", path = "../gix-worktree", default-features = false, features = ["attributes"] } gix-traverse = { version = "^0.43.0", path = "../gix-traverse" } +thiserror = "2.0.0" + [dev-dependencies] gix-ref = { version = "^0.49.0", path = "../gix-ref" } gix-filter = { version = "^0.16.0", path = "../gix-filter" } diff --git a/gix-blame/src/error.rs b/gix-blame/src/error.rs new file mode 100644 index 00000000000..daedf0aecd7 --- /dev/null +++ b/gix-blame/src/error.rs @@ -0,0 +1,30 @@ +use gix_object::bstr::BString; + +/// The error returned by [file()](crate::file()). +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("No commit was given")] + EmptyTraversal, + #[error(transparent)] + BlobDiffSetResource(#[from] gix_diff::blob::platform::set_resource::Error), + #[error(transparent)] + BlobDiffPrepare(#[from] gix_diff::blob::platform::prepare_diff::Error), + #[error("The file to blame at '{file_path}' wasn't found in the first commit at {commit_id}")] + FileMissing { + /// The file-path to the object to blame. + file_path: BString, + /// The commit whose tree didn't contain `file_path`. + commit_id: gix_hash::ObjectId, + }, + #[error("Couldn't find commit or tree in the object database")] + FindObject(#[from] gix_object::find::Error), + #[error("Could not find existing blob or commit")] + FindExistingObject(#[from] gix_object::find::existing_object::Error), + #[error("Could not find existing iterator over a tree")] + FindExistingIter(#[from] gix_object::find::existing_iter::Error), + #[error("Failed to obtain the next commit in the commit-graph traversal")] + Traverse(#[source] Box), + #[error(transparent)] + DiffTree(#[from] gix_diff::tree::Error), +} diff --git a/gix-blame/src/file/function.rs b/gix-blame/src/file/function.rs index b869b81a726..5b3ccd7cc05 100644 --- a/gix-blame/src/file/function.rs +++ b/gix-blame/src/file/function.rs @@ -1,5 +1,5 @@ use super::{process_changes, Change, Offset, UnblamedHunk}; -use crate::{BlameEntry, Outcome, Statistics}; +use crate::{BlameEntry, Error, Outcome, Statistics}; use gix_diff::blob::intern::TokenSource; use gix_hash::ObjectId; use gix_object::{bstr::BStr, FindExt}; @@ -57,18 +57,24 @@ pub fn file( traverse: impl IntoIterator>, resource_cache: &mut gix_diff::blob::Platform, file_path: &BStr, -) -> Result { +) -> Result +where + E: Into>, +{ let mut traverse = traverse.into_iter().peekable(); let Some(Ok(suspect)) = traverse.peek().map(|res| res.as_ref().map(|item| item.id)) else { - todo!("return actual error"); + return Err(Error::EmptyTraversal); }; let _span = gix_trace::coarse!("gix_blame::file()", ?file_path, ?suspect); let mut stats = Statistics::default(); let (mut buf, mut buf2, mut buf3) = (Vec::new(), Vec::new(), Vec::new()); - let original_file_entry = - find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2, &mut stats).unwrap(); - let original_file_blob = odb.find_blob(&original_file_entry.oid, &mut buf).unwrap().data.to_vec(); + let original_file_entry = find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2, &mut stats)? + .ok_or_else(|| Error::FileMissing { + file_path: file_path.to_owned(), + commit_id: suspect, + })?; + let original_file_blob = odb.find_blob(&original_file_entry.oid, &mut buf)?.data.to_vec(); let num_lines_in_original = { let mut interner = gix_diff::blob::intern::Interner::new(original_file_blob.len() / 100); tokens_for_diffing(&original_file_blob) @@ -78,7 +84,7 @@ pub fn file( }; let mut hunks_to_blame = vec![UnblamedHunk::new( - 0..num_lines_in_original.try_into().unwrap(), + 0..num_lines_in_original as u32, suspect, Offset::Added(0), )]; @@ -86,7 +92,7 @@ pub fn file( let mut out = Vec::new(); let mut diff_state = gix_diff::tree::State::default(); 'outer: for item in traverse { - let item = item?; + let item = item.map_err(|err| Error::Traverse(err.into()))?; let suspect = item.id; stats.commits_traversed += 1; @@ -107,14 +113,14 @@ pub fn file( break; } - let Some(entry) = find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2, &mut stats) else { + let Some(entry) = find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2, &mut stats)? else { continue; }; if parent_ids.len() == 1 { let parent_id = parent_ids.pop().expect("just validated there is exactly one"); if let Some(parent_entry) = - find_path_entry_in_commit(&odb, &parent_id, file_path, &mut buf, &mut buf2, &mut stats) + find_path_entry_in_commit(&odb, &parent_id, file_path, &mut buf, &mut buf2, &mut stats)? { if entry.oid == parent_entry.oid { // The blobs storing the blamed file in `entry` and `parent_entry` are identical @@ -126,7 +132,7 @@ pub fn file( } } - let Some(modification) = tree_diff_at_file_path( + let changes_for_file_path = tree_diff_at_file_path( &odb, file_path, item.id, @@ -136,7 +142,8 @@ pub fn file( &mut buf, &mut buf2, &mut buf3, - ) else { + )?; + let Some(modification) = changes_for_file_path else { // None of the changes affected the file we’re currently blaming. Pass blame to parent. for unblamed_hunk in &mut hunks_to_blame { unblamed_hunk.pass_blame(suspect, parent_id); @@ -159,7 +166,7 @@ pub fn file( } gix_diff::tree::recorder::Change::Deletion { .. } => todo!(), gix_diff::tree::recorder::Change::Modification { previous_oid, oid, .. } => { - let changes = blob_changes(&odb, resource_cache, oid, previous_oid, file_path, &mut stats); + let changes = blob_changes(&odb, resource_cache, oid, previous_oid, file_path, &mut stats)?; hunks_to_blame = process_changes(&mut out, hunks_to_blame, changes, suspect); for unblamed_hunk in &mut hunks_to_blame { unblamed_hunk.pass_blame(suspect, parent_id); @@ -169,7 +176,7 @@ pub fn file( } else { for parent_id in &parent_ids { if let Some(parent_entry) = - find_path_entry_in_commit(&odb, parent_id, file_path, &mut buf, &mut buf2, &mut stats) + find_path_entry_in_commit(&odb, parent_id, file_path, &mut buf, &mut buf2, &mut stats)? { if entry.oid == parent_entry.oid { // The blobs storing the blamed file in `entry` and `parent_entry` are @@ -194,7 +201,7 @@ pub fn file( &mut buf, &mut buf2, &mut buf3, - ); + )?; let Some(modification) = changes_for_file_path else { // None of the changes affected the file we’re currently blaming. Pass blame // to parent. @@ -215,7 +222,7 @@ pub fn file( } gix_diff::tree::recorder::Change::Deletion { .. } => todo!(), gix_diff::tree::recorder::Change::Modification { previous_oid, oid, .. } => { - let changes = blob_changes(&odb, resource_cache, oid, previous_oid, file_path, &mut stats); + let changes = blob_changes(&odb, resource_cache, oid, previous_oid, file_path, &mut stats)?; hunks_to_blame = process_changes(&mut out, hunks_to_blame, changes, suspect); for unblamed_hunk in &mut hunks_to_blame { unblamed_hunk.pass_blame(suspect, parent_id); @@ -300,32 +307,28 @@ fn tree_diff_at_file_path( commit_buf: &mut Vec, lhs_tree_buf: &mut Vec, rhs_tree_buf: &mut Vec, -) -> Option { - let parent_tree = odb.find_commit(&parent_id, commit_buf).unwrap().tree(); +) -> Result, Error> { + let parent_tree = odb.find_commit(&parent_id, commit_buf)?.tree(); stats.commits_to_tree += 1; - let parent_tree_iter = odb - .find(&parent_tree, lhs_tree_buf) - .unwrap() - .try_into_tree_iter() - .unwrap(); + let parent_tree_iter = odb.find_tree_iter(&parent_tree, lhs_tree_buf)?; stats.trees_decoded += 1; - let tree_id = odb.find_commit(&id, commit_buf).unwrap().tree(); + let tree_id = odb.find_commit(&id, commit_buf)?.tree(); stats.commits_to_tree += 1; - let tree_iter = odb.find(&tree_id, rhs_tree_buf).unwrap().try_into_tree_iter().unwrap(); + let tree_iter = odb.find_tree_iter(&tree_id, rhs_tree_buf)?; stats.trees_decoded += 1; let mut recorder = gix_diff::tree::Recorder::default(); - gix_diff::tree(parent_tree_iter, tree_iter, state, &odb, &mut recorder).unwrap(); + gix_diff::tree(parent_tree_iter, tree_iter, state, &odb, &mut recorder)?; stats.trees_diffed += 1; - recorder.records.into_iter().find(|change| match change { + Ok(recorder.records.into_iter().find(|change| match change { gix_diff::tree::recorder::Change::Modification { path, .. } => path == file_path, gix_diff::tree::recorder::Change::Addition { path, .. } => path == file_path, gix_diff::tree::recorder::Change::Deletion { path, .. } => path == file_path, - }) + })) } fn blob_changes( @@ -335,7 +338,7 @@ fn blob_changes( previous_oid: ObjectId, file_path: &BStr, stats: &mut Statistics, -) -> Vec { +) -> Result, Error> { /// Record all [`Change`]s to learn about additions, deletions and unchanged portions of a *Blamed File*. struct ChangeRecorder { last_seen_after_end: u32, @@ -387,35 +390,32 @@ fn blob_changes( } } - resource_cache - .set_resource( - previous_oid, - gix_object::tree::EntryKind::Blob, - file_path, - gix_diff::blob::ResourceKind::OldOrSource, - &odb, - ) - .unwrap(); - resource_cache - .set_resource( - oid, - gix_object::tree::EntryKind::Blob, - file_path, - gix_diff::blob::ResourceKind::NewOrDestination, - &odb, - ) - .unwrap(); - - let outcome = resource_cache.prepare_diff().unwrap(); + resource_cache.set_resource( + previous_oid, + gix_object::tree::EntryKind::Blob, + file_path, + gix_diff::blob::ResourceKind::OldOrSource, + &odb, + )?; + resource_cache.set_resource( + oid, + gix_object::tree::EntryKind::Blob, + file_path, + gix_diff::blob::ResourceKind::NewOrDestination, + &odb, + )?; + + let outcome = resource_cache.prepare_diff()?; let input = gix_diff::blob::intern::InternedInput::new( tokens_for_diffing(outcome.old.data.as_slice().unwrap_or_default()), tokens_for_diffing(outcome.new.data.as_slice().unwrap_or_default()), ); let number_of_lines_in_destination = input.after.len(); - let change_recorder = ChangeRecorder::new(number_of_lines_in_destination.try_into().unwrap()); + let change_recorder = ChangeRecorder::new(number_of_lines_in_destination as u32); + let res = gix_diff::blob::diff(gix_diff::blob::Algorithm::Histogram, &input, change_recorder); stats.blobs_diffed += 1; - gix_diff::blob::diff(gix_diff::blob::Algorithm::Histogram, &input, change_recorder) + Ok(res) } fn find_path_entry_in_commit( @@ -425,19 +425,19 @@ fn find_path_entry_in_commit( buf: &mut Vec, buf2: &mut Vec, stats: &mut Statistics, -) -> Option { - let commit_id = odb.find_commit(commit, buf).unwrap().tree(); - let tree_iter = odb.find_tree_iter(&commit_id, buf).unwrap(); +) -> Result, Error> { + let commit_id = odb.find_commit(commit, buf)?.tree(); stats.commits_to_tree += 1; + let tree_iter = odb.find_tree_iter(&commit_id, buf)?; stats.trees_decoded += 1; - tree_iter - .lookup_entry( - odb, - buf2, - file_path.split(|b| *b == b'/').inspect(|_| stats.trees_decoded += 1), - ) - .unwrap() + let res = tree_iter.lookup_entry( + odb, + buf2, + file_path.split(|b| *b == b'/').inspect(|_| stats.trees_decoded += 1), + )?; + stats.trees_decoded -= 1; + Ok(res) } /// Return an iterator over tokens for use in diffing. These usually lines, but iit's important to unify them diff --git a/gix-blame/src/file/mod.rs b/gix-blame/src/file/mod.rs index cdc270bf752..7132af04048 100644 --- a/gix-blame/src/file/mod.rs +++ b/gix-blame/src/file/mod.rs @@ -216,7 +216,10 @@ fn process_change( } } (Some(hunk), Some(Change::Deleted(line_number_in_destination, number_of_lines_deleted))) => { - let range_in_suspect = hunk.suspects.get(&suspect).expect("TODO"); + let range_in_suspect = hunk + .suspects + .get(&suspect) + .expect("Internal and we know suspect is present"); if line_number_in_destination < range_in_suspect.start { // <---> (hunk) @@ -377,7 +380,10 @@ impl UnblamedHunk { } fn offset_for(&self, suspect: ObjectId) -> Offset { - let range_in_suspect = self.suspects.get(&suspect).expect("TODO"); + let range_in_suspect = self + .suspects + .get(&suspect) + .expect("Internal and we know suspect is present"); if self.range_in_blamed_file.start > range_in_suspect.start { Offset::Added(self.range_in_blamed_file.start - range_in_suspect.start) @@ -437,7 +443,10 @@ impl BlameEntry { /// Create an offset from a portion of the *Original File*. fn from_unblamed_hunk(unblamed_hunk: &UnblamedHunk, commit_id: ObjectId) -> Self { - let range_in_original_file = unblamed_hunk.suspects.get(&commit_id).unwrap(); + let range_in_original_file = unblamed_hunk + .suspects + .get(&commit_id) + .expect("Private and only called when we now `commit_id` is in the suspect list"); Self { range_in_blamed_file: unblamed_hunk.range_in_blamed_file.clone(), diff --git a/gix-blame/src/lib.rs b/gix-blame/src/lib.rs index 6ea0a3c61e5..c504835e50e 100644 --- a/gix-blame/src/lib.rs +++ b/gix-blame/src/lib.rs @@ -14,6 +14,8 @@ #![deny(rust_2018_idioms, missing_docs)] #![forbid(unsafe_code)] +mod error; +pub use error::Error; mod types; pub use types::{BlameEntry, Outcome, Statistics}; diff --git a/gix-blame/tests/blame.rs b/gix-blame/tests/blame.rs index 08664e9045e..73c2597933f 100644 --- a/gix-blame/tests/blame.rs +++ b/gix-blame/tests/blame.rs @@ -182,29 +182,29 @@ impl Fixture { macro_rules! mktest { ($name:ident, $case:expr, $number_of_lines:literal) => { #[test] - fn $name() { + fn $name() -> gix_testtools::Result<()> { let Fixture { odb, mut resource_cache, commits, - } = Fixture::new().unwrap(); + } = Fixture::new()?; let lines_blamed = gix_blame::file( &odb, commits, &mut resource_cache, format!("{}.txt", $case).as_str().into(), - ) - .unwrap() + )? .entries; assert_eq!(lines_blamed.len(), $number_of_lines); let git_dir = fixture_path().join(".git"); - let baseline = Baseline::collect(git_dir.join(format!("{}.baseline", $case))).unwrap(); + let baseline = Baseline::collect(git_dir.join(format!("{}.baseline", $case)))?; assert_eq!(baseline.len(), $number_of_lines); assert_eq!(lines_blamed, baseline); + Ok(()) } }; } @@ -235,11 +235,11 @@ mktest!( ); mktest!(file_only_changed_in_branch, "file-only-changed-in-branch", 2); +/// As of 2024-09-24, these tests are expected to fail. +/// +/// Context: https://github.com/Byron/gitoxide/pull/1453#issuecomment-2371013904 #[test] -#[ignore = "TBD: figure out what the problem is"] -// As of 2024-09-24, these tests are expected to fail. -// -// Context: https://github.com/Byron/gitoxide/pull/1453#issuecomment-2371013904 +#[should_panic = "empty-lines-myers"] fn diff_disparity() { for case in ["empty-lines-myers", "empty-lines-histogram"] { let Fixture {