Full error handling for CommitRefIter (#364)

Author: Byron

experiments/traversal/src/main.rs

@@ -209,7 +209,7 @@ where
             for commit in commits {
                 let tree_id = db
                     .try_find(commit, &mut buf)?
-                    .and_then(|o| o.try_into_commit_iter().and_then(|mut c| c.tree_id()))
+                    .and_then(|o| o.try_into_commit_iter().and_then(|mut c| c.tree_id().ok()))
                     .expect("commit as starting point");
 
                 let mut count = Count { entries: 0, seen };

git-actor/src/lib.rs

@@ -32,7 +32,7 @@ pub struct Signature {
 /// A immutable signature is created by an actor at a certain time.
 ///
 /// Note that this is not a cryptographical signature.
-#[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy)]
+#[derive(PartialEq, Eq, Debug, Hash, Ord, PartialOrd, Clone, Copy, Default)]
 #[cfg_attr(feature = "serde1", derive(serde::Serialize, serde::Deserialize))]
 pub struct SignatureRef<'a> {
     /// The actor's name.

git-object/src/commit/ref_iter.rs

@@ -49,8 +49,9 @@ impl<'a> CommitRefIter<'a> {
     /// Errors are coerced into options, hiding whether there was an error or not. The caller should assume an error if they
     /// call the method as intended. Such a squelched error cannot be recovered unless the objects data is retrieved and parsed again.
     /// `next()`.
-    pub fn tree_id(&mut self) -> Option<ObjectId> {
-        self.next().and_then(Result::ok).and_then(Token::try_into_id)
+    pub fn tree_id(&mut self) -> Result<ObjectId, crate::decode::Error> {
+        let tree_id = self.next().ok_or_else(missing_field)??;
+        Token::try_into_id(tree_id).ok_or_else(missing_field)
     }
 
     /// Return all parent_ids as iterator.
@@ -77,32 +78,43 @@ impl<'a> CommitRefIter<'a> {
 
     /// Returns the committer signature if there is no decoding error.
     /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
-    pub fn committer(mut self) -> Option<git_actor::SignatureRef<'a>> {
+    pub fn committer(mut self) -> Result<git_actor::SignatureRef<'a>, crate::decode::Error> {
         self.find_map(|t| match t {
-            Ok(Token::Committer { signature }) => Some(signature),
+            Ok(Token::Committer { signature }) => Some(Ok(signature)),
+            Err(err) => Some(Err(err)),
             _ => None,
         })
+        .ok_or_else(missing_field)?
     }
 
     /// Returns the author signature if there is no decoding error.
     /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
-    pub fn author(mut self) -> Option<git_actor::SignatureRef<'a>> {
+    pub fn author(mut self) -> Result<git_actor::SignatureRef<'a>, crate::decode::Error> {
         self.find_map(|t| match t {
-            Ok(Token::Author { signature }) => Some(signature),
+            Ok(Token::Author { signature }) => Some(Ok(signature)),
+            Err(err) => Some(Err(err)),
             _ => None,
         })
+        .ok_or_else(missing_field)?
     }
 
     /// Returns the message if there is no decoding error.
     /// Errors are coerced into options, hiding whether there was an error or not. The caller knows if there was an error or not.
-    pub fn message(mut self) -> Option<&'a BStr> {
+    pub fn message(mut self) -> Result<&'a BStr, crate::decode::Error> {
         self.find_map(|t| match t {
-            Ok(Token::Message(msg)) => Some(msg),
+            Ok(Token::Message(msg)) => Some(Ok(msg)),
+            Err(err) => Some(Err(err)),
             _ => None,
         })
+        .transpose()
+        .map(|msg| msg.unwrap_or_default())
     }
 }
 
+fn missing_field() -> crate::decode::Error {
+    crate::decode::empty_error()
+}
+
 impl<'a> CommitRefIter<'a> {
     fn next_inner(i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
         use State::*;

git-object/src/lib.rs

@@ -253,6 +253,12 @@ pub mod decode {
         /// The owned type to be used for parse errors.
         pub type ParseErrorOwned = nom::error::VerboseError<BString>;
 
+        pub(crate) fn empty_error() -> Error {
+            Error {
+                inner: nom::error::VerboseError::<BString> { errors: Vec::new() },
+            }
+        }
+
         /// A type to indicate errors during parsing and to abstract away details related to `nom`.
         #[derive(Debug, Clone)]
         pub struct Error {
@@ -292,6 +298,10 @@ pub mod decode {
         /// The owned type to be used for parse errors, discards everything and is zero size
         pub type ParseErrorOwned = ();
 
+        pub(crate) fn empty_error() -> Error {
+            Error { inner: () }
+        }
+
         /// A type to indicate errors during parsing and to abstract away details related to `nom`.
         #[derive(Debug, Clone)]
         pub struct Error {
@@ -316,6 +326,7 @@ pub mod decode {
             }
         }
     }
+    pub(crate) use _decode::empty_error;
     pub use _decode::{Error, ParseError, ParseErrorOwned};
     impl std::error::Error for Error {}
 

git-object/src/tag/ref_iter.rs

@@ -39,11 +39,16 @@ impl<'a> TagRefIter<'a> {
     /// Errors are coerced into options, hiding whether there was an error or not. The caller should assume an error if they
     /// call the method as intended. Such a squelched error cannot be recovered unless the objects data is retrieved and parsed again.
     /// `next()`.
-    pub fn target_id(&mut self) -> Option<ObjectId> {
-        self.next().and_then(Result::ok).and_then(Token::into_id)
+    pub fn target_id(&mut self) -> Result<ObjectId, crate::decode::Error> {
+        let token = self.next().ok_or_else(missing_field)??;
+        Token::into_id(token).ok_or_else(missing_field)
     }
 }
 
+fn missing_field() -> crate::decode::Error {
+    crate::decode::empty_error()
+}
+
 impl<'a> TagRefIter<'a> {
     fn next_inner(i: &'a [u8], state: &mut State) -> Result<(&'a [u8], Token<'a>), crate::decode::Error> {
         use State::*;

git-object/tests/immutable/commit/iter.rs

@@ -25,7 +25,7 @@ fn newline_right_after_signature_multiline_header() -> crate::Result {
 #[test]
 fn signed_with_encoding() -> crate::Result {
     let input = fixture_bytes("commit", "signed-with-encoding.txt");
-    let mut iter = CommitRefIter::from_bytes(&input);
+    let iter = CommitRefIter::from_bytes(&input);
     assert_eq!(
         iter.collect::<Result<Vec<_>, _>>()?,
         vec![
@@ -47,8 +47,8 @@ fn signed_with_encoding() -> crate::Result {
         ]
     );
 
-    assert_eq!(iter.author(), Some(signature(1592448995)));
-    assert_eq!(iter.committer(), Some(signature(1592449083)));
+    assert_eq!(iter.author().ok(), Some(signature(1592448995)));
+    assert_eq!(iter.committer().ok(), Some(signature(1592449083)));
     Ok(())
 }
 
@@ -140,7 +140,7 @@ fn error_handling() -> crate::Result {
 #[test]
 fn mergetag() -> crate::Result {
     let input = fixture_bytes("commit", "mergetag.txt");
-    let mut iter = CommitRefIter::from_bytes(&input);
+    let iter = CommitRefIter::from_bytes(&input);
     assert_eq!(
         iter.collect::<Result<Vec<_>, _>>()?,
         vec![
@@ -170,7 +170,7 @@ fn mergetag() -> crate::Result {
             hex_to_id("8d485da0ddee79d0e6713405694253d401e41b93")
         ]
     );
-    assert_eq!(iter.message(), Some(LONG_MESSAGE.into()));
+    assert_eq!(iter.message().ok(), Some(LONG_MESSAGE.into()));
     Ok(())
 }
 
@@ -187,7 +187,7 @@ mod method {
         let input = fixture_bytes("commit", "unsigned.txt");
         let iter = CommitRefIter::from_bytes(&input);
         assert_eq!(
-            iter.clone().tree_id(),
+            iter.clone().tree_id().ok(),
             Some(hex_to_id("1b2dfb4ac5e42080b682fc676e9738c94ce6d54d"))
         );
         assert_eq!(
@@ -201,14 +201,14 @@ mod method {
     #[test]
     fn signatures() -> crate::Result {
         let input = fixture_bytes("commit", "unsigned.txt");
-        let mut iter = CommitRefIter::from_bytes(&input);
+        let iter = CommitRefIter::from_bytes(&input);
         assert_eq!(
             iter.signatures().collect::<Vec<_>>(),
             vec![signature(1592437401), signature(1592437401)]
         );
-        assert_eq!(iter.author(), Some(signature(1592437401)));
-        assert_eq!(iter.committer(), Some(signature(1592437401)));
-        assert_eq!(iter.committer(), None, "it's consuming");
+        assert_eq!(iter.author().ok(), Some(signature(1592437401)));
+        assert_eq!(iter.committer().ok(), Some(signature(1592437401)));
+        assert_eq!(iter.author().ok(), Some(signature(1592437401)), "it's not consuming");
         Ok(())
     }
 }

git-ref/src/store/file/raw_ext.rs

@@ -118,7 +118,7 @@ impl ReferenceExt for Reference {
                         })?;
                     match kind {
                         git_object::Kind::Tag => {
-                            oid = git_object::TagRefIter::from_bytes(data).target_id().ok_or_else(|| {
+                            oid = git_object::TagRefIter::from_bytes(data).target_id().map_err(|_err| {
                                 peel::to_id::Error::NotFound {
                                     oid,
                                     name: self.name.0.clone(),

git-ref/src/store/packed/transaction.rs

@@ -68,7 +68,7 @@ impl packed::Transaction {
                     let kind = find(next_id, &mut buf)?;
                     match kind {
                         Some(kind) if kind == git_object::Kind::Tag => {
-                            next_id = git_object::TagRefIter::from_bytes(&buf).target_id().ok_or_else(|| {
+                            next_id = git_object::TagRefIter::from_bytes(&buf).target_id().map_err(|_| {
                                 prepare::Error::Resolve(
                                     format!("Couldn't get target object id from tag {}", next_id).into(),
                                 )

git-repository/src/object/commit.rs

@@ -9,7 +9,7 @@ mod error {
         #[error(transparent)]
         FindExistingObject(#[from] object::find::existing::OdbError),
         #[error("The commit could not be decoded fully or partially")]
-        Decode,
+        Decode(#[from] git_object::decode::Error),
         #[error("Expected object of type {}, but got {}", .expected, .actual)]
         ObjectKind {
             expected: git_object::Kind,
@@ -46,14 +46,12 @@ impl<'repo> Commit<'repo> {
     }
 
     /// Parse the commits message into a [`MessageRef`][git_object::commit::MessageRef]
-    pub fn message(&self) -> Result<git_object::commit::MessageRef<'_>, Error> {
+    pub fn message(&self) -> Result<git_object::commit::MessageRef<'_>, git_object::decode::Error> {
         Ok(git_object::commit::MessageRef::from_bytes(self.message_raw()?))
     }
     /// Decode the entire commit object in full and return the raw message bytes.
-    pub fn message_raw(&self) -> Result<&'_ BStr, Error> {
-        git_object::CommitRefIter::from_bytes(&self.data)
-            .message()
-            .ok_or(Error::Decode)
+    pub fn message_raw(&self) -> Result<&'_ BStr, git_object::decode::Error> {
+        git_object::CommitRefIter::from_bytes(&self.data).message()
     }
     /// Decode the commit and obtain the time at which the commit was created.
     ///
@@ -79,17 +77,13 @@ impl<'repo> Commit<'repo> {
     }
 
     /// Return the commits author.
-    pub fn author(&self) -> Result<git_actor::SignatureRef<'_>, Error> {
-        git_object::CommitRefIter::from_bytes(&self.data)
-            .author()
-            .ok_or(Error::Decode)
+    pub fn author(&self) -> Result<git_actor::SignatureRef<'_>, git_object::decode::Error> {
+        git_object::CommitRefIter::from_bytes(&self.data).author()
     }
 
     /// Return the commits committer.
-    pub fn committer(&self) -> Result<git_actor::SignatureRef<'_>, Error> {
-        git_object::CommitRefIter::from_bytes(&self.data)
-            .committer()
-            .ok_or(Error::Decode)
+    pub fn committer(&self) -> Result<git_actor::SignatureRef<'_>, git_object::decode::Error> {
+        git_object::CommitRefIter::from_bytes(&self.data).committer()
     }
 
     /// Decode this commits parent ids on the fly without allocating.
@@ -104,15 +98,15 @@ impl<'repo> Commit<'repo> {
 
     /// Parse the commit and return the the tree object it points to.
     pub fn tree(&self) -> Result<Tree<'repo>, Error> {
-        let tree_id = self.tree_id().ok_or(Error::Decode)?;
+        let tree_id = self.tree_id()?;
         match self.repo.find_object(tree_id)?.try_into_tree() {
             Ok(tree) => Ok(tree),
             Err(crate::object::try_into::Error { actual, expected, .. }) => Err(Error::ObjectKind { actual, expected }),
         }
     }
 
     /// Parse the commit and return the the tree id it points to.
-    pub fn tree_id(&self) -> Option<git_hash::ObjectId> {
+    pub fn tree_id(&self) -> Result<git_hash::ObjectId, git_object::decode::Error> {
         git_object::CommitRefIter::from_bytes(&self.data).tree_id()
     }
 

git-repository/tests/easy/object.rs

@@ -21,7 +21,7 @@ mod commit {
 
         assert_eq!(commit.tree()?.id, commit.tree_id().expect("id present"));
         assert_eq!(
-            commit.tree_id(),
+            commit.tree_id().ok(),
             Some(hex_to_id("21d3ba9a26b790a4858d67754ae05d04dfce4d0c"))
         );
         Ok(())

git-traverse/src/commit.rs

@@ -214,8 +214,8 @@ pub mod ancestors {
                                 Ok(git_object::commit::ref_iter::Token::Parent { id }) => {
                                     let parent = (self.find)(id.as_ref(), &mut state.parents_buf).ok();
 
-                                    let parent_committer_date =
-                                        parent.and_then(|parent| parent.committer().map(|committer| committer.time));
+                                    let parent_committer_date = parent
+                                        .and_then(|parent| parent.committer().ok().map(|committer| committer.time));
 
                                     if let Some(parent_committer_date) = parent_committer_date {
                                         state

gitoxide-core/src/hours.rs

@@ -89,7 +89,7 @@ where
             objs::CommitRefIter::from_bytes(&commit_data)
                 .signatures()
                 .next()
-                .map(|author| mailmap.resolve(&author))
+                .map(|author| mailmap.resolve(author))
         })
         .try_fold(
             || Vec::new(),