Merge branch 'improvements-for-crates-index'

Author: Byron

cargo-smart-release/src/git/history.rs

@@ -259,10 +259,10 @@ fn add_item_if_package_changed<'a>(
             let mut repo = ctx.repo.clone();
             repo.object_cache_size(1024 * 1024);
             let current = gix::Tree::from_data(item.id, data_by_tree_id[&item.tree_id].to_owned(), &ctx.repo)
-                .lookup_entry(components.iter().copied())?;
+                .peel_to_entry(components.iter().copied())?;
             let parent = match item.parent_tree_id {
                 Some(tree_id) => gix::Tree::from_data(tree_id, data_by_tree_id[&tree_id].to_owned(), &ctx.repo)
-                    .lookup_entry(components.iter().copied())?,
+                    .peel_to_entry(components.iter().copied())?,
                 None => None,
             };
             match (current, parent) {

cargo-smart-release/src/git/mod.rs

@@ -46,14 +46,14 @@ pub fn change_since_last_release(package: &Package, ctx: &crate::Context) -> any
                         .object()?
                         .peel_to_kind(object::Kind::Tree)?
                         .into_tree()
-                        .lookup_entry(components.clone())?
+                        .peel_to_entry(components.clone())?
                         .expect("path must exist in current commit")
                         .object_id();
                     let released_dir_id = released_target
                         .object()?
                         .peel_to_kind(object::Kind::Tree)?
                         .into_tree()
-                        .lookup_entry(components)?
+                        .peel_to_entry(components)?
                         .expect("path must exist as it was supposedly released there")
                         .object_id();
 

gix-object/src/tree/mod.rs

@@ -63,7 +63,7 @@ impl EntryMode {
 }
 
 /// An element of a [`TreeRef`][crate::TreeRef::entries].
-#[derive(PartialEq, Eq, Debug, Hash, Clone)]
+#[derive(PartialEq, Eq, Debug, Hash, Clone, Copy)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 pub struct EntryRef<'a> {
     /// The kind of object to which `oid` is pointing.

gix/src/ext/mod.rs

@@ -1,7 +1,7 @@
 pub use object_id::ObjectIdExt;
 pub use reference::ReferenceExt;
 pub use rev_spec::RevSpecExt;
-pub use tree::TreeIterExt;
+pub use tree::{TreeEntryExt, TreeEntryRefExt, TreeIterExt};
 
 mod object_id;
 mod reference;

gix/src/ext/tree.rs

@@ -42,3 +42,27 @@ impl<'d> TreeIterExt for TreeRefIter<'d> {
         breadthfirst(self.clone(), state, find, delegate)
     }
 }
+
+/// Extensions for [EntryRef][gix_object::tree::EntryRef].
+pub trait TreeEntryRefExt<'a>: 'a {
+    /// Attach [`Repository`][crate::Repository] to the given tree entry. It can be detached later with `detach()`.
+    fn attach<'repo>(self, repo: &'repo crate::Repository) -> crate::object::tree::EntryRef<'repo, 'a>;
+}
+
+impl<'a> TreeEntryRefExt<'a> for gix_object::tree::EntryRef<'a> {
+    fn attach<'repo>(self, repo: &'repo crate::Repository) -> crate::object::tree::EntryRef<'repo, 'a> {
+        crate::object::tree::EntryRef { inner: self, repo }
+    }
+}
+
+/// Extensions for [Entry][gix_object::tree::Entry].
+pub trait TreeEntryExt {
+    /// Attach [`Repository`][crate::Repository] to the given tree entry. It can be detached later with `detach()`.
+    fn attach(self, repo: &crate::Repository) -> crate::object::tree::Entry<'_>;
+}
+
+impl TreeEntryExt for gix_object::tree::Entry {
+    fn attach(self, repo: &crate::Repository) -> crate::object::tree::Entry<'_> {
+        crate::object::tree::Entry { inner: self, repo }
+    }
+}

gix/src/object/tree/iter.rs

@@ -25,10 +25,25 @@ impl<'repo, 'a> EntryRef<'repo, 'a> {
         crate::Id::from_id(self.inner.oid, self.repo)
     }
 
-    /// Return the entries id, without repository connection.
-    pub fn oid(&self) -> gix_hash::ObjectId {
+    /// Return the plain object id of this entry, without access to the repository.
+    pub fn oid(&self) -> &gix_hash::oid {
+        self.inner.oid
+    }
+
+    /// Return the object this entry points to.
+    pub fn object(&self) -> Result<crate::Object<'repo>, crate::object::find::existing::Error> {
+        self.id().object()
+    }
+
+    /// Return the plain object id of this entry, without access to the repository.
+    pub fn object_id(&self) -> gix_hash::ObjectId {
         self.inner.oid.to_owned()
     }
