Merge pull request #2163 from cruessler/deprecate-in-place-methods

feat: replace peel_to_x_in_place by peel_to_x

Author: Byron

gix-blame/tests/blame.rs

@@ -166,10 +166,10 @@ impl Fixture {
 
         let mut reference = gix_ref::file::Store::find(&store, "HEAD")?;
 
-        // Needed for `peel_to_id_in_place`.
+        // Needed for `peel_to_id`.
         use gix_ref::file::ReferenceExt;
 
-        let head_id = reference.peel_to_id_in_place(&store, &odb)?;
+        let head_id = reference.peel_to_id(&store, &odb)?;
 
         let git_dir = worktree_path.join(".git");
         let index = gix_index::File::at(git_dir.join("index"), gix_hash::Kind::Sha1, false, Default::default())?;

gix-negotiate/tests/baseline/mod.rs

@@ -36,7 +36,7 @@ fn run() -> crate::Result {
                     .iter()
                     .filter_map(|name| {
                         refs.try_find(*name).expect("one tag per commit").map(|mut r| {
-                            r.peel_to_id_in_place(&refs, &store).expect("works");
+                            r.peel_to_id(&refs, &store).expect("works");
                             r.target.into_id()
                         })
                     })

gix-protocol/src/fetch/negotiate.rs

@@ -373,11 +373,7 @@ fn mark_all_refs_in_repo(
     let _span = gix_trace::detail!("mark_all_refs");
     for local_ref in store.iter()?.all()? {
         let mut local_ref = local_ref?;
-        let id = local_ref.peel_to_id_in_place_packed(
-            store,
-            objects,
-            store.cached_packed_buffer()?.as_ref().map(|b| &***b),
-        )?;
+        let id = local_ref.peel_to_id_packed(store, objects, store.cached_packed_buffer()?.as_ref().map(|b| &***b))?;
         let mut is_complete = false;
         if let Some(commit) = graph
             .get_or_insert_commit(id, |md| {

gix-ref/src/store/file/raw_ext.rs

@@ -26,29 +26,64 @@ pub trait ReferenceExt: Sealed {
     ///
     /// 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.
+    #[deprecated = "Use `peel_to_id()` instead"]
     fn peel_to_id_in_place(
         &mut self,
         store: &file::Store,
         objects: &dyn gix_object::Find,
     ) -> Result<ObjectId, peel::to_id::Error>;
 
+    /// 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 and `store` to lookup symbolic references.
+    ///
+    /// 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.
+    ///
+    /// Note that this method mutates `self` in place if it does not already point to a
+    /// non-symbolic object.
+    fn peel_to_id(
+        &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
     /// to use for resolving symbolic links.
+    #[deprecated = "Use `peel_to_id_packed()` instead"]
     fn peel_to_id_in_place_packed(
         &mut self,
         store: &file::Store,
         objects: &dyn gix_object::Find,
         packed: Option<&packed::Buffer>,
     ) -> Result<ObjectId, peel::to_id::Error>;
 
+    /// 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_packed(
+        &mut self,
+        store: &file::Store,
+        objects: &dyn gix_object::Find,
+        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.
+    #[deprecated = "Use `follow_to_object_packed()` instead"]
     fn follow_to_object_in_place_packed(
         &mut self,
         store: &file::Store,
         packed: Option<&packed::Buffer>,
     ) -> Result<ObjectId, peel::to_object::Error>;
 
+    /// Like [`ReferenceExt::follow()`], but follows all symbolic references while gracefully handling loops,
+    /// altering this instance in place.
+    fn follow_to_object_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.
@@ -84,28 +119,45 @@ impl ReferenceExt for Reference {
         &mut self,
         store: &file::Store,
         objects: &dyn gix_object::Find,
+    ) -> Result<ObjectId, peel::to_id::Error> {
+        self.peel_to_id(store, objects)
+    }
+
+    fn peel_to_id(
+        &mut self,
+        store: &file::Store,
+        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::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))
+        self.peel_to_id_packed(store, objects, packed.as_ref().map(|b| &***b))
     }
 
     fn peel_to_id_in_place_packed(
         &mut self,
         store: &file::Store,
         objects: &dyn gix_object::Find,
         packed: Option<&packed::Buffer>,
+    ) -> Result<ObjectId, peel::to_id::Error> {
+        self.peel_to_id_packed(store, objects, packed)
+    }
+
+    fn peel_to_id_packed(
+        &mut self,
+        store: &file::Store,
+        objects: &dyn gix_object::Find,
+        packed: Option<&packed::Buffer>,
     ) -> Result<ObjectId, peel::to_id::Error> {
         match self.peeled {
             Some(peeled) => {
                 self.target = Target::Object(peeled.to_owned());
                 Ok(peeled)
             }
             None => {
-                let mut oid = self.follow_to_object_in_place_packed(store, packed)?;
+                let mut oid = self.follow_to_object_packed(store, packed)?;
                 let mut buf = Vec::new();
                 let peeled_id = loop {
                     let gix_object::Data { kind, data } =
@@ -138,6 +190,14 @@ impl ReferenceExt for Reference {
         &mut self,
         store: &file::Store,
         packed: Option<&packed::Buffer>,
+    ) -> Result<ObjectId, peel::to_object::Error> {
+        self.follow_to_object_packed(store, packed)
+    }
+
+    fn follow_to_object_packed(
+        &mut self,
+        store: &file::Store,
+        packed: Option<&packed::Buffer>,
     ) -> Result<ObjectId, peel::to_object::Error> {
         match self.target {
             Target::Object(id) => Ok(id),

gix-ref/tests/refs/file/reference.rs

@@ -81,11 +81,11 @@ mod peel {
         let store = store_with_packed_refs()?;
         let mut head: Reference = store.find_loose("HEAD")?.into();
         let expected = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
-        assert_eq!(head.peel_to_id_in_place(&store, &EmptyCommit)?, expected);
+        assert_eq!(head.peel_to_id(&store, &EmptyCommit)?, expected);
         assert_eq!(head.target.try_id().map(ToOwned::to_owned), Some(expected));
 
         let mut head = store.find("dt1")?;
-        assert_eq!(head.peel_to_id_in_place(&store, &gix_object::find::Never)?, expected);
+        assert_eq!(head.peel_to_id(&store, &gix_object::find::Never)?, expected);
         assert_eq!(head.target.into_id(), expected);
         Ok(())
     }
@@ -113,7 +113,7 @@ mod peel {
             "but following doesn't do that, only real peeling does"
         );
 
-        head.peel_to_id_in_place(&store, &EmptyCommit)?;
+        head.peel_to_id(&store, &EmptyCommit)?;
         assert_eq!(
             head.target.try_id().map(ToOwned::to_owned),
             Some(final_stop),
@@ -135,23 +135,19 @@ mod peel {
         assert_eq!(r.kind(), gix_ref::Kind::Symbolic, "there is something to peel");
 
         let commit = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
-        assert_eq!(r.peel_to_id_in_place(&store, &EmptyCommit)?, commit);
+        assert_eq!(r.peel_to_id(&store, &EmptyCommit)?, commit);
         assert_eq!(r.name.as_bstr(), "refs/remotes/origin/multi-link-target3");
 
         let mut r: Reference = store.find_loose("dt1")?.into();
         assert_eq!(
-            r.peel_to_id_in_place(&store, &EmptyCommit)?,
+            r.peel_to_id(&store, &EmptyCommit)?,
             hex_to_id("4c3f4cce493d7beb45012e478021b5f65295e5a3"),
             "points to a tag object without actual object lookup"
         );
 
         let odb = gix_odb::at(store.git_dir().join("objects"))?;
         let mut r: Reference = store.find_loose("dt1")?.into();
-        assert_eq!(
-            r.peel_to_id_in_place(&store, &odb)?,
-            commit,
-            "points to the commit with lookup"
-        );
+        assert_eq!(r.peel_to_id(&store, &odb)?, commit, "points to the commit with lookup");
 
         Ok(())
     }
@@ -162,7 +158,7 @@ mod peel {
             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)?;
+            r.peel_to_id(&store, &odb)?;
 
             let commit_id = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
             assert_eq!(r.peeled, Some(commit_id));
@@ -172,8 +168,7 @@ mod peel {
             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 tag_id = r.follow_to_object_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!(
@@ -183,7 +178,7 @@ mod peel {
             );
             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))?;
+                r.follow_to_object_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(())
@@ -197,14 +192,14 @@ mod peel {
         assert_eq!(r.name.as_bstr(), "refs/loop-a");
 
         assert!(matches!(
-            r.peel_to_id_in_place(&store, &gix_object::find::Never).unwrap_err(),
+            r.peel_to_id(&store, &gix_object::find::Never).unwrap_err(),
             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))
+            .follow_to_object_packed(&store, store.cached_packed_buffer()?.as_ref().map(|p| &***p))
             .unwrap_err();
         assert!(matches!(err, gix_ref::peel::to_object::Error::Cycle { .. }));
         Ok(())

gix-ref/tests/refs/file/worktree.rs

@@ -60,7 +60,7 @@ fn into_peel(
     store: &gix_ref::file::Store,
     odb: gix_odb::Handle,
 ) -> impl Fn(gix_ref::Reference) -> gix_hash::ObjectId + '_ {
-    move |mut r: gix_ref::Reference| r.peel_to_id_in_place(store, &odb).unwrap()
+    move |mut r: gix_ref::Reference| r.peel_to_id(store, &odb).unwrap()
 }
 
 enum Mode {

gix/src/clone/checkout.rs

@@ -97,7 +97,7 @@ pub mod main_worktree {
             })?;
 
             let root_tree_id = match &self.ref_name {
-                Some(reference_val) => Some(repo.find_reference(reference_val)?.peel_to_id_in_place()?),
+                Some(reference_val) => Some(repo.find_reference(reference_val)?.peel_to_id()?),
                 None => repo.head()?.try_peel_to_id_in_place()?,
             };
 

gix/src/commit.rs

@@ -122,7 +122,7 @@ pub mod describe {
                     .filter_map(Result::ok)
                     .filter_map(|mut r: crate::Reference<'_>| {
                         let target_id = r.target().try_id().map(ToOwned::to_owned);
-                        let peeled_id = r.peel_to_id_in_place().ok()?;
+                        let peeled_id = r.peel_to_id().ok()?;
                         let (prio, tag_time) = match target_id {
                             Some(target_id) if peeled_id != *target_id => {
                                 let tag = repo.find_object(target_id).ok()?.try_into_tag().ok()?;

gix/src/head/peel.rs

@@ -128,7 +128,7 @@ impl<'repo> Head<'repo> {
             }
             Kind::Symbolic(r) => {
                 let mut nr = r.clone().attach(self.repo);
-                let peeled = nr.peel_to_id_in_place();
+                let peeled = nr.peel_to_id();
                 *r = nr.detach();
                 peeled?
             }

gix/src/reference/iter.rs

@@ -112,13 +112,9 @@ impl<'r> Iterator for Iter<'_, 'r> {
                 .and_then(|mut r| {
                     if self.peel {
                         let repo = &self.repo;
-                        r.peel_to_id_in_place_packed(
-                            &repo.refs,
-                            &repo.objects,
-                            self.peel_with_packed.as_ref().map(|p| &***p),
-                        )
-                        .map_err(|err| Box::new(err) as Box<dyn std::error::Error + Send + Sync + 'static>)
-                        .map(|_| r)
+                        r.peel_to_id_packed(&repo.refs, &repo.objects, self.peel_with_packed.as_ref().map(|p| &***p))
+                            .map_err(|err| Box::new(err) as Box<dyn std::error::Error + Send + Sync + 'static>)
+                            .map(|_| r)
                     } else {
                         Ok(r)
                     }