{"thread":{"id":"64423","subject":"[PATCH] fetch: fix non-conflicting tags not being committed","startedAt":"2025-11-03T13:49:15Z","lastAt":"2025-12-02T22:35:19Z","messageCount":54,"participants":["Karthik Nayak","Eric Sunshine","Justin Tobler","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530117","messageId":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","threadId":"64423","inReplyTo":null,"subject":"[PATCH] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-03T13:49:06Z","receivedAt":"2025-11-03T13:49:15Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied.\n\nFix this by committing the transaction even when we have an error code.\nThis ensures other references are applied. Do this by extracting out the\ntransaction commit code into a new `commit_ref_transaction()` function\nand using that.\n\nAdd two tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nSigned-off-by: Karthik Nayak \n---\nThis fixes the bug reported by David Bohman [1].\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n---\n builtin/fetch.c | 65 +++++++++++++++++++++++++++++++++-----------------------\n t/t5510-fetch.sh | 41 +++++++++++++++++++++++++++++++++++\n 2 files changed, 79 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..8dea08dc74 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,38 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode) {\n+\t\t/*\n+\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t * aborting an already closed transaction.\n+\t\t */\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\tif (*transaction && !is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1826,6 +1858,10 @@ static int do_fetch(struct transport *transport,\n \n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n+\t\t/* As we're using batched updates, commit any pending updates. */\n+\t\tif (!atomic_fetch)\n+\t\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t\t transport->remote->name, &err);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1858,33 +1894,8 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n-\t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n \n \tcommit_fetch_head(&fetch_head);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..92b3a8e79e 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,46 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\t(\n+\t\tgit init base &&\n+\t\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\t\tgit clone --bare base repo &&\n+\n+\t\tgit -C base tag tag-1 &&\n+\t\tgit -C repo for-each-ref >out &&\n+\t\ttest_grep ! \"tag-1\" out &&\n+\t\tgit -C repo fetch --tags &&\n+\t\tgit -C repo for-each-ref >out &&\n+\t\ttest_grep \"tag-1\" out\n+\t)\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\t(\n+\t\tgit init base &&\n+\t\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\t\tgit -C base tag tag-1 &&\n+\n+\t\tgit clone --bare base repo &&\n+\n+\t\tgit -C base tag tag-2 &&\n+\t\tgit -C repo for-each-ref >out &&\n+\t\ttest_grep ! \"tag-2\" out &&\n+\n+\t\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\t\tgit -C base tag -f tag-1 &&\n+\n+\t\t! git -C repo fetch --tags 2>out &&\n+\t\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\t\tgit -C repo for-each-ref >out &&\n+\t\ttest_grep \"tag-2\" out\n+\t)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n\n\n"},{"id":"530139","messageId":"CAPig+cRF1hb_RQQCuzZWrnu4AvmOUgVT1mVh=LhP17f7_hYVGQ@mail.gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"Re: [PATCH] fetch: fix non-conflicting tags not being committed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-11-03T17:53:16Z","receivedAt":"2025-11-03T17:53:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 3, 2025 at 8:49 AM Karthik Nayak wrote:\n> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n> updated the 'git-fetch(1)' command to use batched updates. This batches\n> updates to gain performance improvements. When fetching references, each\n> update is added to the transaction. Finally, when committing, individual\n> updates are allowed to fail with reason, while the transaction itself\n> succeeds.\n> [...]\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> @@ -1577,6 +1578,46 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n> +test_expect_success 'fetch --tags fetches existing tags' '\n> + test_when_finished rm -rf base repo &&\n> + (\n> + git init base &&\n> + git -C base commit --allow-empty -m \"empty-commit\" &&\n> +\n> + git clone --bare base repo &&\n> +\n> + git -C base tag tag-1 &&\n> + git -C repo for-each-ref >out &&\n> + test_grep ! \"tag-1\" out &&\n> + git -C repo fetch --tags &&\n> + git -C repo for-each-ref >out &&\n> + test_grep \"tag-1\" out\n> + )\n> +'\n\nWhat is the purpose of wrapping this code in a subshell?\n\nSame question regarding the other test added by this patch.\n\n> +test_expect_success 'fetch --tags fetches non-conflicting tags' '\n> + test_when_finished rm -rf base repo &&\n> + (\n> + git init base &&\n> + git -C base commit --allow-empty -m \"empty-commit\" &&\n> + git -C base tag tag-1 &&\n> +\n> + git clone --bare base repo &&\n> +\n> + git -C base tag tag-2 &&\n> + git -C repo for-each-ref >out &&\n> + test_grep ! \"tag-2\" out &&\n> +\n> + git -C base commit --allow-empty -m \"second empty-commit\" &&\n> + git -C base tag -f tag-1 &&\n> +\n> + ! git -C repo fetch --tags 2>out &&\n\nShould this be using `test_must_fail` rather than `!`?\n\n> + test_grep \"tag-1 (would clobber existing tag)\" out &&\n> + git -C repo for-each-ref >out &&\n> + test_grep \"tag-2\" out\n> + )\n> +'\n"},{"id":"530152","messageId":"i3wzd6r2iohohj36fbipc2owrxkqzjni6aqwyv2gw7hb5kdg6b@y6fsmfvphpom","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"Re: [PATCH] fetch: fix non-conflicting tags not being committed","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-11-03T20:52:04Z","receivedAt":"2025-11-03T20:52:12Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/11/03 02:49PM, Karthik Nayak wrote:\n> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n> updated the 'git-fetch(1)' command to use batched updates. This batches\n> updates to gain performance improvements. When fetching references, each\n> update is added to the transaction. Finally, when committing, individual\n> updates are allowed to fail with reason, while the transaction itself\n> succeeds.\n> \n> One scenario which was missed here, was fetching tags. When fetching\n> conflicting tags, the `fetch_and_consume_refs()` function returns '1',\n> which skipped committing the transaction and directly jumped to the\n> cleanup section. This mean that no updates were applied.\n\nOk so when fetching tags, if there is a reference conflict, we are\nbailing out without committing the transaction. In such cases, we\nactually want to handle the rejected reference updates and continue with\nthe transaction.\n\n> Fix this by committing the transaction even when we have an error code.\n> This ensures other references are applied. Do this by extracting out the\n> transaction commit code into a new `commit_ref_transaction()` function\n> and using that.\n\nMakes sense.\n\n> Add two tests to check for this regression. While here, add a missing\n> cleanup from previous test.\n> \n> Reported-by: David Bohman \n> Signed-off-by: Karthik Nayak \n> ---\n> This fixes the bug reported by David Bohman [1].\n> \n> [1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n> ---\n> builtin/fetch.c | 65 +++++++++++++++++++++++++++++++++-----------------------\n> t/t5510-fetch.sh | 41 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 79 insertions(+), 27 deletions(-)\n> \n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index c7ff3480fb..8dea08dc74 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1686,6 +1686,38 @@ static void ref_transaction_rejection_handler(const char *refname,\n> \t*data->retcode = 1;\n> }\n> \n> +static int commit_ref_transaction(struct ref_transaction **transaction,\n> +\t\t\t\t bool is_atomic, const char *remote_name,\n> +\t\t\t\t struct strbuf *err)\n\nnit: I think `commit_ref_transaction()` here can easily be confused with\n`ref_transaction_commit()` and it's not exactly clear how they differ\nfrom the names alone. Maybe we could explain the additional\nresponsibilities in a comment?\n\n> +{\n> +\tint retcode = ref_transaction_commit(*transaction, err);\n> +\tif (retcode) {\n> +\t\t/*\n> +\t\t * Explicitly handle transaction cleanup to avoid\n> +\t\t * aborting an already closed transaction.\n> +\t\t */\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n> +\t}\n> +\n> +\tif (*transaction && !is_atomic) {\n> +\t\tstruct ref_rejection_data data = {\n> +\t\t\t.conflict_msg_shown = 0,\n> +\t\t\t.remote_name = remote_name,\n> +\t\t\t.retcode = &retcode,\n> +\t\t};\n> +\n> +\t\tref_transaction_for_each_rejected_update(*transaction,\n> +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> +\t\t\t\t\t\t\t &data);\n> +\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n> +\t}\n> +\n> +\treturn retcode;\n> +}\n> +\n> static int do_fetch(struct transport *transport,\n> \t\t struct refspec *rs,\n> \t\t const struct fetch_config *config)\n> @@ -1826,6 +1858,10 @@ static int do_fetch(struct transport *transport,\n> \n> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n> \t\t\t\t &fetch_head, config)) {\n> +\t\t/* As we're using batched updates, commit any pending updates. */\n> +\t\tif (!atomic_fetch)\n> +\t\t\tcommit_ref_transaction(&transaction, false,\n> +\t\t\t\t\t transport->remote->name, &err);\n\nIIUC, when we encounter an error via `fetch_and_consume_refs()` we now\nexplicitly commit the transaction early and handle the rejected\nreferences. At first I wondered why we wouldn't just skip the \"goto\ncleanup\" in such cases, but I assume this is in part because we are\ntrying to match the pre-batched updates behavior.\n\nNaive question: I noticed that `backfill_tags()` also invokes\n`fetch_and_consume_refs()`. Do we also need to commit pending updates\nthere in case of reference conflicts?\n\n> \t\tretcode = 1;\n> \t\tgoto cleanup;\n> \t}\n> @@ -1858,33 +1894,8 @@ static int do_fetch(struct transport *transport,\n> \tif (retcode)\n> \t\tgoto cleanup;\n> \n> -\tretcode = ref_transaction_commit(transaction, &err);\n> -\tif (retcode) {\n> -\t\t/*\n> -\t\t * Explicitly handle transaction cleanup to avoid\n> -\t\t * aborting an already closed transaction.\n> -\t\t */\n> -\t\tref_transaction_free(transaction);\n> -\t\ttransaction = NULL;\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tif (!atomic_fetch) {\n> -\t\tstruct ref_rejection_data data = {\n> -\t\t\t.retcode = &retcode,\n> -\t\t\t.conflict_msg_shown = 0,\n> -\t\t\t.remote_name = transport->remote->name,\n> -\t\t};\n> -\n> -\t\tref_transaction_for_each_rejected_update(transaction,\n> -\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> -\t\t\t\t\t\t\t &data);\n> -\t\tif (retcode) {\n> -\t\t\tref_transaction_free(transaction);\n> -\t\t\ttransaction = NULL;\n> -\t\t\tgoto cleanup;\n> -\t\t}\n> -\t}\n> +\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n> +\t\t\t\t\t transport->remote->name, &err);\n\nThis is where we would normally commit the reference transaction and\nhandle rejected reference updates. Now we just reuse\n`commit_ref_transaction()`.\n\nDo we need to check the return value and potentially \"goto cleanup\"\nbefore proceeding?\n\n-Justin\n"},{"id":"530153","messageId":"CAOLa=ZT0DFG8jx8x=OHouFxinobBbqAbdegaUgkNxy0xLY910A@mail.gmail.com","threadId":"64423","inReplyTo":"CAPig+cRF1hb_RQQCuzZWrnu4AvmOUgVT1mVh=LhP17f7_hYVGQ@mail.gmail.com","subject":"Re: [PATCH] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-03T21:22:31Z","receivedAt":"2025-11-03T21:22:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Nov 3, 2025 at 8:49 AM Karthik Nayak wrote:\n>> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n>> updated the 'git-fetch(1)' command to use batched updates. This batches\n>> updates to gain performance improvements. When fetching references, each\n>> update is added to the transaction. Finally, when committing, individual\n>> updates are allowed to fail with reason, while the transaction itself\n>> succeeds.\n>> [...]\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> @@ -1577,6 +1578,46 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n>> +test_expect_success 'fetch --tags fetches existing tags' '\n>> + test_when_finished rm -rf base repo &&\n>> + (\n>> + git init base &&\n>> + git -C base commit --allow-empty -m \"empty-commit\" &&\n>> +\n>> + git clone --bare base repo &&\n>> +\n>> + git -C base tag tag-1 &&\n>> + git -C repo for-each-ref >out &&\n>> + test_grep ! \"tag-1\" out &&\n>> + git -C repo fetch --tags &&\n>> + git -C repo for-each-ref >out &&\n>> + test_grep \"tag-1\" out\n>> + )\n>> +'\n>\n> What is the purpose of wrapping this code in a subshell?\n>\n> Same question regarding the other test added by this patch.\n>\n\nIt's not needed, I first created two subshells with cd into each of\nthem. I let it be when I merged them. So let me remove them.\n\n>> +test_expect_success 'fetch --tags fetches non-conflicting tags' '\n>> + test_when_finished rm -rf base repo &&\n>> + (\n>> + git init base &&\n>> + git -C base commit --allow-empty -m \"empty-commit\" &&\n>> + git -C base tag tag-1 &&\n>> +\n>> + git clone --bare base repo &&\n>> +\n>> + git -C base tag tag-2 &&\n>> + git -C repo for-each-ref >out &&\n>> + test_grep ! \"tag-2\" out &&\n>> +\n>> + git -C base commit --allow-empty -m \"second empty-commit\" &&\n>> + git -C base tag -f tag-1 &&\n>> +\n>> + ! git -C repo fetch --tags 2>out &&\n>\n> Should this be using `test_must_fail` rather than `!`?\n>\n\nYes, will fix!\n\n>> + test_grep \"tag-1 (would clobber existing tag)\" out &&\n>> + git -C repo for-each-ref >out &&\n>> + test_grep \"tag-2\" out\n>> + )\n>> +'\n"},{"id":"530301","messageId":"20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-06T08:39:25Z","receivedAt":"2025-11-06T08:39:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags when using the now deprecated 'branches/'\nformat for remotes.\n\nFix this by committing the transaction even when we have an error code.\nThis ensures other references are applied. Do this by extracting out the\ntransaction commit code into a new `commit_ref_transaction()` function\nand using that.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nSigned-off-by: Karthik Nayak \n---\nThis fixes the bug reported by David Bohman [1].\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n---\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n---\n builtin/fetch.c | 75 +++++++++++++++++++++++++++++++++++---------------------\n t/t5510-fetch.sh | 61 +++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 108 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..d5aee5af10 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode) {\n+\t\t/*\n+\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t * aborting an already closed transaction.\n+\t\t */\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\tif (*transaction && !is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,\n \n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n+\t\t/* As we're using batched updates, commit any pending updates. */\n+\t\tif (!atomic_fetch)\n+\t\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t\t transport->remote->name, &err);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n \t}\n@@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,\n \t\t\t * the transaction and don't commit anything.\n \t\t\t */\n \t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n-\t\t\t\t\t &fetch_head, config))\n+\t\t\t\t\t &fetch_head, config)) {\n+\t\t\t\tif (!atomic_fetch)\n+\t\t\t\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t\t\t\t transport->remote->name, &err);\n \t\t\t\tretcode = 1;\n+\t\t\t}\n \t\t}\n \n \t\tfree_refs(tags_ref_map);\n@@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n-\t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n \n \tcommit_fetch_head(&fetch_head);\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..9ff656a2bc 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,66 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success 'backfill tags with branches remote format' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag1 &&\n+\n+\tgit clone --no-tags base repo &&\n+\n+\tgit -C repo remote remove origin &&\n+\tmkdir -p repo/.git/branches &&\n+\techo \"$(cd base && pwd)#master\" >repo/.git/branches/origin &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag tag2 &&\n+\n+\tgit -C repo fetch origin &&\n+\tgit -C repo for-each-ref refs/tags >out &&\n+\ttest_grep \"tag1\" out &&\n+\ttest_grep \"tag2\" out\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n\n\n"},{"id":"530309","messageId":"aQyLfD_zx0ndCLvU@pks.im","threadId":"64423","inReplyTo":"20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-06T11:50:20Z","receivedAt":"2025-11-06T11:50:32Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:\n> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n> updated the 'git-fetch(1)' command to use batched updates. This batches\n> updates to gain performance improvements. When fetching references, each\n> update is added to the transaction. Finally, when committing, individual\n> updates are allowed to fail with reason, while the transaction itself\n> succeeds.\n> \n> One scenario which was missed here, was fetching tags. When fetching\n> conflicting tags, the `fetch_and_consume_refs()` function returns '1',\n> which skipped committing the transaction and directly jumped to the\n> cleanup section. This mean that no updates were applied.\n\nOkay, this is obviously broken indeed.\n\n> This also extends to backfilling tags when using the now deprecated\n> 'branches/' format for remotes.\n\nI'm a bit lost here -- what does backfilling have to do with the\n\"branches/\" directory? The backfill is supposed to create tags that\npoint into the history that one has just fetched. So:\n\n - With `--tags` we fetch all tags announced by the remote.\n\n - With `--no-tags` we fetch no tags.\n\n - Otherwise we fetch those tags that point into our history.\n\nThe last behaviour is a bit more on the esoteric side, but it's\ndescribed as such in git-fetch(1):\n\n By default, any tag that points into the histories being fetched is\n also fetched; the effect is to fetch tags that point at branches\n that you are interested in. This default behavior can be changed by\n using the --tags or --no-tags options or by configuring\n remote..tagOpt. By using a refspec that fetches tags\n explicitly, you can fetch tags that do not point into branches you\n are interested in as well.\n\nThe following test demonstrates this behaviour:\n\n\ttest_expect_success \"fetch single branch without explicit tag option\" '\n\t\tgit init source &&\n\t\tgit -C source commit --allow-empty --message common &&\n\t\tgit clone file://\"$(pwd)\"/source target &&\n\t\t(\n\t\t\tcd source &&\n\t\t\tgit commit --allow-empty --message discard-me &&\n\t\t\tgit tag discard-me &&\n\t\t\tgit commit --amend --allow-empty --message fetch-me &&\n\t\t\tgit tag fetch-me\n\t\t) &&\n\n\t\t# The \"discard-me\" tag does not point into the history that we are\n\t\t# about to fetch, so it should not have been created.\n\t\tgit -C target fetch origin &&\n\t\tgit -C target tag -l >actual &&\n\t\techo \"fetch-me\" >expect &&\n\n\t\t# But with \"--tags\" we instruct git-fetch(1) to fetch all tags, so we\n\t\t# should now see it.\n\t\tgit -C target fetch origin --tags &&\n\t\tgit -C target tag -l >actual &&\n\t\tcat >expect <<-\\EOF &&\n\t\tdiscard-me\n\t\tfetch-me\n\t\tEOF\n\t\ttest_cmp expect actual\n\t'\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index c7ff3480fb..d5aee5af10 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n> \t*data->retcode = 1;\n> }\n> \n> +/*\n> + * Commit the reference transaction. If it isn't an atomic transaction, handle\n> + * rejected updates as part of using batched updates.\n> + */\n> +static int commit_ref_transaction(struct ref_transaction **transaction,\n> +\t\t\t\t bool is_atomic, const char *remote_name,\n> +\t\t\t\t struct strbuf *err)\n> +{\n> +\tint retcode = ref_transaction_commit(*transaction, err);\n> +\tif (retcode) {\n> +\t\t/*\n> +\t\t * Explicitly handle transaction cleanup to avoid\n> +\t\t * aborting an already closed transaction.\n> +\t\t */\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n> +\t}\n> +\n> +\tif (*transaction && !is_atomic) {\n> +\t\tstruct ref_rejection_data data = {\n> +\t\t\t.conflict_msg_shown = 0,\n> +\t\t\t.remote_name = remote_name,\n> +\t\t\t.retcode = &retcode,\n> +\t\t};\n> +\n> +\t\tref_transaction_for_each_rejected_update(*transaction,\n> +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> +\t\t\t\t\t\t\t &data);\n> +\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n> +\t}\n\nOkay. Do we need to discern cases where this is called and we haven't\nmanaged to even queue a single reference update?\n\n> +\treturn retcode;\n> +}\n> +\n> static int do_fetch(struct transport *transport,\n> \t\t struct refspec *rs,\n> \t\t const struct fetch_config *config)\n\nNit: it might make sense to have a preparatory commit that extracts the\nfunction but that is otherwise a no-op change.\n\n> @@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,\n> \n> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n> \t\t\t\t &fetch_head, config)) {\n> +\t\t/* As we're using batched updates, commit any pending updates. */\n> +\t\tif (!atomic_fetch)\n> +\t\t\tcommit_ref_transaction(&transaction, false,\n> +\t\t\t\t\t transport->remote->name, &err);\n> \t\tretcode = 1;\n> \t\tgoto cleanup;\n> \t}\n\nHm. Don't we also have to unset the transaction now? Ah, no, you pass\nthe pointer to the transaction here and set it to `NULL` in\n`commit_ref_transaction()`. Makes sense.\n\n> @@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,\n> \t\t\t * the transaction and don't commit anything.\n> \t\t\t */\n> \t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n> -\t\t\t\t\t &fetch_head, config))\n> +\t\t\t\t\t &fetch_head, config)) {\n> +\t\t\t\tif (!atomic_fetch)\n> +\t\t\t\t\tcommit_ref_transaction(&transaction, false,\n> +\t\t\t\t\t\t\t transport->remote->name, &err);\n> \t\t\t\tretcode = 1;\n> +\t\t\t}\n> \t\t}\n> \n> \t\tfree_refs(tags_ref_map);\n\nWe now have three different callsites where we commit the transaction.\nIt gets better due to the newly introduced function, but it overall\nfeels somewhat fragile regardless of that.\n\nThanks!\n\nPatrick\n"},{"id":"530335","messageId":"xmqqwm43gfbp.fsf@gitster.g","threadId":"64423","inReplyTo":"aQyLfD_zx0ndCLvU@pks.im","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-06T18:56:42Z","receivedAt":"2025-11-06T18:56:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We now have three different callsites where we commit the transaction.\n> It gets better due to the newly introduced function, but it overall\n> feels somewhat fragile regardless of that.\n\nIndeed.\n"},{"id":"530344","messageId":"cwayobvml63evuasdcamvkx5rpwectmwrwxr3cwxqrkxtketqa@lzm62c2xe75v","threadId":"64423","inReplyTo":"20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-11-06T22:10:58Z","receivedAt":"2025-11-06T22:11:02Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/11/06 09:39AM, Karthik Nayak wrote:\n> @@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,\n> \tif (retcode)\n> \t\tgoto cleanup;\n> \n> -\tretcode = ref_transaction_commit(transaction, &err);\n> -\tif (retcode) {\n> -\t\t/*\n> -\t\t * Explicitly handle transaction cleanup to avoid\n> -\t\t * aborting an already closed transaction.\n> -\t\t */\n> -\t\tref_transaction_free(transaction);\n> -\t\ttransaction = NULL;\n> -\t\tgoto cleanup;\n> -\t}\n> -\n> -\tif (!atomic_fetch) {\n> -\t\tstruct ref_rejection_data data = {\n> -\t\t\t.retcode = &retcode,\n> -\t\t\t.conflict_msg_shown = 0,\n> -\t\t\t.remote_name = transport->remote->name,\n> -\t\t};\n> -\n> -\t\tref_transaction_for_each_rejected_update(transaction,\n> -\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> -\t\t\t\t\t\t\t &data);\n> -\t\tif (retcode) {\n> -\t\t\tref_transaction_free(transaction);\n> -\t\t\ttransaction = NULL;\n> -\t\t\tgoto cleanup;\n> -\t\t}\n> -\t}\n> +\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n> +\t\t\t\t\t transport->remote->name, &err);\n\nIt looks like previously, whenever `ref_transaction_commit()` or\n`ref_transaction_rejection_handler()` returned a non-zero value, we\nwould \"goto cleanup\". Now that these operations are handled via\n`commit_ref_transaction()` though, it looks like we no longer handle the\n\"retcode\" return value and just continue. I think we still need to check\nthe return value here.\n\n-Justin\n"},{"id":"530365","messageId":"CAOLa=ZQpTqnCQs4=wcUwJOWy5mXiG4y_eTiFtPkS2uOk4U66Tw@mail.gmail.com","threadId":"64423","inReplyTo":"aQyLfD_zx0ndCLvU@pks.im","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-07T13:15:32Z","receivedAt":"2025-11-07T13:15:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:\n>> The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n>> updated the 'git-fetch(1)' command to use batched updates. This batches\n>> updates to gain performance improvements. When fetching references, each\n>> update is added to the transaction. Finally, when committing, individual\n>> updates are allowed to fail with reason, while the transaction itself\n>> succeeds.\n>>\n>> One scenario which was missed here, was fetching tags. When fetching\n>> conflicting tags, the `fetch_and_consume_refs()` function returns '1',\n>> which skipped committing the transaction and directly jumped to the\n>> cleanup section. This mean that no updates were applied.\n>\n> Okay, this is obviously broken indeed.\n>\n>> This also extends to backfilling tags when using the now deprecated\n>> 'branches/' format for remotes.\n>\n> I'm a bit lost here -- what does backfilling have to do with the\n> \"branches/\" directory? The backfill is supposed to create tags that\n> point into the history that one has just fetched. So:\n>\n\nI didn't read the code well enough. Let me walk through what I read:\n\nThe block for backfilling tags, is only triggered in `do_fetch()`, if\n\n if (tags == TAGS_DEFAULT && autotags) { ... }\n\nThis means that the autotags must be '1'. And I see at the start of the\nfunction that:\n\n int autotags = (transport->remote->fetch_tags == 1);\n\nSo I went into looking when `transport->remote->fetch_tags` would be set\nto '1'. This is only done in `read_branches_file()` which is done when\nparsing the now deprecated 'branches/' directory.\n\nI was correct until here. But, there is something I missed.\n\nWe also pass a pointer to `autotags` to the `get_ref_map()` function. In\nthis function, we set `autotags` to '1' for any of the following\nconditions:\n\n - When there is a refspec specified by the user.\n\n - We have a default branch with a remote specified.\n\nSo this means there are other scenarios we use the backfill() command.\n\nThat brings us to the second part of it, if we specify the '--tags'\nflag, then we fetch all tags, even the ones which aren't part of our\nhistory. This also happens as part of the `get_ref_map()` function. This\nflow also skips the 'backfill()' function.\n\nSo in effect, we only backfill tags, when the user doesn't specify\neither '--tags' or '--no-tags'.\n\n> - With `--tags` we fetch all tags announced by the remote.\n>\n> - With `--no-tags` we fetch no tags.\n>\n> - Otherwise we fetch those tags that point into our history.\n>\n> The last behaviour is a bit more on the esoteric side, but it's\n> described as such in git-fetch(1):\n>\n> By default, any tag that points into the histories being fetched is\n> also fetched; the effect is to fetch tags that point at branches\n> that you are interested in. This default behavior can be changed by\n> using the --tags or --no-tags options or by configuring\n> remote..tagOpt. By using a refspec that fetches tags\n> explicitly, you can fetch tags that do not point into branches you\n> are interested in as well.\n>\n\nBut backfilling isn't about diverged history, no? It's about fetching\nhistory of refs being requested.\n\n> The following test demonstrates this behaviour:\n>\n> \ttest_expect_success \"fetch single branch without explicit tag option\" '\n> \t\tgit init source &&\n> \t\tgit -C source commit --allow-empty --message common &&\n> \t\tgit clone file://\"$(pwd)\"/source target &&\n> \t\t(\n> \t\t\tcd source &&\n> \t\t\tgit commit --allow-empty --message discard-me &&\n> \t\t\tgit tag discard-me &&\n> \t\t\tgit commit --amend --allow-empty --message fetch-me &&\n> \t\t\tgit tag fetch-me\n> \t\t) &&\n>\n> \t\t# The \"discard-me\" tag does not point into the history that we are\n> \t\t# about to fetch, so it should not have been created.\n> \t\tgit -C target fetch origin &&\n> \t\tgit -C target tag -l >actual &&\n> \t\techo \"fetch-me\" >expect &&\n>\n> \t\t# But with \"--tags\" we instruct git-fetch(1) to fetch all tags, so we\n> \t\t# should now see it.\n> \t\tgit -C target fetch origin --tags &&\n\nHere, we don't really backfill, but rather we request all tags from the\nremote, hence we end up with the 'discard-me' tag. Not because of the\ndiverged history. I also confirmed this by adding a breakpoint into the\n`backfill_tags()` function, while running this test.\n\n> \t\tgit -C target tag -l >actual &&\n> \t\tcat >expect <<-\\EOF &&\n> \t\tdiscard-me\n> \t\tfetch-me\n> \t\tEOF\n> \t\ttest_cmp expect actual\n> \t'\n\nBut I was able to slightly modify the test to get the required affect:\n\n test_expect_success \"backfill tags when providing a refspec\" '\n \tgit init source &&\n \tgit -C source commit --allow-empty --message common &&\n \tgit clone file://\"$(pwd)\"/source target &&\n \t(\n \t cd source &&\n \t git commit --allow-empty --message history &&\n \t git tag history &&\n \t git commit --allow-empty --message fetch-me &&\n \t git tag fetch-me\n \t) &&\n\n \t# The \"history\" tag is backfilled eventhough we requested\n \t# to only fetch the master\n \tgit -C target fetch origin master:branch &&\n \tgit -C target tag -l >actual &&\n \tcat >expect <<-\\EOF &&\n \tfetch-me\n \thistory\n \tEOF\n \ttest_cmp expect actual\n '\n\nI will add this in. Thanks for the explanation, it really helped\nconsolidate my understanding here.\n\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index c7ff3480fb..d5aee5af10 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n>> \t*data->retcode = 1;\n>> }\n>>\n>> +/*\n>> + * Commit the reference transaction. If it isn't an atomic transaction, handle\n>> + * rejected updates as part of using batched updates.\n>> + */\n>> +static int commit_ref_transaction(struct ref_transaction **transaction,\n>> +\t\t\t\t bool is_atomic, const char *remote_name,\n>> +\t\t\t\t struct strbuf *err)\n>> +{\n>> +\tint retcode = ref_transaction_commit(*transaction, err);\n>> +\tif (retcode) {\n>> +\t\t/*\n>> +\t\t * Explicitly handle transaction cleanup to avoid\n>> +\t\t * aborting an already closed transaction.\n>> +\t\t */\n>> +\t\tref_transaction_free(*transaction);\n>> +\t\t*transaction = NULL;\n>> +\t}\n>> +\n>> +\tif (*transaction && !is_atomic) {\n>> +\t\tstruct ref_rejection_data data = {\n>> +\t\t\t.conflict_msg_shown = 0,\n>> +\t\t\t.remote_name = remote_name,\n>> +\t\t\t.retcode = &retcode,\n>> +\t\t};\n>> +\n>> +\t\tref_transaction_for_each_rejected_update(*transaction,\n>> +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n>> +\t\t\t\t\t\t\t &data);\n>> +\n>> +\t\tref_transaction_free(*transaction);\n>> +\t\t*transaction = NULL;\n>> +\t}\n>\n> Okay. Do we need to discern cases where this is called and we haven't\n> managed to even queue a single reference update?\n>\n\nI don't see a reason. This is anyways a post-commit action, if there are\nno updates, there will be no rejections. So this will be a no-op.\n\n>> +\treturn retcode;\n>> +}\n>> +\n>> static int do_fetch(struct transport *transport,\n>> \t\t struct refspec *rs,\n>> \t\t const struct fetch_config *config)\n>\n> Nit: it might make sense to have a preparatory commit that extracts the\n> function but that is otherwise a no-op change.\n>\n\nLet me do that. I was thinking the change is small. But perhaps that'd\nbe easier for reviewing.\n\n>> @@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,\n>>\n>> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n>> \t\t\t\t &fetch_head, config)) {\n>> +\t\t/* As we're using batched updates, commit any pending updates. */\n>> +\t\tif (!atomic_fetch)\n>> +\t\t\tcommit_ref_transaction(&transaction, false,\n>> +\t\t\t\t\t transport->remote->name, &err);\n>> \t\tretcode = 1;\n>> \t\tgoto cleanup;\n>> \t}\n>\n> Hm. Don't we also have to unset the transaction now? Ah, no, you pass\n> the pointer to the transaction here and set it to `NULL` in\n> `commit_ref_transaction()`. Makes sense.\n>\n>> @@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,\n>> \t\t\t * the transaction and don't commit anything.\n>> \t\t\t */\n>> \t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n>> -\t\t\t\t\t &fetch_head, config))\n>> +\t\t\t\t\t &fetch_head, config)) {\n>> +\t\t\t\tif (!atomic_fetch)\n>> +\t\t\t\t\tcommit_ref_transaction(&transaction, false,\n>> +\t\t\t\t\t\t\t transport->remote->name, &err);\n>> \t\t\t\tretcode = 1;\n>> +\t\t\t}\n>> \t\t}\n>>\n>> \t\tfree_refs(tags_ref_map);\n>\n> We now have three different callsites where we commit the transaction.\n> It gets better due to the newly introduced function, but it overall\n> feels somewhat fragile regardless of that.\n>\n\nYeah I must agree with that. I could think of a cleaner way, but will\nspend some time here.\n\n> Thanks!\n>\n> Patrick\n\nThanks,\nKarthik\n"},{"id":"530366","messageId":"CAOLa=ZR9oKD_Zz3R+1W=f3M9Rd_FgNZ+AvFqk+ia+BSaACTsfg@mail.gmail.com","threadId":"64423","inReplyTo":"cwayobvml63evuasdcamvkx5rpwectmwrwxr3cwxqrkxtketqa@lzm62c2xe75v","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-07T14:01:21Z","receivedAt":"2025-11-07T14:01:25Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Justin Tobler writes:\n\n> On 25/11/06 09:39AM, Karthik Nayak wrote:\n>> @@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,\n>> \tif (retcode)\n>> \t\tgoto cleanup;\n>>\n>> -\tretcode = ref_transaction_commit(transaction, &err);\n>> -\tif (retcode) {\n>> -\t\t/*\n>> -\t\t * Explicitly handle transaction cleanup to avoid\n>> -\t\t * aborting an already closed transaction.\n>> -\t\t */\n>> -\t\tref_transaction_free(transaction);\n>> -\t\ttransaction = NULL;\n>> -\t\tgoto cleanup;\n>> -\t}\n>> -\n>> -\tif (!atomic_fetch) {\n>> -\t\tstruct ref_rejection_data data = {\n>> -\t\t\t.retcode = &retcode,\n>> -\t\t\t.conflict_msg_shown = 0,\n>> -\t\t\t.remote_name = transport->remote->name,\n>> -\t\t};\n>> -\n>> -\t\tref_transaction_for_each_rejected_update(transaction,\n>> -\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n>> -\t\t\t\t\t\t\t &data);\n>> -\t\tif (retcode) {\n>> -\t\t\tref_transaction_free(transaction);\n>> -\t\t\ttransaction = NULL;\n>> -\t\t\tgoto cleanup;\n>> -\t\t}\n>> -\t}\n>> +\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n>> +\t\t\t\t\t transport->remote->name, &err);\n>\n> It looks like previously, whenever `ref_transaction_commit()` or\n> `ref_transaction_rejection_handler()` returned a non-zero value, we\n> would \"goto cleanup\". Now that these operations are handled via\n> `commit_ref_transaction()` though, it looks like we no longer handle the\n> \"retcode\" return value and just continue. I think we still need to check\n> the return value here.\n>\n> -Justin\n\nGood catch, will add this in. Thanks\n"},{"id":"530367","messageId":"aQ39P0mAFqDGPYxS@pks.im","threadId":"64423","inReplyTo":"CAOLa=ZQpTqnCQs4=wcUwJOWy5mXiG4y_eTiFtPkS2uOk4U66Tw@mail.gmail.com","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-07T14:07:59Z","receivedAt":"2025-11-07T14:08:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 07, 2025 at 05:15:32AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> > On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:\n> > The following test demonstrates this behaviour:\n> >\n> > \ttest_expect_success \"fetch single branch without explicit tag option\" '\n> > \t\tgit init source &&\n> > \t\tgit -C source commit --allow-empty --message common &&\n> > \t\tgit clone file://\"$(pwd)\"/source target &&\n> > \t\t(\n> > \t\t\tcd source &&\n> > \t\t\tgit commit --allow-empty --message discard-me &&\n> > \t\t\tgit tag discard-me &&\n> > \t\t\tgit commit --amend --allow-empty --message fetch-me &&\n> > \t\t\tgit tag fetch-me\n> > \t\t) &&\n> >\n> > \t\t# The \"discard-me\" tag does not point into the history that we are\n> > \t\t# about to fetch, so it should not have been created.\n> > \t\tgit -C target fetch origin &&\n> > \t\tgit -C target tag -l >actual &&\n> > \t\techo \"fetch-me\" >expect &&\n> >\n> > \t\t# But with \"--tags\" we instruct git-fetch(1) to fetch all tags, so we\n> > \t\t# should now see it.\n> > \t\tgit -C target fetch origin --tags &&\n> \n> Here, we don't really backfill, but rather we request all tags from the\n> remote, hence we end up with the 'discard-me' tag. Not because of the\n> diverged history. I also confirmed this by adding a breakpoint into the\n> `backfill_tags()` function, while running this test.\n\nOh, exactly. But there's two fetches here: the first one only fetches\n\"fetch-me\" because we don't pass \"--tags\". The second one was simply as\na demonstration that we would also fetch the other tag that doesn't\npoint into our fetched history with \"--tags\".\n\nI notice though that the first fetch forgot to `test_cmp`.\n\n> > \t\tgit -C target tag -l >actual &&\n> > \t\tcat >expect <<-\\EOF &&\n> > \t\tdiscard-me\n> > \t\tfetch-me\n> > \t\tEOF\n> > \t\ttest_cmp expect actual\n> > \t'\n> \n> But I was able to slightly modify the test to get the required affect:\n> \n> test_expect_success \"backfill tags when providing a refspec\" '\n> \tgit init source &&\n> \tgit -C source commit --allow-empty --message common &&\n> \tgit clone file://\"$(pwd)\"/source target &&\n> \t(\n> \t cd source &&\n> \t git commit --allow-empty --message history &&\n> \t git tag history &&\n> \t git commit --allow-empty --message fetch-me &&\n> \t git tag fetch-me\n> \t) &&\n> \n> \t# The \"history\" tag is backfilled eventhough we requested\n> \t# to only fetch the master\n> \tgit -C target fetch origin master:branch &&\n> \tgit -C target tag -l >actual &&\n> \tcat >expect <<-\\EOF &&\n> \tfetch-me\n> \thistory\n> \tEOF\n> \ttest_cmp expect actual\n> '\n> \n> I will add this in. Thanks for the explanation, it really helped\n> consolidate my understanding here.\n\nYup, that should work, as well.\n\n> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> >> index c7ff3480fb..d5aee5af10 100644\n> >> --- a/builtin/fetch.c\n> >> +++ b/builtin/fetch.c\n> >> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n[snip]\n> >> +\tif (*transaction && !is_atomic) {\n> >> +\t\tstruct ref_rejection_data data = {\n> >> +\t\t\t.conflict_msg_shown = 0,\n> >> +\t\t\t.remote_name = remote_name,\n> >> +\t\t\t.retcode = &retcode,\n> >> +\t\t};\n> >> +\n> >> +\t\tref_transaction_for_each_rejected_update(*transaction,\n> >> +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> >> +\t\t\t\t\t\t\t &data);\n> >> +\n> >> +\t\tref_transaction_free(*transaction);\n> >> +\t\t*transaction = NULL;\n> >> +\t}\n> >\n> > Okay. Do we need to discern cases where this is called and we haven't\n> > managed to even queue a single reference update?\n> >\n> \n> I don't see a reason. This is anyways a post-commit action, if there are\n> no updates, there will be no rejections. So this will be a no-op.\n\nI guess the question was rather whether we fear a negative consequence\nby trying to commit an empty transaction. The commit doesn't know to\nshort-circuit empty transactions, so we'd still end up locking data even\nthough we eventually end up doing nothing.\n\nThanks!\n\nPatrick\n"},{"id":"530376","messageId":"CAOLa=ZS3oFiopf0ys2ZS5z0MdE8s6jqapPyaR86gj5CcJ9jaYQ@mail.gmail.com","threadId":"64423","inReplyTo":"aQ39P0mAFqDGPYxS@pks.im","subject":"Re: [PATCH v2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-07T15:13:54Z","receivedAt":"2025-11-07T15:13:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Nov 07, 2025 at 05:15:32AM -0800, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>> > On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:\n>> > The following test demonstrates this behaviour:\n>> >\n>> > \ttest_expect_success \"fetch single branch without explicit tag option\" '\n>> > \t\tgit init source &&\n>> > \t\tgit -C source commit --allow-empty --message common &&\n>> > \t\tgit clone file://\"$(pwd)\"/source target &&\n>> > \t\t(\n>> > \t\t\tcd source &&\n>> > \t\t\tgit commit --allow-empty --message discard-me &&\n>> > \t\t\tgit tag discard-me &&\n>> > \t\t\tgit commit --amend --allow-empty --message fetch-me &&\n>> > \t\t\tgit tag fetch-me\n>> > \t\t) &&\n>> >\n>> > \t\t# The \"discard-me\" tag does not point into the history that we are\n>> > \t\t# about to fetch, so it should not have been created.\n>> > \t\tgit -C target fetch origin &&\n>> > \t\tgit -C target tag -l >actual &&\n>> > \t\techo \"fetch-me\" >expect &&\n>> >\n>> > \t\t# But with \"--tags\" we instruct git-fetch(1) to fetch all tags, so we\n>> > \t\t# should now see it.\n>> > \t\tgit -C target fetch origin --tags &&\n>>\n>> Here, we don't really backfill, but rather we request all tags from the\n>> remote, hence we end up with the 'discard-me' tag. Not because of the\n>> diverged history. I also confirmed this by adding a breakpoint into the\n>> `backfill_tags()` function, while running this test.\n>\n> Oh, exactly. But there's two fetches here: the first one only fetches\n> \"fetch-me\" because we don't pass \"--tags\". The second one was simply as\n> a demonstration that we would also fetch the other tag that doesn't\n> point into our fetched history with \"--tags\".\n>\n\nYup, even the first 'fetch' doesn't hit the backfill flow. Since it\npoints to the reference being fetched.\n\n> I notice though that the first fetch forgot to `test_cmp`.\n>\n>> > \t\tgit -C target tag -l >actual &&\n>> > \t\tcat >expect <<-\\EOF &&\n>> > \t\tdiscard-me\n>> > \t\tfetch-me\n>> > \t\tEOF\n>> > \t\ttest_cmp expect actual\n>> > \t'\n>>\n>> But I was able to slightly modify the test to get the required affect:\n>>\n>> test_expect_success \"backfill tags when providing a refspec\" '\n>> \tgit init source &&\n>> \tgit -C source commit --allow-empty --message common &&\n>> \tgit clone file://\"$(pwd)\"/source target &&\n>> \t(\n>> \t cd source &&\n>> \t git commit --allow-empty --message history &&\n>> \t git tag history &&\n>> \t git commit --allow-empty --message fetch-me &&\n>> \t git tag fetch-me\n>> \t) &&\n>>\n>> \t# The \"history\" tag is backfilled eventhough we requested\n>> \t# to only fetch the master\n>> \tgit -C target fetch origin master:branch &&\n>> \tgit -C target tag -l >actual &&\n>> \tcat >expect <<-\\EOF &&\n>> \tfetch-me\n>> \thistory\n>> \tEOF\n>> \ttest_cmp expect actual\n>> '\n>>\n>> I will add this in. Thanks for the explanation, it really helped\n>> consolidate my understanding here.\n>\n> Yup, that should work, as well.\n>\n>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> >> index c7ff3480fb..d5aee5af10 100644\n>> >> --- a/builtin/fetch.c\n>> >> +++ b/builtin/fetch.c\n>> >> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n> [snip]\n>> >> +\tif (*transaction && !is_atomic) {\n>> >> +\t\tstruct ref_rejection_data data = {\n>> >> +\t\t\t.conflict_msg_shown = 0,\n>> >> +\t\t\t.remote_name = remote_name,\n>> >> +\t\t\t.retcode = &retcode,\n>> >> +\t\t};\n>> >> +\n>> >> +\t\tref_transaction_for_each_rejected_update(*transaction,\n>> >> +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n>> >> +\t\t\t\t\t\t\t &data);\n>> >> +\n>> >> +\t\tref_transaction_free(*transaction);\n>> >> +\t\t*transaction = NULL;\n>> >> +\t}\n>> >\n>> > Okay. Do we need to discern cases where this is called and we haven't\n>> > managed to even queue a single reference update?\n>> >\n>>\n>> I don't see a reason. This is anyways a post-commit action, if there are\n>> no updates, there will be no rejections. So this will be a no-op.\n>\n> I guess the question was rather whether we fear a negative consequence\n> by trying to commit an empty transaction. The commit doesn't know to\n> short-circuit empty transactions, so we'd still end up locking data even\n> though we eventually end up doing nothing.\n>\n> Thanks!\n>\n> Patrick\n\nThat's correct, but that's also an internal detail of the reference\nbackend.\n\n - In the files backend, since we lock individual files, no updates\n means no locks.\n\n - The packed backend and reftable backend would lock the entire\n backend.\n\nSo I guess this is something we should fix on the backends themselves.\n"},{"id":"530409","messageId":"20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v3 0/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-08T21:34:42Z","receivedAt":"2025-11-08T21:34:55Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 73 ++++++++++++++++++++++++++++++++++++--------------------\n t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 109 insertions(+), 26 deletions(-)\n\nKarthik Nayak (2):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n\nRange-diff versus v2:\n\n1: 703593ef40 ! 1: 8a1efdd999 fetch: fix non-conflicting tags not being committed\n @@ Metadata\n Author: Karthik Nayak \n \n ## Commit message ##\n - fetch: fix non-conflicting tags not being committed\n + fetch: extract out reference committing logic\n \n - The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\n - updated the 'git-fetch(1)' command to use batched updates. This batches\n - updates to gain performance improvements. When fetching references, each\n - update is added to the transaction. Finally, when committing, individual\n - updates are allowed to fail with reason, while the transaction itself\n - succeeds.\n + The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\n + Part of this is to fetch and store references. This is done by\n \n - One scenario which was missed here, was fetching tags. When fetching\n - conflicting tags, the `fetch_and_consume_refs()` function returns '1',\n - which skipped committing the transaction and directly jumped to the\n - cleanup section. This mean that no updates were applied. This also\n - extends to backfilling tags when using the now deprecated 'branches/'\n - format for remotes.\n + 1. Creating a reference transaction (non-atomic mode uses batched\n + updates).\n + 2. Adding individual reference updates to the transaction.\n + 3. Committing the transaction.\n + 4. When using batched updates, handling the rejected updates.\n \n - Fix this by committing the transaction even when we have an error code.\n - This ensures other references are applied. Do this by extracting out the\n - transaction commit code into a new `commit_ref_transaction()` function\n - and using that.\n + The following commit, will fix a bug wherein fetching tags with\n + conflicts was causing other reference updates to fail. Fixing this\n + requires utilizing this logic in different regions of the function.\n \n - Add tests to check for this regression. While here, add a missing\n - cleanup from previous test.\n + In preparation of the follow up commit, extract the committing and\n + rejection handling logic into a separate function called\n + `commit_ref_transaction()`.\n \n - Reported-by: David Bohman \n Signed-off-by: Karthik Nayak \n \n ## builtin/fetch.c ##\n @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \n - \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n - \t\t\t\t &fetch_head, config)) {\n -+\t\t/* As we're using batched updates, commit any pending updates. */\n -+\t\tif (!atomic_fetch)\n -+\t\t\tcommit_ref_transaction(&transaction, false,\n -+\t\t\t\t\t transport->remote->name, &err);\n - \t\tretcode = 1;\n - \t\tgoto cleanup;\n - \t}\n -@@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n - \t\t\t * the transaction and don't commit anything.\n - \t\t\t */\n - \t\t\tif (backfill_tags(&display_state, transport, transaction, tags_ref_map,\n --\t\t\t\t\t &fetch_head, config))\n -+\t\t\t\t\t &fetch_head, config)) {\n -+\t\t\t\tif (!atomic_fetch)\n -+\t\t\t\t\tcommit_ref_transaction(&transaction, false,\n -+\t\t\t\t\t\t\t transport->remote->name, &err);\n - \t\t\t\tretcode = 1;\n -+\t\t\t}\n - \t\t}\n - \n - \t\tfree_refs(tags_ref_map);\n -@@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n -\t\t */\n -\t\tref_transaction_free(transaction);\n -\t\ttransaction = NULL;\n --\t\tgoto cleanup;\n ++\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n ++\t\t\t\t\t transport->remote->name, &err);\n ++\tif (retcode)\n + \t\tgoto cleanup;\n -\t}\n -\n -\tif (!atomic_fetch) {\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n -\t\t\tgoto cleanup;\n -\t\t}\n -\t}\n -+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n -+\t\t\t\t\t transport->remote->name, &err);\n \n \tcommit_fetch_head(&fetch_head);\n \n -\n - ## t/t5510-fetch.sh ##\n -@@ t/t5510-fetch.sh: test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n - '\n - \n - test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n -+\ttest_when_finished rm -rf base repo &&\n - \t(\n - \t\tgit init --ref-format=reftable base &&\n - \t\tcd base &&\n -@@ t/t5510-fetch.sh: test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n - \t)\n - '\n - \n -+test_expect_success 'fetch --tags fetches existing tags' '\n -+\ttest_when_finished rm -rf base repo &&\n -+\n -+\tgit init base &&\n -+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n -+\n -+\tgit clone --bare base repo &&\n -+\n -+\tgit -C base tag tag-1 &&\n -+\tgit -C repo for-each-ref >out &&\n -+\ttest_grep ! \"tag-1\" out &&\n -+\tgit -C repo fetch --tags &&\n -+\tgit -C repo for-each-ref >out &&\n -+\ttest_grep \"tag-1\" out\n -+'\n -+\n -+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n -+\ttest_when_finished rm -rf base repo &&\n -+\n -+\tgit init base &&\n -+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n -+\tgit -C base tag tag-1 &&\n -+\n -+\tgit clone --bare base repo &&\n -+\n -+\tgit -C base tag tag-2 &&\n -+\tgit -C repo for-each-ref >out &&\n -+\ttest_grep ! \"tag-2\" out &&\n -+\n -+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n -+\tgit -C base tag -f tag-1 &&\n -+\n -+\ttest_must_fail git -C repo fetch --tags 2>out &&\n -+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n -+\tgit -C repo for-each-ref >out &&\n -+\ttest_grep \"tag-2\" out\n -+'\n -+\n -+test_expect_success 'backfill tags with branches remote format' '\n -+\ttest_when_finished rm -rf base repo &&\n -+\n -+\tgit init base &&\n -+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n -+\tgit -C base tag tag1 &&\n -+\n -+\tgit clone --no-tags base repo &&\n -+\n -+\tgit -C repo remote remove origin &&\n -+\tmkdir -p repo/.git/branches &&\n -+\techo \"$(cd base && pwd)#master\" >repo/.git/branches/origin &&\n -+\n -+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n -+\tgit -C base tag tag2 &&\n -+\n -+\tgit -C repo fetch origin &&\n -+\tgit -C repo for-each-ref refs/tags >out &&\n -+\ttest_grep \"tag1\" out &&\n -+\ttest_grep \"tag2\" out\n -+'\n -+\n - . \"$TEST_DIRECTORY\"/lib-httpd.sh\n - start_httpd\n - \n-: ---------- > 2: 1de8d8b953 fetch: fix non-conflicting tags not being committed\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"530410","messageId":"20251108-fix-tags-not-fetching-v3-1-a12ab6c4daef@gmail.com","threadId":"64423","inReplyTo":"20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com","subject":"[PATCH v3 1/2] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-08T21:34:43Z","receivedAt":"2025-11-08T21:34:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 65 ++++++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 39 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..49e195199e 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode) {\n+\t\t/*\n+\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t * aborting an already closed transaction.\n+\t\t */\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\tif (*transaction && !is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\n+\t\tref_transaction_free(*transaction);\n+\t\t*transaction = NULL;\n+\t}\n+\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1894,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.0\n\n"},{"id":"530411","messageId":"20251108-fix-tags-not-fetching-v3-2-a12ab6c4daef@gmail.com","threadId":"64423","inReplyTo":"20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com","subject":"[PATCH v3 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-08T21:34:44Z","receivedAt":"2025-11-08T21:35:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction even when we have an error code.\nThis ensures other references are applied. Add tests to check for this\nregression. While here, add a missing cleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 49e195199e..337ca2b0af 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1963,6 +1963,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode > 0 && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..e62190d5d7 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t git commit --allow-empty --message history &&\n+\t git tag history &&\n+\t git commit --allow-empty --message fetch-me &&\n+\t git tag fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.0\n\n"},{"id":"530432","messageId":"aRGVd7L2DV4DNM-h@pks.im","threadId":"64423","inReplyTo":"20251108-fix-tags-not-fetching-v3-2-a12ab6c4daef@gmail.com","subject":"Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-10T07:34:15Z","receivedAt":"2025-11-10T07:34:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 49e195199e..337ca2b0af 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1963,6 +1963,14 @@ static int do_fetch(struct transport *transport,\n> \t}\n> \n> cleanup:\n> +\t/*\n> +\t * When using batched updates, we want to commit the non-rejected\n> +\t * updates and also handle the rejections.\n> +\t */\n> +\tif (retcode > 0 && !atomic_fetch && transaction)\n> +\t\tcommit_ref_transaction(&transaction, false,\n> +\t\t\t\t transport->remote->name, &err);\n\nI think this needs some explanation why this condition is safe. There's\nquite a bunch of function calls and conditions that assign to it:\n\n - `truncate_fetch_head()` only ever assigns negative. This will be\n ignored as expected.\n\n - `open_fetch_head()` behaves likewise.\n\n - `prune_refs()` returns negative, but we then turn the return code\n into `1`. So we'd end up calling `commit_ref_transaction()` in this\n case, but we didn't in the previous iteration of this patch series.\n Was this intentional?\n\n - `fetch_and_consume_refs()` is one of the intended cases, and it sets\n up a positive retcode indeed.\n\n - `backfill_tags()` behaves likewise, and was intended.\n\nSo this looks good to me, with the only questionable one being\n`prune_refs()`.\n\nPatrick\n"},{"id":"530433","messageId":"aRGVhA4eXnAFxvqE@pks.im","threadId":"64423","inReplyTo":"20251108-fix-tags-not-fetching-v3-1-a12ab6c4daef@gmail.com","subject":"Re: [PATCH v3 1/2] fetch: extract out reference committing logic","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-10T07:34:28Z","receivedAt":"2025-11-10T07:34:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index c7ff3480fb..49e195199e 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n> \t*data->retcode = 1;\n> }\n> \n> +/*\n> + * Commit the reference transaction. If it isn't an atomic transaction, handle\n> + * rejected updates as part of using batched updates.\n> + */\n> +static int commit_ref_transaction(struct ref_transaction **transaction,\n> +\t\t\t\t bool is_atomic, const char *remote_name,\n> +\t\t\t\t struct strbuf *err)\n> +{\n> +\tint retcode = ref_transaction_commit(*transaction, err);\n> +\tif (retcode) {\n> +\t\t/*\n> +\t\t * Explicitly handle transaction cleanup to avoid\n> +\t\t * aborting an already closed transaction.\n> +\t\t */\n> +\t\tref_transaction_free(*transaction);\n> +\t\t*transaction = NULL;\n> +\t}\n> +\n> +\tif (*transaction && !is_atomic) {\n\nThis condition is somewhat weird, as we know that it won't ever execute\nif `retcode` is non-zero. So wouldn't the function be way easier to\nfollow if you turned the above conditional into a `goto out`?\n\n\tstatic int commit_ref_transaction(struct ref_transaction **transaction,\n\t\t\t\t\t bool is_atomic, const char *remote_name,\n\t\t\t\t\t struct strbuf *err)\n\t{\n\t\tint retcode;\n\n\t\tretcode = ref_transaction_commit(*transaction, err);\n\t\tif (retcode)\n\t\t\tgoto out;\n\n\t\tif (!is_atomic) {\n\t\t\tstruct ref_rejection_data data = {\n\t\t\t\t.conflict_msg_shown = 0,\n\t\t\t\t.remote_name = remote_name,\n\t\t\t\t.retcode = &retcode,\n\t\t\t};\n\n\t\t\tref_transaction_for_each_rejected_update(*transaction,\n\t\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n\t\t\t\t\t\t\t\t &data);\n\t\t}\n\nout:\n\t\tref_transaction_free(*transaction);\n\t\t*transaction = NULL;\n\t\treturn retcode;\n\t}\n\nThis feels significantly easier to read to me.\n\nPatrick\n"},{"id":"530439","messageId":"CAOLa=ZQLWF_GBtYXN9F=+=BwqugYOH=Z9OuNV1n3VmnH=rbqpA@mail.gmail.com","threadId":"64423","inReplyTo":"aRGVhA4eXnAFxvqE@pks.im","subject":"Re: [PATCH v3 1/2] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-10T13:11:14Z","receivedAt":"2025-11-10T13:11:16Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index c7ff3480fb..49e195199e 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,\n>> \t*data->retcode = 1;\n>> }\n>>\n>> +/*\n>> + * Commit the reference transaction. If it isn't an atomic transaction, handle\n>> + * rejected updates as part of using batched updates.\n>> + */\n>> +static int commit_ref_transaction(struct ref_transaction **transaction,\n>> +\t\t\t\t bool is_atomic, const char *remote_name,\n>> +\t\t\t\t struct strbuf *err)\n>> +{\n>> +\tint retcode = ref_transaction_commit(*transaction, err);\n>> +\tif (retcode) {\n>> +\t\t/*\n>> +\t\t * Explicitly handle transaction cleanup to avoid\n>> +\t\t * aborting an already closed transaction.\n>> +\t\t */\n>> +\t\tref_transaction_free(*transaction);\n>> +\t\t*transaction = NULL;\n>> +\t}\n>> +\n>> +\tif (*transaction && !is_atomic) {\n>\n> This condition is somewhat weird, as we know that it won't ever execute\n> if `retcode` is non-zero. So wouldn't the function be way easier to\n> follow if you turned the above conditional into a `goto out`?\n>\n\nI don't have any arguments here, I basically simply moved the code as is\nand didn't want to make changes to reduce the review load.\n\n> \tstatic int commit_ref_transaction(struct ref_transaction **transaction,\n> \t\t\t\t\t bool is_atomic, const char *remote_name,\n> \t\t\t\t\t struct strbuf *err)\n> \t{\n> \t\tint retcode;\n>\n> \t\tretcode = ref_transaction_commit(*transaction, err);\n> \t\tif (retcode)\n> \t\t\tgoto out;\n>\n> \t\tif (!is_atomic) {\n> \t\t\tstruct ref_rejection_data data = {\n> \t\t\t\t.conflict_msg_shown = 0,\n> \t\t\t\t.remote_name = remote_name,\n> \t\t\t\t.retcode = &retcode,\n> \t\t\t};\n>\n> \t\t\tref_transaction_for_each_rejected_update(*transaction,\n> \t\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n> \t\t\t\t\t\t\t\t &data);\n> \t\t}\n>\n> out:\n> \t\tref_transaction_free(*transaction);\n> \t\t*transaction = NULL;\n> \t\treturn retcode;\n> \t}\n>\n> This feels significantly easier to read to me.\n>\n> Patrick\n\nI do agree here, and since the code is small, I think it is worthwhile\nmaking this change. Will add in. Thanks\n"},{"id":"530440","messageId":"CAOLa=ZS4wJnsCffg6EcECFEzqBo_xV+dyNi5L=4iaLqcMwPphA@mail.gmail.com","threadId":"64423","inReplyTo":"aRGVd7L2DV4DNM-h@pks.im","subject":"Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-10T13:23:07Z","receivedAt":"2025-11-10T13:23:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 49e195199e..337ca2b0af 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1963,6 +1963,14 @@ static int do_fetch(struct transport *transport,\n>> \t}\n>>\n>> cleanup:\n>> +\t/*\n>> +\t * When using batched updates, we want to commit the non-rejected\n>> +\t * updates and also handle the rejections.\n>> +\t */\n>> +\tif (retcode > 0 && !atomic_fetch && transaction)\n>> +\t\tcommit_ref_transaction(&transaction, false,\n>> +\t\t\t\t transport->remote->name, &err);\n>\n> I think this needs some explanation why this condition is safe. There's\n> quite a bunch of function calls and conditions that assign to it:\n>\n> - `truncate_fetch_head()` only ever assigns negative. This will be\n> ignored as expected.\n>\n> - `open_fetch_head()` behaves likewise.\n>\n\nAlso the transaction isn't even defined until this stage.\n\n> - `prune_refs()` returns negative, but we then turn the return code\n> into `1`. So we'd end up calling `commit_ref_transaction()` in this\n> case, but we didn't in the previous iteration of this patch series.\n> Was this intentional?\n\nIts basically the same, before batched updates, we would return the\nreturn code of `refs_delete_refs()` from within `prune_refs()`.\n\nThe fn `refs_delete_refs()` creates a transaction within to delete all\nrefs, this is done because we delete refs without old OID and hence they\nwouldn't ever fail.\n\nSo now, when we pass our transaction to `prune_refs()`, it is also safe\nto commit it.\n\nOne scenario I didn't think of earlier was that we would now enable\npartial pruning with this change. But I would argue that this is\ndesirable since that is how we deal with other ref updates during fetch.\n\n>\n> - `fetch_and_consume_refs()` is one of the intended cases, and it sets\n> up a positive retcode indeed.\n>\n> - `backfill_tags()` behaves likewise, and was intended.\n>\n> So this looks good to me, with the only questionable one being\n> `prune_refs()`.\n>\n> Patrick\n\nEither ways, I would think that we should elaborate a little here.\n\nThanks,\nKarthik\n"},{"id":"530507","messageId":"20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v4 0/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-11T13:27:06Z","receivedAt":"2025-11-11T13:27:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v4:\n- Cleanup the code in the first commit to make it simpler to read.\n- In the second commit, we were specifically checking for `retcode > 0`\n for committing the transaction. This is a bit confusing since that\n begs the questions why not `retcode < 0`. There is no real reason\n there, so I've change the code to simple do `if (retcode && ...)`.\n I've also added more information about the flows which would commit\n the transaction in the commit message.\n- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com\n\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 67 ++++++++++++++++++++++++++++++++++----------------------\n t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 103 insertions(+), 26 deletions(-)\n\nKarthik Nayak (2):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n\nRange-diff versus v3:\n\n1: ee20b46cc2 ! 1: 49fa9a85ef fetch: extract out reference committing logic\n @@ Commit message\n rejection handling logic into a separate function called\n `commit_ref_transaction()`.\n \n + Helped-by: Patrick Steinhardt \n Signed-off-by: Karthik Nayak \n \n ## builtin/fetch.c ##\n @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna\n +\t\t\t\t struct strbuf *err)\n +{\n +\tint retcode = ref_transaction_commit(*transaction, err);\n -+\tif (retcode) {\n -+\t\t/*\n -+\t\t * Explicitly handle transaction cleanup to avoid\n -+\t\t * aborting an already closed transaction.\n -+\t\t */\n -+\t\tref_transaction_free(*transaction);\n -+\t\t*transaction = NULL;\n -+\t}\n ++\tif (retcode)\n ++\t\tgoto out;\n +\n -+\tif (*transaction && !is_atomic) {\n ++\tif (!is_atomic) {\n +\t\tstruct ref_rejection_data data = {\n +\t\t\t.conflict_msg_shown = 0,\n +\t\t\t.remote_name = remote_name,\n @@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna\n +\t\tref_transaction_for_each_rejected_update(*transaction,\n +\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n +\t\t\t\t\t\t\t &data);\n -+\n -+\t\tref_transaction_free(*transaction);\n -+\t\t*transaction = NULL;\n +\t}\n +\n ++out:\n ++\tref_transaction_free(*transaction);\n ++\t*transaction = NULL;\n +\treturn retcode;\n +}\n +\n2: 543b67c97c ! 2: 12c71b602d fetch: fix non-conflicting tags not being committed\n @@ Commit message\n extends to backfilling tags which is done when fetching specific\n refspecs which contains tags in their history.\n \n - Fix this by committing the transaction even when we have an error code.\n - This ensures other references are applied. Add tests to check for this\n - regression. While here, add a missing cleanup from previous test.\n + Fix this by committing the transaction when we have an error code and\n + not using an atomic transaction. This ensures other references are\n + applied even when some updates fail.\n +\n + The cleanup section is reached with `retcode` set in several scenarios:\n +\n + - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n + before the transaction is created, so no commit is attempted.\n +\n + - `prune_refs()` sets `retcode` after creating the transaction, so\n + the commit will now proceed. Before batched updates, `prune_refs()`\n + created its own transaction internally with all-or-nothing\n + semantics. This was done since all deletions were made without an\n + old OID, which meant they were assumed to never fail. This change\n + allows partial deletions to succeed, consistent with how other\n + reference updates behave during fetch.\n +\n + - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n + cases this fix targets, both setting a positive `retcode` to\n + trigger the committing of the transaction.\n +\n + This simplifies error handling and ensures future modifications to\n + `do_fetch()` don't need special handling for batched updates.\n +\n + Add tests to check for this regression. While here, add a missing\n + cleanup from previous test.\n \n Reported-by: David Bohman \n Helped-by: Patrick Steinhardt \n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n +\t * When using batched updates, we want to commit the non-rejected\n +\t * updates and also handle the rejections.\n +\t */\n -+\tif (retcode > 0 && !atomic_fetch && transaction)\n ++\tif (retcode && !atomic_fetch && transaction)\n +\t\tcommit_ref_transaction(&transaction, false,\n +\t\t\t\t transport->remote->name, &err);\n +\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"530508","messageId":"20251111-fix-tags-not-fetching-v4-1-185d836ec62a@gmail.com","threadId":"64423","inReplyTo":"20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com","subject":"[PATCH v4 1/2] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-11T13:27:07Z","receivedAt":"2025-11-11T13:27:12Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 59 ++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..f90179040b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode)\n+\t\tgoto out;\n+\n+\tif (!is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\t}\n+\n+out:\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.0\n\n"},{"id":"530509","messageId":"20251111-fix-tags-not-fetching-v4-2-185d836ec62a@gmail.com","threadId":"64423","inReplyTo":"20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com","subject":"[PATCH v4 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-11T13:27:08Z","receivedAt":"2025-11-11T13:27:13Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction when we have an error code and\nnot using an atomic transaction. This ensures other references are\napplied even when some updates fail.\n\nThe cleanup section is reached with `retcode` set in several scenarios:\n\n - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n before the transaction is created, so no commit is attempted.\n\n - `prune_refs()` sets `retcode` after creating the transaction, so\n the commit will now proceed. Before batched updates, `prune_refs()`\n created its own transaction internally with all-or-nothing\n semantics. This was done since all deletions were made without an\n old OID, which meant they were assumed to never fail. This change\n allows partial deletions to succeed, consistent with how other\n reference updates behave during fetch.\n\n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n trigger the committing of the transaction.\n\nThis simplifies error handling and ensures future modifications to\n`do_fetch()` don't need special handling for batched updates.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f90179040b..b19fa8e966 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..e62190d5d7 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t git commit --allow-empty --message history &&\n+\t git tag history &&\n+\t git commit --allow-empty --message fetch-me &&\n+\t git tag fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.0\n\n"},{"id":"530557","messageId":"aRQmVPe1RsFcr4hz@pks.im","threadId":"64423","inReplyTo":"20251111-fix-tags-not-fetching-v4-2-185d836ec62a@gmail.com","subject":"Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-12T06:16:52Z","receivedAt":"2025-11-12T06:17:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:\n> The cleanup section is reached with `retcode` set in several scenarios:\n> \n> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n> before the transaction is created, so no commit is attempted.\n> \n> - `prune_refs()` sets `retcode` after creating the transaction, so\n> the commit will now proceed. Before batched updates, `prune_refs()`\n> created its own transaction internally with all-or-nothing\n> semantics. This was done since all deletions were made without an\n> old OID, which meant they were assumed to never fail. This change\n> allows partial deletions to succeed, consistent with how other\n> reference updates behave during fetch.\n\nOkay, so we do have a change in behaviour for `prune_refs()`. I guess\nthe reasoning is sound, but I was wondering why we don't have a test for\nthis.\n\nI guess the reason is that, as you said, it should in theory always\nsucceed. But what if with the \"files\" backend one of the refs that we're\nabout to prune was locked? Would that be a case where we continue with\npruning the remaining refs now?\n\nThanks!\n\nPatrick\n"},{"id":"530578","messageId":"CAOLa=ZQAQ1dtstD+uqh=vzV+w5q2uWsnZkzqucHuj_W_VL931A@mail.gmail.com","threadId":"64423","inReplyTo":"aRQmVPe1RsFcr4hz@pks.im","subject":"Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-12T08:52:47Z","receivedAt":"2025-11-12T08:52:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:\n>> The cleanup section is reached with `retcode` set in several scenarios:\n>>\n>> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n>> before the transaction is created, so no commit is attempted.\n>>\n>> - `prune_refs()` sets `retcode` after creating the transaction, so\n>> the commit will now proceed. Before batched updates, `prune_refs()`\n>> created its own transaction internally with all-or-nothing\n>> semantics. This was done since all deletions were made without an\n>> old OID, which meant they were assumed to never fail. This change\n>> allows partial deletions to succeed, consistent with how other\n>> reference updates behave during fetch.\n>\n> Okay, so we do have a change in behaviour for `prune_refs()`. I guess\n> the reasoning is sound, but I was wondering why we don't have a test for\n> this.\n>\n> I guess the reason is that, as you said, it should in theory always\n> succeed. But what if with the \"files\" backend one of the refs that we're\n> about to prune was locked? Would that be a case where we continue with\n> pruning the remaining refs now?\n>\n\nI was thinking of concurrent writes to lock the reference, and didn't\nthink of a nice way to do this. Your solution works and is indeed better.\n\nI started writing the test and realized that the pruning happens before\nwe create the batched updates transaction. So I was _wrong_ and there is\nno change for `prune_refs()` either, as the transaction is never defined\nat this stage. Will amend and send in a new version.\n\n> Thanks!\n>\n> Patrick\n"},{"id":"530595","messageId":"xmqq346jtdky.fsf@gitster.g","threadId":"64423","inReplyTo":"CAOLa=ZQAQ1dtstD+uqh=vzV+w5q2uWsnZkzqucHuj_W_VL931A@mail.gmail.com","subject":"Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-12T16:34:05Z","receivedAt":"2025-11-12T16:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Patrick Steinhardt writes:\n>\n>> On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:\n>>> The cleanup section is reached with `retcode` set in several scenarios:\n>>>\n>>> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n>>> before the transaction is created, so no commit is attempted.\n>>>\n>>> - `prune_refs()` sets `retcode` after creating the transaction, so\n>>> the commit will now proceed. Before batched updates, `prune_refs()`\n>>> created its own transaction internally with all-or-nothing\n>>> semantics. This was done since all deletions were made without an\n>>> old OID, which meant they were assumed to never fail. This change\n>>> allows partial deletions to succeed, consistent with how other\n>>> reference updates behave during fetch.\n>>\n>> Okay, so we do have a change in behaviour for `prune_refs()`. I guess\n>> the reasoning is sound, but I was wondering why we don't have a test for\n>> this.\n>>\n>> I guess the reason is that, as you said, it should in theory always\n>> succeed. But what if with the \"files\" backend one of the refs that we're\n>> about to prune was locked? Would that be a case where we continue with\n>> pruning the remaining refs now?\n>>\n>\n> I was thinking of concurrent writes to lock the reference, and didn't\n> think of a nice way to do this. Your solution works and is indeed better.\n>\n> I started writing the test and realized that the pruning happens before\n> we create the batched updates transaction. So I was _wrong_ and there is\n> no change for `prune_refs()` either, as the transaction is never defined\n> at this stage. Will amend and send in a new version.\n\nThanks, both. I too was wondering what was going on in that part of\nthe flow.\n"},{"id":"530645","messageId":"20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v5 0/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-13T13:38:35Z","receivedAt":"2025-11-13T13:38:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v5:\n- In the previous version, I assumed that the `prune_refs()` function\n also triggers committing of batched updates. However this was\n incorrect as the transaction for batched updates, is only created\n after the call to `prune_refs()`. This makes sense, since we want to\n isolate deletions from the rest of the ref updates, to avoid\n conflicts. I've amended the commit message accordingly.\n- I noticed I missed cleanup of the repos created in the test, which\n I've now done.\n- Link to v4: https://patch.msgid.link/20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com\n\nChanges in v4:\n- Cleanup the code in the first commit to make it simpler to read.\n- In the second commit, we were specifically checking for `retcode > 0`\n for committing the transaction. This is a bit confusing since that\n begs the questions why not `retcode < 0`. There is no real reason\n there, so I've change the code to simple do `if (retcode && ...)`.\n I've also added more information about the flows which would commit\n the transaction in the commit message.\n- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com\n\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 67 ++++++++++++++++++++++++++++++++++----------------------\n t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 103 insertions(+), 26 deletions(-)\n\nKarthik Nayak (2):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n\nRange-diff versus v4:\n\n1: ba560e030b = 1: cc187b053f fetch: extract out reference committing logic\n2: 0403971a5b ! 2: 27497a1b9d fetch: fix non-conflicting tags not being committed\n @@ Commit message\n \n The cleanup section is reached with `retcode` set in several scenarios:\n \n - - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`\n - before the transaction is created, so no commit is attempted.\n -\n - - `prune_refs()` sets `retcode` after creating the transaction, so\n - the commit will now proceed. Before batched updates, `prune_refs()`\n - created its own transaction internally with all-or-nothing\n - semantics. This was done since all deletions were made without an\n - old OID, which meant they were assumed to never fail. This change\n - allows partial deletions to succeed, consistent with how other\n - reference updates behave during fetch.\n + - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n + `retcode` before the transaction is created, so no commit is\n + attempted.\n \n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n @@ t/t5510-fetch.sh: test_expect_success REFFILES 'D/F conflict on case sensitive f\n +'\n +\n +test_expect_success \"backfill tags when providing a refspec\" '\n ++\ttest_when_finished rm -rf source target &&\n ++\n +\tgit init source &&\n +\tgit -C source commit --allow-empty --message common &&\n +\tgit clone file://\"$(pwd)\"/source target &&\n +\t(\n +\t cd source &&\n -+\t git commit --allow-empty --message history &&\n -+\t git tag history &&\n -+\t git commit --allow-empty --message fetch-me &&\n -+\t git tag fetch-me\n ++\t test_commit history &&\n ++\t test_commit fetch-me\n +\t) &&\n +\n +\t# The \"history\" tag is backfilled eventhough we requested\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"530646","messageId":"20251113-fix-tags-not-fetching-v5-1-371ea7ec638d@gmail.com","threadId":"64423","inReplyTo":"20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com","subject":"[PATCH v5 1/2] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-13T13:38:36Z","receivedAt":"2025-11-13T13:38:41Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 59 ++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..f90179040b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode)\n+\t\tgoto out;\n+\n+\tif (!is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\t}\n+\n+out:\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.0\n\n"},{"id":"530647","messageId":"20251113-fix-tags-not-fetching-v5-2-371ea7ec638d@gmail.com","threadId":"64423","inReplyTo":"20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com","subject":"[PATCH v5 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-13T13:38:37Z","receivedAt":"2025-11-13T13:38:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction when we have an error code and\nnot using an atomic transaction. This ensures other references are\napplied even when some updates fail.\n\nThe cleanup section is reached with `retcode` set in several scenarios:\n\n - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n `retcode` before the transaction is created, so no commit is\n attempted.\n\n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n trigger the committing of the transaction.\n\nThis simplifies error handling and ensures future modifications to\n`do_fetch()` don't need special handling for batched updates.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f90179040b..b19fa8e966 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..4b113d7c27 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\ttest_when_finished rm -rf source target &&\n+\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t test_commit history &&\n+\t test_commit fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.0\n\n"},{"id":"530667","messageId":"xmqq7bvtlj8v.fsf@gitster.g","threadId":"64423","inReplyTo":"20251113-fix-tags-not-fetching-v5-2-371ea7ec638d@gmail.com","subject":"Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-13T21:23:28Z","receivedAt":"2025-11-13T21:23:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The cleanup section is reached with `retcode` set in several scenarios:\n>\n> - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n> `retcode` before the transaction is created, so no commit is\n> attempted.\n>\n> - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n> cases this fix targets, both setting a positive `retcode` to\n> trigger the committing of the transaction.\n>\n> This simplifies error handling and ensures future modifications to\n> `do_fetch()` don't need special handling for batched updates.\n\nThis may be sufficient to clean out the hanging transaction that the\noriginal code forgot to commit, but in scenarios where this change\nmakes a difference, i.e., where the code does \"goto cleanup\" before\nit calls commit_ref_transaction() in the main flow of the code,\nthere are things that are not performed that we may still want to\nperform. Namely, we do not\n\n - call commit_fetch_head()\n\n - run set_upstream processing\n\n - honor do_set_head flag that was left for remote that does not\n have followremotehead=never\n\nbut don't we want to do some of them at least?\n\nIf it turns out that we want to do all of them, I also wonder if the\nresulting code would become easier to follow if we lose the call to\ncommit_ref_transaction() in the main code flow, do the above three\npoints before committing the ref transaction, and then after the\ncleanup label, make a call to commit_ref_transaction() if we have an\nopen transaction and we are not atomic (regardless of the value of\nretcode at that point). That call may yield another retcode that\nthe existing error reporting at the end of this function may have to\nreact to.\n\n> cleanup:\n> +\t/*\n> +\t * When using batched updates, we want to commit the non-rejected\n> +\t * updates and also handle the rejections.\n> +\t */\n> +\tif (retcode && !atomic_fetch && transaction)\n> +\t\tcommit_ref_transaction(&transaction, false,\n> +\t\t\t\t transport->remote->name, &err);\n>\n> \tif (retcode) {\n> \t\tif (err.len) {\n> \t\t\terror(\"%s\", err.buf);\n\nIOW, something like this on top of this patch (not even compile tested).\n\n builtin/fetch.c | 16 +++-------------\n 1 file changed, 3 insertions(+), 13 deletions(-)\n\ndiff --git c/builtin/fetch.c w/builtin/fetch.c\nindex b19fa8e966..d3eb65dac6 100644\n--- c/builtin/fetch.c\n+++ w/builtin/fetch.c\n@@ -1888,11 +1888,6 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n-\t\t\t\t\t transport->remote->name, &err);\n-\tif (retcode)\n-\t\tgoto cleanup;\n-\n \tcommit_fetch_head(&fetch_head);\n \n \tif (set_upstream) {\n@@ -1957,14 +1952,9 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n-\t/*\n-\t * When using batched updates, we want to commit the non-rejected\n-\t * updates and also handle the rejections.\n-\t */\n-\tif (retcode && !atomic_fetch && transaction)\n-\t\tcommit_ref_transaction(&transaction, false,\n-\t\t\t\t transport->remote->name, &err);\n-\n+\tif (!atomic_fetch && transaction)\n+\t\tretcode = commit_ref_transaction(&transaction, false,\n+\t\t\t\t\t\t transport->remote->name, &err);\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\n"},{"id":"530764","messageId":"CAOLa=ZT9wv8B7EKXJQvwR07bUT7Jx0nJSwGGyUZ8+GN3-xdRag@mail.gmail.com","threadId":"64423","inReplyTo":"xmqq7bvtlj8v.fsf@gitster.g","subject":"Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-15T22:16:28Z","receivedAt":"2025-11-15T22:16:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> The cleanup section is reached with `retcode` set in several scenarios:\n>>\n>> - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n>> `retcode` before the transaction is created, so no commit is\n>> attempted.\n>>\n>> - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n>> cases this fix targets, both setting a positive `retcode` to\n>> trigger the committing of the transaction.\n>>\n>> This simplifies error handling and ensures future modifications to\n>> `do_fetch()` don't need special handling for batched updates.\n>\n> This may be sufficient to clean out the hanging transaction that the\n> original code forgot to commit, but in scenarios where this change\n> makes a difference, i.e., where the code does \"goto cleanup\" before\n> it calls commit_ref_transaction() in the main flow of the code,\n> there are things that are not performed that we may still want to\n> perform. Namely, we do not\n>\n> - call commit_fetch_head()\n>\n> - run set_upstream processing\n>\n> - honor do_set_head flag that was left for remote that does not\n> have followremotehead=never\n>\n> but don't we want to do some of them at least?\n>\n\nThanks for bringing this up. I would think we should do all of these,\nbut not if the '--atomic' flag is used. If the '--atomic' flag is used,\nwe shouldn't do anything else and simply skip to the end.\n\n> If it turns out that we want to do all of them, I also wonder if the\n> resulting code would become easier to follow if we lose the call to\n> commit_ref_transaction() in the main code flow, do the above three\n> points before committing the ref transaction, and then after the\n> cleanup label, make a call to commit_ref_transaction() if we have an\n> open transaction and we are not atomic (regardless of the value of\n> retcode at that point). That call may yield another retcode that\n> the existing error reporting at the end of this function may have to\n> react to.\n>\n\nThe issue is with '--atomic' again. I could think of this small change\nover this topic, which passes the fetch tests.\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b19fa8e966..df11f59f56 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1890,7 +1890,7 @@ static int do_fetch(struct transport *transport,\n\n \tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n \t\t\t\t\t transport->remote->name, &err);\n-\tif (retcode)\n+\tif (retcode && atomic_fetch)\n \t\tgoto cleanup;\n\n \tcommit_fetch_head(&fetch_head);\n\nI do wonder if we can cleanup this code, it is a bit messy right now.\n\nThat said, we could either append this change as a new commit with some\nadditional tests and re-roll the series or send it as a separate commit\nbased on this series. I'd prefer the latter so that we have the fix for\nfetching tags merged sooner, but happy to do either.\n\n[snip]\n"},{"id":"530780","messageId":"xmqqtsytbk5w.fsf@gitster.g","threadId":"64423","inReplyTo":"CAOLa=ZT9wv8B7EKXJQvwR07bUT7Jx0nJSwGGyUZ8+GN3-xdRag@mail.gmail.com","subject":"Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-17T00:02:51Z","receivedAt":"2025-11-17T00:02:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n>> perform. Namely, we do not\n>>\n>> - call commit_fetch_head()\n>>\n>> - run set_upstream processing\n>>\n>> - honor do_set_head flag that was left for remote that does not\n>> have followremotehead=never\n>>\n>> but don't we want to do some of them at least?\n>>\n>\n> Thanks for bringing this up. I would think we should do all of these,\n> but not if the '--atomic' flag is used. If the '--atomic' flag is used,\n> we shouldn't do anything else and simply skip to the end.\n\nTrue.\n\nSo when not \"--atomic\", the code with these two patches will still\nmisbehave, but it is not a regression these two patches causes.\nFailing to do any of the above three when \"--atomic\" is not in\neffect is a part of original regression in the previous cycle caused\nby the \"batched ref updates\". These two patches are trying to\naddress the regression, but these three points are not covered. Am\nI reading the situation correctly?\n\n> That said, we could either append this change as a new commit with some\n> additional tests and re-roll the series or send it as a separate commit\n> based on this series. I'd prefer the latter so that we have the fix for\n> fetching tags merged sooner, but happy to do either.\n\nEither is fine, as this won't make Git 2.52, it seems. It is OK as\nit is not a new regression, but it still is a recent one, and would\nbe nice if we have something concrete to address it soon after 2.52.\n\nThanks.\n"},{"id":"530817","messageId":"CAOLa=ZRn5=oK8+T-mt_nuWVDnVvLUMj6OkAMkD_ZTppnYKBJgg@mail.gmail.com","threadId":"64423","inReplyTo":"xmqqtsytbk5w.fsf@gitster.g","subject":"Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-17T15:38:37Z","receivedAt":"2025-11-17T15:38:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>>> perform. Namely, we do not\n>>>\n>>> - call commit_fetch_head()\n>>>\n>>> - run set_upstream processing\n>>>\n>>> - honor do_set_head flag that was left for remote that does not\n>>> have followremotehead=never\n>>>\n>>> but don't we want to do some of them at least?\n>>>\n>>\n>> Thanks for bringing this up. I would think we should do all of these,\n>> but not if the '--atomic' flag is used. If the '--atomic' flag is used,\n>> we shouldn't do anything else and simply skip to the end.\n>\n> True.\n>\n> So when not \"--atomic\", the code with these two patches will still\n> misbehave, but it is not a regression these two patches causes.\n> Failing to do any of the above three when \"--atomic\" is not in\n> effect is a part of original regression in the previous cycle caused\n> by the \"batched ref updates\". These two patches are trying to\n> address the regression, but these three points are not covered. Am\n> I reading the situation correctly?\n\nYes you're.\n\n>\n>> That said, we could either append this change as a new commit with some\n>> additional tests and re-roll the series or send it as a separate commit\n>> based on this series. I'd prefer the latter so that we have the fix for\n>> fetching tags merged sooner, but happy to do either.\n>\n> Either is fine, as this won't make Git 2.52, it seems. It is OK as\n> it is not a new regression, but it still is a recent one, and would\n> be nice if we have something concrete to address it soon after 2.52.\n>\n> Thanks.\n\nAh well, I have something locally already. So will include it in this\nseries and push a new version with the once I see greens on the CI [1].\n\nThanks,\nKarthik\n\n[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/444\n"},{"id":"530897","messageId":"20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v6 0/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-18T11:27:54Z","receivedAt":"2025-11-18T11:28:01Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\nThe third commit fixes another regression around failing to do\npost-fetch operations when ref updates fail with batched updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v6:\n- This version adds a new commit which handles another regression where\n if reference updates fail when using batched updates, we skip doing\n the post-fetch operations. Namely:\n - Updating 'FETCH_HEAD' via `commit_fetch_head()`\n - Adding upstream tracking information via `set_upstream()`\n - Setting remote 'HEAD' values when `do_set_head` is true\n- Link to v5: https://patch.msgid.link/20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com\n\nChanges in v5:\n- In the previous version, I assumed that the `prune_refs()` function\n also triggers committing of batched updates. However this was\n incorrect as the transaction for batched updates, is only created\n after the call to `prune_refs()`. This makes sense, since we want to\n isolate deletions from the rest of the ref updates, to avoid\n conflicts. I've amended the commit message accordingly.\n- I noticed I missed cleanup of the repos created in the test, which\n I've now done.\n- Link to v4: https://patch.msgid.link/20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com\n\nChanges in v4:\n- Cleanup the code in the first commit to make it simpler to read.\n- In the second commit, we were specifically checking for `retcode > 0`\n for committing the transaction. This is a bit confusing since that\n begs the questions why not `retcode < 0`. There is no real reason\n there, so I've change the code to simple do `if (retcode && ...)`.\n I've also added more information about the flows which would commit\n the transaction in the commit message.\n- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com\n\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 71 ++++++++++++++++----------\n t/t5510-fetch.sh | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 194 insertions(+), 26 deletions(-)\n\nKarthik Nayak (3):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n fetch: fix failed batched updates skipping operations\n\nRange-diff versus v5:\n\n1: ab03acf218 = 1: 21f2518724 fetch: extract out reference committing logic\n2: 8a9982ee75 = 2: 9ca27b08fa fetch: fix non-conflicting tags not being committed\n-: ---------- > 3: 33a7654bfa fetch: fix failed batched updates skipping operations\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"530898","messageId":"20251118-fix-tags-not-fetching-v6-1-2a2f15fc137e@gmail.com","threadId":"64423","inReplyTo":"20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com","subject":"[PATCH v6 1/3] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-18T11:27:55Z","receivedAt":"2025-11-18T11:28:03Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 59 ++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..f90179040b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode)\n+\t\tgoto out;\n+\n+\tif (!is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\t}\n+\n+out:\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.2\n\n"},{"id":"530900","messageId":"20251118-fix-tags-not-fetching-v6-2-2a2f15fc137e@gmail.com","threadId":"64423","inReplyTo":"20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com","subject":"[PATCH v6 2/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-18T11:27:56Z","receivedAt":"2025-11-18T11:28:05Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction when we have an error code and\nnot using an atomic transaction. This ensures other references are\napplied even when some updates fail.\n\nThe cleanup section is reached with `retcode` set in several scenarios:\n\n - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n `retcode` before the transaction is created, so no commit is\n attempted.\n\n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n trigger the committing of the transaction.\n\nThis simplifies error handling and ensures future modifications to\n`do_fetch()` don't need special handling for batched updates.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f90179040b..b19fa8e966 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..4b113d7c27 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\ttest_when_finished rm -rf source target &&\n+\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t test_commit history &&\n+\t test_commit fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"530899","messageId":"20251118-fix-tags-not-fetching-v6-3-2a2f15fc137e@gmail.com","threadId":"64423","inReplyTo":"20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com","subject":"[PATCH v6 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-18T11:27:57Z","receivedAt":"2025-11-18T11:28:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Fix a regression introduced with batched updates in 0e358de64a (fetch:\nuse batched reference updates, 2025-05-19) when fetching references. In\nthe `do_fetch()` function, we jump to cleanup if committing the\ntransaction fails, regardless of whether using batched or atomic\nupdates. This skips three subsequent operations:\n\n - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n\n - Add upstream tracking information via `set_upstream()`.\n\n - Setting remote 'HEAD' values when `do_set_head` is true.\n\nFor atomic updates, this is expected behavior. For batched updates,\nwe want to continue with these operations even if some refs fail to\nupdate.\n\nSkipping `commit_fetch_head()` isn't actually a regression because\n'FETCH_HEAD' is already updated via `append_fetch_head()` when not\nusing '--atomic'. However, we add a test to validate this behavior.\n\nSkipping the other two operations (upstream tracking and remote HEAD)\nis a regression. Fix this by only jumping to cleanup when using\n'--atomic', allowing batched updates to continue with post-fetch\noperations. Add tests to prevent future regressions.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 6 +++-\n t/t5510-fetch.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 92 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b19fa8e966..74bf67349d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1890,7 +1890,11 @@ static int do_fetch(struct transport *transport,\n \n \tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n \t\t\t\t\t transport->remote->name, &err);\n-\tif (retcode)\n+\t/*\n+\t * With '--atomic', bail out if the transaction fails. Without '--atomic',\n+\t * continue to fetch head and perform other post-fetch operations.\n+\t */\n+\tif (retcode && atomic_fetch)\n \t\tgoto cleanup;\n \n \tcommit_fetch_head(&fetch_head);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 4b113d7c27..f5c87d81fe 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t! test -f FETCH_HEAD &&\n+\t\tgit remote add origin ../base &&\n+\t\ttouch refs/heads/foo.lock &&\n+\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n+\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n+\t\ttest -f FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success REFFILES \"upstream tracking info is added with --set-upstream\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\tgit fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"upstream tracking info is added even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\ttest_must_fail git config get branch.main.remote &&\n+\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\ttouch refs/remotes/origin/main.lock &&\n+\t\ttest_must_fail git fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"HEAD is updated even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\n+\t\t! test -f refs/remotes/origin/HEAD &&\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\ttouch refs/remotes/origin/branch.lock &&\n+\t\ttest_must_fail git fetch origin &&\n+\t\ttest -f refs/remotes/origin/HEAD\n+\t)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"530913","messageId":"xmqqv7j7dxqy.fsf@gitster.g","threadId":"64423","inReplyTo":"20251118-fix-tags-not-fetching-v6-3-2a2f15fc137e@gmail.com","subject":"Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-18T18:03:17Z","receivedAt":"2025-11-18T18:03:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n> use batched reference updates, 2025-05-19) when fetching references. In\n> the `do_fetch()` function, we jump to cleanup if committing the\n> transaction fails, regardless of whether using batched or atomic\n> updates. This skips three subsequent operations:\n>\n> - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n>\n> - Add upstream tracking information via `set_upstream()`.\n>\n> - Setting remote 'HEAD' values when `do_set_head` is true.\n>\n> For atomic updates, this is expected behavior. For batched updates,\n> we want to continue with these operations even if some refs fail to\n> update.\n>\n> Skipping `commit_fetch_head()` isn't actually a regression because\n> 'FETCH_HEAD' is already updated via `append_fetch_head()` when not\n> using '--atomic'. However, we add a test to validate this behavior.\n>\n> Skipping the other two operations (upstream tracking and remote HEAD)\n> is a regression. Fix this by only jumping to cleanup when using\n> '--atomic', allowing batched updates to continue with post-fetch\n> operations. Add tests to prevent future regressions.\n\nOther than the usual \"unless you care about timestamps, do not use\n'touch' only to create a file\" applies, but other than that the\nadded tests look quite sensible.\n\nAbout the second new test piece, it is a bit surprising that we\ndidn't have test for --set-upstream on successful fetch. It does\nnot need REFFILES prerequisite, does it?\n\nThanks.\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 4b113d7c27..f5c87d81fe 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n> +\ttest_when_finished rm -rf base repo &&\n> +\n> +\tgit init base &&\n> +\t(\n> +\t\tcd base &&\n> +\t\ttest_commit \"updated\" &&\n> +\n> +\t\tgit update-ref refs/heads/foo @ &&\n> +\t\tgit update-ref refs/heads/branch @\n> +\t) &&\n> +\n> +\tgit init --bare repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\t! test -f FETCH_HEAD &&\n> +\t\tgit remote add origin ../base &&\n> +\t\ttouch refs/heads/foo.lock &&\n> +\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n> +\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n> +\t\ttest -f FETCH_HEAD\n> +\t)\n> +'\n> +\n> +test_expect_success REFFILES \"upstream tracking info is added with --set-upstream\" '\n> +\ttest_when_finished rm -rf base repo &&\n> +\n> +\tgit init --initial-branch=main base &&\n> +\ttest_commit -C base \"updated\" &&\n> +\n> +\tgit init --bare --initial-branch=main repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit remote add origin ../base &&\n> +\t\tgit fetch origin --set-upstream main &&\n> +\t\tgit config get branch.main.remote >actual &&\n> +\t\techo \"origin\" >expect &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success REFFILES \"upstream tracking info is added even with conflicts\" '\n> +\ttest_when_finished rm -rf base repo &&\n> +\n> +\tgit init --initial-branch=main base &&\n> +\ttest_commit -C base \"updated\" &&\n> +\n> +\tgit init --bare --initial-branch=main repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit remote add origin ../base &&\n> +\t\ttest_must_fail git config get branch.main.remote &&\n> +\n> +\t\tmkdir -p refs/remotes/origin &&\n> +\t\ttouch refs/remotes/origin/main.lock &&\n> +\t\ttest_must_fail git fetch origin --set-upstream main &&\n> +\t\tgit config get branch.main.remote >actual &&\n> +\t\techo \"origin\" >expect &&\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n> +test_expect_success REFFILES \"HEAD is updated even with conflicts\" '\n> +\ttest_when_finished rm -rf base repo &&\n> +\n> +\tgit init base &&\n> +\t(\n> +\t\tcd base &&\n> +\t\ttest_commit \"updated\" &&\n> +\n> +\t\tgit update-ref refs/heads/foo @ &&\n> +\t\tgit update-ref refs/heads/branch @\n> +\t) &&\n> +\n> +\tgit init --bare repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\tgit remote add origin ../base &&\n> +\n> +\t\t! test -f refs/remotes/origin/HEAD &&\n> +\t\tmkdir -p refs/remotes/origin &&\n> +\t\ttouch refs/remotes/origin/branch.lock &&\n> +\t\ttest_must_fail git fetch origin &&\n> +\t\ttest -f refs/remotes/origin/HEAD\n> +\t)\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n"},{"id":"530984","messageId":"CAOLa=ZQW56JVxa+tahrhk00eOmY2d8b1Lch_XFk4tNkummVPeg@mail.gmail.com","threadId":"64423","inReplyTo":"xmqqv7j7dxqy.fsf@gitster.g","subject":"Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-19T08:59:34Z","receivedAt":"2025-11-19T08:59:36Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n>> use batched reference updates, 2025-05-19) when fetching references. In\n>> the `do_fetch()` function, we jump to cleanup if committing the\n>> transaction fails, regardless of whether using batched or atomic\n>> updates. This skips three subsequent operations:\n>>\n>> - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n>>\n>> - Add upstream tracking information via `set_upstream()`.\n>>\n>> - Setting remote 'HEAD' values when `do_set_head` is true.\n>>\n>> For atomic updates, this is expected behavior. For batched updates,\n>> we want to continue with these operations even if some refs fail to\n>> update.\n>>\n>> Skipping `commit_fetch_head()` isn't actually a regression because\n>> 'FETCH_HEAD' is already updated via `append_fetch_head()` when not\n>> using '--atomic'. However, we add a test to validate this behavior.\n>>\n>> Skipping the other two operations (upstream tracking and remote HEAD)\n>> is a regression. Fix this by only jumping to cleanup when using\n>> '--atomic', allowing batched updates to continue with post-fetch\n>> operations. Add tests to prevent future regressions.\n>\n> Other than the usual \"unless you care about timestamps, do not use\n> 'touch' only to create a file\" applies, but other than that the\n> added tests look quite sensible.\n>\n\nOops, will change that.\n\n> About the second new test piece, it is a bit surprising that we\n> didn't have test for --set-upstream on successful fetch. It does\n> not need REFFILES prerequisite, does it?\n\nI was surprised too, that we don't have a test covering that flag.\nYou're right it doesn't. The test for the conflict does, but the happy\npath doesn't. Will change.\n\n>\n> Thanks.\n>\n\nThanks for the review.\n"},{"id":"531022","messageId":"20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v7 0/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-19T21:46:31Z","receivedAt":"2025-11-19T21:46:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\nThe third commit fixes another regression around failing to do\npost-fetch operations when ref updates fail with batched updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v7:\n- Don't use 'touch' to create new files.\n- Drop the REFFILES requirement for the happy path test of 'git fetch\n --set-upstream'.\n- Link to v6: https://patch.msgid.link/20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com\n\nChanges in v6:\n- This version adds a new commit which handles another regression where\n if reference updates fail when using batched updates, we skip doing\n the post-fetch operations. Namely:\n - Updating 'FETCH_HEAD' via `commit_fetch_head()`\n - Adding upstream tracking information via `set_upstream()`\n - Setting remote 'HEAD' values when `do_set_head` is true\n- Link to v5: https://patch.msgid.link/20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com\n\nChanges in v5:\n- In the previous version, I assumed that the `prune_refs()` function\n also triggers committing of batched updates. However this was\n incorrect as the transaction for batched updates, is only created\n after the call to `prune_refs()`. This makes sense, since we want to\n isolate deletions from the rest of the ref updates, to avoid\n conflicts. I've amended the commit message accordingly.\n- I noticed I missed cleanup of the repos created in the test, which\n I've now done.\n- Link to v4: https://patch.msgid.link/20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com\n\nChanges in v4:\n- Cleanup the code in the first commit to make it simpler to read.\n- In the second commit, we were specifically checking for `retcode > 0`\n for committing the transaction. This is a bit confusing since that\n begs the questions why not `retcode < 0`. There is no real reason\n there, so I've change the code to simple do `if (retcode && ...)`.\n I've also added more information about the flows which would commit\n the transaction in the commit message.\n- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com\n\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 71 ++++++++++++++++----------\n t/t5510-fetch.sh | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 194 insertions(+), 26 deletions(-)\n\nKarthik Nayak (3):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n fetch: fix failed batched updates skipping operations\n\nRange-diff versus v6:\n\n1: e16f0034a7 = 1: c5b451d0a0 fetch: extract out reference committing logic\n2: 5145e93e99 = 2: 59e97f54af fetch: fix non-conflicting tags not being committed\n3: 8fb6ef3079 ! 3: 1bf509a96f fetch: fix failed batched updates skipping operations\n @@ t/t5510-fetch.sh: test_expect_success \"backfill tags when providing a refspec\" '\n +\t\tcd repo &&\n +\t\t! test -f FETCH_HEAD &&\n +\t\tgit remote add origin ../base &&\n -+\t\ttouch refs/heads/foo.lock &&\n ++\t\t>refs/heads/foo.lock &&\n +\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n +\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n +\t\ttest -f FETCH_HEAD\n +\t)\n +'\n +\n -+test_expect_success REFFILES \"upstream tracking info is added with --set-upstream\" '\n ++test_expect_success \"upstream tracking info is added with --set-upstream\" '\n +\ttest_when_finished rm -rf base repo &&\n +\n +\tgit init --initial-branch=main base &&\n @@ t/t5510-fetch.sh: test_expect_success \"backfill tags when providing a refspec\" '\n +\t\ttest_must_fail git config get branch.main.remote &&\n +\n +\t\tmkdir -p refs/remotes/origin &&\n -+\t\ttouch refs/remotes/origin/main.lock &&\n ++\t\t>refs/remotes/origin/main.lock &&\n +\t\ttest_must_fail git fetch origin --set-upstream main &&\n +\t\tgit config get branch.main.remote >actual &&\n +\t\techo \"origin\" >expect &&\n @@ t/t5510-fetch.sh: test_expect_success \"backfill tags when providing a refspec\" '\n +\n +\t\t! test -f refs/remotes/origin/HEAD &&\n +\t\tmkdir -p refs/remotes/origin &&\n -+\t\ttouch refs/remotes/origin/branch.lock &&\n ++\t\t>refs/remotes/origin/branch.lock &&\n +\t\ttest_must_fail git fetch origin &&\n +\t\ttest -f refs/remotes/origin/HEAD\n +\t)\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"531023","messageId":"20251119-fix-tags-not-fetching-v7-1-0c8f9fb1f287@gmail.com","threadId":"64423","inReplyTo":"20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com","subject":"[PATCH v7 1/3] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-19T21:46:32Z","receivedAt":"2025-11-19T21:46:40Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 59 ++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..f90179040b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode)\n+\t\tgoto out;\n+\n+\tif (!is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\t}\n+\n+out:\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.2\n\n"},{"id":"531024","messageId":"20251119-fix-tags-not-fetching-v7-2-0c8f9fb1f287@gmail.com","threadId":"64423","inReplyTo":"20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com","subject":"[PATCH v7 2/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-19T21:46:33Z","receivedAt":"2025-11-19T21:46:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction when we have an error code and\nnot using an atomic transaction. This ensures other references are\napplied even when some updates fail.\n\nThe cleanup section is reached with `retcode` set in several scenarios:\n\n - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n `retcode` before the transaction is created, so no commit is\n attempted.\n\n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n trigger the committing of the transaction.\n\nThis simplifies error handling and ensures future modifications to\n`do_fetch()` don't need special handling for batched updates.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f90179040b..b19fa8e966 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..4b113d7c27 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\ttest_when_finished rm -rf source target &&\n+\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t test_commit history &&\n+\t test_commit fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"531025","messageId":"20251119-fix-tags-not-fetching-v7-3-0c8f9fb1f287@gmail.com","threadId":"64423","inReplyTo":"20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com","subject":"[PATCH v7 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-19T21:46:34Z","receivedAt":"2025-11-19T21:46:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Fix a regression introduced with batched updates in 0e358de64a (fetch:\nuse batched reference updates, 2025-05-19) when fetching references. In\nthe `do_fetch()` function, we jump to cleanup if committing the\ntransaction fails, regardless of whether using batched or atomic\nupdates. This skips three subsequent operations:\n\n - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n\n - Add upstream tracking information via `set_upstream()`.\n\n - Setting remote 'HEAD' values when `do_set_head` is true.\n\nFor atomic updates, this is expected behavior. For batched updates,\nwe want to continue with these operations even if some refs fail to\nupdate.\n\nSkipping `commit_fetch_head()` isn't actually a regression because\n'FETCH_HEAD' is already updated via `append_fetch_head()` when not\nusing '--atomic'. However, we add a test to validate this behavior.\n\nSkipping the other two operations (upstream tracking and remote HEAD)\nis a regression. Fix this by only jumping to cleanup when using\n'--atomic', allowing batched updates to continue with post-fetch\noperations. Add tests to prevent future regressions.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 6 +++-\n t/t5510-fetch.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 92 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b19fa8e966..74bf67349d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1890,7 +1890,11 @@ static int do_fetch(struct transport *transport,\n \n \tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n \t\t\t\t\t transport->remote->name, &err);\n-\tif (retcode)\n+\t/*\n+\t * With '--atomic', bail out if the transaction fails. Without '--atomic',\n+\t * continue to fetch head and perform other post-fetch operations.\n+\t */\n+\tif (retcode && atomic_fetch)\n \t\tgoto cleanup;\n \n \tcommit_fetch_head(&fetch_head);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 4b113d7c27..cd55958bdc 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\t! test -f FETCH_HEAD &&\n+\t\tgit remote add origin ../base &&\n+\t\t>refs/heads/foo.lock &&\n+\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n+\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n+\t\ttest -f FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success \"upstream tracking info is added with --set-upstream\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\tgit fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"upstream tracking info is added even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\ttest_must_fail git config get branch.main.remote &&\n+\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\t>refs/remotes/origin/main.lock &&\n+\t\ttest_must_fail git fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"HEAD is updated even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\n+\t\t! test -f refs/remotes/origin/HEAD &&\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\t>refs/remotes/origin/branch.lock &&\n+\t\ttest_must_fail git fetch origin &&\n+\t\ttest -f refs/remotes/origin/HEAD\n+\t)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"531030","messageId":"CAPig+cRjN85S3oCvazAvUD_V0EwkzdvKAm+DC66+uVijF5=HQA@mail.gmail.com","threadId":"64423","inReplyTo":"20251119-fix-tags-not-fetching-v7-3-0c8f9fb1f287@gmail.com","subject":"Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-11-19T22:20:19Z","receivedAt":"2025-11-19T22:20:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak wrote:\n> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n> use batched reference updates, 2025-05-19) when fetching references. In\n> the `do_fetch()` function, we jump to cleanup if committing the\n> transaction fails, regardless of whether using batched or atomic\n> updates. This skips three subsequent operations:\n> [...]\n> Signed-off-by: Karthik Nayak \n> ---\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> @@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n> + test_when_finished rm -rf base repo &&\n> + [...]\n> + git init --bare repo &&\n> + (\n> + cd repo &&\n> + ! test -f FETCH_HEAD &&\n\nIs this supposed to be asserting that the file does not exist or that\nthe path is not a file? If the former, then test_path_is_missing()\nwould be a better choice.\n\n> + git remote add origin ../base &&\n> + >refs/heads/foo.lock &&\n> + test_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n> + test_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n> + test -f FETCH_HEAD\n> + )\n> +'\n> +\n> +test_expect_success REFFILES \"HEAD is updated even with conflicts\" '\n> + test_when_finished rm -rf base repo &&\n> + [...]\n> + git init --bare repo &&\n> + (\n> + cd repo &&\n> + git remote add origin ../base &&\n> +\n> + ! test -f refs/remotes/origin/HEAD &&\n\nDitto.\n\n> + mkdir -p refs/remotes/origin &&\n> + >refs/remotes/origin/branch.lock &&\n> + test_must_fail git fetch origin &&\n> + test -f refs/remotes/origin/HEAD\n> + )\n> +'\n"},{"id":"531033","messageId":"xmqqo6oxaae6.fsf@gitster.g","threadId":"64423","inReplyTo":"CAPig+cRjN85S3oCvazAvUD_V0EwkzdvKAm+DC66+uVijF5=HQA@mail.gmail.com","subject":"Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-19T23:08:17Z","receivedAt":"2025-11-19T23:08:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak wrote:\n>> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n>> use batched reference updates, 2025-05-19) when fetching references. In\n>> the `do_fetch()` function, we jump to cleanup if committing the\n>> transaction fails, regardless of whether using batched or atomic\n>> updates. This skips three subsequent operations:\n>> [...]\n>> Signed-off-by: Karthik Nayak \n>> ---\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> @@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n>> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n>> + test_when_finished rm -rf base repo &&\n>> + [...]\n>> + git init --bare repo &&\n>> + (\n>> + cd repo &&\n>> + ! test -f FETCH_HEAD &&\n>\n> Is this supposed to be asserting that the file does not exist or that\n> the path is not a file? If the former, then test_path_is_missing()\n> would be a better choice.\n\nThanks for carefully reading. Personally, I think this is not\nneeded, as we have just created a new repository. It might be\neven better to replace it with\n\n\t\trm -f FETCH_HEAD &&\n\nto clarify that we do want to see this _created_ with a failing \"git\nfetch\", not merely left behind.\n\n>\n>> + git remote add origin ../base &&\n>> + >refs/heads/foo.lock &&\n>> + test_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n>> + test_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n>> + test -f FETCH_HEAD\n\nMore importantly, should we inspect the contents of this file to see\nwhat gets recorded. If we are fetching foo and bar, and we made foo\nfail, do we expect foo and bar in the file? Or do we expect only bar\nin the file? Something else?\n\n"},{"id":"531132","messageId":"CAOLa=ZSQZhXEVGXzwg1bWd7En+vz8dzYHZTM+8AvW8UnDk-Fag@mail.gmail.com","threadId":"64423","inReplyTo":"xmqqo6oxaae6.fsf@gitster.g","subject":"Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-21T11:00:58Z","receivedAt":"2025-11-21T11:01:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Eric Sunshine writes:\n>\n>> On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak wrote:\n>>> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n>>> use batched reference updates, 2025-05-19) when fetching references. In\n>>> the `do_fetch()` function, we jump to cleanup if committing the\n>>> transaction fails, regardless of whether using batched or atomic\n>>> updates. This skips three subsequent operations:\n>>> [...]\n>>> Signed-off-by: Karthik Nayak \n>>> ---\n>>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>>> @@ -1639,6 +1639,93 @@ test_expect_success \"backfill tags when providing a refspec\" '\n>>> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n>>> + test_when_finished rm -rf base repo &&\n>>> + [...]\n>>> + git init --bare repo &&\n>>> + (\n>>> + cd repo &&\n>>> + ! test -f FETCH_HEAD &&\n>>\n>> Is this supposed to be asserting that the file does not exist or that\n>> the path is not a file? If the former, then test_path_is_missing()\n>> would be a better choice.\n>\n> Thanks for carefully reading. Personally, I think this is not\n> needed, as we have just created a new repository. It might be\n> even better to replace it with\n>\n> \t\trm -f FETCH_HEAD &&\n>\n> to clarify that we do want to see this _created_ with a failing \"git\n> fetch\", not merely left behind.\n>\n\nThat's fair.\n\n>>\n>>> + git remote add origin ../base &&\n>>> + >refs/heads/foo.lock &&\n>>> + test_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n>>> + test_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n>>> + test -f FETCH_HEAD\n>\n> More importantly, should we inspect the contents of this file to see\n> what gets recorded. If we are fetching foo and bar, and we made foo\n> fail, do we expect foo and bar in the file? Or do we expect only bar\n> in the file? Something else?\n\nI would say we should. Let me send in a version with these changes.\n\nThanks,\nKarthik\n"},{"id":"531133","messageId":"20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com","threadId":"64423","inReplyTo":"20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com","subject":"[PATCH v8 0/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-21T11:13:44Z","receivedAt":"2025-11-21T11:13:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"This fixes the bug reported by David Bohman [1].\n\nThe 'git-fetch(1)' uses batched updates to perform reference updates\nwhen not using 'atomic' transactions. One scenario which was missed\nhere, was fetching tags. When fetching conflicting tags, the\n`fetch_and_consume_refs()` function returns '1', which skipped\ncommitting the transaction and directly jumped to the cleanup section.\nThis mean that no updates were applied. This also extends to backfilling\ntags.\n\nThe first commit, extracts out common code for committing a reference\ntransaction and handling rejected updates. The second commit ensures\nany failures would also commit pending updates.\n\nThe third commit fixes another regression around failing to do\npost-fetch operations when ref updates fail with batched updates.\n\n[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com\n\nSigned-off-by: Karthik Nayak \n---\nChanges in v8:\n- Change the test to delete FETCH_HEAD at the start and verify the\n contents at the end.\n- Use 'test_path_is_missing()' instead of '! test -f ...'\n- Link to v7: https://patch.msgid.link/20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com\n\nChanges in v7:\n- Don't use 'touch' to create new files.\n- Drop the REFFILES requirement for the happy path test of 'git fetch\n --set-upstream'.\n- Link to v6: https://patch.msgid.link/20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com\n\nChanges in v6:\n- This version adds a new commit which handles another regression where\n if reference updates fail when using batched updates, we skip doing\n the post-fetch operations. Namely:\n - Updating 'FETCH_HEAD' via `commit_fetch_head()`\n - Adding upstream tracking information via `set_upstream()`\n - Setting remote 'HEAD' values when `do_set_head` is true\n- Link to v5: https://patch.msgid.link/20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com\n\nChanges in v5:\n- In the previous version, I assumed that the `prune_refs()` function\n also triggers committing of batched updates. However this was\n incorrect as the transaction for batched updates, is only created\n after the call to `prune_refs()`. This makes sense, since we want to\n isolate deletions from the rest of the ref updates, to avoid\n conflicts. I've amended the commit message accordingly.\n- I noticed I missed cleanup of the repos created in the test, which\n I've now done.\n- Link to v4: https://patch.msgid.link/20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com\n\nChanges in v4:\n- Cleanup the code in the first commit to make it simpler to read.\n- In the second commit, we were specifically checking for `retcode > 0`\n for committing the transaction. This is a bit confusing since that\n begs the questions why not `retcode < 0`. There is no real reason\n there, so I've change the code to simple do `if (retcode && ...)`.\n I've also added more information about the flows which would commit\n the transaction in the commit message.\n- Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com\n\nChanges in v3:\n- Split the patch into two commits. One for extracting out existing code\n into a new commit and the other to perform the fix.\n- Add back error handling when commit via the normal flow.\n- Instead of calling the commit function at every failure, make it part\n of the cleanup code.\n- Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com\n\nChanges in v2:\n- Add a comment to explain the purpose of `commit_ref_transaction()` and\n how it works.\n- Also extend the same logic towards backfilling tags. While I was able\n to add a test for the happy path, I couldn't figure out how to test\n when `backfill_tags()` tags would fail.\n Tangentially, this flow seems to only be triggered when using the now\n deprecated 'branches/' remote format.\n- Remove unneeded subshells from the tests.\n- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com\n\n---\n builtin/fetch.c | 71 ++++++++++++++++----------\n t/t5510-fetch.sh | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 195 insertions(+), 26 deletions(-)\n\nKarthik Nayak (3):\n fetch: extract out reference committing logic\n fetch: fix non-conflicting tags not being committed\n fetch: fix failed batched updates skipping operations\n\nRange-diff versus v7:\n\n1: f5d1abef41 = 1: 6553f56915 fetch: extract out reference committing logic\n2: fa2466f2bf = 2: d525415fbb fetch: fix non-conflicting tags not being committed\n3: df59310296 ! 3: 30ddb99550 fetch: fix failed batched updates skipping operations\n @@ t/t5510-fetch.sh: test_expect_success \"backfill tags when providing a refspec\" '\n +\tgit init --bare repo &&\n +\t(\n +\t\tcd repo &&\n -+\t\t! test -f FETCH_HEAD &&\n ++\t\trm -f FETCH_HEAD &&\n +\t\tgit remote add origin ../base &&\n +\t\t>refs/heads/foo.lock &&\n +\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n +\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n -+\t\ttest -f FETCH_HEAD\n ++\t\ttest_grep \"branch ${SQ}branch${SQ} of ../base\" FETCH_HEAD &&\n ++\t\ttest_grep \"branch ${SQ}foo${SQ} of ../base\" FETCH_HEAD\n +\t)\n +'\n +\n @@ t/t5510-fetch.sh: test_expect_success \"backfill tags when providing a refspec\" '\n +\t\tcd repo &&\n +\t\tgit remote add origin ../base &&\n +\n -+\t\t! test -f refs/remotes/origin/HEAD &&\n ++\t\ttest_path_is_missing refs/remotes/origin/HEAD &&\n +\t\tmkdir -p refs/remotes/origin &&\n +\t\t>refs/remotes/origin/branch.lock &&\n +\t\ttest_must_fail git fetch origin &&\n\n\nbase-commit: a99f379adf116d53eb11957af5bab5214915f91d\nchange-id: 20251103-fix-tags-not-fetching-0f1621a474d4\n\nThanks\n- Karthik\n\n"},{"id":"531134","messageId":"20251121-fix-tags-not-fetching-v8-1-23b53a8a8334@gmail.com","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com","subject":"[PATCH v8 1/3] fetch: extract out reference committing logic","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-21T11:13:45Z","receivedAt":"2025-11-21T11:13:51Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The `do_fetch()` function contains the core of the `git-fetch(1)` logic.\nPart of this is to fetch and store references. This is done by\n\n 1. Creating a reference transaction (non-atomic mode uses batched\n updates).\n 2. Adding individual reference updates to the transaction.\n 3. Committing the transaction.\n 4. When using batched updates, handling the rejected updates.\n\nThe following commit, will fix a bug wherein fetching tags with\nconflicts was causing other reference updates to fail. Fixing this\nrequires utilizing this logic in different regions of the function.\n\nIn preparation of the follow up commit, extract the committing and\nrejection handling logic into a separate function called\n`commit_ref_transaction()`.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 59 ++++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 33 insertions(+), 26 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex c7ff3480fb..f90179040b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t*data->retcode = 1;\n }\n \n+/*\n+ * Commit the reference transaction. If it isn't an atomic transaction, handle\n+ * rejected updates as part of using batched updates.\n+ */\n+static int commit_ref_transaction(struct ref_transaction **transaction,\n+\t\t\t\t bool is_atomic, const char *remote_name,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tint retcode = ref_transaction_commit(*transaction, err);\n+\tif (retcode)\n+\t\tgoto out;\n+\n+\tif (!is_atomic) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = remote_name,\n+\t\t\t.retcode = &retcode,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(*transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n+\t}\n+\n+out:\n+\tref_transaction_free(*transaction);\n+\t*transaction = NULL;\n+\treturn retcode;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport,\n \tif (retcode)\n \t\tgoto cleanup;\n \n-\tretcode = ref_transaction_commit(transaction, &err);\n-\tif (retcode) {\n-\t\t/*\n-\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t * aborting an already closed transaction.\n-\t\t */\n-\t\tref_transaction_free(transaction);\n-\t\ttransaction = NULL;\n+\tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n+\t\t\t\t\t transport->remote->name, &err);\n+\tif (retcode)\n \t\tgoto cleanup;\n-\t}\n-\n-\tif (!atomic_fetch) {\n-\t\tstruct ref_rejection_data data = {\n-\t\t\t.retcode = &retcode,\n-\t\t\t.conflict_msg_shown = 0,\n-\t\t\t.remote_name = transport->remote->name,\n-\t\t};\n-\n-\t\tref_transaction_for_each_rejected_update(transaction,\n-\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n-\t\t\t\t\t\t\t &data);\n-\t\tif (retcode) {\n-\t\t\tref_transaction_free(transaction);\n-\t\t\ttransaction = NULL;\n-\t\t\tgoto cleanup;\n-\t\t}\n-\t}\n \n \tcommit_fetch_head(&fetch_head);\n \n\n-- \n2.51.2\n\n"},{"id":"531135","messageId":"20251121-fix-tags-not-fetching-v8-2-23b53a8a8334@gmail.com","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com","subject":"[PATCH v8 2/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-21T11:13:46Z","receivedAt":"2025-11-21T11:13:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)\nupdated the 'git-fetch(1)' command to use batched updates. This batches\nupdates to gain performance improvements. When fetching references, each\nupdate is added to the transaction. Finally, when committing, individual\nupdates are allowed to fail with reason, while the transaction itself\nsucceeds.\n\nOne scenario which was missed here, was fetching tags. When fetching\nconflicting tags, the `fetch_and_consume_refs()` function returns '1',\nwhich skipped committing the transaction and directly jumped to the\ncleanup section. This mean that no updates were applied. This also\nextends to backfilling tags which is done when fetching specific\nrefspecs which contains tags in their history.\n\nFix this by committing the transaction when we have an error code and\nnot using an atomic transaction. This ensures other references are\napplied even when some updates fail.\n\nThe cleanup section is reached with `retcode` set in several scenarios:\n\n - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set\n `retcode` before the transaction is created, so no commit is\n attempted.\n\n - `fetch_and_consume_refs()` and `backfill_tags()` are the primary\n cases this fix targets, both setting a positive `retcode` to\n trigger the committing of the transaction.\n\nThis simplifies error handling and ensures future modifications to\n`do_fetch()` don't need special handling for batched updates.\n\nAdd tests to check for this regression. While here, add a missing\ncleanup from previous test.\n\nReported-by: David Bohman \nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 8 ++++++++\n t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 70 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f90179040b..b19fa8e966 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport,\n \t}\n \n cleanup:\n+\t/*\n+\t * When using batched updates, we want to commit the non-rejected\n+\t * updates and also handle the rejections.\n+\t */\n+\tif (retcode && !atomic_fetch && transaction)\n+\t\tcommit_ref_transaction(&transaction, false,\n+\t\t\t\t transport->remote->name, &err);\n+\n \tif (retcode) {\n \t\tif (err.len) {\n \t\t\terror(\"%s\", err.buf);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex b7059cccaa..4b113d7c27 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti\n '\n \n test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '\n+\ttest_when_finished rm -rf base repo &&\n \t(\n \t\tgit init --ref-format=reftable base &&\n \t\tcd base &&\n@@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n \t)\n '\n \n+test_expect_success 'fetch --tags fetches existing tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-1 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-1\" out &&\n+\tgit -C repo fetch --tags &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-1\" out\n+'\n+\n+test_expect_success 'fetch --tags fetches non-conflicting tags' '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\tgit -C base commit --allow-empty -m \"empty-commit\" &&\n+\tgit -C base tag tag-1 &&\n+\n+\tgit clone --bare base repo &&\n+\n+\tgit -C base tag tag-2 &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep ! \"tag-2\" out &&\n+\n+\tgit -C base commit --allow-empty -m \"second empty-commit\" &&\n+\tgit -C base tag -f tag-1 &&\n+\n+\ttest_must_fail git -C repo fetch --tags 2>out &&\n+\ttest_grep \"tag-1 (would clobber existing tag)\" out &&\n+\tgit -C repo for-each-ref >out &&\n+\ttest_grep \"tag-2\" out\n+'\n+\n+test_expect_success \"backfill tags when providing a refspec\" '\n+\ttest_when_finished rm -rf source target &&\n+\n+\tgit init source &&\n+\tgit -C source commit --allow-empty --message common &&\n+\tgit clone file://\"$(pwd)\"/source target &&\n+\t(\n+\t cd source &&\n+\t test_commit history &&\n+\t test_commit fetch-me\n+\t) &&\n+\n+\t# The \"history\" tag is backfilled eventhough we requested\n+\t# to only fetch HEAD\n+\tgit -C target fetch origin HEAD:branch &&\n+\tgit -C target tag -l >actual &&\n+\tcat >expect <<-\\EOF &&\n+\tfetch-me\n+\thistory\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"531136","messageId":"20251121-fix-tags-not-fetching-v8-3-23b53a8a8334@gmail.com","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com","subject":"[PATCH v8 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-11-21T11:13:47Z","receivedAt":"2025-11-21T11:13:53Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Fix a regression introduced with batched updates in 0e358de64a (fetch:\nuse batched reference updates, 2025-05-19) when fetching references. In\nthe `do_fetch()` function, we jump to cleanup if committing the\ntransaction fails, regardless of whether using batched or atomic\nupdates. This skips three subsequent operations:\n\n - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n\n - Add upstream tracking information via `set_upstream()`.\n\n - Setting remote 'HEAD' values when `do_set_head` is true.\n\nFor atomic updates, this is expected behavior. For batched updates,\nwe want to continue with these operations even if some refs fail to\nupdate.\n\nSkipping `commit_fetch_head()` isn't actually a regression because\n'FETCH_HEAD' is already updated via `append_fetch_head()` when not\nusing '--atomic'. However, we add a test to validate this behavior.\n\nSkipping the other two operations (upstream tracking and remote HEAD)\nis a regression. Fix this by only jumping to cleanup when using\n'--atomic', allowing batched updates to continue with post-fetch\noperations. Add tests to prevent future regressions.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 6 +++-\n t/t5510-fetch.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 93 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b19fa8e966..74bf67349d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1890,7 +1890,11 @@ static int do_fetch(struct transport *transport,\n \n \tretcode = commit_ref_transaction(&transaction, atomic_fetch,\n \t\t\t\t\t transport->remote->name, &err);\n-\tif (retcode)\n+\t/*\n+\t * With '--atomic', bail out if the transaction fails. Without '--atomic',\n+\t * continue to fetch head and perform other post-fetch operations.\n+\t */\n+\tif (retcode && atomic_fetch)\n \t\tgoto cleanup;\n \n \tcommit_fetch_head(&fetch_head);\ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex 4b113d7c27..a1ca4e1ac7 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -1639,6 +1639,94 @@ test_expect_success \"backfill tags when providing a refspec\" '\n \ttest_cmp expect actual\n '\n \n+test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\trm -f FETCH_HEAD &&\n+\t\tgit remote add origin ../base &&\n+\t\t>refs/heads/foo.lock &&\n+\t\ttest_must_fail git fetch -f origin \"refs/heads/*:refs/heads/*\" 2>err &&\n+\t\ttest_grep \"error: fetching ref refs/heads/foo failed: reference already exists\" err &&\n+\t\ttest_grep \"branch ${SQ}branch${SQ} of ../base\" FETCH_HEAD &&\n+\t\ttest_grep \"branch ${SQ}foo${SQ} of ../base\" FETCH_HEAD\n+\t)\n+'\n+\n+test_expect_success \"upstream tracking info is added with --set-upstream\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\tgit fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"upstream tracking info is added even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init --initial-branch=main base &&\n+\ttest_commit -C base \"updated\" &&\n+\n+\tgit init --bare --initial-branch=main repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\t\ttest_must_fail git config get branch.main.remote &&\n+\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\t>refs/remotes/origin/main.lock &&\n+\t\ttest_must_fail git fetch origin --set-upstream main &&\n+\t\tgit config get branch.main.remote >actual &&\n+\t\techo \"origin\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n+test_expect_success REFFILES \"HEAD is updated even with conflicts\" '\n+\ttest_when_finished rm -rf base repo &&\n+\n+\tgit init base &&\n+\t(\n+\t\tcd base &&\n+\t\ttest_commit \"updated\" &&\n+\n+\t\tgit update-ref refs/heads/foo @ &&\n+\t\tgit update-ref refs/heads/branch @\n+\t) &&\n+\n+\tgit init --bare repo &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit remote add origin ../base &&\n+\n+\t\ttest_path_is_missing refs/remotes/origin/HEAD &&\n+\t\tmkdir -p refs/remotes/origin &&\n+\t\t>refs/remotes/origin/branch.lock &&\n+\t\ttest_must_fail git fetch origin &&\n+\t\ttest -f refs/remotes/origin/HEAD\n+\t)\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n\n-- \n2.51.2\n\n"},{"id":"531153","messageId":"xmqqa50f40p9.fsf@gitster.g","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com","subject":"Re: [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-21T19:58:42Z","receivedAt":"2025-11-21T19:58:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> builtin/fetch.c | 71 ++++++++++++++++----------\n> t/t5510-fetch.sh | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 195 insertions(+), 26 deletions(-)\n>\n> Karthik Nayak (3):\n> fetch: extract out reference committing logic\n> fetch: fix non-conflicting tags not being committed\n> fetch: fix failed batched updates skipping operations\n\nLooking good. Will replace.\n\nThanks.\n"},{"id":"531505","messageId":"aS2Q4-U5kgJ2nNVv@pks.im","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-2-23b53a8a8334@gmail.com","subject":"Re: [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-01T12:58:11Z","receivedAt":"2025-12-01T12:58:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 21, 2025 at 12:13:46PM +0100, Karthik Nayak wrote:\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index b7059cccaa..4b113d7c27 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n[snip]\n> +test_expect_success \"backfill tags when providing a refspec\" '\n> +\ttest_when_finished rm -rf source target &&\n> +\n> +\tgit init source &&\n> +\tgit -C source commit --allow-empty --message common &&\n> +\tgit clone file://\"$(pwd)\"/source target &&\n> +\t(\n> +\t cd source &&\n> +\t test_commit history &&\n> +\t test_commit fetch-me\n> +\t) &&\n> +\n> +\t# The \"history\" tag is backfilled eventhough we requested\n\nTiny nit, not worth a reroll: s/eventhough/even though/. Other than that\nthis patch looks good to me.\n\nPatrick\n"},{"id":"531506","messageId":"aS2Q6y_hnwBxycGk@pks.im","threadId":"64423","inReplyTo":"20251121-fix-tags-not-fetching-v8-3-23b53a8a8334@gmail.com","subject":"Re: [PATCH v8 3/3] fetch: fix failed batched updates skipping operations","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-12-01T12:58:19Z","receivedAt":"2025-12-01T12:58:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Nov 21, 2025 at 12:13:47PM +0100, Karthik Nayak wrote:\n> Fix a regression introduced with batched updates in 0e358de64a (fetch:\n> use batched reference updates, 2025-05-19) when fetching references. In\n> the `do_fetch()` function, we jump to cleanup if committing the\n> transaction fails, regardless of whether using batched or atomic\n> updates. This skips three subsequent operations:\n> \n> - Update 'FETCH_HEAD' as part of `commit_fetch_head()`.\n> \n> - Add upstream tracking information via `set_upstream()`.\n> \n> - Setting remote 'HEAD' values when `do_set_head` is true.\n> \n> For atomic updates, this is expected behavior. For batched updates,\n> we want to continue with these operations even if some refs fail to\n> update.\n> \n> Skipping `commit_fetch_head()` isn't actually a regression because\n> 'FETCH_HEAD' is already updated via `append_fetch_head()` when not\n> using '--atomic'. However, we add a test to validate this behavior.\n\nThis raises the question what happens when this function _does_ get\nexecuted again. But we're guarding us:\n\n static void commit_fetch_head(struct fetch_head *fetch_head)\n {\n if (!fetch_head->fp || !atomic_fetch)\n return;\n strbuf_write(&fetch_head->buf, fetch_head->fp);\n }\n\nAnd as we only `goto cleanup` in case `retcode && atomic_fetch` we know\nthat the above function will exit early. So this is a no-op change\nindeed.\n\n> Skipping the other two operations (upstream tracking and remote HEAD)\n> is a regression. Fix this by only jumping to cleanup when using\n> '--atomic', allowing batched updates to continue with post-fetch\n> operations. Add tests to prevent future regressions.\n\nMakes sense.\n\n> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n> index 4b113d7c27..a1ca4e1ac7 100755\n> --- a/t/t5510-fetch.sh\n> +++ b/t/t5510-fetch.sh\n> @@ -1639,6 +1639,94 @@ test_expect_success \"backfill tags when providing a refspec\" '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n> +\ttest_when_finished rm -rf base repo &&\n> +\n> +\tgit init base &&\n> +\t(\n> +\t\tcd base &&\n> +\t\ttest_commit \"updated\" &&\n> +\n> +\t\tgit update-ref refs/heads/foo @ &&\n> +\t\tgit update-ref refs/heads/branch @\n> +\t) &&\n> +\n> +\tgit init --bare repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\trm -f FETCH_HEAD &&\n> +\t\tgit remote add origin ../base &&\n> +\t\t>refs/heads/foo.lock &&\n\nHm. Is this compatible with all supported systems? We typically write\nthis as:\n\n : >refs/heads/foo.lock\n\nBut I have to acknowledge that I only do this because some people that\nare more knowledgeable than I am know that we need this.\n\nOther than that I'm happy with the current state of this patch series.\nIf the above turns out to be a non-issue I think it should be ready for\n'next'.\n\nThanks!\n\nPatrick\n"},{"id":"531591","messageId":"CAOLa=ZQ-O7V9qHbgeuQ78R1bHGDmGEM6fP5Kr9aC0AfvSF8MZA@mail.gmail.com","threadId":"64423","inReplyTo":"aS2Q4-U5kgJ2nNVv@pks.im","subject":"Re: [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-12-02T22:26:01Z","receivedAt":"2025-12-02T22:26:04Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, Nov 21, 2025 at 12:13:46PM +0100, Karthik Nayak wrote:\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index b7059cccaa..4b113d7c27 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc\n> [snip]\n>> +test_expect_success \"backfill tags when providing a refspec\" '\n>> +\ttest_when_finished rm -rf source target &&\n>> +\n>> +\tgit init source &&\n>> +\tgit -C source commit --allow-empty --message common &&\n>> +\tgit clone file://\"$(pwd)\"/source target &&\n>> +\t(\n>> +\t cd source &&\n>> +\t test_commit history &&\n>> +\t test_commit fetch-me\n>> +\t) &&\n>> +\n>> +\t# The \"history\" tag is backfilled eventhough we requested\n>\n> Tiny nit, not worth a reroll: s/eventhough/even though/. Other than that\n> this patch looks good to me.\n>\n> Patrick\n\nWill add it in. Thanks!\n"},{"id":"531593","messageId":"CAOLa=ZTdfkK0ty2YQfE+GTtgYZ7wrOW_04Ony8tN+x2oqoXSCg@mail.gmail.com","threadId":"64423","inReplyTo":"aS2Q6y_hnwBxycGk@pks.im","subject":"Re: [PATCH v8 3/3] fetch: fix failed batched updates skipping operations","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-12-02T22:35:16Z","receivedAt":"2025-12-02T22:35:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\n>> index 4b113d7c27..a1ca4e1ac7 100755\n>> --- a/t/t5510-fetch.sh\n>> +++ b/t/t5510-fetch.sh\n>> @@ -1639,6 +1639,94 @@ test_expect_success \"backfill tags when providing a refspec\" '\n>> \ttest_cmp expect actual\n>> '\n>>\n>> +test_expect_success REFFILES \"FETCH_HEAD is updated even if ref updates fail\" '\n>> +\ttest_when_finished rm -rf base repo &&\n>> +\n>> +\tgit init base &&\n>> +\t(\n>> +\t\tcd base &&\n>> +\t\ttest_commit \"updated\" &&\n>> +\n>> +\t\tgit update-ref refs/heads/foo @ &&\n>> +\t\tgit update-ref refs/heads/branch @\n>> +\t) &&\n>> +\n>> +\tgit init --bare repo &&\n>> +\t(\n>> +\t\tcd repo &&\n>> +\t\trm -f FETCH_HEAD &&\n>> +\t\tgit remote add origin ../base &&\n>> +\t\t>refs/heads/foo.lock &&\n>\n> Hm. Is this compatible with all supported systems? We typically write\n> this as:\n>\n> : >refs/heads/foo.lock\n>\n> But I have to acknowledge that I only do this because some people that\n> are more knowledgeable than I am know that we need this.\n>\n> Other than that I'm happy with the current state of this patch series.\n> If the above turns out to be a non-issue I think it should be ready for\n> 'next'.\n>\n\nI didn't know about this. The CI didn't complain about this too.\n\nA quick search through our repo shows\n\n$ rg --stats '^\\s*>[\\w/.-]+' t/\n...\n969 matches\n969 matched lines\n285 files contained matches\n...\n\n$ rg --stats '^\\s*:\\s*>[\\w/.-]+' t/\n...\n188 matches\n188 matched lines\n58 files contained matches\n...\n\nSo seems like we use both, meaning, this should be okay?\n\n> Thanks!\n>\n> Patrick\n\nThanks for the review.\n\nSince the other comment was a nit. I will hold off on sending a new version :)\n"}]}