From: Michael Heemskerk <mheemskerk@xxxxxxxxxxxxx>
When deleting refs from the loose-files refs backend, files_delete_refs
first rewrites packed-refs if any of the to-be-deleted refs were packed
and then removes loose refs. While making those changes, it invokes the
reference-transaction hook incorrectly; a single transaction is used to
prepare and commit the changes to packed-refs, followed by another
transaction per deleted ref, each with another prepared and committed
reference-transaction hook invocation.
This behaviour is problematic for a number of reasons. First of all,
deleting a ref through `git branch -d` or `git tag -d` results in a
different sequence of reference-transaction callbacks than deleting the
same ref through `git update-ref`:
- update-ref of a loose ref: aborted, prepared, committed
- update-ref of a packed ref: prepared, prepared, committed, commited
- branch -d: prepared, committed, aborted, prepared, committed
The bigger problem is that the packed-refs update is committed before
the prepared reference-transaction invocation is done for the loose
ref. Returning a non-zero exit code from the second
reference-transaction callback leads to an invalid sequence of
reference-transaction callbacks:
1. prepared - hook returns 0, so the change is allowed to go through.
2. committed - git informs the hook that the changes are now final,
but are they really? Any loose refs may still survive if the
subsequent prepared callback is canceled.
3. aborted - 'fake' invocation from the packed-transaction because the
ref does not exist in packed-refs.
4. prepared - hook returns 1, so the change should be blocked.
5. aborted - git informs the hook that it has rolled back the change,
but it really hasn't; packed-refs has already been rewritten and
if the ref only existed as a packed ref, it is now gone.
The changes to the reference-transaction invocations that were planned
for git 2.36 but have been reverted make the problem more pronounced.
Those changes suppress the 'fake' invocations for the packed-transaction
(invocations 1-3 in the above list). In that case, the prepared callback
in step 4 cannot prevent a ref from being deleted if it only existed in
packed-refs.
To address the issue, the use a separate transactions to update the
packed and loose refs has been removed from files_delete_refs. Instead,
it now uses a single transaction, queues up the refs-to-be-deleted and
relies on the standard transaction mechanism to invoke the
reference-transaction hooks as expected.
Signed-off-by: Michael Heemskerk <mheemskerk@xxxxxxxxxxxxx>
---
refs: honor reference-transaction semantics when deleting refs
When deleting refs from the loose-files refs backend, files_delete_refs
first rewrites packed-refs if any of the to-be-deleted refs were packed
and then removes loose refs. While making those changes, it invokes the
reference-transaction hook incorrectly; a single transaction is used to
prepare and commit the changes to packed-refs, followed by another
transaction per deleted ref, each with another prepared and committed
reference-transaction hook invocation.
This behaviour is problematic for a number of reasons. First of all,
deleting a ref through git branch -d or git tag -d results in a
different sequence of reference-transaction callbacks than deleting the
same ref through git update-ref:
* update-ref of a loose ref: aborted, prepared, committed
* update-ref of a packed ref: prepared, prepared, committed, commited
* branch -d: prepared, committed, aborted, prepared, committed
The bigger problem is that the packed-refs update is committed before
the prepared reference-transaction invocation is done for the loose ref.
Returning a non-zero exit code from the second reference-transaction
callback leads to an invalid sequence of reference-transaction
callbacks:
1. prepared - hook returns 0, so the change is allowed to go through.
2. committed - git informs the hook that the changes are now final, but
are they really? Any loose refs may still survive if the subsequent
prepared callback is canceled.
3. aborted - 'fake' invocation from the packed-transaction because the
ref does not exist in packed-refs.
4. prepared - hook returns 1, so the change should be blocked.
5. aborted - git informs the hook that it has rolled back the change,
but it really hasn't; packed-refs has already been rewritten and if
the ref only existed as a packed ref, it is now gone.
The changes to the reference-transaction invocations that were planned
for git 2.36 but have been reverted make the problem more pronounced.
Those changes suppress the 'fake' invocations for the packed-transaction
(invocations 1-3 in the above list). In that case, the prepared callback
in step 4 cannot prevent a ref from being deleted if it only existed in
packed-refs.
To address the issue, the use a separate transactions to update the
packed and loose refs has been removed from files_delete_refs. Instead,
it now uses a single transaction, queues up the refs-to-be-deleted and
relies on the standard transaction mechanism to invoke the
reference-transaction hooks as expected.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1228%2Fmheemskerk%2Ffiles-delete-ref-reference-transaction-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1228/mheemskerk/files-delete-ref-reference-transaction-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1228
refs/files-backend.c | 33 +++++++--------
t/t1416-ref-transaction-hooks.sh | 70 ++++++++++++++++++++++++++++++++
t/t5510-fetch.sh | 6 +--
3 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 8db7882aacb..5c23277eda7 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1265,44 +1265,39 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
struct strbuf err = STRBUF_INIT;
- int i, result = 0;
+ int i;
+ struct ref_transaction *transaction;
if (!refnames->nr)
return 0;
- if (packed_refs_lock(refs->packed_ref_store, 0, &err))
- goto error;
-
- if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
- packed_refs_unlock(refs->packed_ref_store);
+ transaction = ref_store_transaction_begin(&refs->base, &err);
+ if (!transaction)
goto error;
- }
-
- packed_refs_unlock(refs->packed_ref_store);
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
-
- if (refs_delete_ref(&refs->base, msg, refname, NULL, flags))
- result |= error(_("could not remove reference %s"), refname);
+ if (ref_transaction_delete(transaction, refname, NULL,
+ flags, msg, &err))
+ goto error;
}
+ if (ref_transaction_commit(transaction, &err))
+ goto error;
+
+ ref_transaction_free(transaction);
strbuf_release(&err);
- return result;
+ return 0;
error:
- /*
- * If we failed to rewrite the packed-refs file, then it is
- * unsafe to try to remove loose refs, because doing so might
- * expose an obsolete packed value for a reference that might
- * even point at an object that has been garbage collected.
- */
if (refnames->nr == 1)
error(_("could not delete reference %s: %s"),
refnames->items[0].string, err.buf);
else
error(_("could not delete references: %s"), err.buf);
+ if (transaction)
+ ref_transaction_free(transaction);
strbuf_release(&err);
return -1;
}
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 27731722a5b..e3d4fe624f7 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -133,4 +133,74 @@ test_expect_success 'interleaving hook calls succeed' '
test_cmp expect target-repo.git/actual
'
+test_expect_success 'hook allows deleting loose ref if successful' '
+ test_when_finished "rm actual" &&
+ git branch to-be-deleted $PRE_OID &&
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ EOF
+ cat >expect <<-EOF &&
+ aborted
+ prepared
+ committed
+ EOF
+ git branch -d to-be-deleted &&
+ test_cmp expect actual &&
+ test_must_fail git rev-parse refs/heads/to-be-deleted
+'
+
+test_expect_success 'hook allows deleting packed ref if successful' '
+ test_when_finished "rm actual" &&
+ git branch to-be-deleted $PRE_OID &&
+ git pack-refs --all --prune &&
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ EOF
+ cat >expect <<-EOF &&
+ prepared
+ prepared
+ committed
+ committed
+ EOF
+ git branch -d to-be-deleted &&
+ test_cmp expect actual &&
+ test_must_fail git rev-parse refs/heads/to-be-deleted
+'
+
+test_expect_success 'hook aborts deleting loose ref in prepared state' '
+ test_when_finished "rm actual" &&
+ test_when_finished "git branch -d to-be-deleted" &&
+ git branch to-be-deleted $PRE_OID &&
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ exit 1
+ EOF
+ cat >expect <<-EOF &&
+ aborted
+ prepared
+ aborted
+ EOF
+ test_must_fail git branch -d to-be-deleted &&
+ test_cmp expect actual &&
+ git rev-parse refs/heads/to-be-deleted
+'
+
+test_expect_success 'hook aborts deleting packed ref in prepared state' '
+ test_when_finished "rm actual" &&
+ test_when_finished "git branch -d to-be-deleted" &&
+ git branch to-be-deleted $PRE_OID &&
+ git pack-refs --all --prune &&
+ test_hook reference-transaction <<-\EOF &&
+ echo "$*" >>actual
+ exit 1
+ EOF
+ cat >expect <<-EOF &&
+ prepared
+ aborted
+ EOF
+ test_must_fail git branch -d to-be-deleted &&
+ test_cmp expect actual &&
+ git rev-parse refs/heads/to-be-deleted
+'
+
test_done
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 4620f0ca7fa..8b09a99c2e8 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -169,10 +169,10 @@ test_expect_success REFFILES 'fetch --prune fails to delete branches' '
git clone . prune-fail &&
cd prune-fail &&
git update-ref refs/remotes/origin/extrabranch main &&
- : this will prevent --prune from locking packed-refs for deleting refs, but adding loose refs still succeeds &&
- >.git/packed-refs.new &&
+ : this will prevent --prune from locking refs/remotes/origin/extra for deletion &&
+ >.git/refs/remotes/origin/extrabranch.lock &&
- test_must_fail git fetch --prune origin
+ test_must_fail git fetch --prune origin > outputs 2> errors
'
test_expect_success 'fetch --atomic works with a single branch' '
base-commit: 0f828332d5ac36fc63b7d8202652efa152809856
--
gitgitgadget
