From 0c5d96e207b789dee65aafe1b8a730097a9572a2 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 3 Nov 2025 11:18:35 +0100 Subject: [PATCH 1/4] fetch: fix non-conflicting tags not being committed This fixes the bug reported by David Bohman [1]. The 'git-fetch(1)' uses batched updates to perform reference updates when not using 'atomic' transactions. One scenario which was missed here, was fetching tags. When fetching conflicting tags, the `fetch_and_consume_refs()` function returns '1', which skipped committing the transaction and directly jumped to the cleanup section. This mean that no updates were applied. This also extends to backfilling tags. The first commit, extracts out common code for committing a reference transaction and handling rejected updates. The second commit ensures any failures would also commit pending updates. The third commit fixes another regression around failing to do post-fetch operations when ref updates fail with batched updates. [1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com Signed-off-by: Karthik Nayak --- Changes in v9: - EDITME: describe what is new in this series revision. - EDITME: use bulletpoints and terse descriptions. - Link to v8: https://patch.msgid.link/20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com Changes in v8: - Change the test to delete FETCH_HEAD at the start and verify the contents at the end. - Use 'test_path_is_missing()' instead of '! test -f ...' - Link to v7: https://patch.msgid.link/20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com Changes in v7: - Don't use 'touch' to create new files. - Drop the REFFILES requirement for the happy path test of 'git fetch --set-upstream'. - Link to v6: https://patch.msgid.link/20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com Changes in v6: - This version adds a new commit which handles another regression where if reference updates fail when using batched updates, we skip doing the post-fetch operations. Namely: - Updating 'FETCH_HEAD' via `commit_fetch_head()` - Adding upstream tracking information via `set_upstream()` - Setting remote 'HEAD' values when `do_set_head` is true - Link to v5: https://patch.msgid.link/20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com Changes in v5: - In the previous version, I assumed that the `prune_refs()` function also triggers committing of batched updates. However this was incorrect as the transaction for batched updates, is only created after the call to `prune_refs()`. This makes sense, since we want to isolate deletions from the rest of the ref updates, to avoid conflicts. I've amended the commit message accordingly. - I noticed I missed cleanup of the repos created in the test, which I've now done. - Link to v4: https://patch.msgid.link/20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com Changes in v4: - Cleanup the code in the first commit to make it simpler to read. - In the second commit, we were specifically checking for `retcode > 0` for committing the transaction. This is a bit confusing since that begs the questions why not `retcode < 0`. There is no real reason there, so I've change the code to simple do `if (retcode && ...)`. I've also added more information about the flows which would commit the transaction in the commit message. - Link to v3: https://patch.msgid.link/20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com Changes in v3: - Split the patch into two commits. One for extracting out existing code into a new commit and the other to perform the fix. - Add back error handling when commit via the normal flow. - Instead of calling the commit function at every failure, make it part of the cleanup code. - Link to v2: https://patch.msgid.link/20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com Changes in v2: - Add a comment to explain the purpose of `commit_ref_transaction()` and how it works. - Also extend the same logic towards backfilling tags. While I was able to add a test for the happy path, I couldn't figure out how to test when `backfill_tags()` tags would fail. Tangentially, this flow seems to only be triggered when using the now deprecated 'branches/' remote format. - Remove unneeded subshells from the tests. - Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com --- b4-submit-tracking --- # This section is used internally by b4 prep for tracking purposes. { "series": { "revision": 9, "change-id": "20251103-fix-tags-not-fetching-0f1621a474d4", "prefixes": [], "history": { "v1": [ "20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com" ], "v2": [ "20251106-fix-tags-not-fetching-v2-1-610cb4b0e7c8@gmail.com" ], "v3": [ "20251108-fix-tags-not-fetching-v3-0-a12ab6c4daef@gmail.com" ], "v4": [ "20251111-fix-tags-not-fetching-v4-0-185d836ec62a@gmail.com" ], "v5": [ "20251113-fix-tags-not-fetching-v5-0-371ea7ec638d@gmail.com" ], "v6": [ "20251118-fix-tags-not-fetching-v6-0-2a2f15fc137e@gmail.com" ], "v7": [ "20251119-fix-tags-not-fetching-v7-0-0c8f9fb1f287@gmail.com" ], "v8": [ "20251121-fix-tags-not-fetching-v8-0-23b53a8a8334@gmail.com" ] } } } -- GitLab From 0ae9b42c7513bdaa8d7ba18f98de0023d2ced799 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 7 Nov 2025 14:18:34 +0100 Subject: [PATCH 2/4] fetch: extract out reference committing logic The `do_fetch()` function contains the core of the `git-fetch(1)` logic. Part of this is to fetch and store references. This is done by 1. Creating a reference transaction (non-atomic mode uses batched updates). 2. Adding individual reference updates to the transaction. 3. Committing the transaction. 4. When using batched updates, handling the rejected updates. The following commit, will fix a bug wherein fetching tags with conflicts was causing other reference updates to fail. Fixing this requires utilizing this logic in different regions of the function. In preparation of the follow up commit, extract the committing and rejection handling logic into a separate function called `commit_ref_transaction()`. Helped-by: Patrick Steinhardt Signed-off-by: Karthik Nayak --- builtin/fetch.c | 59 +++++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 26 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index c7ff3480fb..f90179040b 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1686,6 +1686,36 @@ static void ref_transaction_rejection_handler(const char *refname, *data->retcode = 1; } +/* + * Commit the reference transaction. If it isn't an atomic transaction, handle + * rejected updates as part of using batched updates. + */ +static int commit_ref_transaction(struct ref_transaction **transaction, + bool is_atomic, const char *remote_name, + struct strbuf *err) +{ + int retcode = ref_transaction_commit(*transaction, err); + if (retcode) + goto out; + + if (!is_atomic) { + struct ref_rejection_data data = { + .conflict_msg_shown = 0, + .remote_name = remote_name, + .retcode = &retcode, + }; + + ref_transaction_for_each_rejected_update(*transaction, + ref_transaction_rejection_handler, + &data); + } + +out: + ref_transaction_free(*transaction); + *transaction = NULL; + return retcode; +} + static int do_fetch(struct transport *transport, struct refspec *rs, const struct fetch_config *config) @@ -1858,33 +1888,10 @@ static int do_fetch(struct transport *transport, if (retcode) goto cleanup; - retcode = ref_transaction_commit(transaction, &err); - if (retcode) { - /* - * Explicitly handle transaction cleanup to avoid - * aborting an already closed transaction. - */ - ref_transaction_free(transaction); - transaction = NULL; + retcode = commit_ref_transaction(&transaction, atomic_fetch, + transport->remote->name, &err); + if (retcode) goto cleanup; - } - - if (!atomic_fetch) { - struct ref_rejection_data data = { - .retcode = &retcode, - .conflict_msg_shown = 0, - .remote_name = transport->remote->name, - }; - - ref_transaction_for_each_rejected_update(transaction, - ref_transaction_rejection_handler, - &data); - if (retcode) { - ref_transaction_free(transaction); - transaction = NULL; - goto cleanup; - } - } commit_fetch_head(&fetch_head); -- GitLab From e4d27f47e6aa40301379bee2a87cb41b2b53d461 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 3 Nov 2025 12:45:07 +0100 Subject: [PATCH 3/4] fetch: fix non-conflicting tags not being committed The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19) updated the 'git-fetch(1)' command to use batched updates. This batches updates to gain performance improvements. When fetching references, each update is added to the transaction. Finally, when committing, individual updates are allowed to fail with reason, while the transaction itself succeeds. One scenario which was missed here, was fetching tags. When fetching conflicting tags, the `fetch_and_consume_refs()` function returns '1', which skipped committing the transaction and directly jumped to the cleanup section. This mean that no updates were applied. This also extends to backfilling tags which is done when fetching specific refspecs which contains tags in their history. Fix this by committing the transaction when we have an error code and not using an atomic transaction. This ensures other references are applied even when some updates fail. The cleanup section is reached with `retcode` set in several scenarios: - `truncate_fetch_head()`, `open_fetch_head()` and `prune_refs()` set `retcode` before the transaction is created, so no commit is attempted. - `fetch_and_consume_refs()` and `backfill_tags()` are the primary cases this fix targets, both setting a positive `retcode` to trigger the committing of the transaction. This simplifies error handling and ensures future modifications to `do_fetch()` don't need special handling for batched updates. Add tests to check for this regression. While here, add a missing cleanup from previous test. Reported-by: David Bohman Helped-by: Patrick Steinhardt Signed-off-by: Karthik Nayak --- builtin/fetch.c | 8 +++++++ t/t5510-fetch.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/builtin/fetch.c b/builtin/fetch.c index f90179040b..b19fa8e966 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1957,6 +1957,14 @@ static int do_fetch(struct transport *transport, } cleanup: + /* + * When using batched updates, we want to commit the non-rejected + * updates and also handle the rejections. + */ + if (retcode && !atomic_fetch && transaction) + commit_ref_transaction(&transaction, false, + transport->remote->name, &err); + if (retcode) { if (err.len) { error("%s", err.buf); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index b7059cccaa..4b113d7c27 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti ' test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' ' + test_when_finished rm -rf base repo && ( git init --ref-format=reftable base && cd base && @@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc ) ' +test_expect_success 'fetch --tags fetches existing tags' ' + test_when_finished rm -rf base repo && + + git init base && + git -C base commit --allow-empty -m "empty-commit" && + + git clone --bare base repo && + + git -C base tag tag-1 && + git -C repo for-each-ref >out && + test_grep ! "tag-1" out && + git -C repo fetch --tags && + git -C repo for-each-ref >out && + test_grep "tag-1" out +' + +test_expect_success 'fetch --tags fetches non-conflicting tags' ' + test_when_finished rm -rf base repo && + + git init base && + git -C base commit --allow-empty -m "empty-commit" && + git -C base tag tag-1 && + + git clone --bare base repo && + + git -C base tag tag-2 && + git -C repo for-each-ref >out && + test_grep ! "tag-2" out && + + git -C base commit --allow-empty -m "second empty-commit" && + git -C base tag -f tag-1 && + + test_must_fail git -C repo fetch --tags 2>out && + test_grep "tag-1 (would clobber existing tag)" out && + git -C repo for-each-ref >out && + test_grep "tag-2" out +' + +test_expect_success "backfill tags when providing a refspec" ' + test_when_finished rm -rf source target && + + git init source && + git -C source commit --allow-empty --message common && + git clone file://"$(pwd)"/source target && + ( + cd source && + test_commit history && + test_commit fetch-me + ) && + + # The "history" tag is backfilled eventhough we requested + # to only fetch HEAD + git -C target fetch origin HEAD:branch && + git -C target tag -l >actual && + cat >expect <<-\EOF && + fetch-me + history + EOF + test_cmp expect actual +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -- GitLab From 7a8561485a9c15d47e507726d1f268f30e0d033a Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Mon, 17 Nov 2025 15:59:08 +0100 Subject: [PATCH 4/4] fetch: fix failed batched updates skipping operations Fix a regression introduced with batched updates in 0e358de64a (fetch: use batched reference updates, 2025-05-19) when fetching references. In the `do_fetch()` function, we jump to cleanup if committing the transaction fails, regardless of whether using batched or atomic updates. This skips three subsequent operations: - Update 'FETCH_HEAD' as part of `commit_fetch_head()`. - Add upstream tracking information via `set_upstream()`. - Setting remote 'HEAD' values when `do_set_head` is true. For atomic updates, this is expected behavior. For batched updates, we want to continue with these operations even if some refs fail to update. Skipping `commit_fetch_head()` isn't actually a regression because 'FETCH_HEAD' is already updated via `append_fetch_head()` when not using '--atomic'. However, we add a test to validate this behavior. Skipping the other two operations (upstream tracking and remote HEAD) is a regression. Fix this by only jumping to cleanup when using '--atomic', allowing batched updates to continue with post-fetch operations. Add tests to prevent future regressions. Helped-by: Junio C Hamano Signed-off-by: Karthik Nayak --- builtin/fetch.c | 6 +++- t/t5510-fetch.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index b19fa8e966..74bf67349d 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1890,7 +1890,11 @@ static int do_fetch(struct transport *transport, retcode = commit_ref_transaction(&transaction, atomic_fetch, transport->remote->name, &err); - if (retcode) + /* + * With '--atomic', bail out if the transaction fails. Without '--atomic', + * continue to fetch head and perform other post-fetch operations. + */ + if (retcode && atomic_fetch) goto cleanup; commit_fetch_head(&fetch_head); diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4b113d7c27..a1ca4e1ac7 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1639,6 +1639,94 @@ test_expect_success "backfill tags when providing a refspec" ' test_cmp expect actual ' +test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" ' + test_when_finished rm -rf base repo && + + git init base && + ( + cd base && + test_commit "updated" && + + git update-ref refs/heads/foo @ && + git update-ref refs/heads/branch @ + ) && + + git init --bare repo && + ( + cd repo && + rm -f FETCH_HEAD && + git remote add origin ../base && + >refs/heads/foo.lock && + test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && + test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && + test_grep "branch ${SQ}branch${SQ} of ../base" FETCH_HEAD && + test_grep "branch ${SQ}foo${SQ} of ../base" FETCH_HEAD + ) +' + +test_expect_success "upstream tracking info is added with --set-upstream" ' + test_when_finished rm -rf base repo && + + git init --initial-branch=main base && + test_commit -C base "updated" && + + git init --bare --initial-branch=main repo && + ( + cd repo && + git remote add origin ../base && + git fetch origin --set-upstream main && + git config get branch.main.remote >actual && + echo "origin" >expect && + test_cmp expect actual + ) +' + +test_expect_success REFFILES "upstream tracking info is added even with conflicts" ' + test_when_finished rm -rf base repo && + + git init --initial-branch=main base && + test_commit -C base "updated" && + + git init --bare --initial-branch=main repo && + ( + cd repo && + git remote add origin ../base && + test_must_fail git config get branch.main.remote && + + mkdir -p refs/remotes/origin && + >refs/remotes/origin/main.lock && + test_must_fail git fetch origin --set-upstream main && + git config get branch.main.remote >actual && + echo "origin" >expect && + test_cmp expect actual + ) +' + +test_expect_success REFFILES "HEAD is updated even with conflicts" ' + test_when_finished rm -rf base repo && + + git init base && + ( + cd base && + test_commit "updated" && + + git update-ref refs/heads/foo @ && + git update-ref refs/heads/branch @ + ) && + + git init --bare repo && + ( + cd repo && + git remote add origin ../base && + + test_path_is_missing refs/remotes/origin/HEAD && + mkdir -p refs/remotes/origin && + >refs/remotes/origin/branch.lock && + test_must_fail git fetch origin && + test -f refs/remotes/origin/HEAD + ) +' + . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd -- GitLab