refactor

- remove unwraps()
- reduce duplication

Author: Byron

gix-merge/src/commit/virtual_merge_base.rs

@@ -30,8 +30,6 @@ pub enum Error {
 }
 
 pub(super) mod function {
-    use std::convert::TryFrom;
-
     use gix_object::FindExt;
 
     use super::Error;
@@ -133,7 +131,7 @@ pub(super) mod function {
     ) -> Result<gix_hash::ObjectId, Error> {
         let mut buf = Vec::new();
         let commit_ref = objects.find_commit(&parent_a, &mut buf)?;
-        let mut commit = gix_object::Commit::try_from(commit_ref)?;
+        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,20 +40,10 @@ 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_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())),
+        (|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_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())),
         repeat(

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.
@@ -74,11 +75,6 @@ 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")
@@ -100,14 +96,14 @@ impl<'a> CommitRef<'a> {
     ///
     /// This is different from the `author` field which may contain whitespace.
     pub fn author(&self) -> Result<gix_actor::SignatureRef<'a>, crate::decode::Error> {
-        Self::parse_signature(self.author).map(|signature| signature.trim())
+        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) -> Result<gix_actor::SignatureRef<'a>, crate::decode::Error> {
-        Self::parse_signature(self.committer).map(|signature| signature.trim())
+        parse_signature(self.committer).map(|signature| signature.trim())
     }
 
     /// Returns a partially parsed message from which more information can be derived.
@@ -117,20 +113,20 @@ 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) -> Result<gix_date::Time, crate::decode::Error> {
-        Self::parse_signature(self.committer).map(|signature| signature.time().unwrap_or_default())
+        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) -> Result<Commit, crate::decode::Error> {
-        Commit::try_from(self)
+        self.try_into()
     }
 
     /// Copy all fields of this instance into a fully owned commit, internally cloning this instance.
     pub fn to_owned(self) -> Result<Commit, crate::decode::Error> {
-        Commit::try_from(self.clone())
+        self.try_into()
     }
 }
 

gix-object/src/object/convert.rs

@@ -1,5 +1,6 @@
 use std::convert::TryFrom;
 
+use crate::parse::parse_signature;
 use crate::{tree, Blob, BlobRef, Commit, CommitRef, Object, ObjectRef, Tag, TagRef, Tree, TreeRef};
 
 impl TryFrom<TagRef<'_>> for Tag {
@@ -14,19 +15,13 @@ impl TryFrom<TagRef<'_>> for Tag {
             tagger,
             pgp_signature,
         } = other;
-        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);
+        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,
+            tagger: untrimmed_tagger,
             pgp_signature: pgp_signature.map(ToOwned::to_owned),
         })
     }
@@ -39,24 +34,23 @@ impl TryFrom<CommitRef<'_>> for Commit {
         let CommitRef {
             tree,
             parents,
-            author: author_raw,
-            committer: committer_raw,
+            author,
+            committer,
             encoding,
             message,
             extra_headers,
         } = other;
-        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()))?;
+
+        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
@@ -113,8 +107,8 @@ impl TryFrom<ObjectRef<'_>> for Object {
         Ok(match v {
             ObjectRef::Tree(v) => Object::Tree(v.into()),
             ObjectRef::Blob(v) => Object::Blob(v.into()),
-            ObjectRef::Commit(v) => Object::Commit(Commit::try_from(v)?),
-            ObjectRef::Tag(v) => Object::Tag(Tag::try_from(v)?),
+            ObjectRef::Commit(v) => Object::Commit(v.try_into()?),
+            ObjectRef::Tag(v) => Object::Tag(v.try_into()?),
         })
     }
 }

gix-object/src/object/mod.rs

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

gix-object/src/parse.rs

@@ -72,7 +72,7 @@ pub(crate) fn signature<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrC
     gix_actor::signature::decode(i)
 }
 
-pub(crate) fn signature_with_raw<'a, E: ParserError<&'a [u8]> + AddContext<&'a [u8], StrContext>>(
+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;
@@ -81,3 +81,8 @@ pub(crate) fn signature_with_raw<'a, E: ParserError<&'a [u8]> + AddContext<&'a [
         (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,13 +19,8 @@ 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_with_raw).map(|(signature, raw)| {
-                let _ = signature;
-                raw
-            })
-        })
-        .context(StrContext::Expected("tagger <signature>".into())),
+        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, tagger, (message, pgp_signature))| TagRef {

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;
 
@@ -26,17 +26,15 @@ impl<'a> TagRef<'a> {
 
     /// Return the tagger, if present.
     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()
+        Ok(self
+            .tagger
+            .map(parse_signature)
+            .transpose()?
+            .map(|signature| signature.trim()))
     }
 
     /// Copy all data into a fully-owned instance.
     pub fn into_owned(self) -> Result<crate::Tag, crate::decode::Error> {
-        crate::Tag::try_from(self)
+        self.try_into()
     }
 }

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
+

gix-object/tests/object/commit/from_bytes.rs

@@ -4,7 +4,7 @@ use smallvec::SmallVec;
 
 use crate::{
     commit::{LONG_MESSAGE, MERGE_TAG, SIGNATURE},
-    fixture_name, hex_to_id, linus_signature, signature,
+    fixture_name, hex_to_id, linus_signature,
 };
 
 #[test]
@@ -26,12 +26,14 @@ fn invalid_timestsamp() {
 }
 
 #[test]
-fn invalid_email_of_committer() {
+fn invalid_email_of_committer() -> crate::Result {
     let actor = gix_actor::SignatureRef {
         name: b"Gregor Hartmann".as_bstr(),
         email: b"gh <Gregor Hartmann<gh@openoffice.org".as_bstr(),
         time: "1282910542 +0200",
     };
+
+    let mut buf = vec![];
     let backing = fixture_name("commit", "invalid-actor.txt");
     let commit = CommitRef::from_bytes(&backing).expect("ignore strangely formed actor format");
     assert_eq!(
@@ -46,8 +48,17 @@ fn invalid_email_of_committer() {
             extra_headers: vec![]
         }
     );
-    assert_eq!(commit.author().unwrap(), actor);
-    assert_eq!(commit.committer().unwrap(), actor);
+    assert_eq!(commit.author()?, actor);
+    assert_eq!(commit.committer()?, actor);
+
+    commit.write_to(&mut buf).expect("we can write invalid actors back");
+    assert_eq!(
+        CommitRef::from_bytes(&buf).expect("this is the same commit and it can be parsed"),
+        commit,
+        "round-tripping works"
+    );
+
+    Ok(())
 }
 
 #[test]
@@ -123,8 +134,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().unwrap(), linus_signature("1591996221 -0700"));
-    assert_eq!(commit.committer().unwrap(), linus_signature("1591996221 -0700"));
+    assert_eq!(commit.author()?, linus_signature("1591996221 -0700"));
+    assert_eq!(commit.committer()?, linus_signature("1591996221 -0700"));
     Ok(())
 }
 
@@ -244,8 +255,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().unwrap(), kim);
-    assert_eq!(commit.committer().unwrap(), kim);
+    assert_eq!(commit.author()?, kim);
+    assert_eq!(commit.committer()?, kim);
     let message = commit.message();
     assert_eq!(message.title, "test: use gitoxide for link-git-protocol tests");
     assert_eq!(
@@ -359,12 +370,3 @@ fn bogus_multi_gpgsig_header() -> crate::Result {
     );
     Ok(())
 }
-
-#[test]
-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().unwrap(), signature("1592437401 +0800"));
-    assert_eq!(commit.committer().unwrap(), signature("1592437401 +0800"));
-    Ok(())
-}