remove duplication and unnecessary parameter

Author: Byron

Cargo.lock

@@ -1,7 +1,5 @@
-use std::{ffi::OsStr, path::PathBuf};
-
-use anyhow::anyhow;
 use gix::bstr::BStr;
+use std::ffi::OsStr;
 
 pub fn blame_file(mut repo: gix::Repository, file: &OsStr, out: impl std::io::Write) -> anyhow::Result<()> {
     repo.object_cache_size_if_unset(repo.compute_object_cache_size_for_tree_diffs(&**repo.index_or_empty()?));
@@ -11,20 +9,9 @@ pub fn blame_file(mut repo: gix::Repository, file: &OsStr, out: impl std::io::Wr
         gix::traverse::commit::topo::Builder::from_iters(&repo.objects, [suspect.id], None::<Vec<gix::ObjectId>>)
             .build()?;
     let mut resource_cache = repo.diff_resource_cache_for_tree_diff()?;
-
-    let work_dir: PathBuf = repo
-        .work_dir()
-        .ok_or_else(|| anyhow!("blame needs a workdir, but there is none"))?
-        .into();
     let file_path: &BStr = gix::path::os_str_into_bstr(file)?;
 
-    let outcome = gix::blame::file(
-        &repo.objects,
-        traverse,
-        &mut resource_cache,
-        work_dir.clone(),
-        file_path,
-    )?;
+    let outcome = gix::blame::file(&repo.objects, traverse, &mut resource_cache, file_path)?;
     write_blame_entries(out, outcome)?;
 
     Ok(())

gitoxide-core/src/repository/blame.rs

@@ -14,7 +14,6 @@ rust-version = "1.65"
 doctest = false
 
 [dependencies]
-gix-path = { version = "^0.10.13", path = "../gix-path" }
 gix-diff = { version = "^0.49.0", path = "../gix-diff", default-features = false, features = ["blob"] }
 gix-object = { version = "^0.46.0", path = "../gix-object" }
 gix-hash = { version = "^0.15.0", path = "../gix-hash" }

gix-blame/Cargo.toml

@@ -1,10 +1,9 @@
+use super::{process_changes, Change, Offset, UnblamedHunk};
+use crate::{BlameEntry, Outcome};
 use gix_diff::blob::intern::TokenSource;
 use gix_hash::ObjectId;
 use gix_object::{bstr::BStr, FindExt};
-use std::{ops::Range, path::PathBuf};
-
-use super::{process_changes, Change, Offset, UnblamedHunk};
-use crate::{BlameEntry, Outcome};
+use std::ops::Range;
 
 // TODO: do not instantiate anything, get everything passed as argument.
 /// Produce a list of consecutive [`BlameEntry`] instances to indicate in which commits the ranges of the file
@@ -57,22 +56,16 @@ pub fn file<E>(
     odb: impl gix_object::Find + gix_object::FindHeader,
     traverse: impl IntoIterator<Item = Result<gix_traverse::commit::Info, E>>,
     resource_cache: &mut gix_diff::blob::Platform,
-    // TODO: remove
-    worktree_root: PathBuf,
     file_path: &BStr,
 ) -> Result<Outcome, E> {
-    // TODO: `worktree_root` should be removed - read everything from Commit.
-    //       Worktree changes should be placed into a temporary commit.
-    // TODO: remove this and deduplicate the respective code.
-    use gix_object::bstr::ByteSlice;
-    let absolute_path = worktree_root.join(gix_path::from_bstr(file_path));
-
     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");
     };
 
-    let original_file_blob = std::fs::read(absolute_path).unwrap();
+    let (mut buf, mut buf2) = (Vec::new(), Vec::new());
+    let original_file_entry = find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2).unwrap();
+    let original_file_blob = odb.find_blob(&original_file_entry.oid, &mut buf).unwrap().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)
@@ -88,12 +81,11 @@ pub fn file<E>(
     )];
 
     let mut out = Vec::new();
-    let mut buf = Vec::with_capacity(512);
     'outer: for item in traverse {
         let item = item?;
         let suspect = item.id;
 
-        let parent_ids = item.parent_ids;
+        let mut 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
@@ -110,29 +102,13 @@ pub fn file<E>(
             break;
         }
 