+
+    /// Detach the repository from this instance.
+    pub fn detach(&self) -> gix_object::tree::EntryRef<'a> {
+        self.inner
+    }
 }
 
 impl<'repo, 'a> std::fmt::Display for EntryRef<'repo, 'a> {

gix/src/object/tree/mod.rs

@@ -1,6 +1,7 @@
 use gix_hash::ObjectId;
 pub use gix_object::tree::EntryMode;
 use gix_object::{bstr::BStr, TreeRefIter};
+use gix_odb::FindExt;
 
 use crate::{object::find, Id, Tree};
 
@@ -28,22 +29,70 @@ impl<'repo> Tree<'repo> {
         gix_object::TreeRef::from_bytes(&self.data)
     }
 
+    /// Find the entry named `name` by iteration, or return `None` if it wasn't found.
+    pub fn find_entry(&self, name: impl PartialEq<BStr>) -> Option<EntryRef<'repo, '_>> {
+        TreeRefIter::from_bytes(&self.data)
+            .filter_map(Result::ok)
+            .find(|entry| name.eq(entry.filename))
+            .map(|entry| EntryRef {
+                inner: entry,
+                repo: self.repo,
+            })
+    }
+
     // TODO: tests.
     /// Follow a sequence of `path` components starting from this instance, and look them up one by one until the last component
     /// is looked up and its tree entry is returned.
+    /// Use `buf` as temporary location for sub-trees to avoid allocating a temporary buffer for each lookup.
     ///
     /// # Performance Notes
     ///
     /// Searching tree entries is currently done in sequence, which allows to the search to be allocation free. It would be possible
     /// to re-use a vector and use a binary search instead, which might be able to improve performance over all.
     /// However, a benchmark should be created first to have some data and see which trade-off to choose here.
     ///
-    /// # Why is this consuming?
+    pub fn lookup_entry<I, P>(&self, path: I, buf: &mut Vec<u8>) -> Result<Option<Entry<'repo>>, find::existing::Error>
+    where
+        I: IntoIterator<Item = P>,
+        P: PartialEq<BStr>,
+    {
+        let mut path = path.into_iter().peekable();
+        while let Some(component) = path.next() {
+            match TreeRefIter::from_bytes(&self.data)
+                .filter_map(Result::ok)
+                .find(|entry| component.eq(entry.filename))
+            {
+                Some(entry) => {
+                    if path.peek().is_none() {
+                        return Ok(Some(Entry {
+                            inner: entry.into(),
+                            repo: self.repo,
+                        }));
+                    } else {
+                        let obj = self.repo.objects.find(entry.oid, buf)?;
+                        if !obj.kind.is_tree() {
+                            return Ok(None);
+                        }
+                    }
+                }
+                None => return Ok(None),
+            }
+        }
+        Ok(None)
+    }
+
+    /// Follow a sequence of `path` components starting from this instance, and look them up one by one until the last component
+    /// is looked up and its tree entry is returned, while changing this instance to point to the last seen tree.
+    /// Note that if the lookup fails, it may be impossible to continue making lookups through this tree.
+    /// It's useful to have this function to be able to reuse the internal buffer of the tree.
+    ///
+    /// # Performance Notes
+    ///
+    /// Searching tree entries is currently done in sequence, which allows to the search to be allocation free. It would be possible
+    /// to re-use a vector and use a binary search instead, which might be able to improve performance over all.
+    /// However, a benchmark should be created first to have some data and see which trade-off to choose here.
     ///
-    /// The borrow checker shows pathological behaviour in loops that mutate a buffer, but also want to return from it.
-    /// Workarounds include keeping an index and doing a separate access to the memory, which seems hard to do here without
-    /// re-parsing the entries.
-    pub fn lookup_entry<I, P>(mut self, path: I) -> Result<Option<Entry<'repo>>, find::existing::Error>
+    pub fn peel_to_entry<I, P>(&mut self, path: I) -> Result<Option<Entry<'repo>>, find::existing::Error>
     where
         I: IntoIterator<Item = P>,
         P: PartialEq<BStr>,
@@ -62,12 +111,11 @@ impl<'repo> Tree<'repo> {
                         }));
                     } else {
                         let next_id = entry.oid.to_owned();
-                        let repo = self.repo;
-                        drop(self);
-                        self = match repo.find_object(next_id)?.try_into_tree() {
-                            Ok(tree) => tree,
-                            Err(_) => return Ok(None),
-                        };
+                        let obj = self.repo.objects.find(next_id, &mut self.data)?;
+                        self.id = next_id;
+                        if !obj.kind.is_tree() {
+                            return Ok(None);
+                        }
                     }
                 }
                 None => return Ok(None),
