feat!: improve head() peeling API

Previously it was partially untested and it was hard to obtain an object of choice.

Further breaking changes:

* rename `Head::peeled()` to `into_peeled_id()`
* rename `Head::into_fully_peeled_id()` to `try_peel_into_id()`
* rename `Head::peel_to_id_in_place()` to `Head::try_peel_to_id_in_place()`

Author: Byron

gix/examples/stats.rs

@@ -11,8 +11,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
     let mut most_recent_commit_id = None;
     let num_commits = repo
         .head()?
-        .into_fully_peeled_id()
-        .ok_or("Cannot provide meaningful stats on empty repos")??
+        .try_into_peeled_id()?
+        .ok_or("Cannot provide meaningful stats on empty repos")?
         .ancestors()
         .all()?
         .map_while(Result::ok)

gix/src/clone/checkout.rs

@@ -87,7 +87,7 @@ pub mod main_worktree {
             let workdir = repo.work_dir().ok_or_else(|| Error::BareRepository {
                 git_dir: repo.git_dir().to_owned(),
             })?;
-            let root_tree = match repo.head()?.peel_to_id_in_place().transpose()? {
+            let root_tree = match repo.head()?.try_peel_to_id_in_place()? {
                 Some(id) => id.object().expect("downloaded from remote").peel_to_tree()?.id,
                 None => {
                     return Ok((

gix/src/head/peel.rs

@@ -1,12 +1,14 @@
 use crate::{
     ext::{ObjectIdExt, ReferenceExt},
+    head::Kind,
     Head,
 };
 
 mod error {
     use crate::{object, reference};
 
-    /// The error returned by [`Head::peel_to_id_in_place()`][super::Head::peel_to_id_in_place()] and [`Head::into_fully_peeled_id()`][super::Head::into_fully_peeled_id()].
+    /// The error returned by [`Head::peel_to_id_in_place()`](super::Head::try_peel_to_id_in_place())
+    /// and [`Head::into_fully_peeled_id()`](super::Head::try_into_peeled_id()).
     #[derive(Debug, thiserror::Error)]
     #[allow(missing_docs)]
     pub enum Error {
@@ -19,7 +21,22 @@ mod error {
 
 pub use error::Error;
 
-use crate::head::Kind;
+///
+pub mod into_id {
+    use crate::object;
+
+    /// The error returned by [`Head::into_peeled_id()`](super::Head::into_peeled_id()).
+    #[derive(Debug, thiserror::Error)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        Peel(#[from] super::Error),
+        #[error("Branch '{name}' does not have any commits")]
+        Unborn { name: gix_ref::FullName },
+        #[error(transparent)]
+        ObjectKind(#[from] object::try_into::Error),
+    }
+}
 
 ///
 pub mod to_commit {
@@ -39,85 +56,79 @@ pub mod to_commit {
 }
 
 impl<'repo> Head<'repo> {
-    // TODO: tests
-    /// Peel this instance to make obtaining its final target id possible, while returning an error on unborn heads.
-    pub fn peeled(mut self) -> Result<Self, Error> {
-        self.peel_to_id_in_place().transpose()?;
-        Ok(self)
+    /// Peel this instance and consume to make obtaining its final target id possible, while returning an error on unborn heads.
+    ///
+    /// The final target is obtained by following symbolic references and peeling tags to their final destination, which
+    /// typically is a commit, but can be any object.
+    pub fn into_peeled_id(mut self) -> Result<crate::Id<'repo>, into_id::Error> {
+        self.try_peel_to_id_in_place()?;
+        self.id().ok_or_else(|| match self.kind {
+            Kind::Symbolic(gix_ref::Reference { name, .. }) | Kind::Unborn(name) => into_id::Error::Unborn { name },
+            Kind::Detached { .. } => unreachable!("id can be returned after peeling"),
+        })
+    }
+
+    /// Consume this instance and transform it into the final object that it points to, or `Ok(None)` if the `HEAD`
+    /// reference is yet to be born.
+    ///
+    /// The final target is obtained by following symbolic references and peeling tags to their final destination, which
+    /// typically is a commit, but can be any object.
+    pub fn try_into_peeled_id(mut self) -> Result<Option<crate::Id<'repo>>, Error> {
+        self.try_peel_to_id_in_place()
     }
 
-    // TODO: tests
-    // TODO: Fix this! It's not consistently peeling tags. The whole peeling business should be reconsidered to do what people usually
-    //       want which is to peel references, if present, and then peel objects with control over which object type to end at.
-    //       Finding a good interface for that isn't easy as ideally, it's an iterator that shows the intermediate objects so the user
-    //       can select which tag of a chain to choose.
     /// Follow the symbolic reference of this head until its target object and peel it by following tag objects until there is no
     /// more object to follow, and return that object id.
     ///
-    /// Returns `None` if the head is unborn.
-    pub fn peel_to_id_in_place(&mut self) -> Option<Result<crate::Id<'repo>, Error>> {
-        Some(match &mut self.kind {
-            Kind::Unborn(_name) => return None,
+    /// Returns `Ok(None)` if the head is unborn.
+    ///
+    /// The final target is obtained by following symbolic references and peeling tags to their final destination, which
+    /// typically is a commit, but can be any object.
+    pub fn try_peel_to_id_in_place(&mut self) -> Result<Option<crate::Id<'repo>>, Error> {
+        Ok(Some(match &mut self.kind {
+            Kind::Unborn(_name) => return Ok(None),
             Kind::Detached {
                 peeled: Some(peeled), ..
-            } => Ok((*peeled).attach(self.repo)),
+            } => (*peeled).attach(self.repo),
             Kind::Detached { peeled: None, target } => {
-                match target
-                    .attach(self.repo)
-                    .object()
-                    .map_err(Into::into)
-                    .and_then(|obj| obj.peel_tags_to_end().map_err(Into::into))
-                    .map(|peeled| peeled.id)
-                {
-                    Ok(peeled) => {
-                        self.kind = Kind::Detached {
-                            peeled: Some(peeled),
-                            target: *target,
-                        };
-                        Ok(peeled.attach(self.repo))
+                let id = target.attach(self.repo);
+                if id.header()?.kind() == gix_object::Kind::Commit {
+                    id
+                } else {
+                    match id.object()?.peel_tags_to_end() {
+                        Ok(obj) => {
+                            self.kind = Kind::Detached {
+                                peeled: Some(obj.id),
+                                target: *target,
+                            };
+                            obj.id()
+                        }
+                        Err(err) => return Err(err.into()),
                     }
-                    Err(err) => Err(err),
                 }
             }
             Kind::Symbolic(r) => {
                 let mut nr = r.clone().attach(self.repo);
-                let peeled = nr.peel_to_id_in_place().map_err(Into::into);
+                let peeled = nr.peel_to_id_in_place();
                 *r = nr.detach();
-                peeled
+                peeled?
             }
-        })
+        }))
     }
 
-    // TODO: tests
-    // TODO: something similar in `crate::Reference`
     /// Follow the symbolic reference of this head until its target object and peel it by following tag objects until there is no
     /// more object to follow, transform the id into a commit if possible and return that.
     ///
     /// Returns an error if the head is unborn or if it doesn't point to a commit.
     pub fn peel_to_commit_in_place(&mut self) -> Result<crate::Commit<'repo>, to_commit::Error> {
-        let id = self.peel_to_id_in_place().ok_or_else(|| to_commit::Error::Unborn {
-            name: self.referent_name().expect("unborn").to_owned(),
-        })??;
-        id.object()
-            .map_err(|err| to_commit::Error::Peel(Error::FindExistingObject(err)))
-            .and_then(|object| object.try_into_commit().map_err(Into::into))
-    }
-
-    /// Consume this instance and transform it into the final object that it points to, or `None` if the `HEAD`
-    /// reference is yet to be born.
-    pub fn into_fully_peeled_id(self) -> Option<Result<crate::Id<'repo>, Error>> {
-        Some(match self.kind {
-            Kind::Unborn(_name) => return None,
-            Kind::Detached {
-                peeled: Some(peeled), ..
-            } => Ok(peeled.attach(self.repo)),
-            Kind::Detached { peeled: None, target } => target
-                .attach(self.repo)
-                .object()
-                .map_err(Into::into)
-                .and_then(|obj| obj.peel_tags_to_end().map_err(Into::into))
-                .map(|obj| obj.id.attach(self.repo)),
-            Kind::Symbolic(r) => r.attach(self.repo).peel_to_id_in_place().map_err(Into::into),
-        })
+        let id = self
+            .try_peel_to_id_in_place()?
+            .ok_or_else(|| to_commit::Error::Unborn {
+                name: self.referent_name().expect("unborn").to_owned(),
+            })?;
+        Ok(id
+            .object()
+            .map_err(|err| to_commit::Error::Peel(Error::FindExistingObject(err)))?
+            .try_into_commit()?)
     }
 }

gix/src/reference/errors.rs

@@ -43,9 +43,7 @@ pub mod head_id {
         #[error(transparent)]
         Head(#[from] crate::reference::find::existing::Error),
         #[error(transparent)]
-        PeelToId(#[from] crate::head::peel::Error),
-        #[error("Branch '{name}' does not have any commits")]
-        Unborn { name: gix_ref::FullName },
+        PeelToId(#[from] crate::head::peel::into_id::Error),
     }
 }
 

gix/src/repository/reference.rs

@@ -174,20 +174,16 @@ impl crate::Repository {
         .attach(self))
     }
 
-    /// Resolve the `HEAD` reference, follow and peel its target and obtain its object id.
+    /// Resolve the `HEAD` reference, follow and peel its target and obtain its object id,
+    /// following symbolic references and tags until a commit is found.
     ///
     /// Note that this may fail for various reasons, most notably because the repository
     /// is freshly initialized and doesn't have any commits yet.
     ///
     /// Also note that the returned id is likely to point to a commit, but could also
     /// point to a tree or blob. It won't, however, point to a tag as these are always peeled.
     pub fn head_id(&self) -> Result<crate::Id<'_>, reference::head_id::Error> {
-        let mut head = self.head()?;
-        head.peel_to_id_in_place()
-            .ok_or_else(|| reference::head_id::Error::Unborn {
-                name: head.referent_name().expect("unborn").to_owned(),
-            })?
-            .map_err(Into::into)
+        Ok(self.head()?.into_peeled_id()?)
     }
 
     /// Return the name to the symbolic reference `HEAD` points to, or `None` if the head is detached.
@@ -203,7 +199,8 @@ impl crate::Repository {
         Ok(self.head()?.try_into_referent())
     }
 
-    /// Return the commit object the `HEAD` reference currently points to after peeling it fully.
+    /// Return the commit object the `HEAD` reference currently points to after peeling it fully,
+    /// following symbolic references and tags until a commit is found.
     ///
     /// Note that this may fail for various reasons, most notably because the repository
     /// is freshly initialized and doesn't have any commits yet. It could also fail if the
@@ -212,7 +209,8 @@ impl crate::Repository {
         Ok(self.head()?.peel_to_commit_in_place()?)
     }
 
-    /// Return the tree id the `HEAD` reference currently points to after peeling it fully.
+    /// Return the tree id the `HEAD` reference currently points to after peeling it fully,
+    /// following symbolic references and tags until a commit is found.
     ///
     /// Note that this may fail for various reasons, most notably because the repository
     /// is freshly initialized and doesn't have any commits yet. It could also fail if the

gix/tests/fixtures/generated-archives/make_head_repos.tar.xz

@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:1de90b3e0c6547fda6759fb5df5e4bdf410dbe062ef9ef4f972308452c3224d7
+size 11508

gix/tests/fixtures/make_head_repos.sh

@@ -0,0 +1,30 @@
+#!/bin/bash
+set -eu -o pipefail
+
+(git init symbolic && cd symbolic
+  git commit -m "init" --allow-empty
+)
+
+git clone symbolic detached
+(cd detached
+  git remote rm origin
+  git checkout @
+)
+
+git clone symbolic tag-symbolic
+(cd tag-symbolic
+  git tag -a -m make-tag-object point-at-commit HEAD
+  git tag point-at-tag point-at-commit
+  git tag -a -m make-tag-object point-at-tag-start point-at-tag
+  git remote rm origin
+  echo "ref: refs/tags/point-at-tag-start" > .git/HEAD
+)
+
+git clone tag-symbolic tag-detached
+(cd tag-detached
+  git remote rm origin
+  git fetch --tags
+  git rev-parse point-at-tag-start > .git/HEAD.tmp
+  mv .git/HEAD.tmp .git/HEAD
+)
+

gix/tests/head/mod.rs

@@ -1,5 +1,24 @@
-mod into_remote {
+mod peel {
+    use crate::util::{hex_to_id, named_subrepo_opts};
 
+    #[test]
+    fn all_cases() -> crate::Result {
+        let expected_commit = hex_to_id("fafd9d08a839d99db60b222cd58e2e0bfaf1f7b2");
+        for name in ["detached", "symbolic", "tag-detached", "tag-symbolic"] {
+            let repo = named_subrepo_opts("make_head_repos.sh", name, gix::open::Options::isolated())?;
+            assert_eq!(repo.head()?.into_peeled_id()?, expected_commit);
+            assert_eq!(repo.head_id()?, expected_commit);
+            let commit = repo.head_commit()?;
+            assert_eq!(commit.id, expected_commit);
+            assert_eq!(repo.head_tree_id()?, commit.tree_id()?);
+            assert_eq!(repo.head()?.try_into_peeled_id()?.expect("born"), expected_commit);
+            assert_eq!(repo.head()?.try_peel_to_id_in_place()?.expect("born"), expected_commit);
+        }
+        Ok(())
+    }
+}
+
+mod into_remote {
     use crate::remote;
 
     #[test]

gix/tests/id/mod.rs

@@ -75,7 +75,7 @@ mod ancestors {
     fn all() -> crate::Result {
         let repo = crate::repo("make_repo_with_fork_and_dates.sh")?.to_thread_local();
         for use_commit_graph in [false, true] {
-            let head = repo.head()?.into_fully_peeled_id().expect("born")?;
+            let head = repo.head()?.into_peeled_id()?;
             let commits_graph_order = head
                 .ancestors()
                 .use_commit_graph(use_commit_graph)
@@ -118,7 +118,7 @@ mod ancestors {
     fn pre_epoch() -> crate::Result {
         let repo = crate::repo("make_pre_epoch_repo.sh")?.to_thread_local();
         for use_commit_graph in [false, true] {
-            let head = repo.head()?.into_fully_peeled_id().expect("born")?;
+            let head = repo.head()?.into_peeled_id()?;
             let commits = head
                 .ancestors()
                 .sorting(commit::Sorting::ByCommitTimeNewestFirst) // assure we have time set
@@ -137,7 +137,7 @@ mod ancestors {
     #[test]
     fn filtered() -> crate::Result {
         let repo = crate::repo("make_repo_with_fork_and_dates.sh")?.to_thread_local();
-        let head = repo.head()?.into_fully_peeled_id().expect("born")?;
+        let head = repo.head()?.into_peeled_id()?;
 
         for use_commit_graph in [false, true] {
             for sorting in [

gix/tests/repository/object.rs

@@ -128,7 +128,7 @@ mod find {
                 2 => repo.object_cache_size(128 * 1024),
                 _ => unreachable!("BUG"),
             }
-            for commit_id in repo.head()?.peeled()?.id().expect("born").ancestors().all()? {
+            for commit_id in repo.head()?.into_peeled_id()?.ancestors().all()? {
                 let commit = commit_id?;
                 assert_eq!(commit.id().object()?.kind, gix_object::Kind::Commit);
                 assert_eq!(commit.id().header()?.kind(), gix_object::Kind::Commit);
@@ -363,7 +363,7 @@ mod commit {
             vec!["commit: hello there", "commit: c2", "commit (initial): c1"],
             "we get the actual HEAD log, not the log of some reference"
         );
-        let current_commit = repo.head()?.into_fully_peeled_id().expect("born")?;
+        let current_commit = repo.head()?.into_peeled_id()?;
         assert_eq!(current_commit, first_commit_id, "the commit was set");
 
         let second_commit_id = repo.commit(