change!: remove unnecessary Arc around should_interrupt flag (#279)

Author: Byron

git-pack/src/bundle/mod.rs

@@ -6,7 +6,7 @@ mod find;
 pub mod write;
 
 mod verify {
-    use std::sync::{atomic::AtomicBool, Arc};
+    use std::sync::atomic::AtomicBool;
 
     use git_features::progress::Progress;
 
@@ -22,7 +22,7 @@ mod verify {
             make_pack_lookup_cache: impl Fn() -> C + Send + Clone,
             thread_limit: Option<usize>,
             progress: Option<P>,
-            should_interrupt: Arc<AtomicBool>,
+            should_interrupt: &AtomicBool,
         ) -> Result<
             (git_hash::ObjectId, Option<crate::index::traverse::Outcome>, Option<P>),
             crate::index::traverse::Error<crate::index::verify::integrity::Error>,

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

@@ -1,9 +1,6 @@
 use std::{
     collections::VecDeque,
-    sync::{
-        atomic::{AtomicBool, Ordering},
-        Arc,
-    },
+    sync::atomic::{AtomicBool, Ordering},
 };
 
 use git_features::{parallel, progress::Progress};
@@ -27,7 +24,7 @@ impl index::File {
         new_processor: impl Fn() -> Processor + Send + Clone,
         mut progress: P,
         pack: &crate::data::File,
-        should_interrupt: Arc<AtomicBool>,
+        should_interrupt: &AtomicBool,
     ) -> Result<(git_hash::ObjectId, index::traverse::Outcome, P), Error<E>>
     where
         P: Progress,
@@ -43,15 +40,8 @@ impl index::File {
             {
                 let pack_progress = progress.add_child("SHA1 of pack");
                 let index_progress = progress.add_child("SHA1 of index");
-                let should_interrupt = Arc::clone(&should_interrupt);
                 move || {
-                    let res = self.possibly_verify(
-                        pack,
-                        check,
-                        pack_progress,
-                        index_progress,
-                        Arc::clone(&should_interrupt),
-                    );
+                    let res = self.possibly_verify(pack, check, pack_progress, index_progress, &should_interrupt);
                     if res.is_err() {
                         should_interrupt.store(true, Ordering::SeqCst);
                     }

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

@@ -1,4 +1,4 @@
-use std::sync::{atomic::AtomicBool, Arc};
+use std::sync::atomic::AtomicBool;
 
 use git_features::{
     parallel,
@@ -21,13 +21,13 @@ mod types;
 pub use types::{Algorithm, Outcome, SafetyCheck};
 
 mod options {
-    use std::sync::{atomic::AtomicBool, Arc};
+    use std::sync::atomic::AtomicBool;
 
     use crate::index::traverse::{Algorithm, SafetyCheck};
 
     /// Traversal options for [`traverse()`][crate::index::File::traverse()]
     #[derive(Debug, Clone)]
-    pub struct Options {
+    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
@@ -37,18 +37,7 @@ mod options {
         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: Arc<AtomicBool>,
-    }
-
-    impl Default for Options {
-        fn default() -> Self {
-            Self {
-                algorithm: Algorithm::Lookup,
-                thread_limit: Default::default(),
-                check: Default::default(),
-                should_interrupt: Default::default(),
-            }
-        }
+        pub should_interrupt: &'a AtomicBool,
     }
 }
 pub use options::Options;
@@ -86,7 +75,7 @@ impl index::File {
             thread_limit,
             check,
             should_interrupt,
-        }: Options,
+        }: Options<'_>,
     ) -> Result<(git_hash::ObjectId, Outcome, Option<P>), Error<E>>
     where
         P: Progress,
@@ -125,7 +114,7 @@ impl index::File {
         check: SafetyCheck,
         pack_progress: impl Progress,
         index_progress: impl Progress,
-        should_interrupt: Arc<AtomicBool>,
+        should_interrupt: &AtomicBool,
     ) -> Result<git_hash::ObjectId, Error<E>>
     where
         E: std::error::Error + Send + Sync + 'static,
@@ -138,11 +127,8 @@ impl index::File {
                 });
             }
             let (pack_res, id) = parallel::join(
-                {
-                    let should_interrupt = Arc::clone(&should_interrupt);
-                    move || pack.verify_checksum(pack_progress, &should_interrupt)
-                },
-                move || self.verify_checksum(index_progress, &should_interrupt),
+                move || pack.verify_checksum(pack_progress, should_interrupt),
+                move || self.verify_checksum(index_progress, should_interrupt),
             );
             pack_res?;
             id?

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

@@ -1,5 +1,3 @@
-use std::sync::Arc;
-
 use git_features::{
     parallel::{self, in_parallel_if},
     progress::{self, unit, Progress},
@@ -9,21 +7,21 @@ use super::{Error, Reducer};
 use crate::{data, index, index::util};
 
 mod options {
-    use std::sync::{atomic::AtomicBool, Arc};
+    use std::sync::atomic::AtomicBool;
 
     use crate::index::traverse::SafetyCheck;
 
     /// Traversal options for [`traverse()`][crate::index::File::traverse_with_lookup()]
-    #[derive(Default, Debug, Clone)]
-    pub struct Options {
+    #[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: Arc<AtomicBool>,
+        pub should_interrupt: &'a AtomicBool,
     }
 }
 use std::sync::atomic::Ordering;
@@ -47,7 +45,7 @@ impl index::File {
             thread_limit,
             check,
             should_interrupt,
-        }: Options,
+        }: Options<'_>,
     ) -> Result<(git_hash::ObjectId, index::traverse::Outcome, P), Error<E>>
     where
         P: Progress,
@@ -64,15 +62,8 @@ impl index::File {
             {
                 let pack_progress = progress.add_child("SHA1 of pack");
                 let index_progress = progress.add_child("SHA1 of index");
-                let should_interrupt = Arc::clone(&should_interrupt);
                 move || {
-                    let res = self.possibly_verify(
-                        pack,
-                        check,
-                        pack_progress,
-                        index_progress,
-                        Arc::clone(&should_interrupt),
-                    );
+                    let res = self.possibly_verify(pack, check, pack_progress, index_progress, should_interrupt);
                     if res.is_err() {
                         should_interrupt.store(true, Ordering::SeqCst);
                     }

git-pack/src/index/verify.rs

@@ -1,4 +1,4 @@
-use std::sync::{atomic::AtomicBool, Arc};
+use std::sync::atomic::AtomicBool;
 
 use git_features::progress::{self, Progress};
 use git_object::{bstr::ByteSlice, WriteTo};
@@ -120,7 +120,7 @@ impl index::File {
         pack: Option<PackContext<'_, C, F>>,
         thread_limit: Option<usize>,
         progress: Option<P>,
-        should_interrupt: Arc<AtomicBool>,
+        should_interrupt: &AtomicBool,
     ) -> Result<
         (git_hash::ObjectId, Option<index::traverse::Outcome>, Option<P>),
         index::traverse::Error<crate::index::verify::integrity::Error>,
@@ -157,7 +157,7 @@ impl index::File {
                 )
                 .map(|(id, outcome, root)| (id, Some(outcome), root)),
             None => self
-                .verify_checksum(root.add_child("Sha1 of index"), &should_interrupt)
+                .verify_checksum(root.add_child("Sha1 of index"), should_interrupt)
                 .map_err(Into::into)
                 .map(|id| (id, None, root.into_inner())),
         }

git-pack/src/multi_index/verify.rs

@@ -25,4 +25,27 @@ impl File {
             should_interrupt,
         )
     }
+
+    /// Similar to [`crate::Bundle::verify_integrity()`] but checks all contained indices and their packs.
+    ///
+    /// Note that it's considered a failure if an index doesn't have a corresponding pack.
+    #[allow(unused)]
+    pub fn verify_integrity<C, P>(
+        &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: 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>,
+    >
+    where
+        P: Progress,
+        C: crate::cache::DecodeEntry,
+    {
+        todo!()
+    }
 }

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

@@ -1,7 +1,4 @@
-use std::{
-    convert::Infallible,
-    sync::{atomic::AtomicBool, Arc},
-};
+use std::{convert::Infallible, sync::atomic::AtomicBool};
 
 use git_features::{parallel::reduce::Finalize, progress};
 use git_odb::{compound, pack, pack::FindExt};
@@ -396,7 +393,7 @@ fn write_and_verify(
         || pack::cache::Never,
         None,
         progress::Discard.into(),
-        Arc::new(should_interrupt),
+        &should_interrupt,
     )?;
 
     Ok(())

git-pack/tests/pack/index.rs

@@ -1,7 +1,9 @@
 mod file {
     const SHA1_SIZE: usize = git_hash::Kind::Sha1.len_in_bytes();
+
     use git_object::{self as object};
     use git_odb::pack;
+    use std::sync::atomic::AtomicBool;
 
     use crate::{
         fixture_path, hex_to_id,
@@ -307,7 +309,7 @@ mod file {
                             }),
                             None,
                             progress::Discard.into(),
-                            Default::default()
+                            &AtomicBool::new(false)
                         )
                         .map(|(a, b, _)| (a, b))?,
                         (idx.index_checksum(), Some(stats.to_owned())),
@@ -407,7 +409,7 @@ mod file {
                     None::<git_pack::index::verify::PackContext<'_, _, fn() -> cache::Never>>,
                     None,
                     progress::Discard.into(),
-                    Default::default()
+                    &AtomicBool::new(false)
                 )
                 .map(|(a, b, _)| (a, b))?,
                 (idx.index_checksum(), None)

gitoxide-core/src/pack/explode.rs

@@ -200,7 +200,7 @@ pub fn pack_or_pack_index(
                 let object_path = object_path.map(|p| p.as_ref().to_owned());
                 move || {
                     let out = OutputWriter::new(object_path.clone(), sink_compress, object_hash);
-                    let object_verifier = if verify { object_path.as_ref().map(|path| loose::Store::at(path, object_hash)) } else { None };
+                    let loose_odb = verify.then(|| object_path.as_ref().map(|path| loose::Store::at(path, object_hash))).flatten();
                     let mut read_buf = Vec::new();
                     move |object_kind, buf, index_entry, progress| {
                         let written_id = out.write_buf(object_kind, buf).map_err(|err| {
@@ -220,7 +220,7 @@ pub fn pack_or_pack_index(
                                 return Err(Error::ObjectEncodeMismatch(object_kind, index_entry.oid, written_id));
                             }
                         }
-                        if let Some(verifier) = object_verifier.as_ref() {
+                        if let Some(verifier) = loose_odb.as_ref() {
                             let obj = verifier
                                 .try_find(written_id, &mut read_buf)
                                 .map_err(|err| Error::WrittenFileCorrupt(err, written_id))?
@@ -236,7 +236,7 @@ pub fn pack_or_pack_index(
                 algorithm,
                 thread_limit,
                 check: check.into(),
-                should_interrupt,
+                should_interrupt: &should_interrupt,
             },
         )
         .map(|(_, _, c)| progress::DoOrDiscard::from(c))

gitoxide-core/src/pack/verify.rs

@@ -1,9 +1,4 @@
-use std::{
-    io,
-    path::Path,
-    str::FromStr,
-    sync::{atomic::AtomicBool, Arc},
-};
+use std::{io, path::Path, str::FromStr, sync::atomic::AtomicBool};
 
 use anyhow::{anyhow, Context as AnyhowContext, Result};
 use bytesize::ByteSize;
@@ -54,7 +49,7 @@ impl From<Algorithm> for index::traverse::Algorithm {
 }
 
 /// A general purpose context for many operations provided here
-pub struct Context<W1: io::Write, W2: io::Write> {
+pub struct Context<'a, W1: io::Write, W2: io::Write> {
     /// If set, provide statistics to `out` in the given format
     pub output_statistics: Option<OutputFormat>,
     /// A stream to which to output operation results
@@ -67,19 +62,20 @@ pub struct Context<W1: io::Write, W2: io::Write> {
     pub thread_limit: Option<usize>,
     pub mode: index::verify::Mode,
     pub algorithm: Algorithm,
-    pub should_interrupt: Arc<AtomicBool>,
+    pub should_interrupt: &'a AtomicBool,
 }
 
-impl Default for Context<Vec<u8>, Vec<u8>> {
-    fn default() -> Self {
+impl<'a> Context<'a, Vec<u8>, Vec<u8>> {
+    /// Create a new default context with all fields being the default.
+    pub fn new(should_interrupt: &'a AtomicBool) -> Self {
         Context {
             output_statistics: None,
             thread_limit: None,
             mode: index::verify::Mode::HashCrc32,
             algorithm: Algorithm::LessMemory,
             out: Vec::new(),
             err: Vec::new(),
-            should_interrupt: Default::default(),
+            should_interrupt,
         }
     }
 }
@@ -116,7 +112,7 @@ pub fn pack_or_pack_index<W1, W2>(
         thread_limit,
         algorithm,
         should_interrupt,
-    }: Context<W1, W2>,
+    }: Context<'_, W1, W2>,
 ) -> Result<(ObjectId, Option<index::traverse::Outcome>)>
 where
     W1: io::Write,
@@ -178,7 +174,7 @@ where
                 }),
                 thread_limit,
                 progress,
-                should_interrupt,
+                &should_interrupt,
             )
             .map(|(a, b, _)| (a, b))
             .with_context(|| "Verification failure")?