fix!: store raw commit/tag actor headers and parse lazily (see #2177)

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-merge/src/commit/virtual_merge_base.rs

@@ -19,6 +19,8 @@ pub enum Error {
     MergeTree(#[from] crate::tree::Error),
     #[error("Failed to write tree for merged merge-base or virtual commit")]
     WriteObject(gix_object::write::Error),
+    #[error("Failed to decode a commit needed to build a virtual merge-base")]
+    DecodeCommit(#[from] gix_object::decode::Error),
     #[error(
         "Conflicts occurred when trying to resolve multiple merge-bases by merging them. This is most certainly a bug."
     )]
@@ -128,7 +130,8 @@ pub(super) mod function {
         tree_id: gix_hash::ObjectId,
     ) -> Result<gix_hash::ObjectId, Error> {
         let mut buf = Vec::new();
-        let mut commit: gix_object::Commit = objects.find_commit(&parent_a, &mut buf)?.into();
+        let commit_ref = objects.find_commit(&parent_a, &mut buf)?;
+        let mut commit = commit_ref.to_owned()?;
         commit.parents = vec![parent_a, parent_b].into();
         commit.tree = tree_id;
         objects.write(&commit).map_err(Error::WriteObject)

gix-object/src/commit/decode.rs

@@ -40,9 +40,9 @@ 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))
+        (|i: &mut _| parse::header_field(i, b"author", parse::signature_and_consumed).map(|(_signature, raw)| raw))
             .context(StrContext::Expected("author <signature>".into())),
-        (|i: &mut _| parse::header_field(i, b"committer", parse::signature))
+        (|i: &mut _| parse::header_field(i, b"committer", parse::signature_and_consumed).map(|(_signature, raw)| 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())),

gix-object/src/commit/mod.rs

@@ -3,6 +3,7 @@ use std::ops::Range;
 use bstr::{BStr, BString, ByteSlice};
 use winnow::prelude::*;
 
+use crate::parse::parse_signature;
 use crate::{Commit, CommitRef, TagRef};
 
 /// The well-known field name for gpg signatures.
@@ -94,15 +95,15 @@ impl<'a> CommitRef<'a> {
     /// Return the author, with whitespace trimmed.
     ///
     /// This is different from the `author` field which may contain whitespace.
-    pub fn author(&self) -> gix_actor::SignatureRef<'a> {
-        self.author.trim()
+    pub fn author(&self) -> Result<gix_actor::SignatureRef<'a>, crate::decode::Error> {
+        parse_signature(self.author).map(|signature| signature.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()
+    pub fn committer(&self) -> Result<gix_actor::SignatureRef<'a>, crate::decode::Error> {
+        parse_signature(self.committer).map(|signature| signature.trim())
     }
 
     /// Returns a partially parsed message from which more information can be derived.
@@ -111,21 +112,21 @@ 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()
+    pub fn time(&self) -> Result<gix_date::Time, crate::decode::Error> {
+        parse_signature(self.committer).map(|signature| signature.time().unwrap_or_default())
     }
 }
 
 /// Conversion
 impl CommitRef<'_> {
     /// Copy all fields of this instance into a fully owned commit, consuming this instance.
-    pub fn into_owned(self) -> Commit {
-        self.into()
+    pub fn into_owned(self) -> Result<Commit, crate::decode::Error> {
+        self.try_into()
     }
 
     /// Copy all fields of this instance into a fully owned commit, internally cloning this instance.
-    pub fn to_owned(self) -> Commit {
-        self.clone().into()
+    pub fn to_owned(self) -> Result<Commit, crate::decode::Error> {
+        self.try_into()
     }
 }
 

gix-object/src/commit/write.rs

@@ -58,8 +58,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)?;
+        encode::trusted_header_field(b"author", self.author.as_ref(), &mut out)?;
+        encode::trusted_header_field(b"committer", self.committer.as_ref(), &mut out)?;
         if let Some(encoding) = self.encoding.as_ref() {
             encode::header_field(b"encoding", encoding, &mut out)?;
         }
@@ -78,8 +78,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 */ + self.author.len() + 1 /* nl */
+            + b"committer".len() + 1 /* space */ + self.committer.len() + 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

@@ -1,28 +1,36 @@
+use std::convert::TryFrom;
+
+use crate::parse::parse_signature;
 use crate::{tree, Blob, BlobRef, Commit, CommitRef, Object, ObjectRef, Tag, TagRef, Tree, TreeRef};
 
-impl From<TagRef<'_>> for Tag {
-    fn from(other: TagRef<'_>) -> Tag {
+impl TryFrom<TagRef<'_>> for Tag {
+    type Error = crate::decode::Error;
+
+    fn try_from(other: TagRef<'_>) -> Result<Tag, Self::Error> {
         let TagRef {
             target,
             name,
             target_kind,
             message,
-            tagger: signature,
+            tagger,
             pgp_signature,
         } = other;
-        Tag {
+        let untrimmed_tagger = tagger.map(parse_signature).transpose()?.map(Into::into);
+        Ok(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: untrimmed_tagger,
             pgp_signature: pgp_signature.map(ToOwned::to_owned),
-        }
+        })
     }
 }
 
