diff mbox series

[v2,9/9] refs: reimplement refs_delete_refs() and run hook once

Message ID 20220819032147.28841-10-worldhello.net@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fix issues of refx-txn hook for various git commands | expand

Commit Message

Jiang Xin Aug. 19, 2022, 3:21 a.m. UTC 
From: Jiang Xin <zhiyou.jx@alibaba-inc.com>

When using "git branch -d" or "git tag -d" to delete one or multiple
references, command "reference-transaction committed" will be called
repeatedly for the same references. This is because the function
"refs_delete_refs()" is called twice, once for loose ref-store and once
for packed ref-store.

The old story starts when running the function "refs_delete_refs()" on a
loose ref-store:

 1. Try to remove the references from packed ref-store.

    1.1. Lock the packed-ref-store by calling "packed_refs_lock()" in
         "files_delete_refs()".

    1.2. Call "refs_delete_refs()" on packed-ref-store, and then call
         "packed_delete_refs()".

    1.3. Create a transaction for packed-ref-store in function
         "packed_delete_refs()" by calling the function
         "ref_store_transaction_begin()".

    2.2. Add update entries for all the references to be removed into
         this transaction by calling "ref_transaction_delete()".

    2.3. Call "ref_transaction_commit()" to commit the transaction.

    2.4. Unlock the packed-ref-store.

 2. Try to remove the references one by one by calling the function
    "refs_delete_ref()".

    2.1. Create a new transaction on loose-ref-store by calling
         "ref_store_transaction_begin()".

    2.2. Call "ref_transaction_delete()" to add a update entry
         for the reference to be deleted into the transaction.

    2.3. In "ref_transaction_commit()", it will call functions
         "files_transaction_prepare()" and "files_transaction_finish()"
         to commit the transaction.

         2.3.1. Lock the loose reference.

         2.3.2. Create a new packed-transaction, and add a new update
                entry to this packed-transaction. The previous step 1
                makes this operation unnecessary.

         2.3.3. Lock the packed-ref-store and call fucntion
                "is_packed_transaction_needed()" to check whether it
                is necessary to commit the transaction, and then
                abort the transaction because the reference is already
                removed from the packed-ref-store in step 1.

         2.3.4. Remove the reflog and the loose reference file for
                the reference to be deleted.

         2.3.4. Unlock the loose reference.

From the above steps, we can see that "refs_delete_refs()" is not an
atomic operation, but a semi-atomic operation. The operation is atomic
if all references to be deleted are in the packed ref-store, but not
if some references are loose references because we delete the loose
references one by one by calling "refs_delete_ref()" .

Refactored function "files_delete_refs()" to delete references within a
transaction, so the "reference-transaction" hook will only run once for
deleted branches and tags.

The behavior of the following git commands and the last two testcases
have been fixed in t1416:

 * git branch -d <branch>
 * git tag -d <tag>

A testcase in t5510 is broken because we used to call the function
"packed_refs_lock()", but it is not necessary if the deleted reference
is not in the "packed-refs" file.

Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
---
 refs/files-backend.c             | 21 +++++-----
 refs/packed-backend.c            | 51 +-----------------------
 t/t1416-ref-transaction-hooks.sh | 68 +-------------------------------
 t/t5510-fetch.sh                 | 16 ++++++++
 4 files changed, 28 insertions(+), 128 deletions(-)
diff mbox series

Patch

diff --git a/refs/files-backend.c b/refs/files-backend.c
index e029f5a885..8f3deddc71 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1268,31 +1268,27 @@  static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
 static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 			     struct string_list *refnames, unsigned int flags)
 {
-	struct files_ref_store *refs =
-		files_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
 	int i, result = 0;
 
 	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(ref_store, &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))
+		if (ref_transaction_delete(transaction, refname, NULL,
+					   flags, msg, &err))
 			result |= error(_("could not remove reference %s"), refname);
 	}
+	if (ref_transaction_commit(transaction, &err))
+		goto error;
 
+	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	return result;
 
