Apply feedback from discussion

We now store a gix-date::Time in the owned `Signature` and a `&BStr` in
`SignatureRef`.

This means that `SignatureRef` round-trips to and from Git, but
`Signature` doesn't necessarily round-trip with `SignatureRef`.

It also means that we need a buffer of bytes when creating a
`SignatureRef`.

Author: Pierre Chevalier

gitoxide-core/src/hours/mod.rs

@@ -82,7 +82,8 @@ where
                         };
                         let name = string_ref(author.name.as_ref());
                         let email = string_ref(author.email.as_ref());
-                        let time = string_ref(author.time.as_ref());
+                        let mut buf = Vec::with_capacity(64);
+                        let time = string_ref(author.time.to_ref(&mut buf));
 
                         out.push((commit_idx, actor::SignatureRef { name, email, time }));
                     }

gix-actor/src/lib.rs

@@ -67,13 +67,13 @@ 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: BString,
+    pub time: date::Time,
 }
 
 impl Signature {
     /// Seconds since unix epoch from the time
     pub fn seconds(&self) -> gix_date::SecondsSinceUnixEpoch {
-        SignatureRef::from(self).seconds()
+        self.time.seconds
     }
 }
 /// A immutable signature is created by an actor at a certain time.

gix-actor/src/signature/mod.rs

