fix!: store raw commit/tag actor headers and parse lazily

Author: Pingasmaster

examples/log.rs

@@ -142,15 +142,16 @@ fn run(args: Args) -> anyhow::Result<()> {
                 let info = info?;
                 let commit = info.object()?;
                 let commit_ref = commit.decode()?;
+                let author = commit_ref.author();
                 Ok(LogEntryInfo {
                     commit_id: commit.id().to_hex().to_string(),
                     parents: info.parent_ids().map(|id| id.shorten_or_id().to_string()).collect(),
                     author: {
                         let mut buf = Vec::new();
-                        commit_ref.author.actor().write_to(&mut buf)?;
+                        author.actor().write_to(&mut buf)?;
                         buf.into()
                     },
-                    time: commit_ref.author.time()?.format_or_unix(format::DEFAULT),
+                    time: author.time()?.format_or_unix(format::DEFAULT),
                     message: commit_ref.message.to_owned(),
                 })
             }),

gix-object/src/commit/decode.rs

@@ -40,10 +40,20 @@ pub fn commit<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>(
             .context(StrContext::Expected(
                 "zero or more 'parent <40 lowercase hex char>'".into(),
             )),
-        (|i: &mut _| parse::header_field(i, b"author", parse::signature))
-            .context(StrContext::Expected("author <signature>".into())),
-        (|i: &mut _| parse::header_field(i, b"committer", parse::signature))
-            .context(StrContext::Expected("committer <signature>".into())),
+        (|i: &mut _| {
+            parse::header_field(i, b"author", parse::signature_with_raw).map(|(signature, raw)| {
+                let _ = signature;
+                raw
+            })
+        })
+        .context(StrContext::Expected("author <signature>".into())),
+        (|i: &mut _| {
+            parse::header_field(i, b"committer", parse::signature_with_raw).map(|(signature, raw)| {
+                let _ = signature;
+                raw
+            })
+        })
+        .context(StrContext::Expected("committer <signature>".into())),
         opt(|i: &mut _| parse::header_field(i, b"encoding", take_till(0.., NL)))
             .context(StrContext::Expected("encoding <encoding>".into())),
         repeat(

gix-object/src/commit/mod.rs

@@ -74,6 +74,10 @@ impl<'a> CommitRef<'a> {
 
 /// Access
 impl<'a> CommitRef<'a> {
+    fn parse_signature(raw: &'a BStr) -> gix_actor::SignatureRef<'a> {
+        gix_actor::SignatureRef::from_bytes::<()>(raw.as_ref()).expect("signatures were validated during parsing")
+    }
+
     /// Return the `tree` fields hash digest.
     pub fn tree(&self) -> gix_hash::ObjectId {
         gix_hash::ObjectId::from_hex(self.tree).expect("prior validation of tree hash during parsing")
@@ -95,14 +99,14 @@ impl<'a> CommitRef<'a> {
     ///
     /// This is different from the `author` field which may contain whitespace.
     pub fn author(&self) -> gix_actor::SignatureRef<'a> {
-        self.author.trim()
+        Self::parse_signature(self.author).trim()
     }
 
     /// Return the committer, with whitespace trimmed.
     ///
     /// This is different from the `committer` field which may contain whitespace.
     pub fn committer(&self) -> gix_actor::SignatureRef<'a> {
-        self.committer.trim()
+        Self::parse_signature(self.committer).trim()
     }
 
     /// Returns a partially parsed message from which more information can be derived.
@@ -112,7 +116,7 @@ impl<'a> CommitRef<'a> {
 
     /// Returns the time at which this commit was created, or a default time if it could not be parsed.
     pub fn time(&self) -> gix_date::Time {
-        self.committer.time.parse().unwrap_or_default()
+        Self::parse_signature(self.committer).time().unwrap_or_default()
     }
 }
 

gix-object/src/commit/write.rs

@@ -4,6 +4,32 @@ use bstr::ByteSlice;
 
 use crate::{encode, encode::NL, Commit, CommitRef, Kind};
 
+fn parse_signature(raw: &bstr::BStr) -> gix_actor::SignatureRef<'_> {
+    gix_actor::SignatureRef::from_bytes::<()>(raw.as_ref()).expect("signatures were validated during parsing")
+}
+
+fn signature_requires_raw(raw: &bstr::BStr) -> bool {
+    let signature = parse_signature(raw);
+    signature.name.find_byteset(b"<>\n").is_some() || signature.email.find_byteset(b"<>\n").is_some()
+}
+
+fn signature_len(raw: &bstr::BStr) -> usize {
+    if signature_requires_raw(raw) {
+        raw.len()
+    } else {
+        parse_signature(raw).size()
+    }
+}
+
+fn write_signature(mut out: &mut dyn io::Write, field: &[u8], raw: &bstr::BStr) -> io::Result<()> {
+    if signature_requires_raw(raw) {
+        encode::trusted_header_field(field, raw.as_ref(), &mut out)
+    } else {
+        let signature = parse_signature(raw);
+        encode::trusted_header_signature(field, &signature, &mut out)
+    }
+}
+
 impl crate::WriteTo for Commit {
     /// Serializes this instance to `out` in the git serialization format.
     fn write_to(&self, mut out: &mut dyn io::Write) -> io::Result<()> {
@@ -58,8 +84,8 @@ impl crate::WriteTo for CommitRef<'_> {
         for parent in self.parents() {
             encode::trusted_header_id(b"parent", &parent, &mut out)?;
         }
-        encode::trusted_header_signature(b"author", &self.author, &mut out)?;
-        encode::trusted_header_signature(b"committer", &self.committer, &mut out)?;
+        write_signature(&mut out, b"author", self.author)?;
+        write_signature(&mut out, b"committer", self.committer)?;
         if let Some(encoding) = self.encoding.as_ref() {
             encode::header_field(b"encoding", encoding, &mut out)?;
         }
@@ -78,8 +104,8 @@ impl crate::WriteTo for CommitRef<'_> {
         let hash_in_hex = self.tree().kind().len_in_hex();
         (b"tree".len() + 1 /* space */ + hash_in_hex + 1 /* nl */
             + self.parents.iter().count() * (b"parent".len() + 1 /* space */ + hash_in_hex + 1 /* nl */)
-            + b"author".len() + 1 /* space */ + self.author.size() + 1 /* nl */
-            + b"committer".len() + 1 /* space */ + self.committer.size() + 1 /* nl */
+            + b"author".len() + 1 /* space */ + signature_len(self.author) + 1 /* nl */
+            + b"committer".len() + 1 /* space */ + signature_len(self.committer) + 1 /* nl */
             + self
                 .encoding
                 .as_ref()

gix-object/src/lib.rs

@@ -88,17 +88,16 @@ pub struct CommitRef<'a> {
     pub tree: &'a BStr,
     /// HEX hash of each parent commit. Empty for first commit in repository.
     pub parents: SmallVec<[&'a BStr; 1]>,
-    /// Who wrote this commit. Name and email might contain whitespace and are not trimmed to ensure round-tripping.
+    /// The raw author header value as encountered during parsing.
     ///
-    /// Use the [`author()`](CommitRef::author()) method to received a trimmed version of it.
-    pub author: gix_actor::SignatureRef<'a>,
-    /// Who committed this commit. Name and email might contain whitespace and are not trimmed to ensure round-tripping.
-    ///
-    /// Use the [`committer()`](CommitRef::committer()) method to received a trimmed version of it.
+    /// Use the [`author()`](CommitRef::author()) method to obtain a parsed version of it.
+    #[cfg_attr(feature = "serde", serde(borrow))]
+    pub author: &'a BStr,
+    /// The raw committer header value as encountered during parsing.
     ///
-    /// This may be different from the `author` in case the author couldn't write to the repository themselves and
-    /// is commonly encountered with contributed commits.
-    pub committer: gix_actor::SignatureRef<'a>,
+    /// Use the [`committer()`](CommitRef::committer()) method to obtain a parsed version of it.
+    #[cfg_attr(feature = "serde", serde(borrow))]
+    pub committer: &'a BStr,
     /// The name of the message encoding, otherwise [UTF-8 should be assumed](https://github.com/git/git/blob/e67fbf927dfdf13d0b21dc6ea15dc3c7ef448ea0/commit.c#L1493:L1493).
     pub encoding: Option<&'a BStr>,
     /// The commit message documenting the change.
@@ -150,8 +149,11 @@ pub struct TagRef<'a> {
     pub target_kind: Kind,
     /// The name of the tag, e.g. "v1.0".
     pub name: &'a BStr,
-    /// The author of the tag.
-    pub tagger: Option<gix_actor::SignatureRef<'a>>,
+    /// The raw tagger header value as encountered during parsing.
+    ///
+    /// Use the [`tagger()`](TagRef::tagger()) method to obtain a parsed version of it.
+    #[cfg_attr(feature = "serde", serde(borrow))]
+    pub tagger: Option<&'a BStr>,
     /// The message describing this release.
     pub message: &'a BStr,
     /// A cryptographic signature over the entire content of the serialized tag object thus far.

gix-object/src/object/convert.rs

@@ -7,15 +7,20 @@ impl From<TagRef<'_>> for Tag {
             name,
             target_kind,
             message,
-            tagger: signature,
+            tagger,
             pgp_signature,
         } = other;
+        let tagger = tagger.map(|raw| {
+            gix_actor::SignatureRef::from_bytes::<()>(raw.as_ref())
+                .expect("signatures were validated during parsing")
+                .into()
+        });
         Tag {
             target: gix_hash::ObjectId::from_hex(target).expect("prior parser validation"),
             name: name.to_owned(),
             target_kind,
             message: message.to_owned(),
-            tagger: signature.map(Into::into),
+            tagger,
             pgp_signature: pgp_signature.map(ToOwned::to_owned),
         }
     }
@@ -32,6 +37,10 @@ impl From<CommitRef<'_>> for Commit {
             message,
             extra_headers,
         } = other;
+        let author = gix_actor::SignatureRef::from_bytes::<()>(author.as_ref())
+            .expect("signatures were validated during parsing");
+        let committer = gix_actor::SignatureRef::from_bytes::<()>(committer.as_ref())
+            .expect("signatures were validated during parsing");
         Commit {
             tree: gix_hash::ObjectId::from_hex(tree).expect("prior parser validation"),
             parents: parents

gix-object/src/parse.rs

@@ -71,3 +71,13 @@ pub(crate) fn signature<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrC
 ) -> ModalResult<gix_actor::SignatureRef<'a>, E> {
     gix_actor::signature::decode(i)
 }
+
+pub(crate) fn signature_with_raw<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>(
+    i: &mut &'a [u8],
+) -> ModalResult<(gix_actor::SignatureRef<'a>, &'a BStr), E> {
+    let original = *i;
+    gix_actor::signature::decode(i).map(|signature| {
+        let consumed = original.len() - i.len();
+        (signature, original[..consumed].as_bstr())
+    })
+}

gix-object/src/tag/decode.rs

@@ -19,20 +19,23 @@ pub fn git_tag<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>(
             .context(StrContext::Expected("type <object kind>".into())),
         (|i: &mut _| parse::header_field(i, b"tag", take_while(1.., |b| b != NL[0])))
             .context(StrContext::Expected("tag <version>".into())),
-        opt(|i: &mut _| parse::header_field(i, b"tagger", parse::signature))
-            .context(StrContext::Expected("tagger <signature>".into())),
+        opt(|i: &mut _| {
+            parse::header_field(i, b"tagger", parse::signature_with_raw).map(|(signature, raw)| {
+                let _ = signature;
+                raw
+            })
+        })
+        .context(StrContext::Expected("tagger <signature>".into())),
         terminated(message, eof),
     )
-        .map(
-            |(target, kind, tag_version, signature, (message, pgp_signature))| TagRef {
-                target,
-                name: tag_version.as_bstr(),
-                target_kind: kind,
-                message,
-                tagger: signature,
-                pgp_signature,
-            },
-        )
+        .map(|(target, kind, tag_version, tagger, (message, pgp_signature))| TagRef {
+            target,
+            name: tag_version.as_bstr(),
+            target_kind: kind,
+            message,
+            tagger,
+            pgp_signature,
+        })
         .parse_next(i)
 }
 

gix-object/src/tag/mod.rs

@@ -24,6 +24,13 @@ impl<'a> TagRef<'a> {
         gix_hash::ObjectId::from_hex(self.target).expect("prior validation")
     }
 
+    /// Return the tagger, if present.
+    pub fn tagger(&self) -> Option<gix_actor::SignatureRef<'a>> {
+        self.tagger.map(|raw| {
+            gix_actor::SignatureRef::from_bytes::<()>(raw.as_ref()).expect("tagger was validated during parsing")
+        })
+    }
+
     /// Copy all data into a fully-owned instance.
     pub fn into_owned(self) -> crate::Tag {
         self.into()

gix-object/src/tag/write.rs

@@ -1,10 +1,27 @@
 use std::io;
 
-use bstr::BStr;
+use bstr::{BStr, ByteSlice};
 use gix_date::parse::TimeBuf;
 
 use crate::{encode, encode::NL, Kind, Tag, TagRef};
 
+fn parse_signature(raw: &BStr) -> gix_actor::SignatureRef<'_> {
+    gix_actor::SignatureRef::from_bytes::<()>(raw.as_ref()).expect("signatures were validated during parsing")
+}
+
+fn signature_requires_raw(raw: &BStr) -> bool {
+    let signature = parse_signature(raw);
+    signature.name.find_byteset(b"<>\n").is_some() || signature.email.find_byteset(b"<>\n").is_some()
+}
+
+fn signature_len(raw: &BStr) -> usize {
+    if signature_requires_raw(raw) {
+        raw.len()
+    } else {
+        parse_signature(raw).size()
+    }
+}
+
 /// An Error used in [`Tag::write_to()`][crate::WriteTo::write_to()].
 #[derive(Debug, thiserror::Error)]
 #[allow(missing_docs)]
@@ -64,8 +81,13 @@ impl crate::WriteTo for TagRef<'_> {
         encode::trusted_header_field(b"object", self.target, &mut out)?;
         encode::trusted_header_field(b"type", self.target_kind.as_bytes(), &mut out)?;
         encode::header_field(b"tag", validated_name(self.name)?, &mut out)?;
-        if let Some(tagger) = &self.tagger {
-            encode::trusted_header_signature(b"tagger", tagger, &mut out)?;
+        if let Some(tagger) = self.tagger {
+            if signature_requires_raw(tagger) {
+                encode::trusted_header_field(b"tagger", tagger.as_ref(), &mut out)?;
+            } else {
+                let signature = parse_signature(tagger);
+                encode::trusted_header_signature(b"tagger", &signature, &mut out)?;
+            }
         }
 
         if !self.message.iter().all(|b| *b == b'\n') {
@@ -89,8 +111,7 @@ impl crate::WriteTo for TagRef<'_> {
             + b"tag".len() + 1 /* space */ + self.name.len() + 1 /* nl */
             + self
                 .tagger
-                .as_ref()
-                .map_or(0, |t| b"tagger".len() + 1 /* space */ + t.size() + 1 /* nl */)
+                .map_or(0, |raw| b"tagger".len() + 1 /* space */ + signature_len(raw) + 1 /* nl */)
             + if self.message.iter().all(|b| *b == b'\n') { 0 } else { 1 /* nl */ } + self.message.len()
             + self.pgp_signature.as_ref().map_or(0, |m| 1 /* nl */ + m.len())) as u64
     }