[commitgraph] Clean up {file,graph}::verify::Error types.

Get rid of `file::verify::EitherError` in favor of having
`graph::verify::Error` manually copy file errors into the graph error
object.

So:
1. `graph::verify::Error`'s `File` variant uses a dummy type parameter
   for its nested `file::verify::Error` value. This dummy type parameter
   is essentially a never type, as
   `graph::verify::Error::File(file::verify::Error::Processor(...))` is
   not valid.
2. `Graph::verify_integrity` calls `File::traverse` with its normal
   error type (`graph::verify::Error`) also serving as the processor's
   error type.
3. `Graph::traverse` propagates processor errors to its caller via
   `Error::Processor(err)`.
4. `Graph::verify_integrity` manually transcribes
   `file::verify::Error<T>` variants into
   `file::verify::Error<NeverType>` variants before embedding the file
    error into `graph::verify::Error::File` variants.

Author: avoidscorn

git-commitgraph/src/file/verify.rs

@@ -10,7 +10,7 @@ use std::convert::TryFrom;
 use std::path::Path;
 
 #[derive(thiserror::Error, Debug)]
-pub enum Error {
+pub enum Error<E: std::error::Error + 'static> {
     #[error(transparent)]
     Commit(#[from] file::commit::Error),
     #[error("commit at file position {pos} has invalid ID {id}")]
@@ -26,24 +26,13 @@ pub enum Error {
     #[error("commit {id} has invalid generation {generation}")]
     Generation { generation: u32, id: owned::Id },
     #[error("checksum mismatch: expected {expected}, got {actual}")]
-    Mismatch { expected: owned::Id, actual: owned::Id },
+    Mismatch { actual: owned::Id, expected: owned::Id },
+    #[error("{0}")]
+    Processor(#[source] E),
     #[error("commit {id} has invalid root tree ID {root_tree_id}")]
     RootTreeId { id: owned::Id, root_tree_id: owned::Id },
 }
 
-// This is a separate type to let `traverse`'s caller use the same error type for its result and its
-// processor error type while also letting that error type contain file::verify::Error values.
-// Is there a better way? Should the caller's error type just use boxes to avoid recursive type
-// errors?
-#[derive(thiserror::Error, Debug)]
-pub enum EitherError<E1: std::error::Error + 'static, E2: std::error::Error + 'static> {
-    #[error(transparent)]
-    Internal(#[from] E1),
-    // Why can't I use #[from] here? Boo!
-    #[error("{0}")]
-    Processor(#[source] E2),
-}
-
 #[derive(Clone, Debug, Eq, PartialEq)]
 #[cfg_attr(feature = "serde1", derive(serde::Deserialize, serde::Serialize))]
 pub struct Outcome {
@@ -59,13 +48,14 @@ impl File {
         borrowed::Id::try_from(&self.data[self.data.len() - SHA1_SIZE..]).expect("file to be large enough for a hash")
     }
 
-    pub fn traverse<'a, E, Processor>(&'a self, mut processor: Processor) -> Result<Outcome, EitherError<Error, E>>
+    pub fn traverse<'a, E, Processor>(&'a self, mut processor: Processor) -> Result<Outcome, Error<E>>
     where
         E: std::error::Error + 'static,
         Processor: FnMut(&file::Commit<'a>) -> Result<(), E>,
     {
-        self.verify_checksum()?;
-        verify_split_chain_filename_hash(&self.path, self.checksum())?;
+        self.verify_checksum()
+            .map_err(|(actual, expected)| Error::Mismatch { actual, expected })?;
+        verify_split_chain_filename_hash(&self.path, self.checksum()).map_err(Error::Filename)?;
 
         // This probably belongs in borrowed::Id itself?
         let null_id = borrowed::Id::from(&[0u8; SHA1_SIZE]);
@@ -86,32 +76,28 @@ impl File {
                     return Err(Error::CommitId {
                         pos: commit.position(),
                         id: commit.id().into(),
-                    }
-                    .into());
+                    });
                 }
                 return Err(Error::CommitsOutOfOrder {
                     pos: commit.position(),
                     id: commit.id().into(),
                     predecessor_id: prev_id.into(),
-                }
-                .into());
+                });
             }
             if commit.root_tree_id() == null_id {
                 return Err(Error::RootTreeId {
                     id: commit.id().into(),
                     root_tree_id: commit.root_tree_id().into(),
-                }
-                .into());
+                });
             }
             if commit.generation() > GENERATION_NUMBER_MAX {
                 return Err(Error::Generation {
                     generation: commit.generation(),
                     id: commit.id().into(),
-                }
-                .into());
+                });
             }
 
-            processor(&commit).map_err(EitherError::Processor)?;
+            processor(&commit).map_err(Error::Processor)?;
 
             stats.max_generation = max(stats.max_generation, commit.generation());
             stats.min_generation = min(stats.min_generation, commit.generation());
@@ -130,7 +116,7 @@ impl File {
         Ok(stats)
     }
 
