fix: assure memory maps don't get in the way of updating .git/packet-refs on Windows. (#1183)

Byron · Byron · commit 6c1da71566a4 · 2023-12-15T10:30:22.000+01:00
The solution entails to not use memory maps on Windows which is certainly fine
on todays systems, even though it's likely to add latency.
diff --git a/gix-ref/src/store/file/loose/mod.rs b/gix-ref/src/store/file/loose/mod.rs
@@ -36,7 +36,7 @@ mod init {
         pub fn at(git_dir: PathBuf, write_reflog: file::WriteReflog, object_hash: gix_hash::Kind) -> Self {
             file::Store {
                 git_dir,
-                packed_buffer_mmap_threshold: 32 * 1024,
+                packed_buffer_mmap_threshold: packed_refs_mmap_threshold(),
                 common_dir: None,
                 write_reflog,
                 namespace: None,
@@ -55,7 +55,7 @@ mod init {
         ) -> Self {
             file::Store {
                 git_dir,
-                packed_buffer_mmap_threshold: 32 * 1024,
+                packed_buffer_mmap_threshold: packed_refs_mmap_threshold(),
                 common_dir: Some(common_dir),
                 write_reflog,
                 namespace: None,
@@ -64,4 +64,12 @@ mod init {
             }
         }
     }
+
+    fn packed_refs_mmap_threshold() -> u64 {
+        if cfg!(windows) {
+            u64::MAX
+        } else {
+            32 * 1024
+        }
+    }
 }
diff --git a/gix-ref/tests/file/store/access.rs b/gix-ref/tests/file/store/access.rs
@@ -0,0 +1,22 @@
+use crate::file::store;
+
+#[test]
+fn set_packed_buffer_mmap_threshold() -> crate::Result {
+    let mut store = store()?;
+    let prev = store.set_packed_buffer_mmap_threshold(0);
+    if cfg!(windows) {
+        assert_eq!(
+            prev,
+            u64::MAX,
+            "on windows mmap are deactivated as otherwise we can't change packed-refs while it's mapped"
+        );
+    } else {
+        assert_eq!(prev, 32 * 1024, "the default is the value that Git uses as well");
+    }
+    assert_eq!(
+        store.set_packed_buffer_mmap_threshold(0),
+        0,
+        "it actually sets the value"
+    );
+    Ok(())
+}
diff --git a/gix-ref/tests/file/store/mod.rs b/gix-ref/tests/file/store/mod.rs
@@ -1,3 +1,4 @@
+mod access;
 mod find;
 mod iter;
 mod reflog;
diff --git a/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs b/gix-ref/tests/file/transaction/prepare_and_commit/create_or_update/mod.rs
@@ -653,52 +653,47 @@ fn write_reference_to_which_head_points_to_does_not_update_heads_reflog_even_tho
 
 #[test]
 fn packed_refs_are_looked_up_when_checking_existing_values() -> crate::Result {
-    for use_mmap in [false, true] {
-        let (_keep, mut store) = store_writable("make_packed_ref_repository.sh")?;
-        if use_mmap {
-            store.set_packed_buffer_mmap_threshold(0);
-        }
-        assert!(
-            store.try_find_loose("main")?.is_none(),
-            "no loose main available, it's packed"
-        );
-        let new_id = hex_to_id("0000000000000000000000000000000000000001");
-        let old_id = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
-        let edits = store
-            .transaction()
-            .prepare(
-                Some(RefEdit {
-                    change: Change::Update {
-                        log: LogChange {
-                            mode: RefLog::AndReference,
-                            force_create_reflog: false,
-                            message: "for pack".into(),
-                        },
-                        expected: PreviousValue::MustExistAndMatch(Target::Peeled(old_id)),
-                        new: Target::Peeled(new_id),
+    let (_keep, store) = store_writable("make_packed_ref_repository.sh")?;
+    assert!(
+        store.try_find_loose("main")?.is_none(),
+        "no loose main available, it's packed"
+    );
+    let new_id = hex_to_id("0000000000000000000000000000000000000001");
+    let old_id = hex_to_id("134385f6d781b7e97062102c6a483440bfda2a03");
+    let edits = store
+        .transaction()
+        .prepare(
+            Some(RefEdit {
+                change: Change::Update {
+                    log: LogChange {
+                        mode: RefLog::AndReference,
+                        force_create_reflog: false,
+                        message: "for pack".into(),
                     },
-                    name: "refs/heads/main".try_into()?,
-                    deref: false,
-                }),
-                Fail::Immediately,
-                Fail::Immediately,
-            )?
-            .commit(committer().to_ref())?;
+                    expected: PreviousValue::MustExistAndMatch(Target::Peeled(old_id)),
+                    new: Target::Peeled(new_id),
+                },
+                name: "refs/heads/main".try_into()?,
+                deref: false,
+            }),
+            Fail::Immediately,
+            Fail::Immediately,
+        )?
+        .commit(committer().to_ref())?;
 
-        assert_eq!(edits.len(), 1, "only one edit was performed in the loose refs store");
+    assert_eq!(edits.len(), 1, "only one edit was performed in the loose refs store");
 
-        let packed = store.open_packed_buffer().unwrap().expect("packed refs is available");
-        assert_eq!(
+    let packed = store.open_packed_buffer().unwrap().expect("packed refs is available");
+    assert_eq!(
             packed.find("main")?.target(),
             old_id,
             "packed refs aren't rewritten, the change goes into the loose ref instead which shadows packed refs of same name"
         );
-        assert_eq!(
-            store.find_loose("main")?.target.try_id(),
-            Some(new_id.as_ref()),
-            "the new id was written to the loose ref"
-        );
-    }
+    assert_eq!(
+        store.find_loose("main")?.target.try_id(),
+        Some(new_id.as_ref()),
+        "the new id was written to the loose ref"
+    );
     Ok(())
 }
 
@@ -709,130 +704,114 @@ fn packed_refs_creation_with_tag_loop_are_not_handled_and_cannot_exist_due_to_ob
 
 #[test]
 fn packed_refs_creation_with_packed_refs_mode_prune_removes_original_loose_refs() -> crate::Result {
-    for use_mmap in [false, true] {
-        let (_keep, mut store) = store_writable("make_ref_repository.sh")?;
-        if use_mmap {
-            let prev = store.set_packed_buffer_mmap_threshold(0);
-            assert_eq!(prev, 32 * 1024, "the default is the value that Git uses as well");
-            assert_eq!(
-                store.set_packed_buffer_mmap_threshold(0),
-                0,
-                "it actually sets the value"
-            );
-        }
-        assert!(
-            store.open_packed_buffer()?.is_none(),
-            "there should be no packed refs to start out with"
-        );
-        let odb = gix_odb::at(store.git_dir().join("objects"))?;
-        let edits = store
-            .transaction()
-            .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
-                Box::new(odb),
-            ))
-            .prepare(
-                store
-                    .loose_iter()?
-                    .filter_map(|r| r.ok().filter(|r| r.kind() == gix_ref::Kind::Peeled))
-                    .map(|r| RefEdit {
-                        change: Change::Update {
-                            log: LogChange::default(),
-                            expected: PreviousValue::MustExistAndMatch(r.target.clone()),
-                            new: r.target,
-                        },
-                        name: r.name,
-                        deref: false,
-                    }),
-                Fail::Immediately,
-                Fail::Immediately,
-            )?
-            .commit(committer().to_ref())?;
-
-        assert_eq!(
-            edits.len(),
-            8,
-            "there are a certain amount of loose refs that are packed"
-        );
-
-        assert!(
+    let (_keep, store) = store_writable("make_ref_repository.sh")?;
+    assert!(
+        store.open_packed_buffer()?.is_none(),
+        "there should be no packed refs to start out with"
+    );
+    let odb = gix_odb::at(store.git_dir().join("objects"))?;
+    let edits = store
+        .transaction()
+        .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdatesRemoveLooseSourceReference(
+            Box::new(odb),
+        ))
+        .prepare(
             store
                 .loose_iter()?
-                .filter_map(Result::ok)
-                .all(|r| r.kind() == gix_ref::Kind::Symbolic),
-            "only symbolic refs are left"
-        );
+                .filter_map(|r| r.ok().filter(|r| r.kind() == gix_ref::Kind::Peeled))
+                .map(|r| RefEdit {
+                    change: Change::Update {
+                        log: LogChange::default(),
+                        expected: PreviousValue::MustExistAndMatch(r.target.clone()),
+                        new: r.target,
+                    },
+                    name: r.name,
+                    deref: false,
+                }),
+            Fail::Immediately,
+            Fail::Immediately,
+        )?
+        .commit(committer().to_ref())?;
 
-        let other_store = store_with_packed_refs()?;
-        let expected_pack_data: BString = std::fs::read(other_store.packed_refs_path())?.into();
-        let actual_packed_data: BString = std::fs::read(store.packed_refs_path())?.into();
-        assert_eq!(
-            actual_packed_data, expected_pack_data,
-            "both gitoxide and git must agree on the packed refs file perfectly"
-        );
-    }
+    assert_eq!(
+        edits.len(),
+        8,
+        "there are a certain amount of loose refs that are packed"
+    );
+
+    assert!(
+        store
+            .loose_iter()?
+            .filter_map(Result::ok)
+            .all(|r| r.kind() == gix_ref::Kind::Symbolic),
+        "only symbolic refs are left"
+    );
+
+    let other_store = store_with_packed_refs()?;
+    let expected_pack_data: BString = std::fs::read(other_store.packed_refs_path())?.into();
+    let actual_packed_data: BString = std::fs::read(store.packed_refs_path())?.into();
+    assert_eq!(
+        actual_packed_data, expected_pack_data,
+        "both gitoxide and git must agree on the packed refs file perfectly"
+    );
     Ok(())
 }
 
 #[test]
 fn packed_refs_creation_with_packed_refs_mode_leave_keeps_original_loose_refs() -> crate::Result {
-    for use_mmap in [false, true] {
-        let (_keep, mut store) = store_writable("make_packed_ref_repository_for_overlay.sh")?;
-        if use_mmap {
-            store.set_packed_buffer_mmap_threshold(0);
-        }
-        let branch = store.find("newer-as-loose")?;
-        let packed = store.open_packed_buffer()?.expect("packed-refs");
-        assert_ne!(
-            packed.find("newer-as-loose")?.target(),
-            branch.target.try_id().expect("peeled"),
-            "the packed ref is outdated"
-        );
-        let previous_reflog_entries = branch.log_iter(&store).all()?.expect("log").count();
-        let previous_packed_refs = packed.iter()?.filter_map(Result::ok).count();
-
-        let edits = store.loose_iter()?.map(|r| r.expect("valid ref")).map(|r| RefEdit {
-            change: Change::Update {
-                log: LogChange::default(),
-                expected: PreviousValue::MustExistAndMatch(r.target.clone()),
-                new: r.target,
-            },
-            name: r.name,
-            deref: false,
-        });
+    let (_keep, store) = store_writable("make_packed_ref_repository_for_overlay.sh")?;
+    let branch = store.find("newer-as-loose")?;
+    let packed = store.open_packed_buffer()?.expect("packed-refs");
+    assert_ne!(
+        packed.find("newer-as-loose")?.target(),
+        branch.target.try_id().expect("peeled"),
+        "the packed ref is outdated"
+    );
+    let previous_reflog_entries = branch.log_iter(&store).all()?.expect("log").count();
+    let previous_packed_refs = packed.iter()?.filter_map(Result::ok).count();
+
+    let edits = store.loose_iter()?.map(|r| r.expect("valid ref")).map(|r| RefEdit {
+        change: Change::Update {
+            log: LogChange::default(),
+            expected: PreviousValue::MustExistAndMatch(r.target.clone()),
+            new: r.target,
+        },
+        name: r.name,
+        deref: false,
+    });
 
-        let edits = store
-            .transaction()
-            .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(EmptyCommit)))
-            .prepare(edits, Fail::Immediately, Fail::Immediately)?
-            .commit(committer().to_ref())?;
-        assert_eq!(
+    let edits = store
+        .transaction()
+        .packed_refs(PackedRefs::DeletionsAndNonSymbolicUpdates(Box::new(EmptyCommit)))
+        .prepare(edits, Fail::Immediately, Fail::Immediately)?
+        .commit(committer().to_ref())?;
+    assert_eq!(
             edits.len(),
             2,
             "it claims to have performed all desired operations, even though some don't make it into the pack as 'side-car'"
         );
 
-        assert_eq!(
-            store.loose_iter()?.filter_map(Result::ok).count(),
-            edits.len(),
-            "the amount of loose refs didn't change and having symbolic ones isn't a problem"
-        );
-        assert_eq!(
-            branch.log_iter(&store).all()?.expect("log").count(),
-            previous_reflog_entries,
-            "reflog isn't adjusted as there is no change"
-        );
+    assert_eq!(
+        store.loose_iter()?.filter_map(Result::ok).count(),
+        edits.len(),
+        "the amount of loose refs didn't change and having symbolic ones isn't a problem"
+    );
+    assert_eq!(
+        branch.log_iter(&store).all()?.expect("log").count(),
+        previous_reflog_entries,
+        "reflog isn't adjusted as there is no change"
+    );
 
-        let packed = store.open_packed_buffer()?.expect("packed-refs");
-        assert_eq!(
-            packed.iter()?.filter_map(Result::ok).count(),
-            previous_packed_refs,
-            "the amount of packed refs doesn't change"
-        );
-        assert_eq!(
-            packed.find("newer-as-loose")?.target(),
-            store.find("newer-as-loose")?.target.into_id(),
-            "the packed ref is now up to date and the loose ref definitely still exists"
-        );
-    }
+    let packed = store.open_packed_buffer()?.expect("packed-refs");
+    assert_eq!(
+        packed.iter()?.filter_map(Result::ok).count(),
+        previous_packed_refs,
+        "the amount of packed refs doesn't change"
+    );
+    assert_eq!(
+        packed.find("newer-as-loose")?.target(),
+        store.find("newer-as-loose")?.target.into_id(),
+        "the packed ref is now up to date and the loose ref definitely still exists"
+    );
     Ok(())
 }
diff --git a/gix-ref/tests/file/transaction/prepare_and_commit/delete.rs b/gix-ref/tests/file/transaction/prepare_and_commit/delete.rs
