change!: cleanup and unify verify_integrity() method signature (#287)

Previously they used many different ways of handling their parameters
despite all boiling down to calling the same 'index::File::traverse()`
method.

This allows for more reuse of `Options` structs and generally makes
clearer how these optinos are used.

git-pack/src/bundle/mod.rs

@@ -29,28 +29,23 @@ pub mod verify {
     impl Bundle {
         /// Similar to [`crate::index::File::verify_integrity()`] but more convenient to call as the presence of the
         /// pack file is a given.
-        pub fn verify_integrity<C, P>(
+        pub fn verify_integrity<C, P, F>(
             &self,
-            verify_mode: crate::index::verify::Mode,
-            traversal: crate::index::traverse::Algorithm,
-            make_pack_lookup_cache: impl Fn() -> C + Send + Clone,
-            thread_limit: Option<usize>,
             progress: P,
             should_interrupt: &AtomicBool,
+            options: crate::index::verify::integrity::Options<F>,
         ) -> Result<integrity::Outcome<P>, crate::index::traverse::Error<crate::index::verify::integrity::Error>>
         where
             P: Progress,
             C: crate::cache::DecodeEntry,
+            F: Fn() -> C + Send + Clone,
         {
             self.index
                 .verify_integrity(
                     Some(crate::index::verify::PackContext {
                         data: &self.pack,
-                        verify_mode,
-                        traversal_algorithm: traversal,
-                        make_cache_fn: make_pack_lookup_cache,
+                        options,
                     }),
-                    thread_limit,
                     progress,
                     should_interrupt,
                 )

git-pack/src/index/traverse/mod.rs

@@ -17,27 +17,30 @@ pub use error::Error;
 mod types;
 pub use types::{Algorithm, SafetyCheck, Statistics};
 
-mod options {
-    use std::sync::atomic::AtomicBool;
-
-    use crate::index::traverse::{Algorithm, SafetyCheck};
+/// Traversal options for [`index::File::traverse()`].
+#[derive(Debug, Clone)]
+pub struct Options<F> {
+    /// The algorithm to employ.
+    pub traversal: Algorithm,
+    /// If `Some`, only use the given amount of threads. Otherwise, the amount of threads to use will be selected based on
+    /// the amount of available logical cores.
+    pub thread_limit: Option<usize>,
+    /// The kinds of safety checks to perform.
+    pub check: SafetyCheck,
+    /// A function to create a pack cache
+    pub make_pack_lookup_cache: F,
+}
 
-    /// Traversal options for [`traverse()`][crate::index::File::traverse()]
-    #[derive(Debug, Clone)]
-    pub struct Options<'a> {
-        /// The algorithm to employ.
-        pub algorithm: Algorithm,
-        /// If `Some`, only use the given amount of threads. Otherwise, the amount of threads to use will be selected based on
-        /// the amount of available logical cores.
-        pub thread_limit: Option<usize>,
-        /// The kinds of safety checks to perform.
-        pub check: SafetyCheck,
-        /// A flag to indicate whether the algorithm should be interrupted. Will be checked occasionally allow stopping a running
-        /// computation.
-        pub should_interrupt: &'a AtomicBool,
+impl Default for Options<fn() -> crate::cache::Never> {
+    fn default() -> Self {
+        Options {
+            check: Default::default(),
+            traversal: Default::default(),
+            thread_limit: None,
+            make_pack_lookup_cache: || crate::cache::Never,
+        }
     }
 }
-pub use options::Options;
 
 /// The outcome of the [`traverse()`][index::File::traverse()] method.
 pub struct Outcome<P> {
@@ -58,7 +61,7 @@ impl index::File {
     ///
     /// # Algorithms
     ///
-    /// Using the [`Options::algorithm`] field one can chose between two algorithms providing different tradeoffs. Both invoke
+    /// Using the [`Options::traversal`] field one can chose between two algorithms providing different tradeoffs. Both invoke
     /// `new_processor()` to create functions receiving decoded objects, their object kind, index entry and a progress instance to provide
     /// progress information.
     ///
@@ -71,18 +74,18 @@ impl index::File {
     ///
     /// Use [`thread_limit`][Options::thread_limit] to further control parallelism and [`check`][SafetyCheck] to define how much the passed
     /// objects shall be verified beforehand.
-    pub fn traverse<P, C, Processor, E>(
+    pub fn traverse<P, C, Processor, E, F>(
         &self,
         pack: &crate::data::File,
         progress: P,
+        should_interrupt: &AtomicBool,
         new_processor: impl Fn() -> Processor + Send + Clone,
-        new_cache: impl Fn() -> C + Send + Clone,
         Options {
-            algorithm,
+            traversal,
             thread_limit,
             check,
-            should_interrupt,
-        }: Options<'_>,
+            make_pack_lookup_cache,
+        }: Options<F>,
     ) -> Result<Outcome<P>, Error<E>>
     where
         P: Progress,
@@ -94,17 +97,18 @@ impl index::File {
             &index::Entry,
             &mut <<P as Progress>::SubProgress as Progress>::SubProgress,
         ) -> Result<(), E>,
+        F: Fn() -> C + Send + Clone,
     {
-        match algorithm {
+        match traversal {
             Algorithm::Lookup => self.traverse_with_lookup(
                 new_processor,
-                new_cache,
-                progress,
                 pack,
+                progress,
+                should_interrupt,
                 with_lookup::Options {
                     thread_limit,
                     check,
-                    should_interrupt,
+                    make_pack_lookup_cache,
                 },
             ),
             Algorithm::DeltaTreeLookup => {

git-pack/src/index/traverse/with_lookup.rs

@@ -2,32 +2,33 @@ use git_features::{
     parallel::{self, in_parallel_if},
     progress::{self, unit, Progress},
 };
+use std::sync::atomic::{AtomicBool, Ordering};
 
 use super::{Error, Reducer};
 use crate::{data, index, index::util};
 
-mod options {
-    use std::sync::atomic::AtomicBool;
-
-    use crate::index::traverse::SafetyCheck;
+/// Traversal options for [`traverse()`][crate::index::File::traverse_with_lookup()]
+pub struct Options<F> {
+    /// If `Some`, only use the given amount of threads. Otherwise, the amount of threads to use will be selected based on
+    /// the amount of available logical cores.
+    pub thread_limit: Option<usize>,
+    /// The kinds of safety checks to perform.
+    pub check: crate::index::traverse::SafetyCheck,
+    /// A function to create a pack cache
+    pub make_pack_lookup_cache: F,
+}
 
-    /// Traversal options for [`traverse()`][crate::index::File::traverse_with_lookup()]
-    #[derive(Debug, Clone)]
-    pub struct Options<'a> {
-        /// If `Some`, only use the given amount of threads. Otherwise, the amount of threads to use will be selected based on
-        /// the amount of available logical cores.
-        pub thread_limit: Option<usize>,
-        /// The kinds of safety checks to perform.
-        pub check: SafetyCheck,
-        /// A flag to indicate whether the algorithm should be interrupted. Will be checked occasionally allow stopping a running
-        /// computation.
-        pub should_interrupt: &'a AtomicBool,
+impl Default for Options<fn() -> crate::cache::Never> {
+    fn default() -> Self {
+        Options {
+            check: Default::default(),
+            thread_limit: None,
+            make_pack_lookup_cache: || crate::cache::Never,
+        }
     }
 }
-use std::sync::atomic::Ordering;
 
 use git_features::threading::{lock, Mutable, OwnShared};
-pub use options::Options;
 
 use crate::index::traverse::Outcome;
 
@@ -37,17 +38,17 @@ impl index::File {
     /// waste while decoding objects.
     ///
     /// For more details, see the documentation on the [`traverse()`][index::File::traverse()] method.
-    pub fn traverse_with_lookup<P, C, Processor, E>(
+    pub fn traverse_with_lookup<P, C, Processor, E, F>(
         &self,
         new_processor: impl Fn() -> Processor + Send + Clone,
-        new_cache: impl Fn() -> C + Send + Clone,
-        mut progress: P,
         pack: &crate::data::File,
+        mut progress: P,
+        should_interrupt: &AtomicBool,
         Options {
             thread_limit,
             check,
-            should_interrupt,
-        }: Options<'_>,
+            make_pack_lookup_cache,
+        }: Options<F>,
     ) -> Result<Outcome<P>, Error<E>>
     where
         P: Progress,
@@ -59,6 +60,7 @@ impl index::File {
             &index::Entry,
             &mut <<P as Progress>::SubProgress as Progress>::SubProgress,
         ) -> Result<(), E>,
+        F: Fn() -> C + Send + Clone,
     {
         let (verify_result, traversal_result) = parallel::join(
             {
@@ -95,7 +97,7 @@ impl index::File {
                     let reduce_progress = reduce_progress.clone();
                     move |index| {
                         (
-                            new_cache(),
+                            make_pack_lookup_cache(),
                             new_processor(),
                             Vec::with_capacity(2048), // decode buffer
                             lock(&reduce_progress).add_child(format!("thread {}", index)), // per thread progress

git-pack/src/index/verify.rs

@@ -41,6 +41,29 @@ pub mod integrity {
         /// The provided progress instance.
         pub progress: P,
     }
+
+    /// Additional options to define how the integrity should be verified.
+    pub struct Options<F> {
+        /// The thoroughness of the verification
+        pub verify_mode: crate::index::verify::Mode,
+        /// The way to traverse packs
+        pub traversal: crate::index::traverse::Algorithm,
+        /// The amount of theads to use of `Some(N)`, with `None|Some(0)` using all available cores are used.
+        pub thread_limit: Option<usize>,
+        /// A function to create a pack cache
+        pub make_pack_lookup_cache: F,
+    }
+
+    impl Default for Options<fn() -> crate::cache::Never> {
+        fn default() -> Self {
+            Options {
+                verify_mode: Default::default(),
+                traversal: Default::default(),
+                thread_limit: None,
+                make_pack_lookup_cache: || crate::cache::Never,
+            }
+        }
+    }
 }
 
 ///
@@ -69,19 +92,11 @@ impl Default for Mode {
 }
 
 /// Information to allow verifying the integrity of an index with the help of its corresponding pack.
-pub struct PackContext<'a, C, F>
-where
-    C: crate::cache::DecodeEntry,
-    F: Fn() -> C + Send + Clone,
-{
+pub struct PackContext<'a, F> {
     /// The pack data file itself.
     pub data: &'a crate::data::File,
-    /// the way to verify the pack data.
-    pub verify_mode: Mode,
-    /// The traversal algorithm to use
-    pub traversal_algorithm: index::traverse::Algorithm,
-    /// A function to create a pack cache for each tread.
-    pub make_cache_fn: F,
+    /// The options further configuring the pack traversal and verification
+    pub options: integrity::Options<F>,
 }
 
 /// Verify and validate the content of the index file
@@ -137,8 +152,7 @@ impl index::File {
     /// error case.
     pub fn verify_integrity<P, C, F>(
         &self,
-        pack: Option<PackContext<'_, C, F>>,
-        thread_limit: Option<usize>,
+        pack: Option<PackContext<'_, F>>,
         mut progress: P,
         should_interrupt: &AtomicBool,
     ) -> Result<integrity::Outcome<P>, index::traverse::Error<index::verify::integrity::Error>>
@@ -159,25 +173,29 @@ impl index::File {
         match pack {
             Some(PackContext {
                 data: pack,
-                verify_mode: mode,
-                traversal_algorithm: algorithm,
-                make_cache_fn: make_cache,
+                options:
+                    integrity::Options {
+                        verify_mode,
+                        traversal,
+                        thread_limit,
+                        make_pack_lookup_cache,
+                    },
             }) => self
                 .traverse(
                     pack,
                     progress,
+                    should_interrupt,
                     || {
                         let mut encode_buf = Vec::with_capacity(2048);
                         move |kind, data, index_entry, progress| {
-                            Self::verify_entry(mode, &mut encode_buf, kind, data, index_entry, progress)
+                            Self::verify_entry(verify_mode, &mut encode_buf, kind, data, index_entry, progress)
                         }
                     },
-                    make_cache,
                     index::traverse::Options {
-                        algorithm,
+                        traversal,
                         thread_limit,
                         check: index::traverse::SafetyCheck::All,
-                        should_interrupt,
+                        make_pack_lookup_cache,
                     },
                 )
                 .map(|o| integrity::Outcome {
@@ -198,7 +216,7 @@ impl index::File {
 
     #[allow(clippy::too_many_arguments)]
     fn verify_entry<P>(
-        mode: Mode,
+        verify_mode: Mode,
         encode_buf: &mut Vec<u8>,
         object_kind: git_object::Kind,
         buf: &[u8],
@@ -208,7 +226,7 @@ impl index::File {
     where
         P: Progress,
     {
-        if let Mode::HashCrc32Decode | Mode::HashCrc32DecodeEncode = mode {
+        if let Mode::HashCrc32Decode | Mode::HashCrc32DecodeEncode = verify_mode {
             use git_object::Kind::*;
             match object_kind {
                 Tree | Commit | Tag => {
@@ -219,7 +237,7 @@ impl index::File {
                             id: index_entry.oid,
                         }
                     })?;
-                    if let Mode::HashCrc32DecodeEncode = mode {
+                    if let Mode::HashCrc32DecodeEncode = verify_mode {
                         encode_buf.clear();
                         object
                             .write_to(&mut *encode_buf)

git-pack/src/multi_index/verify.rs

@@ -281,12 +281,14 @@ impl File {
                     progress: _,
                 } = bundle
                     .verify_integrity(
-                        verify_mode,
-                        traversal,
-                        make_pack_lookup_cache.clone(),
-                        thread_limit,
                         progress,
                         should_interrupt,
+                        crate::index::verify::integrity::Options {
+                            verify_mode,
+                            traversal,
+                            make_pack_lookup_cache: make_pack_lookup_cache.clone(),
+                            thread_limit,
+                        },
                     )
                     .map_err(|err| {
                         use crate::index::traverse::Error::*;

git-pack/tests/pack/data/output/count_and_entries.rs

@@ -393,12 +393,14 @@ fn write_and_verify(
     }
 
     bundle.verify_integrity(
-        pack::index::verify::Mode::HashCrc32DecodeEncode,
-        pack::index::traverse::Algorithm::Lookup,
-        || pack::cache::Never,
-        None,
         progress::Discard,
         &should_interrupt,
+        git_pack::index::verify::integrity::Options {
+            verify_mode: pack::index::verify::Mode::HashCrc32DecodeEncode,
+            traversal: pack::index::traverse::Algorithm::Lookup,
+            make_pack_lookup_cache: || pack::cache::Never,
+            thread_limit: None,
+        },
     )?;
 
     Ok(())