change!: multi-index integrity check; use integrity::Outcome for various integrity checks (#279)

Byron · Byron · commit 6829e5e5d6ae · 2021-12-29T17:23:08.000+08:00
diff --git a/git-pack/src/bundle/mod.rs b/git-pack/src/bundle/mod.rs
@@ -5,11 +5,25 @@ mod find;
 ///
 pub mod write;
 
-mod verify {
+///
+pub mod verify {
     use std::sync::atomic::AtomicBool;
 
     use git_features::progress::Progress;
 
+    ///
+    pub mod integrity {
+        /// Returned by [`Bundle::verify_integrity()`][crate::Bundle::verify_integrity()].
+        pub struct Outcome<P> {
+            /// The computed checksum of the index which matched the stored one.
+            pub actual_index_checksum: git_hash::ObjectId,
+            /// The packs traversal outcome
+            pub pack_traverse_outcome: crate::index::traverse::Outcome,
+            /// The provided progress instance.
+            pub progress: Option<P>,
+        }
+    }
+
     use crate::Bundle;
 
     impl Bundle {
@@ -23,25 +37,28 @@ mod verify {
             thread_limit: Option<usize>,
             progress: Option<P>,
             should_interrupt: &AtomicBool,
-        ) -> Result<
-            (git_hash::ObjectId, Option<crate::index::traverse::Outcome>, Option<P>),
-            crate::index::traverse::Error<crate::index::verify::integrity::Error>,
-        >
+        ) -> Result<integrity::Outcome<P>, crate::index::traverse::Error<crate::index::verify::integrity::Error>>
         where
             P: Progress,
             C: crate::cache::DecodeEntry,
         {
-            self.index.verify_integrity(
-                Some(crate::index::verify::PackContext {
-                    data: &self.pack,
-                    verify_mode,
-                    traversal_algorithm: traversal,
-                    make_cache_fn: make_pack_lookup_cache,
-                }),
-                thread_limit,
-                progress,
-                should_interrupt,
-            )
+            self.index
+                .verify_integrity(
+                    Some(crate::index::verify::PackContext {
+                        data: &self.pack,
+                        verify_mode,
+                        traversal_algorithm: traversal,
+                        make_cache_fn: make_pack_lookup_cache,
+                    }),
+                    thread_limit,
+                    progress,
+                    should_interrupt,
+                )
+                .map(|o| integrity::Outcome {
+                    actual_index_checksum: o.actual_index_checksum,
+                    pack_traverse_outcome: o.pack_traverse_outcome.expect("pack is set"),
+                    progress: o.progress,
+                })
         }
     }
 }
diff --git a/git-pack/src/index/verify.rs b/git-pack/src/index/verify.rs
@@ -27,6 +27,16 @@ pub mod integrity {
             actual: BString,
         },
     }
+
+    /// Returned by [`index::File::verify_integrity()`][crate::index::File::verify_integrity()].
+    pub struct Outcome<P> {
+        /// The computed checksum of the index which matched the stored one.
+        pub actual_index_checksum: git_hash::ObjectId,
+        /// The packs traversal outcome, if one was provided
+        pub pack_traverse_outcome: Option<crate::index::traverse::Outcome>,
+        /// The provided progress instance.
+        pub progress: Option<P>,
+    }
 }
 
 ///
@@ -121,10 +131,7 @@ impl index::File {
         thread_limit: Option<usize>,
         progress: Option<P>,
         should_interrupt: &AtomicBool,
-    ) -> Result<
-        (git_hash::ObjectId, Option<index::traverse::Outcome>, Option<P>),
-        index::traverse::Error<crate::index::verify::integrity::Error>,
-    >
+    ) -> Result<integrity::Outcome<P>, index::traverse::Error<crate::index::verify::integrity::Error>>
     where
         P: Progress,
         C: crate::cache::DecodeEntry,
@@ -155,11 +162,19 @@ impl index::File {
                         should_interrupt,
                     },
                 )
-                .map(|(id, outcome, root)| (id, Some(outcome), root)),
+                .map(|(id, outcome, root)| integrity::Outcome {
+                    actual_index_checksum: id,
+                    pack_traverse_outcome: Some(outcome),
+                    progress: root,
+                }),
             None => self
                 .verify_checksum(root.add_child("Sha1 of index"), should_interrupt)
                 .map_err(Into::into)
-                .map(|id| (id, None, root.into_inner())),
+                .map(|id| integrity::Outcome {
+                    actual_index_checksum: id,
+                    pack_traverse_outcome: None,
+                    progress: root.into_inner(),
+                }),
         }
     }
 
