fix!: Make Signature roundtrip

By storing the raw bytes from Git instead of parsing them into a
`gix_date::Time` from the start, we are able to roundtrip between Git
and gix-actor even with creatively formatted Git data.

Also add a test case that shows a time offset seen in the wild which
would have failed to round-trip without this change.

Author: pierrechevalier83

gix-actor/src/lib.rs

@@ -13,12 +13,11 @@
 ///
 /// For convenience to allow using `bstr` without adding it to own cargo manifest.
 pub use bstr;
-use bstr::{BStr, BString};
+use bstr::{BStr, BString, ByteSlice};
 /// The re-exported `gix-date` crate.
 ///
 /// For convenience to allow using `gix-date` without adding it to own cargo manifest.
 pub use gix_date as date;
-use gix_date::Time;
 
 mod identity;
 ///
@@ -68,9 +67,15 @@ pub struct Signature {
     /// Use [SignatureRef::trim()] or trim manually to be able to clean it up.
     pub email: BString,
     /// The time stamp at which the signature is performed.
-    pub time: Time,
+    pub time: BString,
 }
 
+impl Signature {
+    /// Seconds since unix epoch from the time
+    pub fn seconds(&self) -> gix_date::SecondsSinceUnixEpoch {
+        SignatureRef::from(self).seconds()
+    }
+}
 /// A immutable signature is created by an actor at a certain time.
 ///
 /// Note that this is not a cryptographical signature.
@@ -87,5 +92,18 @@ pub struct SignatureRef<'a> {
     /// Use [SignatureRef::trim()] or trim manually to be able to clean it up.
     pub email: &'a BStr,
     /// The time stamp at which the signature was performed.
-    pub time: gix_date::Time,
+    pub time: &'a BStr,
+}
+
+impl SignatureRef<'_> {
+    /// Seconds since unix epoch from the time
+    pub fn seconds(&self) -> gix_date::SecondsSinceUnixEpoch {
+        use winnow::stream::AsChar;
+        self.time
+            .split(|b| b.is_space())
+            .next()
+            .and_then(|i| i.to_str().ok())
+            .and_then(|s| s.parse().ok())
+            .unwrap_or(Default::default())
+    }
 }

gix-actor/src/signature/decode.rs

