History

[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.

Conor Davis committed 6 years ago

Commit fa22cab1 parent 25711133 files · +64 −44Browse files

Showing the changes to git-commitgraph (3 of 3 files). Show all

Unified · Split

3 files
  1. git-commitgraph/src/file/verify.rs +17 −37
  2. git-commitgraph/src/graph/verify.rs +35 −7
  3. git-commitgraph/src/lib.rs +12 −0
git-commitgraph/src/file/verify.rs+17−37
@@ -10,7 +10,7 @@ use std::convert::TryFrom; · 9 unmodified lines
1010 use std::path::Path;
1111
1212 #[derive(thiserror::Error, Debug)]
13−pub enum Error {
13+pub enum Error<E: std::error::Error + 'static> {
1414 #[error(transparent)]
1515 Commit(#[from] file::commit::Error),
1616 #[error("commit at file position {pos} has invalid ID {id}")]
@@ -26,24 +26,13 @@ pub enum Error { · 9 unmodified lines
2626 #[error("commit {id} has invalid generation {generation}")]
2727 Generation { generation: u32, id: owned::Id },
2828 #[error("checksum mismatch: expected {expected}, got {actual}")]
29− Mismatch { expected: owned::Id, actual: owned::Id },
29+ Mismatch { actual: owned::Id, expected: owned::Id },
30+ #[error("{0}")]
31+ Processor(#[source] E),
3032 #[error("commit {id} has invalid root tree ID {root_tree_id}")]
3133 RootTreeId { id: owned::Id, root_tree_id: owned::Id },
3234 }
3335
34−// This is a separate type to let `traverse`'s caller use the same error type for its result and its
35−// processor error type while also letting that error type contain file::verify::Error values.
36−// Is there a better way? Should the caller's error type just use boxes to avoid recursive type
37−// errors?
38−#[derive(thiserror::Error, Debug)]
39−pub enum EitherError<E1: std::error::Error + 'static, E2: std::error::Error + 'static> {
40− #[error(transparent)]
41− Internal(#[from] E1),
42− // Why can't I use #[from] here? Boo!
43− #[error("{0}")]
44− Processor(#[source] E2),
45−}
46−
4736 #[derive(Clone, Debug, Eq, PartialEq)]
4837 #[cfg_attr(feature = "serde1", derive(serde::Deserialize, serde::Serialize))]
4938 pub struct Outcome {
@@ -59,13 +48,14 @@ impl File { · 9 unmodified lines
5948 borrowed::Id::try_from(&self.data[self.data.len() - SHA1_SIZE..]).expect("file to be large enough for a hash")
6049 }
6150
62− pub fn traverse<'a, E, Processor>(&'a self, mut processor: Processor) -> Result<Outcome, EitherError<Error, E>>
51+ pub fn traverse<'a, E, Processor>(&'a self, mut processor: Processor) -> Result<Outcome, Error<E>>
6352 where
6453 E: std::error::Error + 'static,
6554 Processor: FnMut(&file::Commit<'a>) -> Result<(), E>,
6655 {
67− self.verify_checksum()?;
68− verify_split_chain_filename_hash(&self.path, self.checksum())?;
56+ self.verify_checksum()
57+ .map_err(|(actual, expected)| Error::Mismatch { actual, expected })?;
58+ verify_split_chain_filename_hash(&self.path, self.checksum()).map_err(Error::Filename)?;
6959
7060 // This probably belongs in borrowed::Id itself?
7161 let null_id = borrowed::Id::from(&[0u8; SHA1_SIZE]);
@@ -86,32 +76,28 @@ impl File { · 14 unmodified lines
8676 return Err(Error::CommitId {
8777 pos: commit.position(),
8878 id: commit.id().into(),
89− }
90− .into());
79+ });
9180 }
9281 return Err(Error::CommitsOutOfOrder {
9382 pos: commit.position(),
9483 id: commit.id().into(),
9584 predecessor_id: prev_id.into(),
96− }
97− .into());
85+ });
9886 }
9987 if commit.root_tree_id() == null_id {
10088 return Err(Error::RootTreeId {
10189 id: commit.id().into(),
10290 root_tree_id: commit.root_tree_id().into(),
103− }
104− .into());
91+ });
10592 }
10693 if commit.generation() > GENERATION_NUMBER_MAX {
10794 return Err(Error::Generation {
10895 generation: commit.generation(),
10996 id: commit.id().into(),
110− }
111− .into());
97+ });
11298 }
11399
114− processor(&commit).map_err(EitherError::Processor)?;
100+ processor(&commit).map_err(Error::Processor)?;
115101
116102 stats.max_generation = max(stats.max_generation, commit.generation());
117103 stats.min_generation = min(stats.min_generation, commit.generation());
@@ -130,7 +116,7 @@ impl File { · 12 unmodified lines
130116 Ok(stats)
131117 }
132118
133− pub fn verify_checksum(&self) -> Result<owned::Id, Error> {
119+ pub fn verify_checksum(&self) -> Result<owned::Id, (owned::Id, owned::Id)> {
134120 // TODO: Use/copy git_odb::hash::bytes_of_file.
135121 let data_len_without_trailer = self.data.len() - SHA1_SIZE;
136122 let mut hasher = git_features::hash::Sha1::default();
@@ -141,17 +127,14 @@ impl File { · 4 unmodified lines
141127 if actual.to_borrowed() == expected {
142128 Ok(actual)
143129 } else {
144− Err(Error::Mismatch {
145− actual,
146− expected: expected.into(),
147− })
130+ Err((actual, expected.into()))
148131 }
149132 }
150133 }
151134
152135 /// If the given path's filename matches "graph-{hash}.graph", check that `hash` matches the
153136 /// expected hash.
154−fn verify_split_chain_filename_hash(path: impl AsRef<Path>, expected: borrowed::Id<'_>) -> Result<(), Error> {
137+fn verify_split_chain_filename_hash(path: impl AsRef<Path>, expected: borrowed::Id<'_>) -> Result<(), String> {
155138 let path = path.as_ref();
156139 path.file_name()
157140 .and_then(|filename| filename.to_str())
@@ -159,9 +142,6 @@ fn verify_split_chain_filename_hash(path: impl AsRef<Path>, expected: borrowed:: · 1 unmodified line
159142 .and_then(|stem| stem.strip_prefix("graph-"))
160143 .map_or(Ok(()), |hex| match owned::Id::from_40_bytes_in_hex(hex.as_bytes()) {
161144 Ok(actual) if actual.to_borrowed() == expected => Ok(()),
162− _ => Err(Error::Filename(format!(
163− "graph-{}.graph",
164− expected.to_sha1_hex().as_bstr()
165− ))),
145+ _ => Err(format!("graph-{}.graph", expected.to_sha1_hex().as_bstr())),
166146 })
167147 }
git-commitgraph/src/graph/verify.rs+35−7
@@ -1,6 +1,6 @@
11 use crate::{
22 file::{self, commit},
3− graph, Graph, GENERATION_NUMBER_MAX,
3+ graph, Graph, ImpossibleVariantError, GENERATION_NUMBER_MAX,
44 };
55 use git_object::owned;
66 use std::cmp::{max, min};
@@ -22,7 +22,15 @@ pub enum Error<E: std::error::Error + 'static> { · 15 unmodified lines
2222 #[error(transparent)]
2323 Commit(#[from] commit::Error),
2424 #[error("{}: {err}", .path.display())]
25− File { err: file::verify::Error, path: PathBuf },
25+ File {
26+ // Use zero-size error type. We will never return
27+ // `graph::verify::Error::File(file::verify::Error::Processor(...))`, because we are the
28+ // file's processor, and we convert`file::verify::Error::Processor<graph::verify::Error>`
29+ // variants into direct `graph::verify::Error` values.
30+ // TODO: Use never type when it becomes available.
31+ err: file::verify::Error<ImpossibleVariantError>,
32+ path: PathBuf,
33+ },
2634 #[error("Commit {id}'s generation should be {expected} but is {actual}")]
2735 Generation { actual: u32, expected: u32, id: owned::Id },
2836 #[error(
@@ -132,12 +140,32 @@ impl Graph { · 103 unmodified lines
132140
133141 Ok(())
134142 })
135− .map_err(|e| match e {
136− file::verify::EitherError::Internal(err) => Error::File {
137− err,
138− path: file.path().to_owned(),
143+ .map_err(|err| Error::File {
144+ err: match err {
145+ file::verify::Error::Processor(e) => return e,
146+ file::verify::Error::RootTreeId { id, root_tree_id } => {
147+ file::verify::Error::RootTreeId { id, root_tree_id }
148+ }
149+ file::verify::Error::Mismatch { actual, expected } => {
150+ file::verify::Error::Mismatch { actual, expected }
151+ }
152+ file::verify::Error::Generation { generation, id } => {
153+ file::verify::Error::Generation { generation, id }
154+ }
155+ file::verify::Error::Filename(expected) => file::verify::Error::Filename(expected),
156+ file::verify::Error::Commit(err) => file::verify::Error::Commit(err),
157+ file::verify::Error::CommitId { id, pos } => file::verify::Error::CommitId { id, pos },
158+ file::verify::Error::CommitsOutOfOrder {
159+ id,
160+ pos,
161+ predecessor_id,
162+ } => file::verify::Error::CommitsOutOfOrder {
163+ id,
164+ pos,
165+ predecessor_id,
166+ },
139167 },
140− file::verify::EitherError::Processor(e) => e,
168+ path: file.path().to_owned(),
141169 })?;
142170
143171 max_generation = max(max_generation, file_stats.max_generation);
git-commitgraph/src/lib.rs+12−0
@@ -11,3 +11,15 @@ pub const GENERATION_NUMBER_MAX: u32 = 0x3fff_ffff; · 10 unmodified lines
1111
1212 /// The maximum number of commits that can be stored in a commit graph.
1313 pub const MAX_COMMITS: u32 = (1 << 30) + (1 << 29) + (1 << 28) - 1;
14+
15+// TODO: pub type ImpossibleVariantError = !;
16+#[derive(Debug)]
17+pub struct ImpossibleVariantError;
18+
19+impl std::error::Error for ImpossibleVariantError {}
20+
21+impl std::fmt::Display for ImpossibleVariantError {
22+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
23+ f.write_str("this enum was constructed with an invalid variant")
24+ }
25+}