diff --git a/git-pack/src/multi_index/verify.rs b/git-pack/src/multi_index/verify.rs
@@ -2,9 +2,34 @@ use crate::multi_index::File;
 use git_features::progress::Progress;
 use std::sync::atomic::AtomicBool;
 
+///
+pub mod integrity {
+    /// Returned by [`multi_index::File::verify_integrity()`][crate::multi_index::File::verify_integrity()].
+    #[derive(thiserror::Error, Debug)]
+    #[allow(missing_docs)]
+    pub enum Error {
+        #[error(transparent)]
+        MultiIndexChecksum(#[from] crate::multi_index::verify::checksum::Error),
+        #[error(transparent)]
+        IndexIntegrity(#[from] crate::index::verify::integrity::Error),
+        #[error(transparent)]
+        BundleInit(#[from] crate::bundle::init::Error),
+    }
+
+    /// Returned by [`multi_index::File::verify_integrity()`][crate::multi_index::File::verify_integrity()].
+    pub struct Outcome<P> {
+        /// The computed checksum of the multi-index which matched the stored one.
+        pub actual_index_checksum: git_hash::ObjectId,
+        /// The for each entry in [`index_names()`][super::File::index_names()] provide the corresponding pack traversal outcome.
+        pub pack_traverse_outcomes: Vec<crate::index::traverse::Outcome>,
+        /// The provided progress instance.
+        pub progress: Option<P>,
+    }
+}
+
 ///
 pub mod checksum {
-    /// Returned by [`index::File::verify_checksum()`][crate::index::File::verify_checksum()].
+    /// Returned by [`multi_index::File::verify_checksum()`][crate::multi_index::File::verify_checksum()].
     pub type Error = crate::verify::checksum::Error;
 }
 
@@ -36,16 +61,88 @@ impl File {
         traversal: crate::index::traverse::Algorithm,
         make_pack_lookup_cache: impl Fn() -> C + Send + Clone,
         thread_limit: Option<usize>,
-        progress: Option<P>,
+        mut progress: Option<P>,
         should_interrupt: &AtomicBool,
-    ) -> Result<
-        (git_hash::ObjectId, Option<crate::index::traverse::Outcome>, Option<P>),
-        crate::index::traverse::Error<crate::index::verify::integrity::Error>,
-    >
+    ) -> Result<integrity::Outcome<P>, crate::index::traverse::Error<integrity::Error>>
     where
         P: Progress,
         C: crate::cache::DecodeEntry,
     {
-        todo!()
+        let parent = self.path.parent().expect("must be in a directory");
+
+        let mut progress = git_features::progress::DoOrDiscard::from(progress);
+        let actual_index_checksum = self
+            .verify_checksum(
+                progress.add_child(format!("checksum of '{}'", self.path.display())),
+                should_interrupt,
+            )
+            .map_err(integrity::Error::from)
+            .map_err(crate::index::traverse::Error::Processor)?;
+        let mut progress = progress.into_inner();
+
+        let mut pack_traverse_outcomes = Vec::new();
+        for index_file_name in &self.index_names {
+            let bundle = crate::Bundle::at(parent.join(index_file_name), self.object_hash)
+                .map_err(integrity::Error::from)
+                .map_err(crate::index::traverse::Error::Processor)?;
+            if let Some(progress) = progress.as_mut() {
+                progress.set_name(index_file_name.display().to_string());
+            }
+            let crate::bundle::verify::integrity::Outcome {
+                actual_index_checksum: _,
+                pack_traverse_outcome,
+                progress: used_progress,
+            } = bundle
+                .verify_integrity(
+                    verify_mode,
+                    traversal,
+                    make_pack_lookup_cache.clone(),
+                    thread_limit,
+                    progress,
+                    should_interrupt,
+                )
+                .map_err(|err| {
+                    use crate::index::traverse::Error::*;
+                    match err {
+                        Processor(err) => Processor(integrity::Error::IndexIntegrity(err)),
+                        VerifyChecksum(err) => VerifyChecksum(err),
+                        Tree(err) => Tree(err),
+                        TreeTraversal(err) => TreeTraversal(err),
+                        PackDecode { id, offset, source } => PackDecode { id, offset, source },
+                        PackMismatch { expected, actual } => PackMismatch { expected, actual },
+                        PackObjectMismatch {
+                            expected,
+                            actual,
+                            offset,
+                            kind,
+                        } => PackObjectMismatch {
+                            expected,
+                            actual,
+                            offset,
+                            kind,
+                        },
+                        Crc32Mismatch {
+                            expected,
+                            actual,
+                            offset,
+                            kind,
+                        } => Crc32Mismatch {
+                            expected,
+                            actual,
+                            offset,
+                            kind,
+                        },
+                        Interrupted => Interrupted,
+                    }
+                })?;
+            pack_traverse_outcomes.push(pack_traverse_outcome);
+            progress = used_progress;
+        }
+
+        Ok(integrity::Outcome {
+            actual_index_checksum,
+            pack_traverse_outcomes,
+            progress,
+        })
     }
 }
diff --git a/git-pack/tests/pack/index.rs b/git-pack/tests/pack/index.rs
@@ -311,7 +311,7 @@ mod file {
                             progress::Discard.into(),
                             &AtomicBool::new(false)
                         )
-                        .map(|(a, b, _)| (a, b))?,
+                        .map(|o| (o.actual_index_checksum, o.pack_traverse_outcome))?,
                         (idx.index_checksum(), Some(stats.to_owned())),
                         "{:?} -> {:?}",
                         algo,
@@ -411,7 +411,7 @@ mod file {
                     progress::Discard.into(),
                     &AtomicBool::new(false)
                 )
-                .map(|(a, b, _)| (a, b))?,
+                .map(|o| (o.actual_index_checksum, o.pack_traverse_outcome))?,
                 (idx.index_checksum(), None)
             );
             assert_eq!(idx.index_checksum(), hex_to_id(index_checksum));
