Use generalized reload-on-demand in git-ref (#427)

git-features/src/fs.rs

@@ -151,7 +151,7 @@ mod reload_on_demand {
                     }
                 }
                 (None, Some(_modified_time)) => {
-                    drop(state);
+                    drop(state_opt_lock);
                     let mut state = get_mut(state);
                     // Still in the same situation? If so, load the buffer.
                     if let (None, Some(modified_time)) = (&*state, current_modification_time()) {

git-ref/src/store/file/find.rs

@@ -36,7 +36,7 @@ impl file::Store {
         Error: From<E>,
     {
         let packed = self.assure_packed_refs_uptodate()?;
-        self.find_one_with_verified_input(partial.try_into()?, packed.as_deref())
+        self.find_one_with_verified_input(partial.try_into()?, packed.as_ref().map(|b| &***b))
     }
 
     /// Similar to [`file::Store::find()`] but a non-existing ref is treated as error.
@@ -255,7 +255,7 @@ pub mod existing {
             crate::name::Error: From<E>,
         {
             let packed = self.assure_packed_refs_uptodate().map_err(find::Error::PackedOpen)?;
-            self.find_existing_inner(partial, packed.as_deref())
+            self.find_existing_inner(partial, packed.as_ref().map(|b| &***b))
         }
 
         /// Similar to [`file::Store::find()`], but supports a stable packed buffer.

git-ref/src/store/file/loose/mod.rs

@@ -40,7 +40,6 @@ mod init {
                 write_reflog,
                 namespace: None,
                 packed: Default::default(),
-                packed2: Default::default(),
                 object_hash,
             }
         }
@@ -59,7 +58,6 @@ mod init {
                 write_reflog,
                 namespace: None,
                 packed: Default::default(),
-                packed2: Default::default(),
                 object_hash,
             }
         }

git-ref/src/store/file/mod.rs

@@ -3,8 +3,6 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use git_features::threading::{MutableOnDemand, OwnShared};
-
 use crate::{bstr::BStr, store::WriteReflog, Namespace};
 
 /// A store for reference which uses plain files.
@@ -30,11 +28,7 @@ pub struct Store {
     /// A packed buffer which can be mapped in one version and shared as such.
     /// It's updated only in one spot, which is prior to reading it based on file stamps.
     /// Doing it like this has the benefit of being able to hand snapshots out to people without blocking others from updating it.
-    packed: OwnShared<MutableOnDemand<packed::modifiable::State>>,
-    /// A packed buffer which can be mapped in one version and shared as such.
-    /// It's updated only in one spot, which is prior to reading it based on file stamps.
-    /// Doing it like this has the benefit of being able to hand snapshots out to people without blocking others from updating it.
-    packed2: OwnShared<packed::modifiable::State2>,
+    packed: packed::modifiable::SharedBufferStorage,
 }
 
 mod access {

git-ref/src/store/file/overlay_iter.rs

@@ -6,8 +6,6 @@ use std::{
     path::{Path, PathBuf},
 };
 
-use git_features::threading::OwnShared;
-
 use crate::{
     file::{loose, loose::iter::SortedLoosePaths, path_to_name},
     store_impl::{file, packed},
@@ -39,7 +37,7 @@ enum IterKind {
 #[must_use = "Iterators should be obtained from this platform"]
 pub struct Platform<'s> {
     store: &'s file::Store,
-    packed: Option<OwnShared<packed::Buffer>>,
+    packed: Option<file::packed::SharedBuffer>,
 }
 
 impl<'p, 's> LooseThenPacked<'p, 's> {
@@ -194,14 +192,15 @@ impl<'s> Platform<'s> {
     ///
     /// Errors are returned similarly to what would happen when loose and packed refs where iterated by themeselves.
     pub fn all(&self) -> std::io::Result<LooseThenPacked<'_, '_>> {
-        self.store.iter_packed(self.packed.as_deref())
+        self.store.iter_packed(self.packed.as_ref().map(|b| &***b))
     }
 
     /// As [`iter(…)`][file::Store::iter()], but filters by `prefix`, i.e. "refs/heads".
     ///
     /// Please note that "refs/heads` or "refs\\heads" is equivalent to "refs/heads/"
     pub fn prefixed(&self, prefix: impl AsRef<Path>) -> std::io::Result<LooseThenPacked<'_, '_>> {
-        self.store.iter_prefixed_packed(prefix, self.packed.as_deref())
+        self.store
+            .iter_prefixed_packed(prefix, self.packed.as_ref().map(|b| &***b))
     }
 }
 

git-ref/src/store/file/packed.rs

@@ -36,7 +36,7 @@ impl file::Store {
     ///
     /// Use this to make successive calls to [`file::Store::try_find_packed()`]
     /// or obtain iterators using [`file::Store::iter_packed()`] in a way that assures the packed-refs content won't change.
-    pub fn cached_packed_buffer(&self) -> Result<Option<OwnShared<packed::Buffer>>, packed::buffer::open::Error> {
+    pub fn cached_packed_buffer(&self) -> Result<Option<file::packed::SharedBuffer>, packed::buffer::open::Error> {
         self.assure_packed_refs_uptodate()
     }
 
@@ -71,99 +71,33 @@ pub mod transaction {
     }
 }
 
-pub(crate) mod modifiable {
-    use std::time::SystemTime;
+#[allow(missing_docs)]
+pub type SharedBuffer = OwnShared<git_features::fs::ReloadIfChanged<packed::Buffer>>;
 
-    use git_features::threading::{get_mut, get_ref, OwnShared};
+pub(crate) mod modifiable {
+    use git_features::threading::OwnShared;
 
     use crate::{file, packed};
 
-    pub(crate) type State2 = git_features::fs::ReloadIfChangedStorage<packed::Buffer>;
-
-    #[derive(Debug, Default)]
-    pub(crate) struct State {
-        buffer: Option<OwnShared<packed::Buffer>>,
-        modified: Option<SystemTime>,
-    }
+    pub(crate) type SharedBufferStorage = OwnShared<State>;
+    type State = git_features::fs::ReloadIfChangedStorage<packed::Buffer>;
 
     impl file::Store {
-        pub(crate) fn force_refresh_packed_buffer2(&self) -> Result<(), packed::buffer::open::Error> {
-            git_features::fs::ReloadIfChanged::force_refresh(&self.packed2, || {
+        pub(crate) fn force_refresh_packed_buffer(&self) -> Result<(), packed::buffer::open::Error> {
+            git_features::fs::ReloadIfChanged::force_refresh(&self.packed, || {
                 let modified = self.packed_refs_path().metadata()?.modified()?;
                 self.open_packed_buffer()
                     .and_then(|packed| Ok(Some(modified).zip(packed)))
             })
         }
-        pub(crate) fn assure_packed_refs_uptodate2(
+        pub(crate) fn assure_packed_refs_uptodate(
             &self,
-        ) -> Result<Option<OwnShared<git_features::fs::ReloadIfChanged<packed::Buffer>>>, packed::buffer::open::Error>
-        {
+        ) -> Result<Option<super::SharedBuffer>, packed::buffer::open::Error> {
             git_features::fs::ReloadIfChanged::assure_uptodate(
-                &self.packed2,
+                &self.packed,
                 || self.packed_refs_path().metadata().and_then(|m| m.modified()).ok(),
                 || self.open_packed_buffer(),
             )
         }
-
-        /// Always reload the internally cached packed buffer from disk. This can be necessary if the caller knows something changed
-        /// but fears the change is not picked up due to lack of precision in fstat mtime calls.
-        pub(crate) fn force_refresh_packed_buffer(&self) -> Result<(), packed::buffer::open::Error> {
-            let mut state = get_mut(&self.packed);
-            state.modified = self.packed_refs_path().metadata()?.modified()?.into();
-            state.buffer = self.open_packed_buffer()?.map(OwnShared::new);
-            Ok(())
-        }
-        pub(crate) fn assure_packed_refs_uptodate(
-            &self,
-        ) -> Result<Option<OwnShared<packed::Buffer>>, packed::buffer::open::Error> {
-            let packed_refs_path = self.packed_refs_path();
-            let packed_refs_modified_time = || packed_refs_path.metadata().and_then(|m| m.modified()).ok();
-            let state = get_ref(&self.packed);
-            let recent_modification = packed_refs_modified_time();
-            let buffer = match (&state.modified, recent_modification) {
-                (None, None) => state.buffer.clone(),
-                (Some(_), None) => {
-                    drop(state);
-                    let mut state = get_mut(&self.packed);
-                    // Still in the same situation? If so, drop the loaded buffer
-                    if let (Some(_), None) = (state.modified, packed_refs_modified_time()) {
-                        state.buffer = None;
-                        state.modified = None;
-                    }
-                    state.buffer.clone()
-                }
-                (Some(cached_time), Some(modified_time)) => {
-                    if *cached_time < modified_time {
-                        drop(state);
-                        let mut state = get_mut(&self.packed);
-                        // in the common case, we check again and do what we do only if we are
-                        // still in the same situation, writers pile up.
-                        match (state.modified, packed_refs_modified_time()) {
-                            (Some(cached_time), Some(modified_time)) if cached_time < modified_time => {
-                                state.buffer = self.open_packed_buffer()?.map(OwnShared::new);
-                                state.modified = Some(modified_time);
-                            }
-                            _ => {}
-                        }
-                        state.buffer.clone()
-                    } else {
-                        // Note that this relies on sub-section precision or else is a race when the packed file was just changed.
-                        // It's nothing we can know though, so… up to the caller unfortunately.
-                        state.buffer.clone()
-                    }
-                }
-                (None, Some(_modified_time)) => {
-                    drop(state);
-                    let mut state = get_mut(&self.packed);
-                    // Still in the same situation? If so, load the buffer.
-                    if let (None, Some(modified_time)) = (state.modified, packed_refs_modified_time()) {
-                        state.buffer = self.open_packed_buffer()?.map(OwnShared::new);
-                        state.modified = Some(modified_time);
-                    }
-                    state.buffer.clone()
-                }
-            };
-            Ok(buffer)
-        }
     }
 }

git-ref/src/store/file/raw_ext.rs

@@ -74,7 +74,7 @@ impl ReferenceExt for Reference {
         let packed = store.assure_packed_refs_uptodate().map_err(|err| {
             peel::to_id::Error::Follow(file::find::existing::Error::Find(file::find::Error::PackedOpen(err)))
         })?;
-        self.peel_to_id_in_place_packed(store, find, packed.as_deref())
+        self.peel_to_id_in_place_packed(store, find, packed.as_ref().map(|b| &***b))
     }
 
     fn peel_to_id_in_place_packed<E: std::error::Error + Send + Sync + 'static>(
@@ -143,7 +143,7 @@ impl ReferenceExt for Reference {
             Ok(packed) => packed,
             Err(err) => return Some(Err(err)),
         };
-        self.follow_packed(store, packed.as_deref())
+        self.follow_packed(store, packed.as_ref().map(|b| &***b))
     }
 
     fn follow_packed(

git-ref/src/store/file/transaction/prepare.rs

@@ -280,10 +280,7 @@ impl<'s> Transaction<'s> {
                     // no packed-ref file exists anyway
                     self.store
                         .assure_packed_refs_uptodate()?
-                        .map(|p| {
-                            p.into_transaction(lock_fail_mode)
-                                .map_err(Error::PackedTransactionAcquire)
-                        })
+                        .map(|p| buffer_into_transaction(p, lock_fail_mode).map_err(Error::PackedTransactionAcquire))
                         .transpose()?
                 };
                 if let Some(transaction) = packed_transaction {
@@ -435,6 +432,7 @@ mod error {
     }
 }
 
+use crate::packed::transaction::buffer_into_transaction;
 pub use error::Error;
 
 use crate::transaction::PreviousValue;

git-ref/src/store/packed/mod.rs

@@ -1,11 +1,10 @@
 use std::path::PathBuf;
 
-use git_features::threading::OwnShared;
 use git_hash::ObjectId;
 use git_object::bstr::{BStr, BString};
 use memmap2::Mmap;
 
-use crate::{transaction::RefEdit, FullNameRef};
+use crate::{file, transaction::RefEdit, FullNameRef};
 
 #[derive(Debug)]
 enum Backing {
@@ -34,7 +33,7 @@ struct Edit {
 
 /// A transaction for editing packed references
 pub(crate) struct Transaction {
-    buffer: Option<OwnShared<Buffer>>,
+    buffer: Option<file::packed::SharedBuffer>,
     edits: Option<Vec<Edit>>,
     lock: Option<git_lock::File>,
     #[allow(dead_code)] // It just has to be kept alive, hence no reads

git-ref/src/store/packed/transaction.rs

@@ -1,8 +1,7 @@
 use std::io::Write;
 
-use git_features::threading::OwnShared;
-
 use crate::{
+    file,
     store_impl::{file::transaction::FindObjectFn, packed, packed::Edit},
     transaction::{Change, RefEdit},
     Target,
@@ -12,7 +11,7 @@ pub(crate) const HEADER_LINE: &[u8] = b"# pack-refs with: peeled fully-peeled so
 
 /// Access and instantiation
 impl packed::Transaction {
-    pub(crate) fn new_from_pack_and_lock(buffer: Option<OwnShared<packed::Buffer>>, lock: git_lock::File) -> Self {
+    pub(crate) fn new_from_pack_and_lock(buffer: Option<file::packed::SharedBuffer>, lock: git_lock::File) -> Self {
         packed::Transaction {
             buffer,
             edits: None,
@@ -26,7 +25,7 @@ impl packed::Transaction {
 impl packed::Transaction {
     /// Returns our packed buffer
     pub fn buffer(&self) -> Option<&packed::Buffer> {
-        self.buffer.as_deref()
+        self.buffer.as_ref().map(|b| &***b)
     }
 }
 
@@ -211,20 +210,18 @@ fn write_edit(mut out: impl std::io::Write, edit: &Edit, lines_written: &mut i32
     Ok(())
 }
 
-impl packed::Buffer {
-    /// Convert this buffer to be used as the basis for a transaction.
-    pub(crate) fn into_transaction(
-        self: OwnShared<Self>,
-        lock_mode: git_lock::acquire::Fail,
-    ) -> Result<packed::Transaction, git_lock::acquire::Error> {
-        let lock = git_lock::File::acquire_to_update_resource(&self.path, lock_mode, None)?;
-        Ok(packed::Transaction {
-            buffer: Some(self),
-            lock: Some(lock),
-            closed_lock: None,
-            edits: None,
-        })
-    }
+/// Convert this buffer to be used as the basis for a transaction.
+pub(crate) fn buffer_into_transaction(
+    buffer: file::packed::SharedBuffer,
+    lock_mode: git_lock::acquire::Fail,
+) -> Result<packed::Transaction, git_lock::acquire::Error> {
+    let lock = git_lock::File::acquire_to_update_resource(&buffer.path, lock_mode, None)?;
+    Ok(packed::Transaction {
+        buffer: Some(buffer),
+        lock: Some(lock),
+        closed_lock: None,
+        edits: None,
+    })
 }
 
 ///

git-ref/tests/refs-parallel.rs

@@ -1,7 +1,9 @@
 type Result<T = ()> = std::result::Result<T, Box<dyn std::error::Error>>;
 
 mod file;
+mod fullname;
 mod namespace;
 mod packed;
+mod reference;
 mod store;
 mod transaction;

git-ref/tests/refs.rs

@@ -1,9 +1,17 @@
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 type Result<T = ()> = std::result::Result<T, Box<dyn std::error::Error>>;
 
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod file;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod fullname;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod namespace;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod packed;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod reference;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod store;
+#[cfg(not(feature = "internal-testing-git-features-parallel"))]
 mod transaction;