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 <mhagger@xxxxxxxxxxxx> --- 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