[PATCH v7 0/3] fetch: fix non-conflicting tags not being committed
From: Karthik Nayak <hidden>
Date: 2025-11-19 21:46:39
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 <redacted> --- 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 --- builtin/fetch.c | 71 ++++++++++++++++---------- t/t5510-fetch.sh | 149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 194 insertions(+), 26 deletions(-) Karthik Nayak (3): fetch: extract out reference committing logic fetch: fix non-conflicting tags not being committed fetch: fix failed batched updates skipping operations Range-diff versus v6: 1: e16f0034a7 = 1: c5b451d0a0 fetch: extract out reference committing logic 2: 5145e93e99 = 2: 59e97f54af fetch: fix non-conflicting tags not being committed 3: 8fb6ef3079 ! 3: 1bf509a96f fetch: fix failed batched updates skipping operations @@ t/t5510-fetch.sh: test_expect_success "backfill tags when providing a refspec" ' + cd repo && + ! test -f FETCH_HEAD && + git remote add origin ../base && -+ touch refs/heads/foo.lock && ++ >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 -f FETCH_HEAD + ) +' + -+test_expect_success REFFILES "upstream tracking info is added with --set-upstream" ' ++test_expect_success "upstream tracking info is added with --set-upstream" ' + test_when_finished rm -rf base repo && + + git init --initial-branch=main base && @@ t/t5510-fetch.sh: test_expect_success "backfill tags when providing a refspec" ' + test_must_fail git config get branch.main.remote && + + mkdir -p refs/remotes/origin && -+ touch refs/remotes/origin/main.lock && ++ >refs/remotes/origin/main.lock && + test_must_fail git fetch origin --set-upstream main && + git config get branch.main.remote >actual && + echo "origin" >expect && @@ t/t5510-fetch.sh: test_expect_success "backfill tags when providing a refspec" ' + + ! test -f refs/remotes/origin/HEAD && + mkdir -p refs/remotes/origin && -+ touch refs/remotes/origin/branch.lock && ++ >refs/remotes/origin/branch.lock && + test_must_fail git fetch origin && + test -f refs/remotes/origin/HEAD + ) base-commit: a99f379adf116d53eb11957af5bab5214915f91d change-id: 20251103-fix-tags-not-fetching-0f1621a474d4 Thanks - Karthik