feat!: add ReferenceExt::follow_to_object_in_place().

Byron · Byron · commit 16b5e7e4b342 · 2024-08-21T10:38:56.000+02:00
The missing link that makes it possible to follow to the first object
and then peel the object according to any preferred algorithm.
diff --git a/gix-ref/src/peel.rs b/gix-ref/src/peel.rs
@@ -1,13 +1,29 @@
 ///
 #[allow(clippy::empty_docs)]
 pub mod to_id {
-    use std::path::PathBuf;
-
     use gix_object::bstr::BString;
 
+    /// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`].
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        FollowToObject(#[from] super::to_object::Error),
+        #[error("An error occurred when trying to resolve an object a reference points to")]
+        Find(#[from] gix_object::find::Error),
+        #[error("Object {oid} as referred to by {name:?} could not be found")]
+        NotFound { oid: gix_hash::ObjectId, name: BString },
+    }
+}
+
+///
+#[allow(clippy::empty_docs)]
+pub mod to_object {
+    use std::path::PathBuf;
+
     use crate::file;
 
-    /// The error returned by [`crate::file::ReferenceExt::peel_to_id_in_place()`].
+    /// The error returned by [`file::ReferenceExt::follow_to_object_in_place_packed()`].
     #[derive(Debug, thiserror::Error)]
     #[allow(missing_docs)]
     pub enum Error {
@@ -17,9 +33,5 @@ pub mod to_id {
         Cycle { start_absolute: PathBuf },
         #[error("Refusing to follow more than {max_depth} levels of indirection")]
         DepthLimitExceeded { max_depth: usize },
-        #[error("An error occurred when trying to resolve an object a reference points to")]
-        Find(#[from] gix_object::find::Error),
-        #[error("Object {oid} as referred to by {name:?} could not be found")]
-        NotFound { oid: gix_hash::ObjectId, name: BString },
     }
 }
diff --git a/gix-ref/src/store/file/raw_ext.rs b/gix-ref/src/store/file/raw_ext.rs
@@ -1,13 +1,12 @@
 use std::collections::BTreeSet;
 
-use gix_hash::ObjectId;
-
 use crate::{
     packed, peel,
     raw::Reference,
     store_impl::{file, file::log},
     Target,
 };
+use gix_hash::ObjectId;
 
 pub trait Sealed {}
 impl Sealed for crate::Reference {}
@@ -22,16 +21,17 @@ pub trait ReferenceExt: Sealed {
 
     /// Follow all symbolic targets this reference might point to and peel the underlying object
     /// to the end of the tag-chain, returning the first non-tag object the annotated tag points to,
-    /// using `objects` to access them.
+    /// using `objects` to access them and `store` to lookup symbolic references.
     ///
-    /// This is useful to learn where this reference is ultimately pointing to.
+    /// This is useful to learn where this reference is ultimately pointing to after following all symbolic
+    /// refs and all annotated tags to the first non-tag object.
     fn peel_to_id_in_place(
         &mut self,
         store: &file::Store,
         objects: &dyn gix_object::Find,
     ) -> Result<ObjectId, peel::to_id::Error>;
 
-    /// Like [`ReferenceExt::peel_to_id_in_place()`], but with support for a known stable packed buffer
+    /// Like [`ReferenceExt::peel_to_id_in_place()`], but with support for a known stable `packed` buffer
     /// to use for resolving symbolic links.
     fn peel_to_id_in_place_packed(
         &mut self,
@@ -40,6 +40,14 @@ pub trait ReferenceExt: Sealed {
         packed: Option<&packed::Buffer>,
     ) -> Result<ObjectId, peel::to_id::Error>;
 
+    /// Like [`ReferenceExt::follow()`], but follows all symbolic references while gracefully handling loops,
+    /// altering this instance in place.
+    fn follow_to_object_in_place_packed(
+        &mut self,
+        store: &file::Store,
+        packed: Option<&packed::Buffer>,
+    ) -> Result<ObjectId, peel::to_object::Error>;
+
     /// Follow this symbolic reference one level and return the ref it refers to.
     ///
     /// Returns `None` if this is not a symbolic reference, hence the leaf of the chain.
@@ -77,7 +85,9 @@ impl ReferenceExt for Reference {
         objects: &dyn gix_object::Find,
     ) -> Result<ObjectId, peel::to_id::Error> {
         let packed = store.assure_packed_refs_uptodate().map_err(|err| {
-            peel::to_id::Error::Follow(file::find::existing::Error::Find(file::find::Error::PackedOpen(err)))
+            peel::to_id::Error::FollowToObject(peel::to_object::Error::Follow(file::find::existing::Error::Find(
+                file::find::Error::PackedOpen(err),
+            )))
         })?;
         self.peel_to_id_in_place_packed(store, objects, packed.as_ref().map(|b| &***b))
     }
@@ -94,28 +104,8 @@ impl ReferenceExt for Reference {
                 Ok(peeled)
             }
             None => {
-                if self.target.kind() == crate::Kind::Symbolic {
-                    let mut seen = BTreeSet::new();
-                    let cursor = &mut *self;
-                    while let Some(next) = cursor.follow_packed(store, packed) {
-                        let next = next?;
-                        if seen.contains(&next.name) {
-                            return Err(peel::to_id::Error::Cycle {
-                                start_absolute: store.reference_path(cursor.name.as_ref()),
-                            });
-                        }
-                        *cursor = next;
-                        seen.insert(cursor.name.clone());
-                        const MAX_REF_DEPTH: usize = 5;
-                        if seen.len() == MAX_REF_DEPTH {
-                            return Err(peel::to_id::Error::DepthLimitExceeded {
-                                max_depth: MAX_REF_DEPTH,
-                            });
-                        }
-                    }
-                };
+                let mut oid = self.follow_to_object_in_place_packed(store, packed)?;
                 let mut buf = Vec::new();
-                let mut oid = self.target.try_id().expect("peeled ref").to_owned();
                 let peeled_id = loop {
                     let gix_object::Data { kind, data } =
                         objects
@@ -143,6 +133,38 @@ impl ReferenceExt for Reference {
         }
     }
 
+    fn follow_to_object_in_place_packed(
+        &mut self,
+        store: &file::Store,
+        packed: Option<&packed::Buffer>,
+    ) -> Result<ObjectId, peel::to_object::Error> {
+        match self.target {
+            Target::Object(id) => Ok(id),
+            Target::Symbolic(_) => {
+                let mut seen = BTreeSet::new();
+                let cursor = &mut *self;
+                while let Some(next) = cursor.follow_packed(store, packed) {
+                    let next = next?;
+                    if seen.contains(&next.name) {
+                        return Err(peel::to_object::Error::Cycle {
+                            start_absolute: store.reference_path(cursor.name.as_ref()),
+                        });
+                    }
+                    *cursor = next;
+                    seen.insert(cursor.name.clone());
+                    const MAX_REF_DEPTH: usize = 5;
+                    if seen.len() == MAX_REF_DEPTH {
+                        return Err(peel::to_object::Error::DepthLimitExceeded {
+                            max_depth: MAX_REF_DEPTH,
+                        });
+                    }
+                }
+                let oid = self.target.try_id().expect("peeled ref").to_owned();
+                Ok(oid)
+            }
+        }
+    }
+
     fn follow(&self, store: &file::Store) -> Option<Result<Reference, file::find::existing::Error>> {
         let packed = match store
             .assure_packed_refs_uptodate()
@@ -159,21 +181,14 @@ impl ReferenceExt for Reference {
         store: &file::Store,
         packed: Option<&packed::Buffer>,
     ) -> Option<Result<Reference, file::find::existing::Error>> {
-        match self.peeled {
-            Some(peeled) => Some(Ok(Reference {
-                name: self.name.clone(),
-                target: Target::Object(peeled),
-                peeled: None,
-            })),
-            None => match &self.target {
-                Target::Object(_) => None,
-                Target::Symbolic(full_name) => match store.try_find_packed(full_name.as_ref(), packed) {
-                    Ok(Some(next)) => Some(Ok(next)),
-                    Ok(None) => Some(Err(file::find::existing::Error::NotFound {
-                        name: full_name.to_path().to_owned(),
-                    })),
-                    Err(err) => Some(Err(file::find::existing::Error::Find(err))),
-                },
+        match &self.target {
+            Target::Object(_) => None,
+            Target::Symbolic(full_name) => match store.try_find_packed(full_name.as_ref(), packed) {
+                Ok(Some(next)) => Some(Ok(next)),
+                Ok(None) => Some(Err(file::find::existing::Error::NotFound {
+                    name: full_name.to_path().to_owned(),
+                })),
+                Err(err) => Some(Err(file::find::existing::Error::Find(err))),
             },
         }
     }
diff --git a/gix-ref/tests/file/mod.rs b/gix-ref/tests/file/mod.rs
@@ -18,6 +18,11 @@ pub fn store_at(name: &str) -> crate::Result<Store> {
     Ok(Store::at(path.join(".git"), Default::default()))
 }
 
+pub fn store_at_with_args(name: &str, args: impl IntoIterator<Item = impl Into<String>>) -> crate::Result<Store> {
+    let path = gix_testtools::scripted_fixture_read_only_with_args_standalone(name, args)?;
+    Ok(Store::at(path.join(".git"), Default::default()))
+}
+
 fn store_writable(name: &str) -> crate::Result<(gix_testtools::tempfile::TempDir, Store)> {
     let dir = gix_testtools::scripted_fixture_writable_standalone(name)?;
     let git_dir = dir.path().join(".git");
diff --git a/gix-ref/tests/file/reference.rs b/gix-ref/tests/file/reference.rs
@@ -94,25 +94,37 @@ mod peel {
     fn peel_one_level_with_pack() -> crate::Result {
         let store = store_with_packed_refs()?;
 
-        let head = store.find("dt1")?;
+        let mut head = store.find("dt1")?;
         assert_eq!(
             head.target.try_id().map(ToOwned::to_owned),
             Some(hex_to_id("4c3f4cce493d7beb45012e478021b5f65295e5a3"))
         );
         assert_eq!(
             head.kind(),
             gix_ref::Kind::Object,
-            "its peeled, but does have another step to peel to"
+            "its peeled, but does have another step to peel to…"
+        );
+        let final_stop = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
+        assert_eq!(head.peeled, Some(final_stop), "…it knows its peeled object");
+
+        assert_eq!(
+            head.follow(&store).transpose()?,
+            None,
+            "but following doesn't do that, only real peeling does"
         );
 
-        let peeled = head.follow(&store).expect("a peeled ref for the object")?;
+        head.peel_to_id_in_place(&store, &EmptyCommit)?;
         assert_eq!(
-            peeled.target.try_id().map(ToOwned::to_owned),
-            Some(hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03")),
+            head.target.try_id().map(ToOwned::to_owned),
+            Some(final_stop),
             "packed refs are always peeled (at least the ones we choose to read)"
         );
-        assert_eq!(peeled.kind(), gix_ref::Kind::Object, "it's terminally peeled now");
-        assert!(peeled.follow(&store).is_none());
+        assert_eq!(head.kind(), gix_ref::Kind::Object, "it's terminally peeled now");
+        assert_eq!(
+            head.follow(&store).transpose()?,
+            None,
+            "following doesn't change anything"
+        );
         Ok(())
     }
 
@@ -146,17 +158,34 @@ mod peel {
 
     #[test]
     fn to_id_long_jump() -> crate::Result {
-        let store = file::store_at("make_multi_hop_ref.sh")?;
-        let odb = gix_odb::at(store.git_dir().join("objects"))?;
-        let mut r: Reference = store.find("multi-hop")?;
-        r.peel_to_id_in_place(&store, &odb)?;
+        for packed in [None, Some("packed")] {
+            let store = file::store_at_with_args("make_multi_hop_ref.sh", packed)?;
+            let odb = gix_odb::at(store.git_dir().join("objects"))?;
+            let mut r: Reference = store.find("multi-hop")?;
+            r.peel_to_id_in_place(&store, &odb)?;
 
-        let commit = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
-        assert_eq!(r.peeled, Some(commit));
+            let commit_id = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
+            assert_eq!(r.peeled, Some(commit_id));
 
-        let mut buf = Vec::new();
-        let obj = odb.find(&commit, &mut buf)?;
-        assert_eq!(obj.kind, gix_object::Kind::Commit, "always peeled to the first non-tag");
+            let mut buf = Vec::new();
+            let obj = odb.find(&commit_id, &mut buf)?;
+            assert_eq!(obj.kind, gix_object::Kind::Commit, "always peeled to the first non-tag");
+
+            let mut r: Reference = store.find("multi-hop")?;
+            let tag_id =
+                r.follow_to_object_in_place_packed(&store, store.cached_packed_buffer()?.as_ref().map(|p| &***p))?;
+            let obj = odb.find(&tag_id, &mut buf)?;
+            assert_eq!(obj.kind, gix_object::Kind::Tag, "the first direct object target");
+            assert_eq!(
+                obj.decode()?.into_tag().expect("tag").name,
+                "dt2",
+                "this is the first annotated tag, which points at dt1"
+            );
+            let mut r: Reference = store.find("multi-hop2")?;
+            let other_tag_id =
+                r.follow_to_object_in_place_packed(&store, store.cached_packed_buffer()?.as_ref().map(|p| &***p))?;
+            assert_eq!(other_tag_id, tag_id, "it can follow with multiple hops as well");
+        }
         Ok(())
     }
 
@@ -169,9 +198,15 @@ mod peel {
 
         assert!(matches!(
             r.peel_to_id_in_place(&store, &gix_object::find::Never).unwrap_err(),
-            gix_ref::peel::to_id::Error::Cycle { .. }
+            gix_ref::peel::to_id::Error::FollowToObject(gix_ref::peel::to_object::Error::Cycle { .. })
         ));
         assert_eq!(r.name.as_bstr(), "refs/loop-a", "the ref is not changed on error");
+
+        let mut r: Reference = store.find_loose("loop-a")?.into();
+        let err = r
+            .follow_to_object_in_place_packed(&store, store.cached_packed_buffer()?.as_ref().map(|p| &***p))
+            .unwrap_err();
+        assert!(matches!(err, gix_ref::peel::to_object::Error::Cycle { .. }));
         Ok(())
     }
 }
diff --git a/gix-ref/tests/fixtures/generated-archives/.gitignore b/gix-ref/tests/fixtures/generated-archives/.gitignore
@@ -1,3 +1,3 @@
 make_worktree_repo.tar
 make_worktree_repo_packed.tar
-make_multi_hop_ref.tar
+make_multi_hop_ref*.tar
diff --git a/gix-ref/tests/fixtures/make_multi_hop_ref.sh b/gix-ref/tests/fixtures/make_multi_hop_ref.sh
@@ -11,3 +11,8 @@ git tag -m "tag object" dt1
 git tag -m "tag object indirect" dt2 dt1
 
 echo "ref: refs/tags/dt2" > .git/refs/multi-hop
+echo "ref: refs/multi-hop" > .git/refs/multi-hop2
+
+if [ "${1:-}" = "packed" ]; then
+  git pack-refs --all --prune
+fi
