fix!: `Tree::lookup_entry()` looses its `buf` argument.

The buffer will now be previded from the free-list of the repository.

Sebastian Thiel committed 2 years ago

Commit c545d711 parent a10e7c23 files · +13 −21Browse files

Unified · Split

3 files

gix/src/object/tree/mod.rs +11 −14
gix/src/repository/object.rs +1 −1
gix/tests/object/tree/mod.rs +1 −6

     /// 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
     ///
     /// to reuse 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.
     ///
-    pub fn lookup_entry<I, P>(&self, path: I, buf: &mut Vec<u8>) -> Result<Option<Entry<'repo>>, find::existing::Error>
+    pub fn lookup_entry<I, P>(&self, path: I) -> Result<Option<Entry<'repo>>, find::existing::Error>
     where
         I: IntoIterator<Item = P>,
         P: PartialEq<BStr>,
     {
-        let mut path = path.into_iter().peekable();
+        let mut buf = self.repo.shared_empty_buf();
         buf.clear();
+        let mut path = path.into_iter().peekable();
         buf.extend_from_slice(&self.data);
         while let Some(component) = path.next() {
-            match TreeRefIter::from_bytes(buf)
+            match TreeRefIter::from_bytes(&buf)
                 .filter_map(Result::ok)
                 .find(|entry| component.eq(entry.filename))
             {
                         }));
                     } else {
                         let next_id = entry.oid.to_owned();
-                        let obj = self.repo.objects.find(&next_id, buf)?;
+                        let obj = self.repo.objects.find(&next_id, &mut buf)?;
                         if !obj.kind.is_tree() {
                             return Ok(None);
                         }
     pub fn lookup_entry_by_path(
         &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(
+        self.lookup_entry(relative_path.as_ref().components().map(|c: std::path::Component<'_>| {
-            relative_path.as_ref().components().map(|c: std::path::Component<'_>| {
+            gix_path::os_str_into_bstr(c.as_os_str())
-                gix_path::os_str_into_bstr(c.as_os_str())
+                .unwrap_or_else(|_| "".into())
-                    .unwrap_or_else(|_| "".into())
+                .as_bytes()
-                    .as_bytes()
+        }))
-            }),
-            buf,
-        )
     }
     /// Like [`Self::peel_to_entry()`], but takes a `Path` directly via `relative_path`, a path relative to this tree.

gix/src/repository/object.rs+1−1

@@ -139,7 +139,7 @@ impl crate::Repository { · 138 unmodified lines
139	}	139	}
140	}	140	}
141		141
142	fn shared_empty_buf(&self) -> std::cell::RefMut<'_, Vec<u8>> {	142	pub(crate) fn shared_empty_buf(&self) -> std::cell::RefMut<'_, Vec<u8>> {
143	let mut bufs = self.bufs.borrow_mut();	143	let mut bufs = self.bufs.borrow_mut();
144	if bufs.last().is_none() {	144	if bufs.last().is_none() {
145	bufs.push(Vec::with_capacity(512));	145	bufs.push(Vec::with_capacity(512));

gix/tests/object/tree/mod.rs+1−6

 fn lookup_entry_by_path() -> crate::Result {
     let repo = named_subrepo_opts("make_worktree_repo.sh", "repo", gix::open::Options::isolated())?;
     let tree = repo.head_commit()?.tree()?;
-    assert_eq!(
+    assert_eq!(tree.lookup_entry_by_path("dir/c")?.expect("present").filename(), "c");
-        tree.lookup_entry_by_path("dir/c", &mut Vec::new())?
-            .expect("present")
-            .filename(),
-        "c"
-    );
     Ok(())
 }