@@ -1,20 +1,16 @@
 pub(crate) mod function {
     use crate::{IdentityRef, SignatureRef};
     use bstr::ByteSlice;
-    use gix_date::{time::Sign, OffsetInSeconds, SecondsSinceUnixEpoch, Time};
-    use gix_utils::btoi::to_signed;
     use winnow::error::ErrMode;
     use winnow::stream::Stream;
     use winnow::{
-        combinator::{alt, opt, separated_pair, terminated},
+        combinator::{opt, separated_pair},
         error::{AddContext, ParserError, StrContext},
         prelude::*,
         stream::AsChar,
-        token::{take, take_until, take_while},
+        token::take_while,
     };
 
-    const SPACE: &[u8] = b" ";
-
     /// Parse a signature from the bytes input `i` using `nom`.
     pub fn decode<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>(
         i: &mut &'a [u8],
@@ -23,36 +19,13 @@ pub(crate) mod function {
             identity,
             opt(b" "),
             opt((
-                terminated(take_until(0.., SPACE), take(1usize))
-                    .verify_map(|v| to_signed::<SecondsSinceUnixEpoch>(v).ok())
-                    .context(StrContext::Expected("<timestamp>".into())),
-                alt((
-                    take_while(1.., b'-').map(|_| Sign::Minus),
-                    take_while(1.., b'+').map(|_| Sign::Plus),
-                ))
-                .context(StrContext::Expected("+|-".into())),
-                take_while(2, AsChar::is_dec_digit)
-                    .verify_map(|v| to_signed::<OffsetInSeconds>(v).ok())
-                    .context(StrContext::Expected("HH".into())),
-                take_while(1..=2, AsChar::is_dec_digit)
-                    .verify_map(|v| to_signed::<OffsetInSeconds>(v).ok())
-                    .context(StrContext::Expected("MM".into())),
-                take_while(0.., AsChar::is_dec_digit).map(|v: &[u8]| v),
+                take_while(0.., |b: u8| b == b'+' || b == b'-' || b.is_space() || b.is_dec_digit()).map(|v: &[u8]| v),
             ))
-            .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 {
-                        0
-                    };
-                    Time {
-                        seconds: time,
-                        offset,
-                        sign,
-                    }
+            .map(|maybe_bytes| {
+                if let Some((bytes,)) = maybe_bytes {
+                    bytes.into()
                 } else {
-                    Time::new(0, 0)
+                    b"".into()
                 }
             }),
         )
@@ -109,29 +82,22 @@ pub use function::identity;
 mod tests {
     mod parse_signature {
         use bstr::ByteSlice;
-        use gix_date::{time::Sign, OffsetInSeconds, SecondsSinceUnixEpoch};
         use gix_testtools::to_bstr_err;
         use winnow::prelude::*;
 
-        use crate::{signature, SignatureRef, Time};
+        use crate::{signature, SignatureRef};
 
         fn decode<'i>(
             i: &mut &'i [u8],
         ) -> ModalResult<SignatureRef<'i>, winnow::error::TreeError<&'i [u8], winnow::error::StrContext>> {
             signature::decode.parse_next(i)
         }
 
-        fn signature(
-            name: &'static str,
-            email: &'static str,
-            seconds: SecondsSinceUnixEpoch,
-            sign: Sign,
-            offset: OffsetInSeconds,
-        ) -> SignatureRef<'static> {
+        fn signature(name: &'static str, email: &'static str, time: &'static str) -> SignatureRef<'static> {
             SignatureRef {
                 name: name.as_bytes().as_bstr(),
                 email: email.as_bytes().as_bstr(),
-                time: Time { seconds, offset, sign },
+                time: time.as_bytes().as_bstr(),
             }
         }
 
@@ -142,7 +108,7 @@ mod tests {
                     .parse_peek(b"Sebastian Thiel <byronimo@gmail.com> 1528473343 -0230")
                     .expect("parse to work")
                     .1,
-                signature("Sebastian Thiel", "byronimo@gmail.com", 1528473343, Sign::Minus, -9000)
+                signature("Sebastian Thiel", "byronimo@gmail.com", "1528473343 -0230")
             );
         }
 
@@ -153,7 +119,7 @@ mod tests {
                     .parse_peek(b"Sebastian Thiel <byronimo@gmail.com> 1528473343 +0230")
                     .expect("parse to work")
                     .1,
-                signature("Sebastian Thiel", "byronimo@gmail.com", 1528473343, Sign::Plus, 9000)
+                signature("Sebastian Thiel", "byronimo@gmail.com", "1528473343 +0230")
             );
         }
 
@@ -164,7 +130,7 @@ mod tests {
                     .parse_peek(b"Sebastian Thiel <\tbyronimo@gmail.com > 1528473343 +0230")
                     .expect("parse to work")
                     .1,
-                signature("Sebastian Thiel", "\tbyronimo@gmail.com ", 1528473343, Sign::Plus, 9000)
+                signature("Sebastian Thiel", "\tbyronimo@gmail.com ", "1528473343 +0230")
             );
         }
 
@@ -175,7 +141,7 @@ mod tests {
                     .parse_peek(b"Sebastian Thiel <byronimo@gmail.com> 1528473343 -0000")
                     .expect("parse to work")
                     .1,
-                signature("Sebastian Thiel", "byronimo@gmail.com", 1528473343, Sign::Minus, 0)
+                signature("Sebastian Thiel", "byronimo@gmail.com", "1528473343 -0000")
             );
         }
 
@@ -186,15 +152,15 @@ mod tests {
                     .parse_peek(b"name <name@example.com> 1288373970 --700")
                     .expect("parse to work")
                     .1,
-                signature("name", "name@example.com", 1288373970, Sign::Minus, -252000)
+                signature("name", "name@example.com", "1288373970 --700")
             );
         }
 
         #[test]
         fn empty_name_and_email() {
             assert_eq!(
                 decode.parse_peek(b" <> 12345 -1215").expect("parse to work").1,
-                signature("", "", 12345, Sign::Minus, -44100)
+                signature("", "", "12345 -1215")
             );
         }
 
@@ -213,7 +179,7 @@ mod tests {
         fn invalid_time() {
             assert_eq!(
                 decode.parse_peek(b"hello <> abc -1215").expect("parse to work").1,
-                signature("hello", "", 0, Sign::Plus, 0)
+                signature("hello", "", "")
             );
         }
     }

