fix: case-insentively conflicting references can be created even on case-insensitie filesystems*. (#595)

The asterisk indicates that this only works if packed-refs are present
and these references are written straight to packed references without
ever trying to handle the otherwise conflicting loose reference files.

This is done by leveraging the fact that in presence of packed-refs
or a pending creation of packed-refs, there is no need to create
per-file locks as concurrent transactions also have to obtain the
packed-refs lock and fail (or wait) until it's done.

Author: Byron

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

@@ -44,6 +44,12 @@ impl file::Store {
     pub fn packed_refs_path(&self) -> PathBuf {
         self.common_dir_resolved().join("packed-refs")
     }
+
+    pub(crate) fn packed_refs_lock_path(&self) -> PathBuf {
+        let mut p = self.packed_refs_path();
+        p.set_extension("lock");
+        p
+    }
 }
 
 ///

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

@@ -36,7 +36,11 @@ impl<'s, 'p> Transaction<'s, 'p> {
             match &change.update.change {
                 // reflog first, then reference
                 Change::Update { log, new, expected } => {
-                    let lock = change.lock.take().expect("each ref is locked");
+                    let lock = match change.lock.take() {
+                        Some(l) => l,
+                        // Some updates are never locked as they are no-ops
+                        None => continue,
+                    };
                     let (update_ref, update_reflog) = match log.mode {
                         RefLog::Only => (false, true),
                         RefLog::AndReference => (true, true),
@@ -151,7 +155,7 @@ impl<'s, 'p> Transaction<'s, 'p> {
                 Change::Delete { log: mode, .. } => *mode == RefLog::AndReference,
             };
             if take_lock_and_delete {
-                let lock = change.lock.take().expect("lock must still be present in delete mode");
+                let lock = change.lock.take();
                 let reference_path = self.store.reference_path(change.update.name.as_ref());
                 if let Err(err) = std::fs::remove_file(reference_path) {
                     if err.kind() != std::io::ErrorKind::NotFound {

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

@@ -1,5 +1,6 @@
 use git_hash::ObjectId;
 use git_object::bstr::BString;
+use std::fmt::Formatter;
 
 use crate::transaction::{Change, PreviousValue};
 use crate::{
@@ -112,6 +113,15 @@ impl<'s, 'p> Transaction<'s, 'p> {
     }
 }
 
+impl std::fmt::Debug for Transaction<'_, '_> {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("Transaction")
+            .field("store", self.store)
+            .field("edits", &self.updates.as_ref().map(|u| u.len()))
+            .finish_non_exhaustive()
+    }
+}
+
 ///
 pub mod prepare;
 

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

@@ -18,6 +18,8 @@ impl<'s, 'p> Transaction<'s, 'p> {
         lock_fail_mode: git_lock::acquire::Fail,
         packed: Option<&packed::Buffer>,
         change: &mut Edit,
+        has_global_lock: bool,
+        direct_to_packed_refs: bool,
     ) -> Result<(), Error> {
         use std::io::Write;
         assert!(
@@ -94,19 +96,22 @@ impl<'s, 'p> Transaction<'s, 'p> {
                     *expected = PreviousValue::MustExistAndMatch(existing.target);
                 }
 
-                lock
+                Some(lock)
             }
             Change::Update { expected, new, .. } => {
                 let (base, relative_path) = store.reference_path_with_base(change.update.name.as_ref());
-                let mut lock = git_lock::File::acquire_to_update_resource(
-                    base.join(relative_path),
-                    lock_fail_mode,
-                    Some(base.into_owned()),
-                )
-                .map_err(|err| Error::LockAcquire {
-                    source: err,
-                    full_name: "borrowchk won't allow change.name() and this will be corrected by caller".into(),
-                })?;
+                let obtain_lock = || {
+                    git_lock::File::acquire_to_update_resource(
+                        base.join(relative_path.as_ref()),
+                        lock_fail_mode,
+                        Some(base.clone().into_owned()),
+                    )
+                    .map_err(|err| Error::LockAcquire {
+                        source: err,
+                        full_name: "borrowchk won't allow change.name() and this will be corrected by caller".into(),
+                    })
+                };
+                let mut lock = (!has_global_lock).then(|| obtain_lock()).transpose()?;
 
                 let existing_ref = existing_ref?;
                 match (&expected, &existing_ref) {
@@ -158,17 +163,20 @@ impl<'s, 'p> Transaction<'s, 'p> {
                     true
                 };
 
-                if is_effective {
+                if is_effective && !direct_to_packed_refs {
+                    let mut lock = lock.take().map(Ok).unwrap_or_else(obtain_lock)?;
+
                     lock.with_mut(|file| match new {
                         Target::Peeled(oid) => write!(file, "{}", oid),
                         Target::Symbolic(name) => write!(file, "ref: {}", name.0),
                     })?;
+                    Some(lock.close()?)
+                } else {
+                    None
                 }
-
-                lock.close()?
             }
         };
-        change.lock = Some(lock);
+        change.lock = lock;
         Ok(())
     }
 }
@@ -219,7 +227,10 @@ impl<'s, 'p> Transaction<'s, 'p> {
             | PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(_) => Some(0_usize),
             PackedRefs::DeletionsOnly => None,
         };
-        if maybe_updates_for_packed_refs.is_some() || self.store.packed_refs_path().is_file() {
+        if maybe_updates_for_packed_refs.is_some()
+            || self.store.packed_refs_path().is_file()
+            || self.store.packed_refs_lock_path().is_file()
+        {
             let mut edits_for_packed_transaction = Vec::<RefEdit>::new();
             let mut needs_packed_refs_lookups = false;
             for edit in updates.iter() {
@@ -271,28 +282,29 @@ impl<'s, 'p> Transaction<'s, 'p> {
                 // What follows means that we will only create a transaction if we have to access packed refs for looking
                 // up current ref values, or that we definitely have a transaction if we need to make updates. Otherwise
                 // we may have no transaction at all which isn't required if we had none and would only try making deletions.
-                let packed_transaction: Option<_> = if maybe_updates_for_packed_refs.unwrap_or(0) > 0 {
-                    // We have to create a packed-ref even if it doesn't exist
-                    self.store
-                        .packed_transaction(packed_refs_lock_fail_mode)
-                        .map_err(|err| match err {
-                            file::packed::transaction::Error::BufferOpen(err) => Error::from(err),
-                            file::packed::transaction::Error::TransactionLock(err) => {
-                                Error::PackedTransactionAcquire(err)
-                            }
-                        })?
-                        .into()
-                } else {
-                    // A packed transaction is optional - we only have deletions that can't be made if
-                    // no packed-ref file exists anyway
-                    self.store
-                        .assure_packed_refs_uptodate()?
-                        .map(|p| {
-                            buffer_into_transaction(p, packed_refs_lock_fail_mode)
-                                .map_err(Error::PackedTransactionAcquire)
-                        })
-                        .transpose()?
-                };
+                let packed_transaction: Option<_> =
+                    if maybe_updates_for_packed_refs.unwrap_or(0) > 0 || self.store.packed_refs_lock_path().is_file() {
+                        // We have to create a packed-ref even if it doesn't exist
+                        self.store
+                            .packed_transaction(packed_refs_lock_fail_mode)
+                            .map_err(|err| match err {
+                                file::packed::transaction::Error::BufferOpen(err) => Error::from(err),
+                                file::packed::transaction::Error::TransactionLock(err) => {
+                                    Error::PackedTransactionAcquire(err)
+                                }
+                            })?
+                            .into()
+                    } else {
+                        // A packed transaction is optional - we only have deletions that can't be made if
+                        // no packed-ref file exists anyway
+                        self.store
+                            .assure_packed_refs_uptodate()?
+                            .map(|p| {
+                                buffer_into_transaction(p, packed_refs_lock_fail_mode)
+                                    .map_err(Error::PackedTransactionAcquire)
+                            })
+                            .transpose()?
+                    };
                 if let Some(transaction) = packed_transaction {
                     self.packed_transaction = Some(match &mut self.packed_refs {
                         PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(f)
@@ -315,6 +327,11 @@ impl<'s, 'p> Transaction<'s, 'p> {
                 ref_files_lock_fail_mode,
                 self.packed_transaction.as_ref().and_then(|t| t.buffer()),
                 change,
+                self.packed_transaction.is_some(),
+                matches!(
+                    self.packed_refs,
+                    PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(_)
+                ),
             ) {
                 let err = match err {
                     Error::LockAcquire {

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

@@ -1,3 +1,4 @@
+use std::fmt::Formatter;
 use std::io::Write;
 
 use crate::{
@@ -24,6 +25,15 @@ impl packed::Transaction {
     }
 }
 
+impl std::fmt::Debug for packed::Transaction {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("packed::Transaction")
+            .field("edits", &self.edits.as_ref().map(|e| e.len()))
+            .field("lock", &self.lock)
+            .finish_non_exhaustive()
+    }
+}
+
 /// Access
 impl packed::Transaction {
     /// Returns our packed buffer

git-ref/tests/file/transaction/prepare_and_commit/create_or_update.rs

@@ -22,17 +22,20 @@ use crate::file::{
 };
 
 mod collisions {
-    use crate::file::transaction::prepare_and_commit::{committer, create_at, empty_store};
+    use crate::file::transaction::prepare_and_commit::{committer, create_at, delete_at, empty_store};
     use git_lock::acquire::Fail;
     use git_ref::file::transaction::PackedRefs;
+    use git_ref::transaction::{Change, LogChange, PreviousValue, RefEdit};
+    use git_ref::Target;
+    use std::convert::TryInto;
 
     fn case_sensitive(tmp_dir: &std::path::Path) -> bool {
         std::fs::write(tmp_dir.join("config"), "").expect("can create file once");
         !git_worktree::fs::Capabilities::probe(tmp_dir).ignore_case
     }
 
     #[test]
-    fn conflicting_creation_without_packedrefs() -> crate::Result {
+    fn conflicting_creation_without_packed_refs() -> crate::Result {
         let (dir, store) = empty_store()?;
         let res = store.transaction().prepare(
             [create_at("refs/a"), create_at("refs/A")],
@@ -57,33 +60,131 @@ mod collisions {
     }
 
     #[test]
-    fn conflicting_creation_into_packed_refs() -> crate::Result {
+    fn non_conflicting_creation_without_packed_refs_work() -> crate::Result {
         let (_dir, store) = empty_store()?;
+        let ongoing = store
+            .transaction()
+            .prepare([create_at("refs/new")], Fail::Immediately, Fail::Immediately)
+            .unwrap();
+
+        let t2 = store.transaction().prepare(
+            [create_at("refs/non-conflicting")],
+            Fail::Immediately,
+            Fail::Immediately,
+        )?;
+
+        t2.commit(committer().to_ref())?;
+        ongoing.commit(committer().to_ref())?;
+
+        Ok(())
+    }
+
+    #[test]
+    fn packed_refs_lock_is_mandatory_for_multiple_ongoing_transactions_even_if_one_does_not_need_it() -> crate::Result {
+        let (_dir, store) = empty_store()?;
+        let ref_name = "refs/a";
+        let _t1 = store
+            .transaction()
+            .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
+                Box::new(|_, _| Ok(Some(git_object::Kind::Commit))),
+            ))
+            .prepare([create_at(ref_name)], Fail::Immediately, Fail::Immediately)?;
+
+        let t2res = store
+            .transaction()
+            .prepare([delete_at(ref_name)], Fail::Immediately, Fail::Immediately);
+        assert_eq!(&t2res.unwrap_err().to_string()[..51], "The lock for the packed-ref file could not be obtai", "if packed-refs are about to be created, other transactions always acquire a packed-refs lock as to not miss anything");
+        Ok(())
+    }
+
+    #[test]
+    fn conflicting_creation_into_packed_refs() {
+        let (_dir, store) = empty_store().unwrap();
         store
             .transaction()
             .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
                 Box::new(|_, _| Ok(Some(git_object::Kind::Commit))),
             ))
             .prepare(
-                [create_at("refs/a"), create_at("refs/Ab")],
+                [create_at("refs/a"), create_at("refs/A")],
                 Fail::Immediately,
                 Fail::Immediately,
-            )?
-            .commit(committer().to_ref())?;
+            )
+            .unwrap()
+            .commit(committer().to_ref())
+            .unwrap();
 
         assert_eq!(
-            store.cached_packed_buffer()?.expect("created").iter()?.count(),
+            store
+                .cached_packed_buffer()
+                .unwrap()
+                .expect("created")
+                .iter()
+                .unwrap()
+                .count(),
             2,
             "packed-refs can store everything in case-insensitive manner"
         );
-
         assert_eq!(
-            store.loose_iter()?.count(),
+            store.loose_iter().unwrap().count(),
             0,
             "refs/ directory isn't present as there is no loose ref - it removed every up to the base dir"
         );
 
-        Ok(())
+        // The following works because locks aren't actually obtained if there would be no change.
+        store
+            .transaction()
+            .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
+                Box::new(|_, _| Ok(Some(git_object::Kind::Commit))),
+            ))
+            .prepare(
+                [
+                    RefEdit {
+                        change: Change::Update {
+                            log: LogChange::default(),
+                            expected: PreviousValue::Any,
+                            new: Target::Peeled(git_hash::Kind::Sha1.null()),
+                        },
+                        name: "refs/a".try_into().expect("valid"),
+                        deref: false,
+                    },
+                    RefEdit {
+                        change: Change::Update {
+                            log: LogChange::default(),
+                            expected: PreviousValue::MustExistAndMatch(Target::Peeled(git_hash::Kind::Sha1.null())),
+                            new: Target::Peeled(git_hash::Kind::Sha1.null()),
+                        },
+                        name: "refs/A".try_into().expect("valid"),
+                        deref: false,
+                    },
+                ],
+                Fail::Immediately,
+                Fail::Immediately,
+            )
+            .unwrap()
+            .commit(committer().to_ref())
+            .unwrap();
+
+        {
+            let _ongoing = store
+                .transaction()
+                .prepare([create_at("refs/new")], Fail::Immediately, Fail::Immediately)
+                .unwrap();
+
+            let t2res = store.transaction().prepare(
+                [create_at("refs/non-conflicting")],
+                Fail::Immediately,
+                Fail::Immediately,
+            );
+
+            assert_eq!(
+                &t2res.unwrap_err().to_string()[..40],
+                "The lock for the packed-ref file could n",
+                "packed-refs files will always be locked if they are present as we have to look up their content"
+            );
+        }
+
+        // TODO: parallel deletion
     }
 }