Assure reflogs aren't skipped just because there is no per-loose lock file.

The per-loose lock file isn't a requirement anymore as the packed-refs
lock can be used to enforce consistency, at least among
`gitoxide` powered tools.
Didn't actually check `git` does it similarly, but also couldn't
find any special behaviour related to clone and fetch ref updates.

Author: Byron

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

@@ -101,7 +101,6 @@ pub mod create_or_update {
         pub(crate) fn reflog_create_or_append(
             &self,
             name: &FullNameRef,
-            _lock: &git_lock::Marker,
             previous_oid: Option<ObjectId>,
             new: &oid,
             committer: git_actor::SignatureRef<'_>,

git-ref/src/store/file/loose/reflog/create_or_update/tests.rs

@@ -1,7 +1,6 @@
 use std::{convert::TryInto, path::Path};
 
 use git_actor::{Sign, Signature, Time};
-use git_lock::acquire::Fail;
 use git_object::bstr::ByteSlice;
 use git_testtools::hex_to_id;
 use tempfile::TempDir;
@@ -17,16 +16,6 @@ fn empty_store(writemode: WriteReflog) -> Result<(TempDir, file::Store)> {
     Ok((dir, store))
 }
 
-fn reflock(store: &file::Store, full_name: &str) -> Result<git_lock::Marker> {
-    let full_name: &FullNameRef = full_name.try_into()?;
-    git_lock::Marker::acquire_to_hold_resource(
-        store.reference_path(full_name),
-        Fail::Immediately,
-        Some(store.git_dir.clone()),
-    )
-    .map_err(Into::into)
-}
-
 fn reflog_lines(store: &file::Store, name: &str, buf: &mut Vec<u8>) -> Result<Vec<crate::log::Line>> {
     store
         .reflog_iter(name, buf)?
@@ -56,7 +45,6 @@ fn missing_reflog_creates_it_even_if_similarly_named_empty_dir_exists_and_append
         let (_keep, store) = empty_store(*mode)?;
         let full_name_str = "refs/heads/main";
         let full_name: &FullNameRef = full_name_str.try_into()?;
-        let lock = reflock(&store, full_name_str)?;
         let new = hex_to_id("28ce6a8b26aa170e1de65536fe8abe1832bd3242");
         let committer = Signature {
             name: "committer".into(),
@@ -69,7 +57,6 @@ fn missing_reflog_creates_it_even_if_similarly_named_empty_dir_exists_and_append
         };
         store.reflog_create_or_append(
             full_name,
-            &lock,
             None,
             &new,
             committer.to_ref(),
@@ -92,7 +79,6 @@ fn missing_reflog_creates_it_even_if_similarly_named_empty_dir_exists_and_append
                 let previous = hex_to_id("0000000000000000000000111111111111111111");
                 store.reflog_create_or_append(
                     full_name,
-                    &lock,
                     Some(previous),
                     &new,
                     committer.to_ref(),
@@ -123,14 +109,12 @@ fn missing_reflog_creates_it_even_if_similarly_named_empty_dir_exists_and_append
         // create onto existing directory
         let full_name_str = "refs/heads/other";
         let full_name: &FullNameRef = full_name_str.try_into()?;
-        let lock = reflock(&store, full_name_str)?;
         let reflog_path = store.reflog_path(full_name_str.try_into().expect("valid"));
         let directory_in_place_of_reflog = reflog_path.join("empty-a").join("empty-b");
         std::fs::create_dir_all(&directory_in_place_of_reflog)?;
 
         store.reflog_create_or_append(
             full_name,
-            &lock,
             None,
             &new,
             committer.to_ref(),

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

@@ -36,11 +36,7 @@ impl<'s, 'p> Transaction<'s, 'p> {
             match &change.update.change {
                 // reflog first, then reference
                 Change::Update { log, new, expected } => {
-                    let lock = match change.lock.take() {
-                        Some(l) => l,
-                        // Some updates are never locked as they are no-ops
-                        None => continue,
-                    };
+                    let lock = change.lock.take();
                     let (update_ref, update_reflog) = match log.mode {
                         RefLog::Only => (false, true),
                         RefLog::AndReference => (true, true),
@@ -71,7 +67,6 @@ impl<'s, 'p> Transaction<'s, 'p> {
                             if do_update {
                                 self.store.reflog_create_or_append(
                                     change.update.name.as_ref(),
-                                    &lock,
                                     previous,
                                     new_oid,
                                     committer,
@@ -85,11 +80,11 @@ impl<'s, 'p> Transaction<'s, 'p> {
                     // We delay deletion of the reference and dropping the lock to after the packed-refs were
                     // safely written.
                     if delete_loose_refs {
-                        change.lock = Some(lock);
+                        change.lock = lock;
                         continue;
                     }
-                    if update_ref && change.is_effective() {
-                        if let Err(err) = lock.commit() {
+                    if update_ref {
+                        if let Some(Err(err)) = lock.map(|l| l.commit()) {
                             // TODO: when Kind::IsADirectory becomes stable, use that.
                             let err = if err.instance.resource_path().is_dir() {
                                 git_tempfile::remove_dir::empty_depth_first(err.instance.resource_path())

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

@@ -2,7 +2,6 @@ use git_hash::ObjectId;
 use git_object::bstr::BString;
 use std::fmt::Formatter;
 
-use crate::transaction::{Change, PreviousValue};
 use crate::{
     store_impl::{file, file::Transaction},
     transaction::RefEdit,
@@ -52,19 +51,6 @@ impl Edit {
     fn name(&self) -> BString {
         self.update.name.0.clone()
     }
-
-    fn is_effective(&self) -> bool {
-        match &self.update.change {
-            Change::Update { new, expected, .. } => match expected {
-                PreviousValue::Any
-                | PreviousValue::MustExist
-                | PreviousValue::MustNotExist
-                | PreviousValue::ExistingMustMatch(_) => true,
-                PreviousValue::MustExistAndMatch(existing) => new_would_change_existing(new, existing),
-            },
-            Change::Delete { .. } => unreachable!("must not be called on deletions"),
-        }
-    }
 }
 
 fn new_would_change_existing(new: &Target, existing: &Target) -> bool {

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

@@ -4,6 +4,7 @@ pub(crate) mod prepare_and_commit {
     use git_object::bstr::BString;
     use git_ref::transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog};
     use git_ref::{file, Target};
+    use git_testtools::hex_to_id;
     use std::convert::TryInto;
 
     fn reflog_lines(store: &file::Store, name: &str) -> crate::Result<Vec<git_ref::log::Line>> {
@@ -46,9 +47,13 @@ pub(crate) mod prepare_and_commit {
     fn create_at(name: &str) -> RefEdit {
         RefEdit {
             change: Change::Update {
-                log: LogChange::default(),
+                log: LogChange {
+                    mode: RefLog::AndReference,
+                    force_create_reflog: true,
+                    message: "log peeled".into(),
+                },
                 expected: PreviousValue::MustNotExist,
-                new: Target::Peeled(git_hash::Kind::Sha1.null()),
+                new: Target::Peeled(hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391")),
             },
             name: name.try_into().expect("valid"),
             deref: false,

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

@@ -3,6 +3,7 @@ use git_lock::acquire::Fail;
 use git_ref::file::transaction::PackedRefs;
 use git_ref::transaction::{Change, LogChange, PreviousValue, RefEdit};
 use git_ref::Target;
+use git_testtools::hex_to_id;
 use std::convert::TryInto;
 
 fn case_sensitive(tmp_dir: &std::path::Path) -> bool {
@@ -21,7 +22,10 @@ fn conflicting_creation_without_packed_refs() -> crate::Result {
 
     let case_sensitive = case_sensitive(dir.path());
     match res {
-        Ok(_) if case_sensitive => {}
+        Ok(_) if case_sensitive => {
+            assert!(store.reflog_exists("refs/a")?);
+            assert!(store.reflog_exists("refs/A")?);
+        }
         Ok(_) if !case_sensitive => panic!("should fail as 'a' and 'A' clash"),
         Err(err) if case_sensitive => panic!(
             "should work as case sensitivity allows 'a' and 'A' to coexist: {:?}",
@@ -52,6 +56,9 @@ fn non_conflicting_creation_without_packed_refs_work() -> crate::Result {
     t2.commit(committer().to_ref())?;
     ongoing.commit(committer().to_ref())?;
 
+    assert!(store.reflog_exists("refs/new")?);
+    assert!(store.reflog_exists("refs/non-conflicting")?);
+
     Ok(())
 }
 
@@ -98,8 +105,11 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
         0,
         "refs/ directory isn't present as there is no loose ref - it removed every up to the base dir"
     );
+    assert!(store.reflog_exists("refs/a")?);
+    assert!(store.reflog_exists("refs/A")?);
 
     // 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
     store
         .transaction()
         .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
@@ -119,7 +129,9 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
                 RefEdit {
                     change: Change::Update {
                         log: LogChange::default(),
-                        expected: PreviousValue::MustExistAndMatch(Target::Peeled(git_hash::Kind::Sha1.null())),
+                        expected: PreviousValue::MustExistAndMatch(Target::Peeled(hex_to_id(
+                            "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391",
+                        ))),
                         new: Target::Peeled(git_hash::Kind::Sha1.null()),
                     },
                     name: "refs/A".try_into().expect("valid"),
@@ -204,5 +216,7 @@ fn conflicting_creation_into_packed_refs() -> crate::Result {
         0,
         "we deleted our only two packed refs and one loose ref with the same name"
     );
+    assert!(!store.reflog_exists("refs/a")?);
+    assert!(!store.reflog_exists("refs/A")?);
     Ok(())
 }