-impl From<CommitRef<'_>> for Commit {
-    fn from(other: CommitRef<'_>) -> Commit {
+impl TryFrom<CommitRef<'_>> for Commit {
+    type Error = crate::decode::Error;
+
+    fn try_from(other: CommitRef<'_>) -> Result<Commit, Self::Error> {
         let CommitRef {
             tree,
             parents,
@@ -32,21 +40,24 @@ impl From<CommitRef<'_>> for Commit {
             message,
             extra_headers,
         } = other;
-        Commit {
+
+        let untrimmed_author = parse_signature(author)?;
+        let untrimmed_committer = parse_signature(committer)?;
+        Ok(Commit {
             tree: gix_hash::ObjectId::from_hex(tree).expect("prior parser validation"),
             parents: parents
                 .iter()
                 .map(|parent| gix_hash::ObjectId::from_hex(parent).expect("prior parser validation"))
                 .collect(),
-            author: author.into(),
-            committer: committer.into(),
+            author: untrimmed_author.into(),
+            committer: untrimmed_committer.into(),
             encoding: encoding.map(ToOwned::to_owned),
             message: message.to_owned(),
             extra_headers: extra_headers
                 .into_iter()
                 .map(|(k, v)| (k.into(), v.into_owned()))
                 .collect(),
-        }
+        })
     }
 }
 
@@ -89,14 +100,16 @@ impl<'a> From<&'a tree::Entry> for tree::EntryRef<'a> {
     }
 }
 
-impl From<ObjectRef<'_>> for Object {
-    fn from(v: ObjectRef<'_>) -> Self {
-        match v {
+impl TryFrom<ObjectRef<'_>> for Object {
+    type Error = crate::decode::Error;
+
+    fn try_from(v: ObjectRef<'_>) -> Result<Self, Self::Error> {
+        Ok(match v {
             ObjectRef::Tree(v) => Object::Tree(v.into()),
             ObjectRef::Blob(v) => Object::Blob(v.into()),
-            ObjectRef::Commit(v) => Object::Commit(v.into()),
-            ObjectRef::Tag(v) => Object::Tag(v.into()),
-        }
+            ObjectRef::Commit(v) => Object::Commit(v.try_into()?),
+            ObjectRef::Tag(v) => Object::Tag(v.try_into()?),
+        })
     }
 }
 

gix-object/src/object/mod.rs

@@ -216,15 +216,15 @@ impl<'a> ObjectRef<'a> {
     /// Convert the immutable object into a mutable version, consuming the source in the process.
     ///
     /// Note that this is an expensive operation.
-    pub fn into_owned(self) -> Object {
-        self.into()
+    pub fn into_owned(self) -> Result<Object, crate::decode::Error> {
+        self.try_into()
     }
 
     /// Convert this immutable object into its mutable counterpart.
     ///
     /// Note that this is an expensive operation.
-    pub fn to_owned(&self) -> Object {
-        self.clone().into()
+    pub fn to_owned(&self) -> Result<Object, crate::decode::Error> {
+        self.clone().try_into()
     }
 }
 

gix-object/src/parse.rs

@@ -71,3 +71,18 @@ 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_and_consumed<'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())
+    })
+}
+
+pub(crate) fn parse_signature(raw: &BStr) -> Result<gix_actor::SignatureRef<'_>, crate::decode::Error> {
+    gix_actor::SignatureRef::from_bytes::<crate::decode::ParseError>(raw.as_ref())
+        .map_err(|err| crate::decode::Error::with_err(err, raw.as_ref()))
+}

gix-object/src/tag/decode.rs

@@ -19,20 +19,18 @@ 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))
+        opt(|i: &mut _| parse::header_field(i, b"tagger", parse::signature_and_consumed).map(|(_signature, raw)| 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

@@ -1,6 +1,6 @@
-use winnow::prelude::*;
-
+use crate::parse::parse_signature;
 use crate::TagRef;
+use winnow::prelude::*;
 
 mod decode;
 
@@ -24,8 +24,17 @@ impl<'a> TagRef<'a> {
         gix_hash::ObjectId::from_hex(self.target).expect("prior validation")
     }
 
+    /// Return the tagger, if present.
+    pub fn tagger(&self) -> Result<Option<gix_actor::SignatureRef<'a>>, crate::decode::Error> {
+        Ok(self
+            .tagger
+            .map(parse_signature)
+            .transpose()?
+            .map(|signature| signature.trim()))
+    }
+
     /// Copy all data into a fully-owned instance.
-    pub fn into_owned(self) -> crate::Tag {
-        self.into()
+    pub fn into_owned(self) -> Result<crate::Tag, crate::decode::Error> {
+        self.try_into()
     }
 }

gix-object/src/tag/write.rs

@@ -64,8 +64,8 @@ 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 {
+            encode::trusted_header_field(b"tagger", tagger.as_ref(), &mut out)?;
         }
 
         if !self.message.iter().all(|b| *b == b'\n') {
@@ -89,8 +89,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 */ + raw.len() + 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
     }

gix-object/tests/fixtures/tag/tagger-with-whitespace.txt

@@ -0,0 +1,5 @@
+object 01dd4e2a978a9f5bd773dae6da7aa4a5ac1cdbbc
+type commit
+tag empty
+tagger Sebastian Thiel <sebastian.thiel@icloud.com > 1592381636 +0800
+