fix!: make signature access fallible and preserve raw actor headers

Author: Pingasmaster

examples/log.rs

@@ -142,7 +142,7 @@ fn run(args: Args) -> anyhow::Result<()> {
                 let info = info?;
                 let commit = info.object()?;
                 let commit_ref = commit.decode()?;
-                let author = commit_ref.author();
+                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(),

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/mod.rs

@@ -74,8 +74,9 @@ 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")
+    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.
@@ -98,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::parse_signature(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::parse_signature(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.
@@ -115,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::parse_signature(self.committer).time().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

@@ -4,32 +4,6 @@ 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<()> {
@@ -84,8 +58,8 @@ impl crate::WriteTo for CommitRef<'_> {
         for parent in self.parents() {
             encode::trusted_header_id(b"parent", &parent, &mut out)?;
         }
-        write_signature(&mut out, b"author", self.author)?;
-        write_signature(&mut out, b"committer", self.committer)?;
+        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)?;
         }
@@ -104,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 */ + signature_len(self.author) + 1 /* nl */
-            + b"committer".len() + 1 /* space */ + signature_len(self.committer) + 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/object/convert.rs

@@ -1,7 +1,11 @@
+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,
@@ -10,38 +14,42 @@ impl From<TagRef<'_>> for Tag {
             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 {
+        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,
             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;
-        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 {
+        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()
@@ -55,7 +63,7 @@ impl From<CommitRef<'_>> for Commit {
                 .into_iter()
                 .map(|(k, v)| (k.into(), v.into_owned()))
                 .collect(),
-        }
+        })
     }
 }
 
@@ -98,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/tag/mod.rs

@@ -25,14 +25,18 @@ impl<'a> TagRef<'a> {
     }
 
     /// 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")
-        })
+    pub fn tagger(&self) -> Result<Option<gix_actor::SignatureRef<'a>>, crate::decode::Error> {
+        self.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()))
+                    .map(|signature| signature.trim())
+            })
+            .transpose()
     }
 
     /// 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> {
+        crate::Tag::try_from(self)
     }
 }

gix-object/src/tag/write.rs

@@ -1,27 +1,10 @@
 use std::io;
 
-use bstr::{BStr, ByteSlice};
+use bstr::BStr;
 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)]
@@ -82,12 +65,7 @@ impl crate::WriteTo for TagRef<'_> {
         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 {
-            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)?;
-            }
+            encode::trusted_header_field(b"tagger", tagger.as_ref(), &mut out)?;
         }
 
         if !self.message.iter().all(|b| *b == b'\n') {
@@ -111,7 +89,7 @@ impl crate::WriteTo for TagRef<'_> {
             + b"tag".len() + 1 /* space */ + self.name.len() + 1 /* nl */
             + self
                 .tagger
-                .map_or(0, |raw| b"tagger".len() + 1 /* space */ + signature_len(raw) + 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/object/commit/from_bytes.rs

@@ -46,8 +46,8 @@ fn invalid_email_of_committer() {
             extra_headers: vec![]
         }
     );
-    assert_eq!(commit.author(), actor);
-    assert_eq!(commit.committer(), actor);
+    assert_eq!(commit.author().unwrap(), actor);
+    assert_eq!(commit.committer().unwrap(), actor);
 }
 
 #[test]
@@ -123,8 +123,8 @@ fn mergetag() -> crate::Result {
     assert_eq!(commit, expected);
     assert_eq!(commit.extra_headers().find_all("mergetag").count(), 1);
     assert_eq!(commit.extra_headers().mergetags().count(), 1);
-    assert_eq!(commit.author(), linus_signature("1591996221 -0700"));
-    assert_eq!(commit.committer(), linus_signature("1591996221 -0700"));
+    assert_eq!(commit.author().unwrap(), linus_signature("1591996221 -0700"));
+    assert_eq!(commit.committer().unwrap(), linus_signature("1591996221 -0700"));
     Ok(())
 }
 
@@ -244,8 +244,8 @@ Signed-off-by: Kim Altintop <kim@eagain.st>"
             extra_headers: vec![(b"gpgsig".as_bstr(), b"-----BEGIN PGP SIGNATURE-----\n\niHUEABYIAB0WIQSuZwcGWSQItmusNgR5URpSUCnwXQUCYT7xpAAKCRB5URpSUCnw\nXWB3AP9q323HlxnI8MyqszNOeYDwa7Y3yEZaUM2y/IRjz+z4YQEAq0yr1Syt3mrK\nOSFCqL2vDm3uStP+vF31f6FnzayhNg0=\n=Mhpp\n-----END PGP SIGNATURE-----\n".as_bstr().into())]
         }
     );
-    assert_eq!(commit.author(), kim);
-    assert_eq!(commit.committer(), kim);
+    assert_eq!(commit.author().unwrap(), kim);
+    assert_eq!(commit.committer().unwrap(), kim);
     let message = commit.message();
     assert_eq!(message.title, "test: use gitoxide for link-git-protocol tests");
     assert_eq!(
@@ -364,7 +364,7 @@ fn bogus_multi_gpgsig_header() -> crate::Result {
 fn author_method_returns_trimmed_signature() -> crate::Result {
     let backing = fixture_name("commit", "unsigned.txt");
     let commit = CommitRef::from_bytes(&backing)?;
-    assert_eq!(commit.author(), signature("1592437401 +0800"));
-    assert_eq!(commit.committer(), signature("1592437401 +0800"));
+    assert_eq!(commit.author().unwrap(), signature("1592437401 +0800"));
+    assert_eq!(commit.committer().unwrap(), signature("1592437401 +0800"));
     Ok(())
 }