Thread (43 messages) 43 messages, 4 authors, 2025-12-02
STALE223d

[PATCH] fetch: fix non-conflicting tags not being committed

From: Karthik Nayak <hidden>
Date: 2025-11-03 13:49:15
Subsystem: the rest · Maintainer: Linus Torvalds 

The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)
updated the 'git-fetch(1)' command to use batched updates. This batches
updates to gain performance improvements. When fetching references, each
update is added to the transaction. Finally, when committing, individual
updates are allowed to fail with reason, while the transaction itself
succeeds.

One scenario which was missed here, was fetching tags. When fetching
conflicting tags, the `fetch_and_consume_refs()` function returns '1',
which skipped committing the transaction and directly jumped to the
cleanup section. This mean that no updates were applied.

Fix this by committing the transaction even when we have an error code.
This ensures other references are applied. Do this by extracting out the
transaction commit code into a new `commit_ref_transaction()` function
and using that.

Add two tests to check for this regression. While here, add a missing
cleanup from previous test.

Reported-by: David Bohman <redacted>
Signed-off-by: Karthik Nayak <redacted>
---
This fixes the bug reported by David Bohman [1].

[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
---
 builtin/fetch.c  | 65 +++++++++++++++++++++++++++++++++-----------------------
 t/t5510-fetch.sh | 41 +++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c7ff3480fb..8dea08dc74 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1686,6 +1686,38 @@ static void ref_transaction_rejection_handler(const char *refname,
 	*data->retcode = 1;
 }
 
+static int commit_ref_transaction(struct ref_transaction **transaction,
+				  bool is_atomic, const char *remote_name,
+				  struct strbuf *err)
+{
+	int retcode = ref_transaction_commit(*transaction, err);
+	if (retcode) {
+		/*
+		 * Explicitly handle transaction cleanup to avoid
+		 * aborting an already closed transaction.
+		 */
+		ref_transaction_free(*transaction);
+		*transaction = NULL;
+	}
+
+	if (*transaction && !is_atomic) {
+		struct ref_rejection_data data = {
+			.conflict_msg_shown = 0,
+			.remote_name = remote_name,
+			.retcode = &retcode,
+		};
+
+		ref_transaction_for_each_rejected_update(*transaction,
+							 ref_transaction_rejection_handler,
+							 &data);
+
+		ref_transaction_free(*transaction);
+		*transaction = NULL;
+	}
+
+	return retcode;
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config)
@@ -1826,6 +1858,10 @@ static int do_fetch(struct transport *transport,
 
 	if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
 				   &fetch_head, config)) {
+		/* As we're using batched updates, commit any pending updates. */
+		if (!atomic_fetch)
+			commit_ref_transaction(&transaction, false,
+					       transport->remote->name, &err);
 		retcode = 1;
 		goto cleanup;
 	}
@@ -1858,33 +1894,8 @@ static int do_fetch(struct transport *transport,
 	if (retcode)
 		goto cleanup;
 
-	retcode = ref_transaction_commit(transaction, &err);
-	if (retcode) {
-		/*
-		 * Explicitly handle transaction cleanup to avoid
-		 * aborting an already closed transaction.
-		 */
-		ref_transaction_free(transaction);
-		transaction = NULL;
-		goto cleanup;
-	}
-
-	if (!atomic_fetch) {
-		struct ref_rejection_data data = {
-			.retcode = &retcode,
-			.conflict_msg_shown = 0,
-			.remote_name = transport->remote->name,
-		};
-
-		ref_transaction_for_each_rejected_update(transaction,
-							 ref_transaction_rejection_handler,
-							 &data);
-		if (retcode) {
-			ref_transaction_free(transaction);
-			transaction = NULL;
-			goto cleanup;
-		}
-	}
+	retcode = commit_ref_transaction(&transaction, atomic_fetch,
+					 transport->remote->name, &err);
 
 	commit_fetch_head(&fetch_head);
 
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index b7059cccaa..92b3a8e79e 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti
 '
 
 test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
+	test_when_finished rm -rf base repo &&
 	(
 		git init --ref-format=reftable base &&
 		cd base &&
@@ -1577,6 +1578,46 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
 	)
 '
 
+test_expect_success 'fetch --tags fetches existing tags' '
+	test_when_finished rm -rf base repo &&
+	(
+		git init base &&
+		git -C base commit --allow-empty -m "empty-commit" &&
+
+		git clone --bare base repo &&
+
+		git -C base tag tag-1 &&
+		git -C repo for-each-ref >out &&
+		test_grep ! "tag-1" out &&
+		git -C repo fetch --tags &&
+		git -C repo for-each-ref >out &&
+		test_grep "tag-1" out
+	)
+'
+
+test_expect_success 'fetch --tags fetches non-conflicting tags' '
+	test_when_finished rm -rf base repo &&
+	(
+		git init base &&
+		git -C base commit --allow-empty -m "empty-commit" &&
+		git -C base tag tag-1 &&
+
+		git clone --bare base repo &&
+
+		git -C base tag tag-2 &&
+		git -C repo for-each-ref >out &&
+		test_grep ! "tag-2" out &&
+
+		git -C base commit --allow-empty -m "second empty-commit" &&
+		git -C base tag -f tag-1 &&
+
+		! git -C repo fetch --tags 2>out &&
+		test_grep "tag-1  (would clobber existing tag)" out &&
+		git -C repo for-each-ref >out &&
+		test_grep "tag-2" out
+	)
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
Other recent patches touching these files
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help