-        let commit_id = odb.find_commit(&suspect, &mut buf).unwrap().tree();
-        let tree_iter = odb.find_tree_iter(&commit_id, &mut buf).unwrap();
-
-        let mut entry_buffer = Vec::new();
-        let Some(entry) = tree_iter
-            .lookup_entry_by_path(&odb, &mut entry_buffer, file_path.to_str().unwrap())
-            .unwrap()
-        else {
+        let Some(entry) = find_path_entry_in_commit(&odb, &suspect, file_path, &mut buf, &mut buf2) else {
             continue;
         };
 
         if parent_ids.len() == 1 {
-            let parent_id: ObjectId = *parent_ids.last().unwrap();
-
-            let mut buffer = Vec::new();
-            let parent_commit_id = odb.find_commit(&parent_id, &mut buffer).unwrap().tree();
-            let parent_tree_iter = odb.find_tree_iter(&parent_commit_id, &mut buffer).unwrap();
-
-            let mut entry_buffer = Vec::new();
-            if let Some(parent_entry) = parent_tree_iter
-                .lookup_entry_by_path(&odb, &mut entry_buffer, file_path.to_str().unwrap())
-                .unwrap()
-            {
+            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) {
                 if entry.oid == parent_entry.oid {
                     // The blobs storing the blamed file in `entry` and `parent_entry` are identical
                     // which is why we can pass blame to the parent without further checks.
@@ -175,25 +151,8 @@ pub fn file<E>(
                 }
             }
         } else {
-            let mut buffer = Vec::new();
-            let commit_id = odb.find_commit(&suspect, &mut buffer).unwrap().tree();
-            let tree_iter = odb.find_tree_iter(&commit_id, &mut buffer).unwrap();
-
-            let mut entry_buffer = Vec::new();
-            let entry = tree_iter
-                .lookup_entry_by_path(&odb, &mut entry_buffer, file_path.to_str().unwrap())
-                .unwrap()
-                .unwrap();
-
             for parent_id in &parent_ids {
-                let mut buffer = Vec::new();
-                let parent_commit_id = odb.find_commit(parent_id, &mut buffer).unwrap().tree();
-                let parent_tree_iter = odb.find_tree_iter(&parent_commit_id, &mut buffer).unwrap();
-
-                let mut entry_buffer = Vec::new();
-                if let Some(parent_entry) = parent_tree_iter
-                    .lookup_entry_by_path(&odb, &mut entry_buffer, file_path.to_str().unwrap())
-                    .unwrap()
+                if let Some(parent_entry) = find_path_entry_in_commit(&odb, &parent_id, file_path, &mut buf, &mut buf2)
                 {
                     if entry.oid == parent_entry.oid {
                         // The blobs storing the blamed file in `entry` and `parent_entry` are
@@ -435,6 +394,21 @@ fn blob_changes(
     gix_diff::blob::diff(gix_diff::blob::Algorithm::Histogram, &input, change_recorder)
 }
 
+fn find_path_entry_in_commit(
+    odb: &impl gix_object::Find,
+    commit: &gix_hash::oid,
+    file_path: &BStr,
+    buf: &mut Vec<u8>,
+    buf2: &mut Vec<u8>,
+) -> Option<gix_object::tree::Entry> {
+    let commit_id = odb.find_commit(commit, buf).unwrap().tree();
+    let tree_iter = odb.find_tree_iter(&commit_id, buf).unwrap();
+
+    tree_iter
+        .lookup_entry(odb, buf2, file_path.split(|b| *b == b'/'))
+        .unwrap()
+}
+
 /// Return an iterator over tokens for use in diffing. These usually lines, but iit's important to unify them
 /// so the later access shows the right thing.
 pub(crate) fn tokens_for_diffing(data: &[u8]) -> impl TokenSource<Token = &[u8]> {

gix-blame/src/file/function.rs

@@ -108,7 +108,6 @@ mod baseline {
 }
 
 struct Fixture {
-    worktree_path: PathBuf,
     odb: gix_odb::Handle,
     resource_cache: gix_diff::blob::Platform,
     commits: Vec<Result<gix_traverse::commit::Info, gix_traverse::commit::topo::Error>>,
@@ -174,7 +173,6 @@ impl Fixture {
         );
         Ok(Fixture {
             odb,
-            worktree_path,
             resource_cache,
             commits,
         })
@@ -186,7 +184,6 @@ macro_rules! mktest {
         #[test]
         fn $name() {
             let Fixture {
-                worktree_path,
                 odb,
                 mut resource_cache,
                 commits,
@@ -196,7 +193,6 @@ macro_rules! mktest {
                 &odb,
                 commits,
                 &mut resource_cache,
-                worktree_path,
                 format!("{}.txt", $case).as_str().into(),
             )
             .unwrap()
@@ -247,7 +243,6 @@ mktest!(file_only_changed_in_branch, "file-only-changed-in-branch", 2);
 fn diff_disparity() {
     for case in ["empty-lines-myers", "empty-lines-histogram"] {
         let Fixture {
-            worktree_path,
             odb,
             mut resource_cache,
             commits,
@@ -257,7 +252,6 @@ fn diff_disparity() {
             &odb,
             commits,
             &mut resource_cache,
-            worktree_path,
             format!("{case}.txt").as_str().into(),
         )
         .unwrap()