refactors

- return `EntryRef`
- more structured tests

Author: Byron

gix-object/src/tree/ref_iter.rs

@@ -3,40 +3,36 @@ use winnow::{error::ParserError, prelude::*};
 
 use crate::{tree, tree::EntryRef, TreeRef, TreeRefIter};
 
-/// The error type returned by the [`Tree`](crate::Tree) trait.
-pub type Error = Box<dyn std::error::Error + Send + Sync + 'static>;
-
 impl<'a> TreeRefIter<'a> {
     /// Instantiate an iterator from the given tree data.
     pub fn from_bytes(data: &'a [u8]) -> TreeRefIter<'a> {
         TreeRefIter { data }
     }
 
-    /// 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.
+    /// Follow a sequence of `path` components starting from this instance, and look them up in `odb` one by one using `buffer`
+    /// 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 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,
         odb: impl crate::Find,
         buffer: &'a mut Vec<u8>,
         path: I,
-    ) -> Result<Option<tree::Entry>, Error>
+    ) -> Result<Option<tree::Entry>, crate::find::Error>
     where
         I: IntoIterator<Item = P>,
         P: PartialEq<BStr>,
     {
         buffer.clear();
 
         let mut path = path.into_iter().peekable();
-        buffer.extend_from_slice(&self.data);
+        buffer.extend_from_slice(self.data);
         while let Some(component) = path.next() {
-            match TreeRefIter::from_bytes(&buffer)
+            match TreeRefIter::from_bytes(buffer)
                 .filter_map(Result::ok)
                 .find(|entry| component.eq(entry.filename))
             {
@@ -46,10 +42,9 @@ impl<'a> TreeRefIter<'a> {
                     } else {
                         let next_id = entry.oid.to_owned();
                         let obj = odb.try_find(&next_id, buffer)?;
-                        if let Some(obj) = obj {
-                            if !obj.kind.is_tree() {
-                                return Ok(None);
-                            }
+                        let Some(obj) = obj else { return Ok(None) };
+                        if !obj.kind.is_tree() {
+                            return Ok(None);
                         }
                     }
                 }
@@ -59,7 +54,9 @@ impl<'a> TreeRefIter<'a> {
         Ok(None)
     }
 
-    /// Like [`Self::lookup_entry()`], but takes a `Path` directly via `relative_path`, a path relative to this tree.
+    /// Like [`Self::lookup_entry()`], but takes any [`AsRef<Path>`](`std::path::Path`) directly via `relative_path`,
+    /// a path relative to this tree.
+    /// `odb` and `buffer` are used to lookup intermediate trees.
     ///
     /// # Note
     ///
@@ -70,7 +67,7 @@ impl<'a> TreeRefIter<'a> {
         odb: impl crate::Find,
         buffer: &'a mut Vec<u8>,
         relative_path: impl AsRef<std::path::Path>,
-    ) -> Result<Option<tree::Entry>, Error> {
+    ) -> Result<Option<tree::Entry>, crate::find::Error> {
         use crate::bstr::ByteSlice;
         self.lookup_entry(
             odb,

gix-object/tests/object/tree/iter.rs

@@ -1,7 +1,6 @@
-use bstr::BString;
 use gix_object::{
     bstr::ByteSlice,
-    tree::{self, Entry, EntryRef},
+    tree::{self, EntryRef},
     TreeRefIter,
 };
 use pretty_assertions::assert_eq;
@@ -59,59 +58,73 @@ fn everything() -> crate::Result {
     Ok(())
 }
 
-#[test]
-fn lookup_entry_toplevel() -> crate::Result {
-    let entry = utils::lookup_entry_by_path("bin")?;
-
-    let mode: tree::EntryMode = tree::EntryMode(33188);
-    let filename: BString = "bin".into();
-    let oid = hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
-
-    assert_eq!(entry, Some(Entry { mode, filename, oid }));
-
-    Ok(())
-}
-
-#[test]
-fn lookup_entry_nested_path() -> crate::Result {
-    let entry = utils::lookup_entry_by_path("file/a")?;
-
-    let mode: tree::EntryMode = tree::EntryMode(33188);
-    let filename: BString = "a".into();
-    let oid = hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
-
-    assert_eq!(entry, Some(Entry { mode, filename, oid }));
-
-    Ok(())
-}
+mod lookup_entry {
+    use crate::hex_to_id;
+    use gix_object::tree::EntryKind;
+    use utils::entry;
+
+    #[test]
+    fn top_level_directory() -> crate::Result {
+        assert_eq!(
+            utils::lookup_entry_by_path("bin")?,
+            entry(
+                "bin",
+                EntryKind::Blob,
+                hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391")
+            )
+        );
+        Ok(())
+    }
 
-#[test]
-fn lookup_entry_that_does_not_exist() -> crate::Result {
-    let entry = utils::lookup_entry_by_path("file/does-not-exist")?;
+    #[test]
+    fn nested_file() -> crate::Result {
+        assert_eq!(
+            utils::lookup_entry_by_path("file/a")?,
+            entry(
+                "a",
+                EntryKind::Blob,
+                hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391")
+            )
+        );
+        Ok(())
+    }
 
-    assert_eq!(entry, None);
+    #[test]
+    fn non_existing_nested_file() -> crate::Result {
+        for path in ["file/does-not-exist", "non-existing", "file/a/through-file"] {
+            let actual = utils::lookup_entry_by_path(path)?;
+            assert_eq!(actual, None);
+        }
+        Ok(())
+    }
 
-    Ok(())
-}
+    mod utils {
+        use crate::hex_to_id;
 
-mod utils {
-    use crate::hex_to_id;
+        use gix_object::{tree, FindExt};
 
-    use gix_object::FindExt;
+        pub(super) fn entry(filename: &str, mode: tree::EntryKind, oid: gix_hash::ObjectId) -> Option<tree::Entry> {
+            Some(tree::Entry {
+                mode: mode.into(),
+                filename: filename.into(),
+                oid,
+            })
+        }
 
-    pub(super) fn tree_odb() -> gix_testtools::Result<gix_odb::Handle> {
-        let root = gix_testtools::scripted_fixture_read_only("make_trees.sh")?;
-        Ok(gix_odb::at(root.join(".git/objects"))?)
-    }
+        pub(super) fn tree_odb() -> gix_testtools::Result<gix_odb::Handle> {
+            let root = gix_testtools::scripted_fixture_read_only("make_trees.sh")?;
+            Ok(gix_odb::at(root.join(".git/objects"))?)
+        }
 
-    pub(super) fn lookup_entry_by_path(path: &str) -> gix_testtools::Result<Option<gix_object::tree::Entry>> {
-        let odb = tree_odb()?;
-        let root_tree_id = hex_to_id("ff7e7d2aecae1c3fb15054b289a4c58aa65b8646");
+        pub(super) fn lookup_entry_by_path(path: &str) -> gix_testtools::Result<Option<gix_object::tree::Entry>> {
+            let odb = tree_odb()?;
+            let root_tree_id = hex_to_id("ff7e7d2aecae1c3fb15054b289a4c58aa65b8646");
 
-        let mut buf = Vec::new();
-        let root_tree = odb.find_tree_iter(&root_tree_id, &mut buf)?;
+            let mut buf = Vec::new();
+            let root_tree = odb.find_tree_iter(&root_tree_id, &mut buf)?;
 
-        let mut buf = Vec::new();
-        Ok(root_tree.lookup_entry_by_path(&odb, &mut buf, path).unwrap())
+            let mut buf = Vec::new();
+            root_tree.lookup_entry_by_path(&odb, &mut buf, path)
+        }
     }
 }