Add lookup_entry and lookup_entry_by_path to TreeRef

[cruessler]

This PR is related to a comment in my gix blame PR: #1453 (comment).

I wrote the new code in the context of #1453 and then cherry-picked the result into this separate PR. I initially took both methods from

gitoxide/gix/src/object/tree/mod.rs

Lines 51 to 93 in 9ab86a2

	/// 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.
	///
	/// # Performance Notes
	///
	/// Searching tree entries is currently done in sequence, which allows to the search to be allocation free. It would be possible
	/// 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) -> Result<Option<Entry<'repo>>, find::existing::Error>
	where
	I: IntoIterator<Item = P>,
	P: PartialEq<BStr>,
	{
	let mut buf = self.repo.empty_reusable_buffer();
	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)
	.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 next_id = entry.oid.to_owned();
	let obj = self.repo.objects.find(&next_id, &mut buf)?;
	if !obj.kind.is_tree() {
	return Ok(None);
	}
	}
	}
	None => return Ok(None),
	}
	}
	Ok(None)
	}

and

gitoxide/gix/src/object/tree/mod.rs

Lines 138 to 154 in 9ab86a2

	/// 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,
	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<'_>| {
	gix_path::os_str_into_bstr(c.as_os_str())
	.unwrap_or_else(|_| "".into())
	.as_bytes()
	}))
	}

, respectively. I left the doc comments untouched as they seem adequate in their new context as well. The code was modified to work with the APIs available in gix-object. I needed to add gix-path as a dependency in order to have access to gix_path::os_str_into_bstr.

[Byron]

Thanks a lot for making more convenient tree access available on 'the lower levels'.

Something that is sorely missing is tests. With that I think it would quickly become clear that this isn't actually working as the implementation doesn't attempt to load its subtrees.

[cruessler]

Thanks a lot for making more convenient tree access available on 'the lower levels'.

Something that is sorely missing is tests. With that I think it would quickly become clear that this isn't actually working as the implementation doesn't attempt to load its subtrees.

Thanks for having a first look! (Sorry if this first attempt is incomplete. I only have limited context yet on how this works, so any feedback is greatly appreciated. I hope that this does not take away too much of your time. I’m a big fan of getting feedback very early as I’m certain one or two hints from your side will help me a lot initially. I’ll mark this PR as draft for the time being.)

[Byron]

No worries at all. I think this this crate also has just the trait you need: Find, so these subtrees can be loaded into a buffer which probably would have to be passed as parameter as well. And from there it should work very similarly as 'on the upper levels'.

[cruessler]

I hope I now have a better understanding of how lookup by path works. Just to confirm: is it correct to recursively load trees and run lookup_entry on them?

The code as I just pushed it, does not compile as the buffer that is passed into lookup_entry cannot be passed to a recursive call to lookup_entry. Before I dig into how to best solve that, I first want to make sure this is indeed the correct approach. If it is, what would be the most idiomatic solution to this problem?

[Byron]

Just to confirm: is it correct to recursively load trees and run lookup_entry on them?

In abstract terms, yes, but it should really be implemented like in gix so it's a loop. Real recursion is only ever done when it's also limited by some counter to prevent attacks.

If it is, what would be the most idiomatic solution to this problem?

Rust helps :). Like mentioned before, it should be possible to translate the gix level implementation to this one, it remains the same except that everything that's Repository is replaced with something passed as parameter.

[cruessler]

I just pushed a non-recursive implementation that passes the 2 tests that I had added before, so it seems to be working. :-)

I’m not really happy that it has self.entries.clone(), even though that was the most straightforward way to work around the ownership issues that I had when reading from both self.entries as well as obj.entries in subsequent iterations of the loop.

[Byron]

This looks much more like it, thank you!

The following patch would fix the problem of clone()…

diff --git a/gix-object/src/tree/ref_iter.rs b/gix-object/src/tree/ref_iter.rs
index fda6f7275..76f063a9f 100644
--- a/gix-object/src/tree/ref_iter.rs
+++ b/gix-object/src/tree/ref_iter.rs
@@ -1,4 +1,5 @@
 use bstr::BStr;
+use std::borrow::Cow;
 use winnow::{error::ParserError, prelude::*};
 
 use crate::{tree, tree::EntryRef, TreeRef, TreeRefIter};
@@ -65,7 +66,7 @@ impl<'a> TreeRef<'a> {
         P: PartialEq<BStr>,
     {
         let mut path = path.into_iter().peekable();
-        let mut entries = self.entries.clone();
+        let mut entries = Cow::Borrowed(&self.entries);
 
         while let Some(component) = path.next() {
             match entries.iter().find(|entry| component.eq(entry.filename)) {
@@ -76,7 +77,7 @@ impl<'a> TreeRef<'a> {
                         let next_id = entry.oid.to_owned();
                         let obj = odb.find_tree(&next_id, buffer)?;
 
-                        entries = obj.entries;
+                        entries = Cow::Owned(obj.entries);
                     }
                 }
                 None => return Ok(None),

…but it would still leave the problem of inefficiency as it allocates where it shouldn't.

TreeRefIter should be used instead.

[Byron]

The trick should be to stick to what's done in gix and implement this on the TreeRefIter, something I thought I saw in an earlier version.

[cruessler]

The reason I implemented the method on TreeRef was that, in its current iteration, the gix-blame PR uses odb.find_tree which returns a TreeRef. Since it seems I could switch to odb.find_tree_iter in the gix-blame PR instead, I’ll move lookup_entry to TreeRefIter as you suggested.

[Byron]

Great! This has the chance to be a small but measurable performance boost as well. I still remember comparing two versions of this - what it is now (in gix), and one where it would allocate a Vec and binary-search through it. And it turned out the allocation really kills performance.

[Byron]

This should be crate::Find, which always implements FindExt.

[cruessler]

Thanks for the hint! Changed.

[Byron]

I would deduplicate the tests and compare the whole entry, showing that one can get trees and files. I'd also show what happens if the requested entry doesn't exist.

[cruessler]

Makes sense! Changed.

[cruessler]

As far as I can see, I’m done with the changes in response to your comments.

[Byron]

Thanks a lot!

If you think it's ready, I think it's ready as well :).

[cruessler]

Thanks a lot!

If you think it's ready, I think it's ready as well :).

Awesome!

One additional thing that I had in mind that didn’t make it into the PR was adding a comment to bisect_entry, providing context on the difference between bisect_entry and lookup_entry_by_path. When I was looking for a function to get an Entry from a Tree, I immediately chose bisect_entry because the name seemed to promise the performance I was looking for. What I didn’t notice was that this was the wrong function for what I wanted to do.

[Byron]

I see. Probably it would be easiest to make these doc-adjustments in the gix blame PR - I plan to not take it into the next year :D .