From c3cf8e5907adb55380801007ff14f0e3b7cf7152 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 21 Nov 2025 12:13:45 +0100 Subject: [PATCH 1/3] 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 Signed-off-by: Junio C Hamano --- 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); From 8ff2eef8ada18c2d7ef61b1e8e13d53937524908 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 21 Nov 2025 12:13:46 +0100 Subject: [PATCH 2/3] 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 Signed-off-by: Junio C Hamano --- 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..f500cb83ca 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 even though 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 From b7b17ec8a6b1cb176206ad69c194b84eb3490b99 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Fri, 21 Nov 2025 12:13:47 +0100 Subject: [PATCH 3/3] 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 Signed-off-by: Junio C Hamano --- 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 f500cb83ca..ce1c23684e 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