Merge pull request #1853 from GitoxideLabs/odb-issue

repro and fix for #1788

Author: Byron

gix-odb/src/store_impls/dynamic/init.rs

@@ -32,20 +32,20 @@ impl Default for Options {
     }
 }
 
-/// Configures the amount of slots in the index slotmap, which is fixed throughout the existence of the store.
+/// Configures the number of slots in the index slotmap, which is fixed throughout the existence of the store.
 #[derive(Copy, Clone, Debug)]
 pub enum Slots {
-    /// The amount of slots to use, that is the total amount of indices we can hold at a time.
+    /// The number of slots to use, that is the total number of indices we can hold at a time.
     /// Using this has the advantage of avoiding an initial directory listing of the repository, and is recommended
     /// on the server side where the repository setup is controlled.
     ///
     /// Note that this won't affect their packs, as each index can have one or more packs associated with it.
     Given(u16),
-    /// Compute the amount of slots needed, as probably best used on the client side where a variety of repositories is encountered.
+    /// Compute the number of slots needed, as probably best used on the client side where a variety of repositories is encountered.
     AsNeededByDiskState {
         /// 1.0 means no safety, 1.1 means 10% more slots than needed
         multiplier: f32,
-        /// The minimum amount of slots to assume
+        /// The minimum number of slots to assume
         minimum: usize,
     },
 }

gix-odb/src/store_impls/dynamic/types.rs

@@ -84,10 +84,11 @@ impl PackId {
 /// An index that changes only if the packs directory changes and its contents is re-read.
 #[derive(Default)]
 pub struct SlotMapIndex {
-    /// The index into the slot map at which we expect an index or pack file. Neither of these might be loaded yet.
+    /// The index into the slot map at which we expect an index or pack file. Neither of these might be already loaded.
     pub(crate) slot_indices: Vec<usize>,
-    /// A list of loose object databases as resolved by their alternates file in the `object_directory`. The first entry is this objects
-    /// directory loose file database. All other entries are the loose stores of alternates.
+    /// A list of loose object databases as resolved by their alternates file in the `object_directory`.
+    /// The first entry is this repository's directory for the loose file database.
+    /// All other entries are the loose stores of alternates.
     /// It's in an Arc to be shared to Handles, but not to be shared across SlotMapIndices.
     pub(crate) loose_dbs: Arc<Vec<crate::loose::Store>>,
 

gix/tests/gix/remote/fetch.rs

@@ -102,6 +102,39 @@ mod blocking_and_async_io {
             )?;
             Ok(())
         }
+        fn check_fetch_output(
+            repo: &gix::Repository,
+            out: gix::remote::fetch::Outcome,
+            expected_count: usize,
+        ) -> gix_testtools::Result {
+            for local_tracking_branch_name in out.ref_map.mappings.into_iter().filter_map(|m| m.local) {
+                let r = repo.find_reference(&local_tracking_branch_name)?;
+                r.id()
+                    .object()
+                    .expect("object should be present after fetching, triggering pack refreshes works");
+                repo.head_ref()?.unwrap().set_target_id(r.id(), "post fetch")?;
+            }
+            check_odb_accessability(repo, expected_count)?;
+            Ok(())
+        }
+        fn check_odb_accessability(repo: &gix::Repository, expected_count: usize) -> gix_testtools::Result {
+            let mut count_unique = 0;
+            // TODO: somehow there is a lot of duplication when receiving objects.
+            let mut seen = gix_hashtable::HashSet::default();
+            for id in repo.objects.iter()? {
+                let id = id?;
+                if !seen.insert(id) {
+                    continue;
+                }
+                let _obj = repo.find_object(id)?;
+                count_unique += 1;
+            }
+            assert_eq!(
+                count_unique, expected_count,
+                "Each round we receive exactly one commit, effectively"
+            );
+            Ok(())
+        }
         for max_packs in 1..=3 {
             let remote_dir = tempfile::tempdir()?;
             let mut remote_repo = gix::init_bare(remote_dir.path())?;
@@ -128,25 +161,33 @@ mod blocking_and_async_io {
                     Fetch,
                 )
                 .expect("remote is configured after clone")?;
-            for _round_to_create_pack in 1..12 {
+            let minimum_slots = 5;
+            let slots = Slots::AsNeededByDiskState {
+                multiplier: 1.1,
+                minimum: minimum_slots,
+            };
+            let one_more_than_minimum = minimum_slots + 1;
+            for round_to_create_pack in 1..one_more_than_minimum {
+                let expected_object_count = round_to_create_pack + 1 + 1 /* first commit + tree */;
                 create_empty_commit(&remote_repo)?;
                 match remote
                     .connect(Fetch)?
                     .prepare_fetch(gix::progress::Discard, Default::default())?
                     .receive(gix::progress::Discard, &IS_INTERRUPTED)
                 {
-                    Ok(out) => {
-                        for local_tracking_branch_name in out.ref_map.mappings.into_iter().filter_map(|m| m.local) {
-                            let r = local_repo.find_reference(&local_tracking_branch_name)?;
-                            r.id()
-                                .object()
-                                .expect("object should be present after fetching, triggering pack refreshes works");
-                            local_repo.head_ref()?.unwrap().set_target_id(r.id(), "post fetch")?;
-                        }
+                    Ok(out) => check_fetch_output(&local_repo, out, expected_object_count)?,
+                    Err(err) => {
+                        assert!(err
+                            .to_string()
+                            .starts_with("The slotmap turned out to be too small with "));
+                        // But opening a new repo will always be able to read all objects
+                        // as it dynamically sizes the otherwise static slotmap.
+                        let local_repo = gix::open_opts(
+                            local_repo.path(),
+                            gix::open::Options::isolated().object_store_slots(slots),
+                        )?;
+                        check_odb_accessability(&local_repo, expected_object_count)?;
                     }
-                    Err(err) => assert!(err
-                        .to_string()
-                        .starts_with("The slotmap turned out to be too small with ")),
                 }
             }
         }