Merge branch 'mh/avoid-rewriting-packed-refs'
Recent update to the refs infrastructure implementation started rewriting packed-refs file more often than before; this has been optimized again for most trivial cases. * mh/avoid-rewriting-packed-refs: files-backend: don't rewrite the `packed-refs` file unnecessarily t1409: check that `packed-refs` is not rewritten unnecessarily
This commit is contained in:
Junio C Hamano
commit
ffb0b5762e
@ -2606,7 +2606,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_needed(refs->packed_ref_store,
|
||||
packed_transaction)) {
|
||||
ret = ref_transaction_prepare(packed_transaction, err);
|
||||
} else {
|
||||
/*
|
||||
* 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;
|
||||
}
|
||||
}
|
||||
|
||||
cleanup:
|
||||
|
||||
@ -1261,6 +1261,100 @@ error:
|
||||
return -1;
|
||||
}
|
||||
|
||||
int is_packed_transaction_needed(struct ref_store *ref_store,
|
||||
struct ref_transaction *transaction)
|
||||
{
|
||||
struct packed_ref_store *refs = packed_downcast(
|
||||
ref_store,
|
||||
REF_STORE_READ,
|
||||
"is_packed_transaction_needed");
|
||||
struct strbuf referent = STRBUF_INIT;
|
||||
size_t i;
|
||||
int ret;
|
||||
|
||||
if (!is_lock_file_locked(&refs->lock))
|
||||
BUG("is_packed_transaction_needed() called while unlocked");
|
||||
|
||||
/*
|
||||
* We're only going to bother returning false 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). This gives
|
||||
* false positives for some other cases that could
|
||||
* theoretically be optimized away:
|
||||
*
|
||||
* 1. It could be that the old value is being verified without
|
||||
* setting a new value. In this case, we could verify the
|
||||
* old value here and skip the update if it agrees. If it
|
||||
* disagrees, we could either let the update go through
|
||||
* (the actual commit would re-detect and report the
|
||||
* problem), or come up with a way of reporting such an
|
||||
* error to *our* caller.
|
||||
*
|
||||
* 2. It could be that a new value is being set, but that it
|
||||
* is identical to the current packed value of the
|
||||
* reference.
|
||||
*
|
||||
* Neither of these cases will come up in the current code,
|
||||
* because the only caller of this function passes to it a
|
||||
* transaction that only includes `delete` updates with no
|
||||
* `old_id`. Even if that ever changes, false positives only
|
||||
* cause an optimization to be missed; they do not affect
|
||||
* correctness.
|
||||
*/
|
||||
|
||||
/*
|
||||
* Start with the cheap checks that don't require old
|
||||
* reference values to be read:
|
||||
*/
|
||||
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 -> needed. */
|
||||
return 1;
|
||||
|
||||
if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))
|
||||
/* Have to set a new value -> needed. */
|
||||
return 1;
|
||||
}
|
||||
|
||||
/*
|
||||
* The transaction isn't checking any old values nor is it
|
||||
* setting any nonzero new values, so it still might be able
|
||||
* to be skipped. Now do the more expensive check: the update
|
||||
* is needed if any of the updates is a delete, and the old
|
||||
* `packed-refs` file contains a value for that reference.
|
||||
*/
|
||||
ret = 0;
|
||||
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 -> not
|
||||
* needed.
|
||||
*/
|
||||
continue;
|
||||
|
||||
if (!refs_read_raw_ref(ref_store, update->refname,
|
||||
&oid, &referent, &type) ||
|
||||
errno != ENOENT) {
|
||||
/*
|
||||
* We have to actually delete that reference
|
||||
* -> this transaction is needed.
|
||||
*/
|
||||
ret = 1;
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
strbuf_release(&referent);
|
||||
return ret;
|
||||
}
|
||||
|
||||
struct packed_transaction_backend_data {
|
||||
/* True iff the transaction owns the packed-refs lock. */
|
||||
int own_lock;
|
||||
|
||||
@ -23,4 +23,13 @@ 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` really needs to be carried out against
|
||||
* the specified packed_ref_store, or false if it can be skipped
|
||||
* (i.e., because it is an obvious NOOP). `ref_store` must be locked
|
||||
* before calling this function.
|
||||
*/
|
||||
int is_packed_transaction_needed(struct ref_store *ref_store,
|
||||
struct ref_transaction *transaction);
|
||||
|
||||
#endif /* REFS_PACKED_BACKEND_H */
|
||||
|
||||
118
t/t1409-avoid-packing-refs.sh
Executable file
118
t/t1409-avoid-packing-refs.sh
Executable file
@ -0,0 +1,118 @@
|
||||
#!/bin/sh
|
||||
|
||||
test_description='avoid rewriting packed-refs unnecessarily'
|
||||
|
||||
. ./test-lib.sh
|
||||
|
||||
# Add an identifying mark to the packed-refs file header line. This
|
||||
# shouldn't upset readers, and it should be omitted if the file is
|
||||
# ever rewritten.
|
||||
mark_packed_refs () {
|
||||
sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
|
||||
mv .git/packed-refs.new .git/packed-refs
|
||||
}
|
||||
|
||||
# Verify that the packed-refs file is still marked.
|
||||
check_packed_refs_marked () {
|
||||
grep -q '^#.* t1409 ' .git/packed-refs
|
||||
}
|
||||
|
||||
test_expect_success 'setup' '
|
||||
git commit --allow-empty -m "Commit A" &&
|
||||
A=$(git rev-parse HEAD) &&
|
||||
git commit --allow-empty -m "Commit B" &&
|
||||
B=$(git rev-parse HEAD) &&
|
||||
git commit --allow-empty -m "Commit C" &&
|
||||
C=$(git rev-parse HEAD)
|
||||
'
|
||||
|
||||
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 &&
|
||||
git update-ref refs/heads/foo $B &&
|
||||
test_must_fail test -f .git/packed-refs &&
|
||||
git update-ref refs/heads/foo $C $B &&
|
||||
test_must_fail test -f .git/packed-refs &&
|
||||
git update-ref -d refs/heads/foo &&
|
||||
test_must_fail test -f .git/packed-refs
|
||||
'
|
||||
|
||||
test_expect_success 'check that marking the packed-refs file works' '
|
||||
git for-each-ref >expected &&
|
||||
git pack-refs --all &&
|
||||
mark_packed_refs &&
|
||||
check_packed_refs_marked &&
|
||||
git for-each-ref >actual &&
|
||||
test_cmp expected actual &&
|
||||
git pack-refs --all &&
|
||||
test_must_fail check_packed_refs_marked &&
|
||||
git for-each-ref >actual2 &&
|
||||
test_cmp expected actual2
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on update of packed' '
|
||||
git update-ref refs/heads/packed-update $A &&
|
||||
git pack-refs --all &&
|
||||
mark_packed_refs &&
|
||||
git update-ref refs/heads/packed-update $B &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on checked update of packed' '
|
||||
git update-ref refs/heads/packed-checked-update $A &&
|
||||
git pack-refs --all &&
|
||||
mark_packed_refs &&
|
||||
git update-ref refs/heads/packed-checked-update $B $A &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on verify of packed' '
|
||||
git update-ref refs/heads/packed-verify $A &&
|
||||
git pack-refs --all &&
|
||||
mark_packed_refs &&
|
||||
echo "verify refs/heads/packed-verify $A" | git update-ref --stdin &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'touch packed-refs on delete of packed' '
|
||||
git update-ref refs/heads/packed-delete $A &&
|
||||
git pack-refs --all &&
|
||||
mark_packed_refs &&
|
||||
git update-ref -d refs/heads/packed-delete &&
|
||||
test_must_fail check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on update of loose' '
|
||||
git pack-refs --all &&
|
||||
git update-ref refs/heads/loose-update $A &&
|
||||
mark_packed_refs &&
|
||||
git update-ref refs/heads/loose-update $B &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on checked update of loose' '
|
||||
git pack-refs --all &&
|
||||
git update-ref refs/heads/loose-checked-update $A &&
|
||||
mark_packed_refs &&
|
||||
git update-ref refs/heads/loose-checked-update $B $A &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_expect_success 'leave packed-refs untouched on verify of loose' '
|
||||
git pack-refs --all &&
|
||||
git update-ref refs/heads/loose-verify $A &&
|
||||
mark_packed_refs &&
|
||||
echo "verify refs/heads/loose-verify $A" | git update-ref --stdin &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
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 &&
|
||||
git update-ref -d refs/heads/loose-delete &&
|
||||
check_packed_refs_marked
|
||||
'
|
||||
|
||||
test_done
|
||||
Loading…
Reference in New Issue
Block a user