fix: assure symrefs don't get deleted when moving refs to packed-refs. (#595)

Previously it was possible for symbolic refs to be deleted right after
they have been created or updated as they were included in the set of
refs that was assumed to be part of packed-refs, which isn't the case
for symbolic refs.

Author: Byron

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

@@ -79,7 +79,7 @@ impl<'s, 'p> Transaction<'s, 'p> {
                     // Don't do anything else while keeping the lock after potentially updating the reflog.
                     // We delay deletion of the reference and dropping the lock to after the packed-refs were
                     // safely written.
-                    if delete_loose_refs {
+                    if delete_loose_refs && matches!(new, Target::Peeled(_)) {
                         change.lock = lock;
                         continue;
                     }
@@ -145,8 +145,9 @@ impl<'s, 'p> Transaction<'s, 'p> {
             let take_lock_and_delete = match &change.update.change {
                 Change::Update {
                     log: LogChange { mode, .. },
+                    new,
                     ..
-                } => delete_loose_refs && *mode == RefLog::AndReference,
+                } => delete_loose_refs && *mode == RefLog::AndReference && matches!(new, Target::Peeled(_)),
                 Change::Delete { log: mode, .. } => *mode == RefLog::AndReference,
             };
             if take_lock_and_delete {

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

@@ -5,7 +5,6 @@ use std::fmt::Formatter;
 use crate::{
     store_impl::{file, file::Transaction},
     transaction::RefEdit,
-    Target,
 };
 
 /// A function receiving an object id to resolve, returning its decompressed bytes,
@@ -26,6 +25,7 @@ pub enum PackedRefs<'a> {
     DeletionsAndNonSymbolicUpdates(Box<FindObjectFn<'a>>),
     /// Propagate deletions as well as updates to references which are peeled, that is contain an object id. Furthermore delete the
     /// reference which is originally updated if it exists. If it doesn't, the new value will be written into the packed ref right away.
+    /// Note that this doesn't affect symbolic references at all, which can't be placed into packed refs.
     DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(Box<FindObjectFn<'a>>),
 }
 
@@ -53,14 +53,6 @@ impl Edit {
     }
 }
 
-fn new_would_change_existing(new: &Target, existing: &Target) -> bool {
-    match (new, existing) {
-        (Target::Peeled(new), Target::Peeled(old)) => old != new,
-        (Target::Symbolic(new), Target::Symbolic(old)) => old != new,
-        (_, _) => true,
-    }
-}
-
 impl std::borrow::Borrow<RefEdit> for Edit {
     fn borrow(&self) -> &RefEdit {
         &self.update

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

@@ -12,6 +12,8 @@ use crate::{
     FullName, FullNameRef, Reference, Target,
 };
 
+use crate::{packed::transaction::buffer_into_transaction, transaction::PreviousValue};
+
 impl<'s, 'p> Transaction<'s, 'p> {
     fn lock_ref_and_apply_change(
         store: &file::Store,
@@ -161,15 +163,24 @@ impl<'s, 'p> Transaction<'s, 'p> {
                     }
                 };
 
-                let is_effective = if let Some(existing) = existing_ref {
-                    let effective = new_would_change_existing(new, &existing.target);
+                fn new_would_change_existing(new: &Target, existing: &Target) -> (bool, bool) {
+                    match (new, existing) {
+                        (Target::Peeled(new), Target::Peeled(old)) => (old != new, false),
+                        (Target::Symbolic(new), Target::Symbolic(old)) => (old != new, true),
+                        (Target::Peeled(_), _) => (true, false),
+                        (Target::Symbolic(_), _) => (true, true),
+                    }
+                }
+
+                let (is_effective, is_symbolic) = if let Some(existing) = existing_ref {
+                    let (effective, is_symbolic) = new_would_change_existing(new, &existing.target);
                     *expected = PreviousValue::MustExistAndMatch(existing.target);
-                    effective
+                    (effective, is_symbolic)
                 } else {
-                    true
+                    (true, matches!(new, Target::Symbolic(_)))
                 };
 
-                if is_effective && !direct_to_packed_refs {
+                if (is_effective && !direct_to_packed_refs) || is_symbolic {
                     let mut lock = lock.take().map(Ok).unwrap_or_else(obtain_lock)?;
 
                     lock.with_mut(|file| match new {
@@ -466,6 +477,3 @@ mod error {
 }
 
 pub use error::Error;
-
-use crate::file::transaction::new_would_change_existing;
-use crate::{packed::transaction::buffer_into_transaction, transaction::PreviousValue};

git-ref/tests/file/store/iter.rs

@@ -9,6 +9,15 @@ mod with_namespace {
     use git_object::bstr::{BString, ByteSlice};
 
     use crate::file::store_at;
+    use crate::file::transaction::prepare_and_commit::empty_store;
+
+    #[test]
+    fn missing_refs_dir_yields_empty_iteration() -> crate::Result {
+        let (_dir, store) = empty_store()?;
+        assert_eq!(store.iter()?.all()?.count(), 0);
+        assert_eq!(store.loose_iter()?.count(), 0);
+        Ok(())
+    }
 
     #[test]
     fn iteration_can_trivially_use_namespaces_as_prefixes() -> crate::Result {

git-ref/tests/file/transaction/mod.rs

@@ -17,7 +17,7 @@ pub(crate) mod prepare_and_commit {
         Ok(res)
     }
 
-    fn empty_store() -> crate::Result<(tempfile::TempDir, file::Store)> {
+    pub(crate) fn empty_store() -> crate::Result<(tempfile::TempDir, file::Store)> {
         let dir = tempfile::TempDir::new().unwrap();
         let store = file::Store::at(dir.path(), git_ref::store::WriteReflog::Normal, git_hash::Kind::Sha1);
         Ok((dir, store))

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

@@ -1,4 +1,4 @@
-use crate::file::transaction::prepare_and_commit::{committer, create_at, delete_at, empty_store};
+use crate::file::transaction::prepare_and_commit::{committer, create_at, create_symbolic_at, delete_at, empty_store};
 use git_lock::acquire::Fail;
 use git_ref::file::transaction::PackedRefs;
 use git_ref::transaction::{Change, LogChange, PreviousValue, RefEdit};
@@ -89,7 +89,11 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
             Box::new(|_, _| Ok(Some(git_object::Kind::Commit))),
         ))
         .prepare(
-            [create_at("refs/a"), create_at("refs/A")],
+            [
+                create_at("refs/a"),
+                create_at("refs/A"),
+                create_symbolic_at("refs/symbolic", "refs/heads/target"),
+            ],
             Fail::Immediately,
             Fail::Immediately,
         )?
@@ -102,11 +106,12 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
     );
     assert_eq!(
         store.loose_iter()?.count(),
-        0,
-        "refs/ directory isn't present as there is no loose ref - it removed every up to the base dir"
+        1,
+        "symbolic refs can't be packed and stay loose"
     );
     assert!(store.reflog_exists("refs/a")?);
     assert!(store.reflog_exists("refs/A")?);
+    assert!(!store.reflog_exists("refs/symbolic")?, "and they can't have reflogs");
 
     // The following works because locks aren't actually obtained if there would be no change.
     // Otherwise there would be a conflict on case-insensitive filesystems
@@ -142,7 +147,7 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
             Fail::Immediately,
         )?
         .commit(committer().to_ref())?;
-    assert_eq!(store.iter()?.all()?.count(), 2);
+    assert_eq!(store.iter()?.all()?.count(), 3);
 
     {
         let _ongoing = store
@@ -179,7 +184,7 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
     }
 
     // Create a loose ref at a path
-    assert_eq!(store.loose_iter()?.count(), 0);
+    assert_eq!(store.loose_iter()?.count(), 1, "a symref");
     store
         .transaction()
         .prepare(
@@ -198,14 +203,14 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
         .commit(committer().to_ref())?;
     assert_eq!(
         store.loose_iter()?.count(),
-        1,
-        "we created a loose ref, overlaying the packed one"
+        2,
+        "we created a loose ref, overlaying the packed one, and have a symbolic one"
     );
 
     store
         .transaction()
         .prepare(
-            [delete_at("refs/a"), delete_at("refs/A")],
+            [delete_at("refs/a"), delete_at("refs/A"), delete_at("refs/symbolic")],
             Fail::Immediately,
             Fail::Immediately,
         )?