[PATCH v2] fetch: fix non-conflicting tags not being committed
From: Karthik Nayak <hidden>
Date: 2025-11-06 08:39:31
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. This also extends to backfilling tags when using the now deprecated 'branches/' format for remotes. 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 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 --- Changes in v2: - Add a comment to explain the purpose of `commit_ref_transaction()` and how it works. - Also extend the same logic towards backfilling tags. While I was able to add a test for the happy path, I couldn't figure out how to test when `backfill_tags()` tags would fail. Tangentially, this flow seems to only be triggered when using the now deprecated 'branches/' remote format. - Remove unneeded subshells from the tests. - Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com --- builtin/fetch.c | 75 +++++++++++++++++++++++++++++++++++--------------------- t/t5510-fetch.sh | 61 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 28 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c7ff3480fb..d5aee5af10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname, *data->retcode = 1; } +/* + * Commit the reference transaction. If it isn't an atomic transaction, handle + * rejected updates as part of using batched updates. + */ +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 +1862,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; }
@@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport, * the transaction and don't commit anything. */ if (backfill_tags(&display_state, transport, transaction, tags_ref_map, - &fetch_head, config)) + &fetch_head, config)) { + if (!atomic_fetch) + commit_ref_transaction(&transaction, false, + transport->remote->name, &err); retcode = 1; + } } free_refs(tags_ref_map);
@@ -1858,33 +1902,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..9ff656a2bc 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,66 @@ 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 && + + test_must_fail 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_expect_success 'backfill tags with branches remote format' ' + test_when_finished rm -rf base repo && + + git init base && + git -C base commit --allow-empty -m "empty-commit" && + git -C base tag tag1 && + + git clone --no-tags base repo && + + git -C repo remote remove origin && + mkdir -p repo/.git/branches && + echo "$(cd base && pwd)#master" >repo/.git/branches/origin && + + git -C base commit --allow-empty -m "second empty-commit" && + git -C base tag tag2 && + + git -C repo fetch origin && + git -C repo for-each-ref refs/tags >out && + test_grep "tag1" out && + test_grep "tag2" out +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd