From 6d023e3cbed6a24821ab8a1d36084a350a39415b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 18 Apr 2022 13:00:05 +0800 Subject: [PATCH 1/5] reproduce commit-describe name ordering issue (#389) --- git-repository/tests/commit/mod.rs | 20 +++++++++++++++++++ .../make_commit_describe_multiple_tags.tar.xz | 3 +++ .../make_commit_describe_multiple_tags.sh | 10 ++++++++++ git-repository/tests/git.rs | 1 + 4 files changed, 34 insertions(+) create mode 100644 git-repository/tests/commit/mod.rs create mode 100644 git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz create mode 100644 git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh diff --git a/git-repository/tests/commit/mod.rs b/git-repository/tests/commit/mod.rs new file mode 100644 index 00000000000..c2dc6d388d1 --- /dev/null +++ b/git-repository/tests/commit/mod.rs @@ -0,0 +1,20 @@ +mod describe { + use crate::named_repo; + + #[test] + #[ignore] + fn tags_are_sorted_by_date_and_lexigraphically() { + let repo = named_repo("make_commit_describe_multiple_tags.sh").unwrap(); + assert_eq!( + repo.head_commit() + .unwrap() + .describe() + .format() + .unwrap() + .name + .expect("name set") + .as_ref(), + "v1" + ); + } +} diff --git a/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz b/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz new file mode 100644 index 00000000000..776b5cfb163 --- /dev/null +++ b/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:28fb09e31eca8d5c43d5b93918691ef3cc9007d97edb73ef803e505353f3ed46 +size 10396 diff --git a/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh b/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh new file mode 100644 index 00000000000..89b9e95dc1c --- /dev/null +++ b/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh @@ -0,0 +1,10 @@ +#!/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_COMMITTER_DATE="2022-01-02 00:00:00 +0000" git tag v2 -m "tag object 2" diff --git a/git-repository/tests/git.rs b/git-repository/tests/git.rs index 6610ce02798..5bd0fdbfb48 100644 --- a/git-repository/tests/git.rs +++ b/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; From 0d22ab459ce14bc57549270142595d8ebd98ea41 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 18 Apr 2022 13:20:38 +0800 Subject: [PATCH 2/5] feat: `TagRefIter::tagger()`. (#389) Additionally ergonomics have been improved as the iterator is now `Copy`, similarly to the other iterators. --- git-object/src/commit/ref_iter.rs | 3 --- git-object/src/lib.rs | 1 + git-object/src/tag/ref_iter.rs | 14 +++++++++++++- git-object/tests/immutable/tag.rs | 14 +++++++++----- 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/git-object/src/commit/ref_iter.rs b/git-object/src/commit/ref_iter.rs index d934c84ce99..7b3d34329b7 100644 --- a/git-object/src/commit/ref_iter.rs +++ b/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, 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, 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)), diff --git a/git-object/src/lib.rs b/git-object/src/lib.rs index 06d8caa79c7..40551d9554b 100644 --- a/git-object/src/lib.rs +++ b/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, diff --git a/git-object/src/tag/ref_iter.rs b/git-object/src/tag/ref_iter.rs index 94e6f654919..76b3e4a1e18 100644 --- a/git-object/src/tag/ref_iter.rs +++ b/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 { + pub fn target_id(mut self) -> Result { 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>, 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 { diff --git a/git-object/tests/immutable/tag.rs b/git-object/tests/immutable/tag.rs index bd7687c4dd8..d3457a4ea60 100644 --- a/git-object/tests/immutable/tag.rs +++ b/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::, _>>()?, + tag_iter.collect::, _>>()?, 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(()) } From 35019f282ca7f91bef11cacd03117a756a1bd9f2 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 18 Apr 2022 14:26:17 +0800 Subject: [PATCH 3/5] first crude fix (#389) --- git-repository/src/commit.rs | 36 +++++++++++++++++++++--------- git-repository/src/object/tag.rs | 7 +++++- git-repository/tests/commit/mod.rs | 3 ++- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/git-repository/src/commit.rs b/git-repository/src/commit.rs index d3175990552..f3ebb9279f0 100644 --- a/git-repository/src/commit.rs +++ b/git-repository/src/commit.rs @@ -86,17 +86,31 @@ pub mod describe { .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())) - }) - .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(); + // TODO: this kind of sorting would also have to apply to the AllRefs/AlLTags cases, where annotated tags are preferred + // and sorted accordingly. + peeled_commits_and_tag_date.sort_by(|a, b| a.1.cmp(&b.1).reverse()); // by time, ascending, causing older names to overwrite newer ones. + peeled_commits_and_tag_date + .into_iter() + .map(|(a, _, c)| (a, c)) + .collect() + } }) } } diff --git a/git-repository/src/object/tag.rs b/git-repository/src/object/tag.rs index f7532aa1f27..d2684ee9820 100644 --- a/git-repository/src/object/tag.rs +++ b/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, 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>, git_object::decode::Error> { + git_object::TagRefIter::from_bytes(&self.data).tagger() + } } diff --git a/git-repository/tests/commit/mod.rs b/git-repository/tests/commit/mod.rs index c2dc6d388d1..7c8a8d86583 100644 --- a/git-repository/tests/commit/mod.rs +++ b/git-repository/tests/commit/mod.rs @@ -1,14 +1,15 @@ mod describe { use crate::named_repo; + use git_repository::commit::describe::SelectRef::AnnotatedTags; #[test] - #[ignore] fn tags_are_sorted_by_date_and_lexigraphically() { let repo = named_repo("make_commit_describe_multiple_tags.sh").unwrap(); assert_eq!( repo.head_commit() .unwrap() .describe() + .names(AnnotatedTags) .format() .unwrap() .name From 0d9f6c6687d7b2a4c473daa1115c100ef40369e7 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 18 Apr 2022 15:16:35 +0800 Subject: [PATCH 4/5] test all cases for the names filter in describe (#389) --- git-repository/src/commit.rs | 7 ++++--- git-repository/tests/commit/mod.rs | 22 +++++++++------------- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/git-repository/src/commit.rs b/git-repository/src/commit.rs index f3ebb9279f0..df1a29b2841 100644 --- a/git-repository/src/commit.rs +++ b/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, @@ -160,7 +161,7 @@ pub mod describe { /// if one was found. /// /// Note that there will always be `Some(format)` - pub fn try_format(self) -> Result>, Error> { + pub fn try_format(&self) -> Result>, Error> { self.try_resolve()?.map(|r| r.format()).transpose() } @@ -168,7 +169,7 @@ pub mod describe { /// if one was found. /// /// The outcome provides additional information, but leaves the caller with the burden - pub fn try_resolve(self) -> Result>, Error> { + pub fn try_resolve(&self) -> Result>, Error> { // TODO: dirty suffix with respective dirty-detection let outcome = git_revision::describe( &self.id, @@ -194,7 +195,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, Error> { + pub fn format(&mut self) -> Result, Error> { self.id_as_fallback = true; Ok(self.try_format()?.expect("BUG: fallback must always produce a format")) } diff --git a/git-repository/tests/commit/mod.rs b/git-repository/tests/commit/mod.rs index 7c8a8d86583..350a0f07c6b 100644 --- a/git-repository/tests/commit/mod.rs +++ b/git-repository/tests/commit/mod.rs @@ -1,21 +1,17 @@ mod describe { use crate::named_repo; - use git_repository::commit::describe::SelectRef::AnnotatedTags; + use git_repository::commit::describe::SelectRef::{AllRefs, AllTags, AnnotatedTags}; #[test] + #[ignore] fn tags_are_sorted_by_date_and_lexigraphically() { let repo = named_repo("make_commit_describe_multiple_tags.sh").unwrap(); - assert_eq!( - repo.head_commit() - .unwrap() - .describe() - .names(AnnotatedTags) - .format() - .unwrap() - .name - .expect("name set") - .as_ref(), - "v1" - ); + 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(), "v1", "{:?}", filter); + } + let mut describe = describe.names(AllRefs); + assert_eq!(describe.format().unwrap().to_string(), "v1"); } } From a1680b44ef568317465d2971da6e0930f9885530 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 18 Apr 2022 15:59:28 +0800 Subject: [PATCH 5/5] fix: `Commit::describe()` now returns annnotated tags before lighweight ones and prefers more recent ones as well (#389) Fixes #389 --- git-repository/src/commit.rs | 45 ++++++++++++------- git-repository/tests/commit/mod.rs | 5 +-- .../make_commit_describe_multiple_tags.tar.xz | 4 +- .../make_commit_describe_multiple_tags.sh | 1 + 4 files changed, 33 insertions(+), 22 deletions(-) diff --git a/git-repository/src/commit.rs b/git-repository/src/commit.rs index df1a29b2841..e944db8e8d4 100644 --- a/git-repository/src/commit.rs +++ b/git-repository/src/commit.rs @@ -71,22 +71,37 @@ pub mod describe { repo: &Repository, ) -> Result>, 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(), + .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(); + 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()? @@ -104,9 +119,7 @@ pub mod describe { Some((commit_id, tag_time, r.name().shorten().to_owned().into())) }) .collect(); - // TODO: this kind of sorting would also have to apply to the AllRefs/AlLTags cases, where annotated tags are preferred - // and sorted accordingly. - peeled_commits_and_tag_date.sort_by(|a, b| a.1.cmp(&b.1).reverse()); // by time, ascending, causing older names to overwrite newer ones. + 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)) diff --git a/git-repository/tests/commit/mod.rs b/git-repository/tests/commit/mod.rs index 350a0f07c6b..1f4e1e27a13 100644 --- a/git-repository/tests/commit/mod.rs +++ b/git-repository/tests/commit/mod.rs @@ -3,15 +3,12 @@ mod describe { use git_repository::commit::describe::SelectRef::{AllRefs, AllTags, AnnotatedTags}; #[test] - #[ignore] 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(), "v1", "{:?}", filter); + assert_eq!(describe.format().unwrap().to_string(), "v2", "{:?}", filter); } - let mut describe = describe.names(AllRefs); - assert_eq!(describe.format().unwrap().to_string(), "v1"); } } diff --git a/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz b/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz index 776b5cfb163..19c70cb4076 100644 --- a/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz +++ b/git-repository/tests/fixtures/generated-archives/make_commit_describe_multiple_tags.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:28fb09e31eca8d5c43d5b93918691ef3cc9007d97edb73ef803e505353f3ed46 -size 10396 +oid sha256:22fd60100187fc3f4180b31d3ae1d8c5506a65c3ee059b4bbc3a40009fb23f60 +size 10408 diff --git a/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh b/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh index 89b9e95dc1c..cdafd0b9892 100644 --- a/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh +++ b/git-repository/tests/fixtures/make_commit_describe_multiple_tags.sh @@ -7,4 +7,5 @@ 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"