@@ -1309,6 +1305,7 @@  static int files_delete_refs(struct ref_store *ref_store, const char *msg,
 	else
 		error(_("could not delete references: %s"), err.buf);
 
+	ref_transaction_free(transaction);
 	strbuf_release(&err);
 	return -1;
 }
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 97b6837767..fdb7a0a52c 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1519,55 +1519,6 @@  static int packed_initial_transaction_commit(struct ref_store *ref_store,
 	return ref_transaction_commit(transaction, err);
 }
 
-static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
-			     struct string_list *refnames, unsigned int flags)
-{
-	struct packed_ref_store *refs =
-		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
-	struct strbuf err = STRBUF_INIT;
-	struct ref_transaction *transaction;
-	struct string_list_item *item;
-	int ret;
-
-	(void)refs; /* We need the check above, but don't use the variable */
-
-	if (!refnames->nr)
-		return 0;
-
-	/*
-	 * Since we don't check the references' old_oids, the
-	 * individual updates can't fail, so we can pack all of the
-	 * updates into a single transaction.
-	 */
-
-	transaction = ref_store_transaction_begin(ref_store, &err);
-	if (!transaction)
-		return -1;
-
-	for_each_string_list_item(item, refnames) {
-		if (ref_transaction_delete(transaction, item->string, NULL,
-					   flags, msg, &err)) {
-			warning(_("could not delete reference %s: %s"),
-				item->string, err.buf);
-			strbuf_reset(&err);
-		}
-	}
-
-	ret = ref_transaction_commit(transaction, &err);
-
-	if (ret) {
-		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);
-	}
-
-	ref_transaction_free(transaction);
-	strbuf_release(&err);
-	return ret;
-}
-
 static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
 {
 	/*
@@ -1595,7 +1546,7 @@  struct ref_storage_be refs_be_packed = {
 
 	.pack_refs = packed_pack_refs,
 	.create_symref = NULL,
-	.delete_refs = packed_delete_refs,
+	.delete_refs = NULL,
 	.rename_ref = NULL,
 	.copy_ref = NULL,
 
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh
index 77996017d7..3d39e1634a 100755
--- a/t/t1416-ref-transaction-hooks.sh
+++ b/t/t1416-ref-transaction-hooks.sh
@@ -789,39 +789,7 @@  test_expect_success "branch: rename branches" '
 	test_cmp_heads_and_tags -C workdir expect
 '
 
-# Mismatched hook output for "git branch -d":
-#
-#  * The delete branches operation should be treated as one transaction,
-#    but was splitted into several transactions on loose references,
-#    and the "reference-transaction committed" command was executed
-#    redundantly on the packed-ref-store.
-#
-# The differences are as follows:
-#
-#     @@ -2,11 +2,19 @@
-#      <ZERO-OID> <ZERO-OID> refs/heads/topic1
-#      <ZERO-OID> <ZERO-OID> refs/heads/topic2
-#      <ZERO-OID> <ZERO-OID> refs/heads/topic3
-#     +## Call hook: reference-transaction committed ##
-#     +<ZERO-OID> <ZERO-OID> refs/heads/topic1
-#     +<ZERO-OID> <ZERO-OID> refs/heads/topic2
-#     +<ZERO-OID> <ZERO-OID> refs/heads/topic3
-#     +## Call hook: reference-transaction  prepared ##
-#     +<ZERO-OID> <ZERO-OID> refs/heads/topic1
-#     +## Call hook: reference-transaction committed ##
-#     +<ZERO-OID> <ZERO-OID> refs/heads/topic1
-#      ## Call hook: reference-transaction  prepared ##
-#     -<COMMIT-A> <ZERO-OID> refs/heads/topic1
-#      <COMMIT-B> <ZERO-OID> refs/heads/topic2
-#     -<COMMIT-C> <ZERO-OID> refs/heads/topic3
-#      ## Call hook: reference-transaction committed ##
-#     -<COMMIT-A> <ZERO-OID> refs/heads/topic1
-#      <COMMIT-B> <ZERO-OID> refs/heads/topic2
-#     +## Call hook: reference-transaction  prepared ##
-#     +<COMMIT-C> <ZERO-OID> refs/heads/topic3
-#     +## Call hook: reference-transaction committed ##
-#      <COMMIT-C> <ZERO-OID> refs/heads/topic3
-test_expect_failure "branch: remove branches" '
+test_expect_success "branch: remove branches" '
 	test_when_finished "rm -f $HOOK_OUTPUT" &&
 
 	cat >expect <<-\EOF &&
@@ -950,39 +918,7 @@  test_expect_success "tag: update refs to create loose refs" '
 	test_cmp_heads_and_tags -C workdir expect
 '
 
-# Mismatched hook output for "git tag -d":
-#
-#  * The delete tags operation should be treated as one transaction,
-#    but was splitted into several transactions on loose references,
-#    and the "reference-transaction committed" command was executed
-#    redundantly on the packed-ref-store.
-#
-# The differences are as follows:
-#
-#     @@ -2,11 +2,19 @@
-#      <ZERO-OID> <ZERO-OID> refs/tags/v1
-#      <ZERO-OID> <ZERO-OID> refs/tags/v2
-#      <ZERO-OID> <ZERO-OID> refs/tags/v3
-#     +## Call hook: reference-transaction committed ##
-#     +<ZERO-OID> <ZERO-OID> refs/tags/v1
-#     +<ZERO-OID> <ZERO-OID> refs/tags/v2
-#     +<ZERO-OID> <ZERO-OID> refs/tags/v3
-#     +## Call hook: reference-transaction  prepared ##
-#     +<ZERO-OID> <ZERO-OID> refs/tags/v1
-#     +## Call hook: reference-transaction committed ##
-#     +<ZERO-OID> <ZERO-OID> refs/tags/v1
-#      ## Call hook: reference-transaction  prepared ##
-#     -<COMMIT-A> <ZERO-OID> refs/tags/v1
-#      <COMMIT-B> <ZERO-OID> refs/tags/v2
-#     -<COMMIT-C> <ZERO-OID> refs/tags/v3
-#      ## Call hook: reference-transaction committed ##
-#     -<COMMIT-A> <ZERO-OID> refs/tags/v1
-#      <COMMIT-B> <ZERO-OID> refs/tags/v2
-#     +## Call hook: reference-transaction  prepared ##
-#     +<COMMIT-C> <ZERO-OID> refs/tags/v3
-#     +## Call hook: reference-transaction committed ##
-#      <COMMIT-C> <ZERO-OID> refs/tags/v3
-test_expect_failure "tag: remove tags with mixed ref_stores" '
+test_expect_success "tag: remove tags with mixed ref_stores" '
 	test_when_finished "rm -f $HOOK_OUTPUT" &&
 
 	cat >expect <<-\EOF &&
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index b45879a760..dfdab09600 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -168,6 +168,8 @@  test_expect_success REFFILES 'fetch --prune fails to delete branches' '
 	cd "$D" &&
 	git clone . prune-fail &&
 	cd prune-fail &&
+	git update-ref refs/remotes/origin/extrabranch main~ &&
+	git pack-refs --all &&
 	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 &&
@@ -175,6 +177,20 @@  test_expect_success REFFILES 'fetch --prune fails to delete branches' '
 	test_must_fail git fetch --prune origin
 '
 
+test_expect_success REFFILES 'fetch --prune ok for loose refs not in locked packed-refs' '
+	cd "$D" &&
+	git clone . prune-ok-ref-not-packed &&
+	(
+		cd prune-ok-ref-not-packed &&
+		git update-ref refs/remotes/origin/extrabranch main &&
+		: for loose refs not in packed-refs, we can delete them even the packed-refs is locked &&
+		:>.git/packed-refs.new &&
+
+		git fetch --prune origin &&
+		test_must_fail git rev-parse refs/remotes/origin/extrabranch --
+	)
+'
+
 test_expect_success 'fetch --atomic works with a single branch' '
 	test_when_finished "rm -rf \"$D\"/atomic" &&