gix-actor/src/signature/mod.rs

@@ -18,7 +18,7 @@ mod _ref {
             Signature {
                 name: self.name.to_owned(),
                 email: self.email.to_owned(),
-                time: self.time,
+                time: self.time.to_owned(),
             }
         }
 
@@ -27,7 +27,7 @@ mod _ref {
             SignatureRef {
                 name: self.name.trim().as_bstr(),
                 email: self.email.trim().as_bstr(),
-                time: self.time,
+                time: self.time.trim().as_bstr(),
             }
         }
 
@@ -50,7 +50,7 @@ mod convert {
             SignatureRef {
                 name: self.name.as_ref(),
                 email: self.email.as_ref(),
-                time: self.time,
+                time: self.time.as_ref(),
             }
         }
     }
@@ -61,7 +61,7 @@ mod convert {
             Signature {
                 name: name.to_owned(),
                 email: email.to_owned(),
-                time,
+                time: time.to_owned(),
             }
         }
     }
@@ -112,11 +112,11 @@ pub(crate) mod write {
             out.write_all(b"<")?;
             out.write_all(validated_token(self.email)?)?;
             out.write_all(b"> ")?;
-            self.time.write_to(out)
+            out.write_all(validated_token(self.time)?)
         }
         /// Computes the number of bytes necessary to serialize this signature
         pub fn size(&self) -> usize {
-            self.name.len() + 2 /* space <*/ + self.email.len() +  2 /* > space */ + self.time.size()
+            self.name.len() + 2 /* space <*/ + self.email.len() +  2 /* > space */ + self.time.len()
         }
     }
 

gix-actor/tests/signature/mod.rs

@@ -1,14 +1,13 @@
 mod write_to {
     mod invalid {
         use gix_actor::Signature;
-        use gix_date::{time::Sign, Time};
 
         #[test]
         fn name() {
             let signature = Signature {
                 name: "invalid < middlename".into(),
                 email: "ok".into(),
-                time: default_time(),
+                time: "0 0000".into(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
@@ -21,7 +20,7 @@ mod write_to {
             let signature = Signature {
                 name: "ok".into(),
                 email: "server>.example.com".into(),
-                time: default_time(),
+                time: "0 0000".into(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
@@ -34,21 +33,13 @@ mod write_to {
             let signature = Signature {
                 name: "hello\nnewline".into(),
                 email: "name@example.com".into(),
-                time: default_time(),
+                time: "0 0000".into(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
                 "Err(Custom { kind: Other, error: IllegalCharacter })"
             );
         }
-
-        fn default_time() -> Time {
-            Time {
-                seconds: 0,
-                offset: 0,
-                sign: Sign::Plus,
-            }
-        }
     }
 }
 
@@ -68,6 +59,7 @@ fn round_trip() -> Result<(), Box<dyn std::error::Error>> {
     static DEFAULTS: &[&[u8]] =     &[
         b"Sebastian Thiel <byronimo@gmail.com> 1 -0030",
         b"Sebastian Thiel <byronimo@gmail.com> -1500 -0030",
+        b"Sebastian Thiel <byronimo@gmail.com> 1313584730 +051800", // Seen in the wild
         ".. ☺️Sebastian 王知明 Thiel🙌 .. <byronimo@gmail.com> 1528473343 +0230".as_bytes(),
         b".. whitespace  \t  is explicitly allowed    - unicode aware trimming must be done elsewhere  <byronimo@gmail.com> 1528473343 +0230"
     ];
@@ -90,7 +82,7 @@ fn parse_timestamp_with_trailing_digits() {
         SignatureRef {
             name: "first last".into(),
             email: "name@example.com".into(),
-            time: gix_actor::date::Time::new(1312735823, 0),
+            time: "1312735823 +051800".into(),
         }
     );
 
@@ -101,7 +93,7 @@ fn parse_timestamp_with_trailing_digits() {
         SignatureRef {
             name: "first last".into(),
             email: "name@example.com".into(),
-            time: gix_actor::date::Time::new(1312735823, 19080),
+            time: "1312735823 +0518".into(),
         }
     );
 }
@@ -115,7 +107,7 @@ fn parse_missing_timestamp() {
         SignatureRef {
             name: "first last".into(),
             email: "name@example.com".into(),
-            time: gix_actor::date::Time::new(0, 0),
+            time: "".into(),
         }
     );
 }