@@ -76,18 +124,40 @@ impl<'repo> Tree<'repo> {
         Ok(None)
     }
 
-    /// Like [`lookup_entry()`][Self::lookup_entry()], but takes a `Path` directly via `relative_path`, a path relative to this tree.
+    /// Like [`Self::lookup_entry()`], but takes a `Path` directly via `relative_path`, a path relative to this tree.
     ///
     /// # Note
     ///
     /// If any path component contains illformed UTF-8 and thus can't be converted to bytes on platforms which can't do so natively,
     /// the returned component will be empty which makes the lookup fail.
     pub fn lookup_entry_by_path(
-        self,
+        &self,
+        relative_path: impl AsRef<std::path::Path>,
+        buf: &mut Vec<u8>,
+    ) -> Result<Option<Entry<'repo>>, find::existing::Error> {
+        use crate::bstr::ByteSlice;
+        self.lookup_entry(
+            relative_path.as_ref().components().map(|c: std::path::Component<'_>| {
+                gix_path::os_str_into_bstr(c.as_os_str())
+                    .unwrap_or_else(|_| "".into())
+                    .as_bytes()
+            }),
+            buf,
+        )
+    }
+
+    /// Like [`Self::peel_to_entry()`], but takes a `Path` directly via `relative_path`, a path relative to this tree.
+    ///
+    /// # Note
+    ///
+    /// If any path component contains illformed UTF-8 and thus can't be converted to bytes on platforms which can't do so natively,
+    /// the returned component will be empty which makes the lookup fail.
+    pub fn peel_to_entry_by_path(
+        &mut self,
         relative_path: impl AsRef<std::path::Path>,
     ) -> Result<Option<Entry<'repo>>, find::existing::Error> {
         use crate::bstr::ByteSlice;
-        self.lookup_entry(relative_path.as_ref().components().map(|c: std::path::Component<'_>| {
+        self.peel_to_entry(relative_path.as_ref().components().map(|c: std::path::Component<'_>| {
             gix_path::os_str_into_bstr(c.as_os_str())
                 .unwrap_or_else(|_| "".into())
                 .as_bytes()
@@ -114,8 +184,8 @@ impl<'r> std::fmt::Debug for Tree<'r> {
 /// An entry in a [`Tree`], similar to an entry in a directory.
 #[derive(PartialEq, Debug, Clone)]
 pub struct Entry<'repo> {
-    inner: gix_object::tree::Entry,
-    repo: &'repo crate::Repository,
+    pub(crate) inner: gix_object::tree::Entry,
+    pub(crate) repo: &'repo crate::Repository,
 }
 
 mod entry {

gix/src/repository/snapshots.rs

@@ -37,12 +37,11 @@ impl crate::Repository {
             });
         match self.work_dir() {
             None => {
-                // TODO: replace with ref-spec `HEAD:.mailmap` for less verbose way of getting the blob id
                 blob_id = blob_id.or_else(|| {
                     self.head().ok().and_then(|mut head| {
                         let commit = head.peel_to_commit_in_place().ok()?;
                         let tree = commit.tree().ok()?;
-                        tree.lookup_entry(Some(".mailmap")).ok()?.map(|e| e.object_id())
+                        tree.find_entry(".mailmap").map(|e| e.object_id())
                     })
                 });
             }

gix/src/revision/spec/parse/delegate/navigate.rs

@@ -123,9 +123,9 @@ impl<'repo> delegate::Navigate for Delegate<'repo> {
                     if path.is_empty() {
                         return Ok(tree_id);
                     }
-                    let tree = repo.find_object(tree_id)?.into_tree();
+                    let mut tree = repo.find_object(tree_id)?.into_tree();
                     let entry =
-                        tree.lookup_entry_by_path(gix_path::from_bstr(path))?
+                        tree.peel_to_entry_by_path(gix_path::from_bstr(path))?
                             .ok_or_else(|| Error::PathNotFound {
                                 path: path.into(),
                                 object: obj.attach(repo).shorten_or_id(),

gix/tests/object/tree/mod.rs

@@ -1 +1,13 @@
+use crate::util::named_repo;
+
 mod diff;
+
+#[test]
+fn find_entry() -> crate::Result {
+    let repo = named_repo("make_basic_repo.sh")?;
+    let tree = repo.head_commit()?.tree()?;
+    assert_eq!(tree.find_entry("this").expect("present").filename(), "this");
+
+    assert!(tree.find_entry("not there").is_none());
+    Ok(())
+}