Even when we are deleting references, we needn't overwrite the
`packed-refs` file if the references that we are deleting are not
present in that file. Implement this optimization as follows:
* Add a function `is_packed_transaction_noop()`, which checks whether
a given packed-refs transaction doesn't actually have to do
anything. This function must be called while holding the
`packed-refs` lock to avoid races.
* Change `files_transaction_prepare()` to check whether the
packed-refs transaction is unneeded. If so, squelch it, but continue
holding the `packed-refs` lock until the end of the transaction to
avoid races.
This fixes two tests in t1409.
Signed-off-by: Michael Haggerty <mhag...@alum.mit.edu>
---
refs/files-backend.c | 18 +++++++++++-
refs/packed-backend.c | 68 +++++++++++++++++++++++++++++++++++++++++++
refs/packed-backend.h | 8 +++++
t/t1409-avoid-packing-refs.sh | 4 +--
4 files changed, 95 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 014dabb0bf..5689e3a58d 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2605,7 +2605,23 @@ static int files_transaction_prepare(struct ref_store
*ref_store,
goto cleanup;
}
backend_data->packed_refs_locked = 1;
- ret = ref_transaction_prepare(packed_transaction, err);
+
+ if (is_packed_transaction_noop(refs->packed_ref_store,
+ packed_transaction)) {
+ /*
+ * We can skip rewriting the `packed-refs`
+ * file. But we do need to leave it locked, so
+ * that somebody else doesn't pack a reference
+ * that we are trying to delete.
+ */
+ if (ref_transaction_abort(packed_transaction, err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
+ backend_data->packed_transaction = NULL;
+ } else {
+ ret = ref_transaction_prepare(packed_transaction, err);
+ }
}
cleanup:
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 3279d42c5a..064b1b58a2 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1261,6 +1261,74 @@ static int write_with_updates(struct packed_ref_store
*refs,
return -1;
}
+int is_packed_transaction_noop(struct ref_store *ref_store,
+ struct ref_transaction *transaction)
+{
+ struct packed_ref_store *refs = packed_downcast(
+ ref_store,
+ REF_STORE_READ,
+ "is_packed_transaction_noop");
+ struct strbuf referent = STRBUF_INIT;
+ size_t i;
+ int ret;
+
+ if (!is_lock_file_locked(&refs->lock))
+ BUG("is_packed_transaction_noop() called while unlocked");
+
+ /*
+ * We're only going to bother returning true for the common,
+ * trivial case that references are only being deleted, their
+ * old values are not being checked, and the old `packed-refs`
+ * file doesn't contain any of those reference(s). More
+ * complicated cases (1) are unlikely to be able to be
+ * optimized away anyway, and (2) are more expensive to check.
+ * Start with cheap checks:
+ */
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+
+ if (update->flags & REF_HAVE_OLD)
+ /* Have to check the old value -> not a NOOP. */
+ return 0;
+
+ if ((update->flags & REF_HAVE_NEW) &&
!is_null_oid(&update->new_oid))
+ /* Have to set a new value -> not a NOOP. */
+ return 0;
+ }
+
+ /*
+ * The transaction isn't checking any old values nor is it
+ * setting any nonzero new values, so it still might be a
+ * NOOP. Now do the more expensive check: the update is not a
+ * NOOP if one of the updates is a delete, and the old
+ * `packed-refs` file contains a value for that reference.
+ */
+ ret = 1;
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+ unsigned int type;
+ struct object_id oid;
+
+ if (!(update->flags & REF_HAVE_NEW))
+ /* This reference isn't being deleted -> NOOP. */
+ continue;
+
+ if (!refs_read_raw_ref(ref_store, update->refname,
+ oid.hash, &referent, &type) ||
+ errno != ENOENT) {
+ /*
+ * We might have to actually delete that
+ * reference -> not a NOOP.
+ */
+ ret = 0;
+ break;
+ }
+ }
+
+ strbuf_release(&referent);
+ return ret;
+}
+
struct packed_transaction_backend_data {
/* True iff the transaction owns the packed-refs lock. */
int own_lock;
diff --git a/refs/packed-backend.h b/refs/packed-backend.h
index 61687e408a..0b8b2b9695 100644
--- a/refs/packed-backend.h
+++ b/refs/packed-backend.h
@@ -23,4 +23,12 @@ int packed_refs_lock(struct ref_store *ref_store, int flags,
struct strbuf *err)
void packed_refs_unlock(struct ref_store *ref_store);
int packed_refs_is_locked(struct ref_store *ref_store);
+/*
+ * Return true if `transaction` is an obvious NOOP with respect to the
+ * specified packed_ref_store. `ref_store` must be locked before
+ * calling this function.
+ */
+int is_packed_transaction_noop(struct ref_store *ref_store,
+ struct ref_transaction *transaction);
+
#endif /* REFS_PACKED_BACKEND_H */
diff --git a/t/t1409-avoid-packing-refs.sh b/t/t1409-avoid-packing-refs.sh
index a2397c7b71..e5cb8a252d 100755
--- a/t/t1409-avoid-packing-refs.sh
+++ b/t/t1409-avoid-packing-refs.sh
@@ -26,7 +26,7 @@ test_expect_success 'setup' '
C=$(git rev-parse HEAD)
'
-test_expect_failure 'do not create packed-refs file gratuitously' '
+test_expect_success 'do not create packed-refs file gratuitously' '
test_must_fail test -f .git/packed-refs &&
git update-ref refs/heads/foo $A &&
test_must_fail test -f .git/packed-refs &&
@@ -107,7 +107,7 @@ test_expect_success 'leave packed-refs untouched on verify
of loose' '
check_packed_refs_marked
'
-test_expect_failure 'leave packed-refs untouched on delete of loose' '
+test_expect_success 'leave packed-refs untouched on delete of loose' '
git pack-refs --all &&
git update-ref refs/heads/loose-delete $A &&
mark_packed_refs &&
--
2.14.1