@@ -1,5 +1,6 @@
 mod _ref {
     use bstr::ByteSlice;
+    use gix_date::Time;
     use winnow::{error::StrContext, prelude::*};
 
     use crate::{signature::decode, IdentityRef, Signature, SignatureRef};
@@ -18,7 +19,7 @@ mod _ref {
             Signature {
                 name: self.name.to_owned(),
                 email: self.email.to_owned(),
-                time: self.time.to_owned(),
+                time: Time::from_bytes(self.time).expect("Time must be valid"),
             }
         }
 
@@ -43,14 +44,15 @@ mod _ref {
 
 mod convert {
     use crate::{Signature, SignatureRef};
+    use gix_date::Time;
 
     impl Signature {
         /// Borrow this instance as immutable
-        pub fn to_ref(&self) -> SignatureRef<'_> {
+        pub fn to_ref<'a>(&'a self, buf: &'a mut Vec<u8>) -> SignatureRef<'a> {
             SignatureRef {
                 name: self.name.as_ref(),
                 email: self.email.as_ref(),
-                time: self.time.as_ref(),
+                time: self.time.to_ref(buf),
             }
         }
     }
@@ -61,16 +63,10 @@ mod convert {
             Signature {
                 name: name.to_owned(),
                 email: email.to_owned(),
-                time: time.to_owned(),
+                time: Time::from_bytes(time).expect("Time must be valid"),
             }
         }
     }
-
-    impl<'a> From<&'a Signature> for SignatureRef<'a> {
-        fn from(other: &'a Signature) -> SignatureRef<'a> {
-            other.to_ref()
-        }
-    }
 }
 
 pub(crate) mod write {
@@ -96,11 +92,12 @@ pub(crate) mod write {
     impl Signature {
         /// Serialize this instance to `out` in the git serialization format for actors.
         pub fn write_to(&self, out: &mut dyn std::io::Write) -> std::io::Result<()> {
-            self.to_ref().write_to(out)
+            let mut buf = Vec::<u8>::new();
+            self.to_ref(&mut buf).write_to(out)
         }
         /// Computes the number of bytes necessary to serialize this signature
         pub fn size(&self) -> usize {
-            self.to_ref().size()
+            self.name.len() + 2 /* space <*/ + self.email.len() +  2 /* > space */ + self.time.size()
         }
     }
 

gix-actor/tests/signature/mod.rs

@@ -1,13 +1,14 @@
 mod write_to {
     mod invalid {
         use gix_actor::Signature;
+        use gix_date::Time;
 
         #[test]
         fn name() {
             let signature = Signature {
                 name: "invalid < middlename".into(),
                 email: "ok".into(),
-                time: "0 0000".into(),
+                time: Time::default(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
@@ -20,7 +21,7 @@ mod write_to {
             let signature = Signature {
                 name: "ok".into(),
                 email: "server>.example.com".into(),
-                time: "0 0000".into(),
+                time: Time::default(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
@@ -33,7 +34,7 @@ mod write_to {
             let signature = Signature {
                 name: "hello\nnewline".into(),
                 email: "name@example.com".into(),
-                time: "0 0000".into(),
+                time: Time::default(),
             };
             assert_eq!(
                 format!("{:?}", signature.write_to(&mut Vec::new())),
@@ -59,7 +60,6 @@ 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"
     ];
@@ -73,6 +73,16 @@ fn round_trip() -> Result<(), Box<dyn std::error::Error>> {
     Ok(())
 }
 
+#[test]
+fn signature_ref_round_trips_with_seconds_in_offset() -> Result<(), Box<dyn std::error::Error>> {
+    let input = b"Sebastian Thiel <byronimo@gmail.com> 1313584730 +051800"; // Seen in the wild
+    let signature: SignatureRef = gix_actor::SignatureRef::from_bytes::<()>(input).unwrap();
+    let mut output = Vec::new();
+    signature.write_to(&mut output)?;
+    assert_eq!(output.as_bstr(), input.as_bstr());
+    Ok(())
+}
+
 #[test]
 fn parse_timestamp_with_trailing_digits() {
     let signature = gix_actor::SignatureRef::from_bytes::<()>(b"first last <name@example.com> 1312735823 +051800")

gix-date/src/lib.rs

@@ -17,7 +17,7 @@ pub mod time;
 pub mod parse;
 use bstr::{BStr, ByteSlice};
 pub use parse::function::parse;
-use parse::function::parse_raw;
+pub use parse::function::parse_raw;
 use parse::Error;
 use std::time::SystemTime;
 
@@ -44,6 +44,11 @@ impl Time {
         let s = i.as_bstr().to_str().expect("Input must be ascii");
         parse_raw(s).ok_or(Error::InvalidDateString { input: s.into() })
     }
+    /// Write time into buffer
+    pub fn to_ref<'a>(&self, buf: &'a mut Vec<u8>) -> &'a BStr {
+        self.write_to(buf).expect("write to memory cannot fail");
+        buf.as_bstr()
+    }
 }
 
 /// The amount of seconds since unix epoch.

gix-date/src/parse.rs

@@ -60,7 +60,8 @@ pub(crate) mod function {
         })
     }
 
-    pub(crate) fn parse_raw(input: &str) -> Option<Time> {
+    #[allow(missing_docs)]
+    pub fn parse_raw(input: &str) -> Option<Time> {
         let mut split = input.split_whitespace();
         let seconds: SecondsSinceUnixEpoch = split.next()?.parse().ok()?;
         let offset = split.next()?;

gix-mailmap/src/snapshot/mod.rs

@@ -157,17 +157,17 @@ fn enriched_signature<'a>(
         (Some(new_email), Some(new_name)) => Signature {
             email: new_email.to_owned().into(),
             name: new_name.to_owned().into(),
-            time: time.into(),
+            time: gix_date::Time::from_bytes(time).expect("Time must be valid"),
         },
         (Some(new_email), None) => Signature {
             email: new_email.to_owned().into(),
             name: name.into(),
-            time: time.into(),
+            time: gix_date::Time::from_bytes(time).expect("Time must be valid"),
         },
         (None, Some(new_name)) => Signature {
             email: email.into(),
             name: new_name.to_owned().into(),
-            time: time.into(),
+            time: gix_date::Time::from_bytes(time).expect("Time must be valid"),
         },
         (None, None) => unreachable!("BUG: ResolvedSignatures don't exist here when nothing is set"),
     }

gix-mailmap/src/snapshot/signature.rs

@@ -11,15 +11,15 @@ pub struct Signature<'a> {
     /// The possibly mapped email.
     pub email: Cow<'a, BStr>,
     /// The time stamp at which the signature is performed.
-    pub time: Cow<'a, BStr>,
+    pub time: gix_date::Time,
 }
 
 impl<'a> From<Signature<'a>> for gix_actor::Signature {
     fn from(s: Signature<'a>) -> Self {
         gix_actor::Signature {
             name: s.name.into_owned(),
             email: s.email.into_owned(),
-            time: s.time.into_owned(),
+            time: s.time,
         }
     }
 }
@@ -29,7 +29,7 @@ impl<'a> From<gix_actor::SignatureRef<'a>> for Signature<'a> {
         Signature {
             name: s.name.into(),
             email: s.email.into(),
-            time: s.time.into(),
+            time: gix_date::Time::from_bytes(s.time).expect("Time must be valid"),
         }
     }
 }

gix-mailmap/tests/snapshot/mod.rs

@@ -4,55 +4,65 @@ use gix_testtools::fixture_bytes;
 #[test]
 fn try_resolve() {
     let snapshot = Snapshot::from_bytes(&fixture_bytes("typical.txt"));
+    let mut buf = Vec::with_capacity(64);
     assert_eq!(
-        snapshot.try_resolve(signature("Foo", "Joe@example.com").to_ref()),
+        snapshot.try_resolve(signature("Foo", "Joe@example.com").to_ref(&mut buf)),
         Some(signature("Joe R. Developer", "joe@example.com")),
         "resolved signatures contain all original fields, and normalize the email as well to match the one that it was looked up with"
     );
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("Joe", "bugs@example.com").to_ref()),
+        snapshot.try_resolve(signature("Joe", "bugs@example.com").to_ref(&mut buf)),
         Some(signature("Joe R. Developer", "joe@example.com")),
         "name and email can be mapped specifically"
     );
 
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("Jane", "jane@laptop.(none)").to_ref()),
+        snapshot.try_resolve(signature("Jane", "jane@laptop.(none)").to_ref(&mut buf)),
         Some(signature("Jane Doe", "jane@example.com")),
         "fix name and email by email"
     );
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("Jane", "jane@desktop.(none)").to_ref()),
+        snapshot.try_resolve(signature("Jane", "jane@desktop.(none)").to_ref(&mut buf)),
         Some(signature("Jane Doe", "jane@example.com")),
         "fix name and email by other email"
     );
 
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("janE", "Bugs@example.com").to_ref()),
+        snapshot.try_resolve(signature("janE", "Bugs@example.com").to_ref(&mut buf)),
         Some(signature("Jane Doe", "jane@example.com")),
         "name and email can be mapped specifically, case insensitive matching of name"
     );