diff --git a/git-pack/tests/pack/multi_index.rs b/git-pack/tests/pack/multi_index.rs
@@ -53,6 +53,7 @@ fn access() {
 
 mod verify {
     use crate::pack::multi_index::multi_index;
+    use common_macros::b_tree_map;
     use git_features::progress;
     use std::sync::atomic::AtomicBool;
 
@@ -65,4 +66,47 @@ mod verify {
             file.checksum()
         );
     }
+
+    #[test]
+    fn integrity() {
+        let (file, _) = multi_index();
+        let outcome = file
+            .verify_integrity(
+                git_pack::index::verify::Mode::HashCrc32DecodeEncode,
+                git_pack::index::traverse::Algorithm::DeltaTreeLookup,
+                || git_pack::cache::Never,
+                None,
+                Some(progress::Discard),
+                &AtomicBool::new(false),
+            )
+            .unwrap();
+        assert_eq!(outcome.actual_index_checksum, file.checksum());
+        assert_eq!(
+            outcome.pack_traverse_outcomes,
+            vec![git_pack::index::traverse::Outcome {
+                average: git_pack::data::decode_entry::Outcome {
+                    kind: git_object::Kind::Tree,
+                    num_deltas: 1,
+                    decompressed_size: 47,
+                    compressed_size: 46,
+                    object_size: 152
+                },
+                objects_per_chain_length: b_tree_map! {
+                    0 => 326,
+                    1 => 106,
+                    2 => 326,
+                    3 => 108,
+                    4 => 2,
+                },
+                total_compressed_entries_size: 40628,
+                total_decompressed_entries_size: 40919,
+                total_object_size: 131993,
+                pack_size: 42856,
+                num_commits: 16,
+                num_trees: 40,
+                num_tags: 1,
+                num_blobs: 811
+            }]
+        );
+    }
 }
diff --git a/gitoxide-core/src/pack/verify.rs b/gitoxide-core/src/pack/verify.rs
@@ -176,7 +176,7 @@ where
                 progress,
                 &should_interrupt,
             )
-            .map(|(a, b, _)| (a, b))
+            .map(|o| (o.actual_index_checksum, o.pack_traverse_outcome))
             .with_context(|| "Verification failure")?
         }
         ext => return Err(anyhow!("Unknown extension {:?}, expecting 'idx' or 'pack'", ext)),

Original file line number	Diff line number	Diff line change
`@@ -176,7 +176,7 @@ where`
`176`	`176`	`progress,`
`177`	`177`	`&should_interrupt,`
`178`	`178`	`)`
`179`		`- .map(\|(a, b, _)\| (a, b))`
	`179`	`+ .map(\|o\| (o.actual_index_checksum, o.pack_traverse_outcome))`
`180`	`180`	`.with_context(\|\| "Verification failure")?`
`181`	`181`	`}`
`182`	`182`	`ext => return Err(anyhow!("Unknown extension {:?}, expecting 'idx' or 'pack'", ext)),`