fix!: expose raw commit/tag actor headers for round-tripping.

Note that this means you have to call `CommitRef::commiter|author()?` and `TagRef::tagger()?` instead
of assuming pre-parsed fields.

This PR makes signature handling truly lossless for "creative" emails and other info.
We now stash the raw name <email> slice on IdentityRef/SignatureRef and fall back to it when rewriting,
so even commits with embedded angle brackets round-trip cleanly (might want to expand to other malformed characters before merging?
Parsing and serialization honor that flag but still keep strict validation for normal input.
I also added regression coverage for these scenarios.

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-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."
     )]
@@ -28,6 +30,8 @@ pub enum Error {
 }
 
 pub(super) mod function {
+    use std::convert::TryFrom;
+
     use gix_object::FindExt;
 
     use super::Error;
@@ -128,7 +132,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 = gix_object::Commit::try_from(commit_ref)?;
         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,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,11 @@ impl<'a> CommitRef<'a> {
 
 /// Access
 impl<'a> CommitRef<'a> {
+    fn parse_signature(raw: &'a BStr) -> Result<gix_actor::SignatureRef<'a>, 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()))
+    }
+
     /// 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")
@@ -94,15 +99,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> {
+        Self::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> {
+        Self::parse_signature(self.committer).map(|signature| signature.trim())
     }
 
     /// Returns a partially parsed message from which more information can be derived.
@@ -111,21 +116,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> {
+        Self::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> {
+        Commit::try_from(self)
     }
 
     /// 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> {
+        Commit::try_from(self.clone())
     }
 }
 

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,38 +1,55 @@
+use std::convert::TryFrom;
+
 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 tagger = tagger
+            .map(|raw| {
+                gix_actor::SignatureRef::from_bytes::<crate::decode::ParseError>(raw.as_ref())
+                    .map_err(|err| crate::decode::Error::with_err(err, raw.as_ref()))
+            })
+            .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,
             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,
-            author,
-            committer,
+            author: author_raw,
+            committer: committer_raw,
             encoding,
             message,
             extra_headers,
         } = other;
-        Commit {
+        let author = gix_actor::SignatureRef::from_bytes::<crate::decode::ParseError>(author_raw.as_ref())
+            .map_err(|err| crate::decode::Error::with_err(err, author_raw.as_ref()))?;
+        let committer = gix_actor::SignatureRef::from_bytes::<crate::decode::ParseError>(committer_raw.as_ref())
+            .map_err(|err| crate::decode::Error::with_err(err, committer_raw.as_ref()))?;
+        Ok(Commit {
             tree: gix_hash::ObjectId::from_hex(tree).expect("prior parser validation"),
             parents: parents
                 .iter()
@@ -46,7 +63,7 @@ impl From<CommitRef<'_>> for Commit {
                 .into_iter()
                 .map(|(k, v)| (k.into(), v.into_owned()))
                 .collect(),
-        }
+        })
     }
 }
 
@@ -89,14 +106,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(Commit::try_from(v)?),
+            ObjectRef::Tag(v) => Object::Tag(Tag::try_from(v)?),
+        })
     }
 }
 

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> {
+        Object::try_from(self)
     }
 
     /// 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> {
+        Object::try_from(self.clone())
     }
 }
 

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)
 }