+    buf.clear();
     assert_eq!(
-        snapshot.resolve(signature("janE", "jane@ipad.(none)").to_ref()),
+        snapshot.resolve(signature("janE", "jane@ipad.(none)").to_ref(&mut buf)),
         signature("janE", "jane@example.com"),
         "an email can be mapped by name and email specifically, both match case-insensitively"
     );
 
     let sig = signature("Jane", "other@example.com");
-    assert_eq!(snapshot.try_resolve(sig.to_ref()), None, "unmatched email");
+    buf.clear();
+    assert_eq!(snapshot.try_resolve(sig.to_ref(&mut buf)), None, "unmatched email");
 
+    buf.clear();
     assert_eq!(
-        snapshot.resolve(sig.to_ref()),
+        snapshot.resolve(sig.to_ref(&mut buf)),
         sig,
         "resolution always works here, returning a copy of the original"
     );
 
     let sig = signature("Jean", "bugs@example.com");
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(sig.to_ref()),
+        snapshot.try_resolve(sig.to_ref(&mut buf)),
         None,
         "matched email, unmatched name"
     );
-    assert_eq!(snapshot.resolve(sig.to_ref()), sig);
+    buf.clear();
+    assert_eq!(snapshot.resolve(sig.to_ref(&mut buf)), sig);
 
     assert_eq!(
         snapshot.entries(),
@@ -85,16 +95,19 @@ fn non_name_and_name_mappings_will_not_clash() {
             "old-email",
         ),
     ];
