git-repository: describe does not return more recent tag, in case of duplicate tags

git-object/src/commit/ref_iter.rs

@@ -77,7 +77,6 @@ impl<'a> CommitRefIter<'a> {
     }
 
     /// Returns the committer signature if there is no decoding error.
-    /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
     pub fn committer(mut self) -> Result<git_actor::SignatureRef<'a>, crate::decode::Error> {
         self.find_map(|t| match t {
             Ok(Token::Committer { signature }) => Some(Ok(signature)),
@@ -90,7 +89,6 @@ impl<'a> CommitRefIter<'a> {
     /// Returns the author signature if there is no decoding error.
     ///
     /// It may contain white space surrounding it, and is exactly as parsed.
-    /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
     pub fn author(mut self) -> Result<git_actor::SignatureRef<'a>, crate::decode::Error> {
         self.find_map(|t| match t {
             Ok(Token::Author { signature }) => Some(Ok(signature)),
@@ -104,7 +102,6 @@ impl<'a> CommitRefIter<'a> {
     ///
     /// It may contain white space surrounding it, and is exactly as
     //  parsed.
-    /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
     pub fn message(mut self) -> Result<&'a BStr, crate::decode::Error> {
         self.find_map(|t| match t {
             Ok(Token::Message(msg)) => Some(Ok(msg)),

git-object/src/lib.rs

@@ -148,6 +148,7 @@ pub struct TagRef<'a> {
 
 /// Like [`TagRef`], but as `Iterator` to support entirely allocation free parsing.
 /// It's particularly useful to dereference only the target chain.
+#[derive(Copy, Clone)]
 pub struct TagRefIter<'a> {
     data: &'a [u8],
     state: tag::ref_iter::State,

git-object/src/tag/ref_iter.rs

@@ -9,6 +9,7 @@ use nom::{
 
 use crate::{bstr::ByteSlice, parse, parse::NL, tag::decode, Kind, TagRefIter};
 
+#[derive(Copy, Clone)]
 pub(crate) enum State {
     Target,
     TargetKind,
@@ -39,10 +40,21 @@ impl<'a> TagRefIter<'a> {
     /// Errors are coerced into options, hiding whether there was an error or not. The caller should assume an error if they
     /// call the method as intended. Such a squelched error cannot be recovered unless the objects data is retrieved and parsed again.
     /// `next()`.
-    pub fn target_id(&mut self) -> Result<ObjectId, crate::decode::Error> {
+    pub fn target_id(mut self) -> Result<ObjectId, crate::decode::Error> {
         let token = self.next().ok_or_else(missing_field)??;
         Token::into_id(token).ok_or_else(missing_field)
     }
+
+    /// Returns the taggers signature if there is no decoding error, and if this field exists.
+    /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
+    pub fn tagger(mut self) -> Result<Option<git_actor::SignatureRef<'a>>, crate::decode::Error> {
+        self.find_map(|t| match t {
+            Ok(Token::Tagger(signature)) => Some(Ok(signature)),
+            Err(err) => Some(Err(err)),
+            _ => None,
+        })
+        .ok_or_else(missing_field)?
+    }
 }
 
 fn missing_field() -> crate::decode::Error {

git-object/tests/immutable/tag.rs

@@ -26,17 +26,21 @@ mod iter {
 
     #[test]
     fn empty() -> crate::Result {
+        let tag = fixture_bytes("tag", "empty.txt");
+        let tag_iter = TagRefIter::from_bytes(&tag);
+        let target_id = hex_to_id("01dd4e2a978a9f5bd773dae6da7aa4a5ac1cdbbc");
+        let tagger = Some(signature(1592381636));
         assert_eq!(
-            TagRefIter::from_bytes(&fixture_bytes("tag", "empty.txt")).collect::<Result<Vec<_>, _>>()?,
+            tag_iter.collect::<Result<Vec<_>, _>>()?,
             vec![
-                Token::Target {
-                    id: hex_to_id("01dd4e2a978a9f5bd773dae6da7aa4a5ac1cdbbc")
-                },
+                Token::Target { id: target_id },
                 Token::TargetKind(Kind::Commit),
                 Token::Name(b"empty".as_bstr()),
-                Token::Tagger(Some(signature(1592381636))),
+                Token::Tagger(tagger),
             ]
         );
+        assert_eq!(tag_iter.target_id()?, target_id);
+        assert_eq!(tag_iter.tagger()?, tagger);
         Ok(())
     }
 

git-repository/src/commit.rs

@@ -55,6 +55,7 @@ pub mod describe {
     }
 
     /// A selector to choose what kind of references should contribute to names.
+    #[derive(Debug, Clone, Copy, PartialOrd, PartialEq, Ord, Eq, Hash)]
     pub enum SelectRef {
         /// Only use annotated tags for names.
         AnnotatedTags,
@@ -70,33 +71,60 @@ pub mod describe {
             repo: &Repository,
         ) -> Result<git_revision::hash_hasher::HashedMap<ObjectId, Cow<'static, BStr>>, Error> {
             let platform = repo.references()?;
-            let into_tuple =
-                |r: crate::Reference<'_>| (r.inner.target.into_id(), Cow::from(r.inner.name.shorten().to_owned()));
 
             Ok(match self {
-                SelectRef::AllTags => platform
-                    .tags()?
-                    .peeled()
+                SelectRef::AllTags | SelectRef::AllRefs => {
+                    let mut refs: Vec<_> = match self {
+                        SelectRef::AllRefs => platform.all()?,
+                        SelectRef::AllTags => platform.tags()?,
+                        _ => unreachable!(),
+                    }
                     .filter_map(Result::ok)
-                    .map(into_tuple)
-                    .collect(),
-                SelectRef::AllRefs => platform
-                    .all()?
-                    .peeled()
-                    .filter_map(Result::ok)
-                    .map(into_tuple)
-                    .collect(),
-                SelectRef::AnnotatedTags => platform
-                    .tags()?
-                    .filter_map(Result::ok)
-                    .filter_map(|r: crate::Reference<'_>| {
-                        // TODO: we assume direct refs for tags, which is the common case, but it doesn't have to be
-                        //       so rather follow symrefs till the first object and then peel tags after the first object was found.
-                        let tag = r.try_id()?.object().ok()?.try_into_tag().ok()?;
-                        let commit_id = tag.target_id().ok()?.object().ok()?.try_into_commit().ok()?.id;
-                        Some((commit_id, r.name().shorten().to_owned().into()))
+                    .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 (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()?;
+                                (1, tag.tagger().ok()??.time.seconds_since_unix_epoch)
+                            }
+                            _ => (0, 0),
+                        };
+                        (
+                            peeled_id.inner,
+                            prio,
+                            tag_time,
+                            Cow::from(r.inner.name.shorten().to_owned()),
+                        )
+                            .into()
                     })
-                    .collect(),
+                    .collect();
+                    refs.sort_by(|a, b| a.2.cmp(&b.2).then_with(|| a.1.cmp(&b.1))); // by time ascending, then by priority. Older entries overwrite newer ones.
+                    refs.into_iter().map(|(a, _, _, b)| (a, b)).collect()
+                }
+                SelectRef::AnnotatedTags => {
+                    let mut peeled_commits_and_tag_date: Vec<_> = platform
+                        .tags()?
+                        .filter_map(Result::ok)
+                        .filter_map(|r: crate::Reference<'_>| {
+                            // TODO: we assume direct refs for tags, which is the common case, but it doesn't have to be
+                            //       so rather follow symrefs till the first object and then peel tags after the first object was found.
+                            let tag = r.try_id()?.object().ok()?.try_into_tag().ok()?;
+                            let tag_time = tag
+                                .tagger()
+                                .ok()
+                                .and_then(|s| s.map(|s| s.time.seconds_since_unix_epoch))
+                                .unwrap_or(0);
+                            let commit_id = tag.target_id().ok()?.object().ok()?.try_into_commit().ok()?.id;
+                            Some((commit_id, tag_time, r.name().shorten().to_owned().into()))
+                        })
+                        .collect();
+                    peeled_commits_and_tag_date.sort_by(|a, b| a.1.cmp(&b.1)); // by time, ascending, causing older names to overwrite newer ones.
+                    peeled_commits_and_tag_date
+                        .into_iter()
+                        .map(|(a, _, c)| (a, c))
+                        .collect()
+                }
             })
         }
     }
@@ -146,15 +174,15 @@ pub mod describe {
         /// if one was found.
         ///
         /// Note that there will always be `Some(format)`
-        pub fn try_format(self) -> Result<Option<git_revision::describe::Format<'static>>, Error> {
+        pub fn try_format(&self) -> Result<Option<git_revision::describe::Format<'static>>, Error> {
             self.try_resolve()?.map(|r| r.format()).transpose()
         }
 
         /// Try to find a name for the configured commit id using all prior configuration, returning `Some(Outcome)`
         /// if one was found.
         ///
         /// The outcome provides additional information, but leaves the caller with the burden
-        pub fn try_resolve(self) -> Result<Option<crate::commit::describe::Resolution<'repo>>, Error> {
+        pub fn try_resolve(&self) -> Result<Option<crate::commit::describe::Resolution<'repo>>, Error> {
             // TODO: dirty suffix with respective dirty-detection
             let outcome = git_revision::describe(
                 &self.id,
@@ -180,7 +208,7 @@ pub mod describe {
         }
 
         /// Like [`try_format()`][Platform::try_format()], but turns `id_as_fallback()` on to always produce a format.
-        pub fn format(mut self) -> Result<git_revision::describe::Format<'static>, Error> {
+        pub fn format(&mut self) -> Result<git_revision::describe::Format<'static>, Error> {
             self.id_as_fallback = true;
             Ok(self.try_format()?.expect("BUG: fallback must always produce a format"))
         }

git-repository/src/object/tag.rs

@@ -1,10 +1,15 @@
 use crate::{ext::ObjectIdExt, Tag};
 
 impl<'repo> Tag<'repo> {
-    /// Decode this tag and return the id of its target.
+    /// Decode this tag partially and return the id of its target.
     pub fn target_id(&self) -> Result<crate::Id<'repo>, git_object::decode::Error> {
         git_object::TagRefIter::from_bytes(&self.data)
             .target_id()
             .map(|id| id.attach(self.repo))
     }
+
+    /// Decode this tag partially and return the tagger, if the field exists.
+    pub fn tagger(&self) -> Result<Option<git_actor::SignatureRef<'_>>, git_object::decode::Error> {
+        git_object::TagRefIter::from_bytes(&self.data).tagger()
+    }
 }

git-repository/tests/commit/mod.rs

@@ -0,0 +1,14 @@
+mod describe {
+    use crate::named_repo;
+    use git_repository::commit::describe::SelectRef::{AllRefs, AllTags, AnnotatedTags};
+
+    #[test]
+    fn tags_are_sorted_by_date_and_lexigraphically() {
+        let repo = named_repo("make_commit_describe_multiple_tags.sh").unwrap();
+        let mut describe = repo.head_commit().unwrap().describe();
+        for filter in &[AnnotatedTags, AllTags, AllRefs] {
+            describe = describe.names(*filter);
+            assert_eq!(describe.format().unwrap().to_string(), "v2", "{:?}", filter);
+        }
+    }
+}

git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh

@@ -0,0 +1,11 @@
+#!/bin/bash
+set -eu -o pipefail
+
+git init -q
+git commit --allow-empty -q -m c1
+git commit --allow-empty -q -m c2
+
+git tag v0 -m "tag object 0" "HEAD~1"
+git tag v1 -m "tag object 1"
+git tag v1.5
+GIT_COMMITTER_DATE="2022-01-02 00:00:00 +0000" git tag v2 -m "tag object 2"

git-repository/tests/git.rs

@@ -28,6 +28,7 @@ fn basic_rw_repo() -> crate::Result<(Repository, tempfile::TempDir)> {
     repo_rw("make_basic_repo.sh")
 }
 
+mod commit;
 mod discover;
 mod id;
 mod init;