-    pub fn verify_checksum(&self) -> Result<owned::Id, Error> {
+    pub fn verify_checksum(&self) -> Result<owned::Id, (owned::Id, owned::Id)> {
         // TODO: Use/copy git_odb::hash::bytes_of_file.
         let data_len_without_trailer = self.data.len() - SHA1_SIZE;
         let mut hasher = git_features::hash::Sha1::default();
@@ -141,27 +127,21 @@ impl File {
         if actual.to_borrowed() == expected {
             Ok(actual)
         } else {
-            Err(Error::Mismatch {
-                actual,
-                expected: expected.into(),
-            })
+            Err((actual, expected.into()))
         }
     }
 }
 
 /// If the given path's filename matches "graph-{hash}.graph", check that `hash` matches the
 /// expected hash.
-fn verify_split_chain_filename_hash(path: impl AsRef<Path>, expected: borrowed::Id<'_>) -> Result<(), Error> {
+fn verify_split_chain_filename_hash(path: impl AsRef<Path>, expected: borrowed::Id<'_>) -> Result<(), String> {
     let path = path.as_ref();
     path.file_name()
         .and_then(|filename| filename.to_str())
         .and_then(|filename| filename.strip_suffix(".graph"))
         .and_then(|stem| stem.strip_prefix("graph-"))
         .map_or(Ok(()), |hex| match owned::Id::from_40_bytes_in_hex(hex.as_bytes()) {
             Ok(actual) if actual.to_borrowed() == expected => Ok(()),
-            _ => Err(Error::Filename(format!(
-                "graph-{}.graph",
-                expected.to_sha1_hex().as_bstr()
-            ))),
+            _ => Err(format!("graph-{}.graph", expected.to_sha1_hex().as_bstr())),
         })
 }

git-commitgraph/src/graph/verify.rs

@@ -1,6 +1,6 @@
 use crate::{
     file::{self, commit},
-    graph, Graph, GENERATION_NUMBER_MAX,
+    graph, Graph, ImpossibleVariantError, GENERATION_NUMBER_MAX,
 };
 use git_object::owned;
 use std::cmp::{max, min};
@@ -22,7 +22,15 @@ pub enum Error<E: std::error::Error + 'static> {
     #[error(transparent)]
     Commit(#[from] commit::Error),
     #[error("{}: {err}", .path.display())]
-    File { err: file::verify::Error, path: PathBuf },
+    File {
+        // Use zero-size error type. We will never return
+        // `graph::verify::Error::File(file::verify::Error::Processor(...))`, because we are the
+        // file's processor, and we convert`file::verify::Error::Processor<graph::verify::Error>`
+        // variants into direct `graph::verify::Error` values.
+        // TODO: Use never type when it becomes available.
+        err: file::verify::Error<ImpossibleVariantError>,
+        path: PathBuf,
+    },
     #[error("Commit {id}'s generation should be {expected} but is {actual}")]
     Generation { actual: u32, expected: u32, id: owned::Id },
     #[error(
@@ -132,12 +140,32 @@ impl Graph {
 
                     Ok(())
                 })
-                .map_err(|e| match e {
-                    file::verify::EitherError::Internal(err) => Error::File {
-                        err,
-                        path: file.path().to_owned(),
+                .map_err(|err| Error::File {
+                    err: match err {
+                        file::verify::Error::Processor(e) => return e,
+                        file::verify::Error::RootTreeId { id, root_tree_id } => {
+                            file::verify::Error::RootTreeId { id, root_tree_id }
+                        }
+                        file::verify::Error::Mismatch { actual, expected } => {
+                            file::verify::Error::Mismatch { actual, expected }
+                        }
+                        file::verify::Error::Generation { generation, id } => {
+                            file::verify::Error::Generation { generation, id }
+                        }
+                        file::verify::Error::Filename(expected) => file::verify::Error::Filename(expected),
+                        file::verify::Error::Commit(err) => file::verify::Error::Commit(err),
+                        file::verify::Error::CommitId { id, pos } => file::verify::Error::CommitId { id, pos },
+                        file::verify::Error::CommitsOutOfOrder {
+                            id,
+                            pos,
+                            predecessor_id,
+                        } => file::verify::Error::CommitsOutOfOrder {
+                            id,
+                            pos,
+                            predecessor_id,
+                        },
                     },
-                    file::verify::EitherError::Processor(e) => e,
+                    path: file.path().to_owned(),
                 })?;
 
             max_generation = max(max_generation, file_stats.max_generation);

git-commitgraph/src/lib.rs

@@ -11,3 +11,15 @@ pub const GENERATION_NUMBER_MAX: u32 = 0x3fff_ffff;
 
 /// The maximum number of commits that can be stored in a commit graph.
 pub const MAX_COMMITS: u32 = (1 << 30) + (1 << 29) + (1 << 28) - 1;
+
+// TODO: pub type ImpossibleVariantError = !;
+#[derive(Debug)]
+pub struct ImpossibleVariantError;
+
+impl std::error::Error for ImpossibleVariantError {}
+
+impl std::fmt::Display for ImpossibleVariantError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str("this enum was constructed with an invalid variant")
+    }
+}