+    let mut buf = Vec::with_capacity(64);
     for entries in [entries.clone().into_iter().rev().collect::<Vec<_>>(), entries] {
         let snapshot = Snapshot::new(entries);
 
+        buf.clear();
         assert_eq!(
-            snapshot.try_resolve(signature("replace-by-email", "Old-Email").to_ref()),
+            snapshot.try_resolve(signature("replace-by-email", "Old-Email").to_ref(&mut buf)),
             Some(signature("new-name", "old-email")),
             "it can match by email only, and the email is normalized"
         );
+        buf.clear();
         assert_eq!(
-            snapshot.try_resolve(signature("old-name", "Old-Email").to_ref()),
+            snapshot.try_resolve(signature("old-name", "Old-Email").to_ref(&mut buf)),
             Some(signature("other-new-name", "other-new-email")),
             "it can match by email and name as well"
         );
@@ -117,26 +130,30 @@ fn non_name_and_name_mappings_will_not_clash() {
 #[test]
 fn overwrite_entries() {
     let snapshot = Snapshot::from_bytes(&fixture_bytes("overwrite.txt"));
+    let mut buf = Vec::with_capacity(64);
     assert_eq!(
-        snapshot.try_resolve(signature("does not matter", "old-a-email").to_ref()),
+        snapshot.try_resolve(signature("does not matter", "old-a-email").to_ref(&mut buf)),
         Some(signature("A-overwritten", "old-a-email")),
         "email only by email"
     );
 
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("to be replaced", "old-b-EMAIL").to_ref()),
+        snapshot.try_resolve(signature("to be replaced", "old-b-EMAIL").to_ref(&mut buf)),
         Some(signature("B-overwritten", "new-b-email-overwritten")),
         "name and email by email"
     );
 
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("old-c", "old-C-email").to_ref()),
+        snapshot.try_resolve(signature("old-c", "old-C-email").to_ref(&mut buf)),
         Some(signature("C-overwritten", "new-c-email-overwritten")),
         "name and email by name and email"
     );
 
+    buf.clear();
     assert_eq!(
-        snapshot.try_resolve(signature("unchanged", "old-d-email").to_ref()),
+        snapshot.try_resolve(signature("unchanged", "old-d-email").to_ref(&mut buf)),
         Some(signature("unchanged", "new-d-email-overwritten")),
         "email by email"
     );
@@ -161,6 +178,6 @@ fn signature(name: &str, email: &str) -> gix_actor::Signature {
     gix_actor::Signature {
         name: name.into(),
         email: email.into(),
-        time: b"42 +0800".into(),
+        time: gix_date::parse_raw("42 +0800").unwrap(),
     }
 }

gix-object/src/commit/write.rs

@@ -11,8 +11,10 @@ impl crate::WriteTo for Commit {
         for parent in &self.parents {
             encode::trusted_header_id(b"parent", parent, &mut out)?;
         }
-        encode::trusted_header_signature(b"author", &self.author.to_ref(), &mut out)?;
-        encode::trusted_header_signature(b"committer", &self.committer.to_ref(), &mut out)?;
+        let mut buf = Vec::with_capacity(64);
+        encode::trusted_header_signature(b"author", &self.author.to_ref(&mut buf), &mut out)?;
+        buf.clear();
+        encode::trusted_header_signature(b"committer", &self.committer.to_ref(&mut buf), &mut out)?;
         if let Some(encoding) = self.encoding.as_ref() {
             encode::header_field(b"encoding", encoding, &mut out)?;
         }