fix!: Make SignatureRef::to_owned() fallible.

The contained `time` field is now a string, which has to be parsed into
a time for conversion to an owned type.
Additionally, replace `From<SignatureRef> for Signature` with `TryFrom`.

Author: Byron

gix-actor/src/lib.rs

@@ -13,7 +13,7 @@
 ///
 /// For convenience to allow using `bstr` without adding it to own cargo manifest.
 pub use bstr;
-use bstr::{BStr, BString, ByteSlice};
+use bstr::{BStr, BString};
 /// The re-exported `gix-date` crate.
 ///
 /// For convenience to allow using `gix-date` without adding it to own cargo manifest.
@@ -52,7 +52,7 @@ pub struct IdentityRef<'a> {
     pub email: &'a BStr,
 }
 
-/// A mutable signature is created by an actor at a certain time.
+/// A mutable signature that is created by an actor at a certain time.
 ///
 /// Note that this is not a cryptographical signature.
 #[derive(Default, PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone)]
@@ -70,13 +70,7 @@ pub struct Signature {
     pub time: date::Time,
 }
 
-impl Signature {
-    /// Seconds since unix epoch from the time
-    pub fn seconds(&self) -> gix_date::SecondsSinceUnixEpoch {
-        self.time.seconds
-    }
-}
-/// A immutable signature is created by an actor at a certain time.
+/// An immutable signature that is created by an actor at a certain time.
 ///
 /// Note that this is not a cryptographical signature.
 #[derive(Default, PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)]
@@ -94,16 +88,3 @@ pub struct SignatureRef<'a> {
     /// The time stamp at which the signature was performed.
     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

@@ -81,12 +81,12 @@ pub use function::identity;
 #[cfg(test)]
 mod tests {
     mod parse_signature {
+        use crate::{signature, SignatureRef};
         use bstr::ByteSlice;
+        use gix_date::time::Sign;
         use gix_testtools::to_bstr_err;
         use winnow::prelude::*;
 
-        use crate::{signature, SignatureRef};
-
         fn decode<'i>(
             i: &mut &'i [u8],
         ) -> ModalResult<SignatureRef<'i>, winnow::error::TreeError<&'i [u8], winnow::error::StrContext>> {
@@ -103,13 +103,23 @@ mod tests {
 
         #[test]
         fn tz_minus() {
+            let actual = decode
+                .parse_peek(b"Sebastian Thiel <[email protected]> 1528473343 -0230")
+                .expect("parse to work")
+                .1;
             assert_eq!(
-                decode
-                    .parse_peek(b"Sebastian Thiel <[email protected]> 1528473343 -0230")
-                    .expect("parse to work")
-                    .1,
+                actual,
                 signature("Sebastian Thiel", "[email protected]", "1528473343 -0230")
             );
+            assert_eq!(actual.seconds(), 1528473343);
+            assert_eq!(
+                actual.time().expect("valid"),
+                gix_date::Time {
+                    seconds: 1528473343,
+                    offset: -9000,
+                    sign: Sign::Minus
+                }
+            );
         }
 
         #[test]

gix-actor/src/signature/mod.rs

@@ -5,6 +5,7 @@ mod _ref {
 
     use crate::{signature::decode, IdentityRef, Signature, SignatureRef};
 
+    /// Lifecycle
     impl<'a> SignatureRef<'a> {
         /// Deserialize a signature from the given `data`.
         pub fn from_bytes<E>(mut data: &'a [u8]) -> Result<SignatureRef<'a>, winnow::error::ErrMode<E>>
@@ -15,15 +16,18 @@ mod _ref {
         }
 
         /// Create an owned instance from this shared one.
-        pub fn to_owned(&self) -> Signature {
-            Signature {
+        pub fn to_owned(&self) -> Result<Signature, gix_date::parse::Error> {
+            Ok(Signature {
                 name: self.name.to_owned(),
                 email: self.email.to_owned(),
-                time: Time::from_bytes(self.time).expect("Time must be valid"),
-            }
+                time: self.time()?,
+            })
         }
+    }
 
-        /// Trim whitespace surrounding the name and email and return a new signature.
+    /// Access
+    impl<'a> SignatureRef<'a> {
+        /// Trim the whitespace surrounding the `name`, `email` and `time` and return a new signature.
         pub fn trim(&self) -> SignatureRef<'a> {
             SignatureRef {
                 name: self.name.trim().as_bstr(),
@@ -32,22 +36,40 @@ mod _ref {
             }
         }
 
-        /// Return the actor's name and email, effectively excluding the time stamp of this signature.
+        /// Return the actor's name and email, effectively excluding the timestamp of this signature.
         pub fn actor(&self) -> IdentityRef<'a> {
             IdentityRef {
                 name: self.name,
                 email: self.email,
             }
         }
+
+        /// Parse only the seconds since unix epoch from the `time` field, or silently default to 0
+        /// if parsing fails. Note that this ignores the timezone, so it can parse otherwise broken dates.
+        ///
+        /// For a fallible and more complete, but slower version, use [`time()`](Self::time).
+        pub fn seconds(&self) -> gix_date::SecondsSinceUnixEpoch {
+            use winnow::stream::AsChar;
+            self.time
+                .trim()
+                .split(|b| b.is_space())
+                .next()
+                .and_then(|i| i.to_str().ok()?.parse().ok())
+                .unwrap_or_default()
+        }
+
+        /// Parse the `time` field for access to the passed time since unix epoch, and the time offset.
+        pub fn time(&self) -> Result<gix_date::Time, gix_date::parse::Error> {
+            Time::from_bytes(self.time)
+        }
     }
 }
 
 mod convert {
     use crate::{Signature, SignatureRef};
-    use gix_date::Time;
 
     impl Signature {
-        /// Borrow this instance as immutable
+        /// Borrow this instance as immutable, serializing the `time` field into `buf`.
         pub fn to_ref<'a>(&'a self, buf: &'a mut Vec<u8>) -> SignatureRef<'a> {
             SignatureRef {
                 name: self.name.as_ref(),
@@ -57,14 +79,11 @@ mod convert {
         }
     }
 
-    impl From<SignatureRef<'_>> for Signature {
-        fn from(other: SignatureRef<'_>) -> Signature {
-            let SignatureRef { name, email, time } = other;
-            Signature {
-                name: name.to_owned(),
-                email: email.to_owned(),
-                time: Time::from_bytes(time).expect("Time must be valid"),
-            }
+    impl TryFrom<SignatureRef<'_>> for Signature {
+        type Error = gix_date::parse::Error;
+
+        fn try_from(other: SignatureRef<'_>) -> Result<Signature, Self::Error> {
+            other.to_owned()
         }
     }
 }

gix-actor/tests/signature/mod.rs

@@ -65,7 +65,10 @@ fn round_trip() -> Result<(), Box<dyn std::error::Error>> {
     ];
 
     for input in DEFAULTS {
-        let signature: Signature = gix_actor::SignatureRef::from_bytes::<()>(input).unwrap().into();
+        let signature: Signature = gix_actor::SignatureRef::from_bytes::<()>(input)
+            .unwrap()
+            .try_into()
+            .unwrap();
         let mut output = Vec::new();
         signature.write_to(&mut output)?;
         assert_eq!(output.as_bstr(), input.as_bstr());