Store UnblamedHunks by ObjectId

This is a prerequisite to being able to blame commits with more than one
parent.

Author: cruessler

gix-blame/src/lib.rs

@@ -3,6 +3,7 @@
 #![forbid(unsafe_code)]
 
 use std::{
+    collections::BTreeMap,
     ops::{Add, AddAssign, Range, SubAssign},
     path::PathBuf,
 };
@@ -222,6 +223,30 @@ impl UnblamedHunk {
     }
 }
 
+struct UnblamedHunkRecorder {
+    hunks: BTreeMap<ObjectId, Vec<UnblamedHunk>>,
+}
+
+impl UnblamedHunkRecorder {
+    fn new() -> Self {
+        Self { hunks: BTreeMap::new() }
+    }
+
+    fn add_hunk(&mut self, object_id: ObjectId, unblamed_hunk: UnblamedHunk) {
+        self.hunks
+            .entry(object_id)
+            .and_modify(|unblamed_hunks| unblamed_hunks.push(unblamed_hunk.clone()))
+            .or_insert_with(|| vec![unblamed_hunk.clone()]);
+    }
+
+    fn add_hunks(&mut self, object_id: ObjectId, new_unblamed_hunks: Vec<UnblamedHunk>) {
+        self.hunks
+            .entry(object_id)
+            .and_modify(|unblamed_hunks| unblamed_hunks.extend(new_unblamed_hunks.clone()))
+            .or_insert_with(|| new_unblamed_hunks);
+    }
+}
+
 #[derive(Clone, Debug, PartialEq)]
 pub enum Change {
     Unchanged(Range<u32>),
@@ -742,6 +767,7 @@ pub fn blame_file<E>(
     traverse: impl IntoIterator<Item = Result<gix_traverse::commit::Info, E>>,
     resource_cache: &mut gix_diff::blob::Platform,
     worktree_path: PathBuf,
+    start_id: ObjectId,
     file_path: &BStr,
 ) -> Result<Vec<BlameEntry>, E> {
     // TODO
@@ -770,24 +796,33 @@ pub fn blame_file<E>(
     // TODO Verify that `imara-diff` tokenizes lines the same way `lines` does.
     let number_of_lines = std::fs::read_to_string(absolute_path).unwrap().lines().count();
 
-    let mut lines_to_blame: Vec<UnblamedHunk> = vec![UnblamedHunk::new(
-        0..number_of_lines.try_into().unwrap(),
-        Offset::Added(0),
-    )];
+    let mut unblamed_hunk_recorder = UnblamedHunkRecorder::new();
+    unblamed_hunk_recorder.add_hunk(
+        start_id,
+        UnblamedHunk::new(0..number_of_lines.try_into().unwrap(), Offset::Added(0)),
+    );
     let mut out: Vec<BlameEntry> = vec![];
 
     for item in traverse {
         let item = item?;
         let suspect = item.id;
 
+        // TODO
+        // When we call `.remove(&suspect)`, we need to make sure that all commits that have
+        // `suspect` as their parent have already been processed. `git` accomplishes that by
+        // keeping unprocessed commits in a priority queue sorted by commit date.
+        let Some(hunks_to_blame) = unblamed_hunk_recorder.hunks.remove(&suspect) else {
+            continue;
+        };
+
         let parent_ids = item.parent_ids;
         if parent_ids.is_empty() {
             // I’m not entirely sure if this is correct yet. `suspect`, at this point, is the `id` of
             // the last `item` that was yielded by `traverse`, so it makes sense to assign the
             // remaining lines to it, even though we don’t explicitly check whether that is true
             // here. We could perhaps use `needed_to_obtain` to compare `suspect` against an empty
             // tree to validate this assumption.
-            out.extend(lines_to_blame.iter().map(|hunk| {
+            out.extend(hunks_to_blame.iter().map(|hunk| {
                 BlameEntry::new(
                     hunk.range_in_blamed_file.clone(),
                     // TODO
@@ -798,8 +833,6 @@ pub fn blame_file<E>(
                 )
             }));
 
-            lines_to_blame = vec![];
-
             break;
         }
 
@@ -813,14 +846,16 @@ pub fn blame_file<E>(
 
         let [ref modification]: [gix_diff::tree::recorder::Change] = changes_for_file_path[..] else {
             // None of the changes affected the file we’re currently blaming.
+            unblamed_hunk_recorder.add_hunks(last_parent_id, hunks_to_blame);
+
             continue;
         };
 
         match modification {
             gix_diff::tree::recorder::Change::Addition { .. } => {
                 // Every line that has not been blamed yet on a commit, is expected to have been
                 // added when the file was added to the repository.
-                out.extend(lines_to_blame.iter().map(|hunk| {
+                out.extend(hunks_to_blame.iter().map(|hunk| {
                     BlameEntry::new(
                         hunk.range_in_blamed_file.clone(),
                         // TODO
@@ -831,20 +866,20 @@ pub fn blame_file<E>(
                     )
                 }));
 
-                lines_to_blame = vec![];
-
                 break;
             }
             gix_diff::tree::recorder::Change::Deletion { .. } => todo!(),
             gix_diff::tree::recorder::Change::Modification { previous_oid, oid, .. } => {
                 let changes = get_changes(&odb, resource_cache, *oid, *previous_oid, file_path);
 
-                lines_to_blame = process_changes(&mut out, &lines_to_blame, &changes, suspect);
+                let new_hunks_to_blame = process_changes(&mut out, &hunks_to_blame, &changes, suspect);
+
+                unblamed_hunk_recorder.add_hunks(last_parent_id, new_hunks_to_blame.clone());
             }
         }
     }
 
-    assert_eq!(lines_to_blame, vec![]);
+    assert!(unblamed_hunk_recorder.hunks.is_empty());
 
     // I don’t know yet whether it would make sense to use a data structure instead that preserves
     // order on insertion.

gix-blame/tests/blame.rs

@@ -111,6 +111,7 @@ struct Fixture {
     worktree_path: PathBuf,
     odb: gix_odb::Handle,
     resource_cache: gix_diff::blob::Platform,
+    start_id: ObjectId,
     commits: Vec<Result<gix_traverse::commit::Info, gix_traverse::commit::simple::Error>>,
 }
 
@@ -173,6 +174,7 @@ impl Fixture {
         Ok(Fixture {
             odb,
             worktree_path,
+            start_id: head_id,
             commits,
             resource_cache,
         })
@@ -187,6 +189,7 @@ macro_rules! mktest {
                 worktree_path,
                 odb,
                 mut resource_cache,
+                start_id,
                 commits,
             } = Fixture::new().unwrap();
 
@@ -195,6 +198,7 @@ macro_rules! mktest {
                 commits,
                 &mut resource_cache,
                 worktree_path,
+                start_id,
                 format!("{}.txt", $case).as_str().into(),
             )
             .unwrap();
@@ -238,13 +242,15 @@ fn diff_disparity() {
             worktree_path,
             odb,
             mut resource_cache,
+            start_id,
             commits,
         } = Fixture::new().unwrap();
         let lines_blamed = blame_file(
             &odb,
             commits,
             &mut resource_cache,
             worktree_path,
+            start_id,
             format!("{case}.txt").as_str().into(),
         )
         .unwrap();