assure stable handles can actually access the indices hey need (#266)

This works by bypassing the central index, which doesn't know about the
garbaged indices anymore, to obtain the indices directly.

git-odb/src/store_impls/dynamic/find.rs

@@ -272,36 +272,21 @@ where
             "BUG: handle must be configured to `prevent_pack_unload()` before using this method"
         );
         let pack_id = PackId::from_intrinsic_pack_id(location.pack_id);
-        'outer: loop {
+        loop {
             let mut snapshot = self.snapshot.borrow_mut();
+            let marker = snapshot.marker;
             {
-                let marker = snapshot.marker;
                 for index in snapshot.indices.iter_mut() {
                     if let Some(possibly_pack) = index.pack(pack_id) {
                         let pack = match possibly_pack {
                             Some(pack) => pack,
-                            None => match self.store.load_pack(pack_id, marker).ok()? {
-                                Some(pack) => {
-                                    *possibly_pack = Some(pack);
-                                    possibly_pack.as_deref().expect("just put it in")
-                                }
-                                None => {
-                                    // The pack wasn't available anymore so we are supposed to try another round with a fresh index
-                                    match self.store.load_one_index(self.refresh_mode, snapshot.marker).ok()? {
-                                        Some(new_snapshot) => {
-                                            drop(snapshot);
-                                            *self.snapshot.borrow_mut() = new_snapshot;
-                                            continue 'outer;
-                                        }
-                                        None => {
-                                            // nothing new in the index, kind of unexpected to not have a pack but to also
-                                            // to have no new index yet. We set the new index before removing any slots, so
-                                            // this should be observable.
-                                            return None;
-                                        }
-                                    }
-                                }
-                            },
+                            None => {
+                                let pack = self.store.load_pack(pack_id, marker).ok()?.expect(
+                                "BUG: pack must exist from previous call to locaion_by_oid() and must not be unloaded",
+                            );
+                                *possibly_pack = Some(pack);
+                                possibly_pack.as_deref().expect("just put it in")
+                            }
                         };
                         return pack
                             .entry_slice(location.entry_range(location.pack_offset))
@@ -313,13 +298,12 @@ where
                 }
             }
 
-            match self.store.load_one_index(self.refresh_mode, snapshot.marker).ok()? {
-                Some(new_snapshot) => {
-                    drop(snapshot);
-                    *self.snapshot.borrow_mut() = new_snapshot;
-                }
-                None => return None,
-            }
+            snapshot.indices.insert(
+                0,
+                self.store
+                    .index_by_id(pack_id, marker)
+                    .expect("BUG: index must always be present, must not be unloaded or overwritten"),
+            );
         }
     }
 }

git-odb/src/store_impls/dynamic/load_pack.rs → git-odb/src/store_impls/dynamic/load_one.rs

@@ -1,7 +1,7 @@
 use std::sync::atomic::Ordering;
 use std::{path::Path, sync::Arc};
 
-use crate::store::types;
+use crate::store::{handle, types};
 
 impl super::Store {
     /// If Ok(None) is returned, the pack-id was stale and referred to an unloaded pack or a pack which couldn't be
@@ -75,4 +75,32 @@ impl super::Store {
             Some(_multipack_id) => todo!("load from given multipack which needs additional lookup"),
         }
     }
+
+    /// Similar to `.load_pack()`, but for entire indices, bypassing the index entirely and going solely by marker and id.
+    /// Returns `None` if the index wasn't available anymore or could otherwise not be loaded, which can be considered a bug
+    /// as we should always keep needed indices available.
+    pub(crate) fn index_by_id(&self, id: types::PackId, marker: types::SlotIndexMarker) -> Option<handle::IndexLookup> {
+        let slot = self.files.get(id.index)?;
+        if slot.generation.load(Ordering::SeqCst) > marker.generation {
+            // This means somebody just overwrote our trashed slot with a new (or about to be stored) index, which means the slot isn't
+            // what we need it to be.
+            // This shouldn't as we won't overwrite slots while handles need stable indices.
+            return None;
+        }
+        let lookup = match (&**slot.files.load()).as_ref()? {
+            types::IndexAndPacks::Index(bundle) => handle::SingleOrMultiIndex::Single {
+                index: bundle.index.loaded()?.clone(),
+                data: bundle.data.loaded().cloned(),
+            },
+            types::IndexAndPacks::MultiIndex(multi) => handle::SingleOrMultiIndex::Multi {
+                index: multi.multi_index.loaded()?.clone(),
+                data: multi.data.iter().map(|f| f.loaded().cloned()).collect(),
+            },
+        };
+        handle::IndexLookup {
+            id: id.index,
+            file: lookup,
+        }
+        .into()
+    }
 }

git-odb/src/store_impls/dynamic/mod.rs

@@ -54,6 +54,6 @@ pub(crate) mod handle;
 ///
 pub mod load_index;
 
-mod load_pack;
+mod load_one;
 
 mod metrics;

git-odb/tests/odb/store/dynamic.rs

@@ -243,7 +243,6 @@ fn a_bunch_of_loose_and_packed_objects() -> crate::Result {
 }
 
 #[test]
-#[ignore]
 fn auto_refresh_with_and_without_id_stability() {
     let tmp = git_testtools::tempfile::TempDir::new().unwrap();
     assert!(
@@ -357,7 +356,9 @@ fn auto_refresh_with_and_without_id_stability() {
             stable_handle.entry_by_location(&location).is_some(),
             "it finds the old removed location (still loaded) on the old id, it's still cached in the handle, too"
         );
-        // TODO: get a new stable handle without cache and retry, should still work. Actually, won't because it needs the old index snapshot
-        // and all of them loaded.
+        assert!(
+            stable_handle.clone().entry_by_location(&location).is_some(),
+            "handles without any internal cache also work"
+        );
     }
 }