From ce5d79c181babfdb8aa4a0c4cc1db29bfb1dc21a Mon Sep 17 00:00:00 2001 From: Pierre Chevalier Date: Fri, 23 Aug 2024 04:10:49 -0700 Subject: [PATCH 1/5] Add test for tag missing timestamp in tagger (#1542) This tag exists in an opensource repository: https://kernel.googlesource.com/pub/scm/network/bridge/bridge-utils/+/refs/tags/ChangeLog gitoxide can't parse it because it's got a tagger line, but no timestamp. git itself is more lenient about such cases, basically returning a timestamp of 0 if the timestamp is missing: https://github.com/git/git/blob/master/tag.c#L116 This is a new test to show the current behaviour. --- .../fixtures/tag/tagger-without-timestamp.txt | 4 ++++ gix-object/tests/tag/mod.rs | 15 +++++++++++++++ 2 files changed, 19 insertions(+) create mode 100644 gix-object/tests/fixtures/tag/tagger-without-timestamp.txt diff --git a/gix-object/tests/fixtures/tag/tagger-without-timestamp.txt b/gix-object/tests/fixtures/tag/tagger-without-timestamp.txt new file mode 100644 index 00000000000..c271b47254e --- /dev/null +++ b/gix-object/tests/fixtures/tag/tagger-without-timestamp.txt @@ -0,0 +1,4 @@ +object 4fcd840c4935e4c7a5ea3552710a0f26b9178c24 +type commit +tag ChangeLog +tagger shemminger diff --git a/gix-object/tests/tag/mod.rs b/gix-object/tests/tag/mod.rs index d5bdef77b9d..ac0572cce65 100644 --- a/gix-object/tests/tag/mod.rs +++ b/gix-object/tests/tag/mod.rs @@ -229,6 +229,21 @@ KLMHist5yj0sw1E4hDTyQa0= ); Ok(()) } + + #[test] + fn tagger_without_timestamp() -> crate::Result { + // Note: this behaviour is inconsistent with Git's + // Git would successfully interpret this tag and set the timestamp to 0 + // See https://github.com/git/git/blob/3a7362eb9fad0c4838f5cfaa95ed3c51a4c18d93/tag.c#L116 + assert_eq!( + format!( + "{:?}", + TagRef::from_bytes(&fixture_name("tag", "tagger-without-timestamp.txt")) + ), + "Err(Error { inner: () })".to_string() + ); + Ok(()) + } } fn tag_fixture(offset: i32) -> TagRef<'static> { From 3868767a4827effb1e645a43ac1df01e9bc886f0 Mon Sep 17 00:00:00 2001 From: Pierre Chevalier Date: Fri, 23 Aug 2024 04:41:08 -0700 Subject: [PATCH 2/5] Add unit test for missing timestamp in signature (#1542) --- gix-actor/tests/signature/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/gix-actor/tests/signature/mod.rs b/gix-actor/tests/signature/mod.rs index 55effea170d..3c6fc891319 100644 --- a/gix-actor/tests/signature/mod.rs +++ b/gix-actor/tests/signature/mod.rs @@ -105,3 +105,14 @@ fn parse_timestamp_with_trailing_digits() { } ); } + +#[test] +fn parse_missing_timestamp() { + assert_eq!( + format!( + "{:?}", + gix_actor::SignatureRef::from_bytes::<()>(b"first last ") + ), + "Err(Backtrack(()))".to_string() + ); +} From 059c9f9ece4edf631e3ca5f8298ce2660cba946b Mon Sep 17 00:00:00 2001 From: pierrec Date: Fri, 23 Aug 2024 05:40:45 -0700 Subject: [PATCH 3/5] Be lenient to missing timestamp in signature (#1542) --- gix-actor/src/signature/decode.rs | 25 +++++++++++++------------ gix-actor/tests/signature/mod.rs | 13 ++++++++----- gix-object/tests/tag/mod.rs | 29 ++++++++++++++++++++--------- 3 files changed, 41 insertions(+), 26 deletions(-) diff --git a/gix-actor/src/signature/decode.rs b/gix-actor/src/signature/decode.rs index 460e14e84c2..9abb3a62483 100644 --- a/gix-actor/src/signature/decode.rs +++ b/gix-actor/src/signature/decode.rs @@ -6,7 +6,7 @@ pub(crate) mod function { use winnow::error::{ErrMode, ErrorKind}; use winnow::stream::Stream; use winnow::{ - combinator::{alt, separated_pair, terminated}, + combinator::{alt, opt, separated_pair, terminated}, error::{AddContext, ParserError, StrContext}, prelude::*, stream::AsChar, @@ -21,8 +21,8 @@ pub(crate) mod function { ) -> PResult, E> { separated_pair( identity, - b" ", - ( + opt(b" "), + opt(( terminated(take_until(0.., SPACE), take(1usize)) .verify_map(|v| to_signed::(v).ok()) .context(StrContext::Expected("".into())), @@ -38,8 +38,9 @@ pub(crate) mod function { .verify_map(|v| to_signed::(v).ok()) .context(StrContext::Expected("MM".into())), take_while(0.., AsChar::is_dec_digit).map(|v: &[u8]| v), - ) - .map(|(time, sign, hours, minutes, trailing_digits)| { + )) + .map(|maybe_timestamp| { + if let Some((time, sign, hours, minutes, trailing_digits)) = maybe_timestamp { let offset = if trailing_digits.is_empty() { (hours * 3600 + minutes * 60) * if sign == Sign::Minus { -1 } else { 1 } } else { @@ -50,7 +51,10 @@ pub(crate) mod function { offset, sign, } - }), + } else { + Time::new(0, 0) + } + }), ) .context(StrContext::Expected(" <> <+|->".into())) .map(|(identity, time)| SignatureRef { @@ -206,12 +210,9 @@ mod tests { #[test] fn invalid_time() { assert_eq!( - decode.parse_peek(b"hello <> abc -1215") - .map_err(to_bstr_err) - .expect_err("parse fails as > is missing") - .to_string(), - "in predicate verification at 'abc -1215'\n 0: expected `` at 'abc -1215'\n 1: expected ` <> <+|->` at 'hello <> abc -1215'\n" - ); + decode.parse_peek(b"hello <> abc -1215").expect("parse to work").1, + signature("hello", "", 0, Sign::Plus, 0) + ); } } } diff --git a/gix-actor/tests/signature/mod.rs b/gix-actor/tests/signature/mod.rs index 3c6fc891319..15a04f6a6c9 100644 --- a/gix-actor/tests/signature/mod.rs +++ b/gix-actor/tests/signature/mod.rs @@ -108,11 +108,14 @@ fn parse_timestamp_with_trailing_digits() { #[test] fn parse_missing_timestamp() { + let signature = gix_actor::SignatureRef::from_bytes::<()>(b"first last ") + .expect("deal with missing timestamp in signature by zeroing it"); assert_eq!( - format!( - "{:?}", - gix_actor::SignatureRef::from_bytes::<()>(b"first last ") - ), - "Err(Backtrack(()))".to_string() + signature, + SignatureRef { + name: "first last".into(), + email: "name@example.com".into(), + time: gix_actor::date::Time::new(0, 0), + } ); } diff --git a/gix-object/tests/tag/mod.rs b/gix-object/tests/tag/mod.rs index ac0572cce65..923d21d9775 100644 --- a/gix-object/tests/tag/mod.rs +++ b/gix-object/tests/tag/mod.rs @@ -137,9 +137,11 @@ fn invalid() { } mod from_bytes { + use gix_actor::SignatureRef; + use gix_date::Time; use gix_object::{bstr::ByteSlice, Kind, TagRef}; - use crate::{fixture_name, signature, tag::tag_fixture}; + use crate::{fixture_name, signature, tag::tag_fixture, Sign}; #[test] fn signed() -> crate::Result { @@ -232,15 +234,24 @@ KLMHist5yj0sw1E4hDTyQa0= #[test] fn tagger_without_timestamp() -> crate::Result { - // Note: this behaviour is inconsistent with Git's - // Git would successfully interpret this tag and set the timestamp to 0 - // See https://github.com/git/git/blob/3a7362eb9fad0c4838f5cfaa95ed3c51a4c18d93/tag.c#L116 assert_eq!( - format!( - "{:?}", - TagRef::from_bytes(&fixture_name("tag", "tagger-without-timestamp.txt")) - ), - "Err(Error { inner: () })".to_string() + TagRef::from_bytes(&fixture_name("tag", "tagger-without-timestamp.txt"))?, + TagRef { + target: b"4fcd840c4935e4c7a5ea3552710a0f26b9178c24".as_bstr(), + name: b"ChangeLog".as_bstr(), + target_kind: Kind::Commit, + message: b"".as_bstr(), + tagger: Some(SignatureRef { + name: b"shemminger".as_bstr(), + email: b"shemminger".as_bstr(), + time: Time { + seconds: 0, + offset: 0, + sign: Sign::Plus + } + }), + pgp_signature: None + } ); Ok(()) } From bfd50091d7ec5784f0af8581d1c14c463e9e9417 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 23 Aug 2024 16:28:07 +0200 Subject: [PATCH 4/5] adjust expectation now that author parsing is more lenient. --- gix-object/tests/commit/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gix-object/tests/commit/mod.rs b/gix-object/tests/commit/mod.rs index bb10b716371..c9ce4060f4e 100644 --- a/gix-object/tests/commit/mod.rs +++ b/gix-object/tests/commit/mod.rs @@ -170,7 +170,7 @@ fn invalid() { assert_eq!( CommitRef::from_bytes(partial_commit).unwrap_err().to_string(), if cfg!(feature = "verbose-object-parsing-errors") { - "object parsing failed at `1`\nexpected ``, ` <> <+|->`, `author `" + "object parsing failed at `1`\nexpected `author `" } else { "object parsing failed" } From 1b600f31db5b5061764e7a845ac24def57faf144 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 23 Aug 2024 16:39:19 +0200 Subject: [PATCH 5/5] Make binary-search based test more robust --- gix-object/tests/tree/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gix-object/tests/tree/mod.rs b/gix-object/tests/tree/mod.rs index 81a9d32b688..a55a51d7bce 100644 --- a/gix-object/tests/tree/mod.rs +++ b/gix-object/tests/tree/mod.rs @@ -187,8 +187,8 @@ mod entries { ) } - assert_eq!( - failures_when_searching_by_name, 2, + assert_ne!( + failures_when_searching_by_name, 0, "it's not possible to do a binary search by name alone" );