* Re: [PATCH] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-03 17:53 ` Eric Sunshine
2025-11-03 21:22 ` Karthik Nayak
2025-11-03 20:52 ` Justin Tobler
` (7 subsequent siblings)
8 siblings, 1 reply; 54+ messages in thread
From: Eric Sunshine @ 2025-11-03 17:53 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, David Bohman
On Mon, Nov 3, 2025 at 8:49 AM Karthik Nayak <karthik.188@gmail.com> wrote:
> 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.
> [...]
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -1577,6 +1578,46 @@ 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
> + )
> +'
What is the purpose of wrapping this code in a subshell?
Same question regarding the other test added by this patch.
> +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 &&
> +
> + ! git -C repo fetch --tags 2>out &&
Should this be using `test_must_fail` rather than `!`?
> + test_grep "tag-1 (would clobber existing tag)" out &&
> + git -C repo for-each-ref >out &&
> + test_grep "tag-2" out
> + )
> +'
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH] fetch: fix non-conflicting tags not being committed
2025-11-03 17:53 ` Eric Sunshine
@ 2025-11-03 21:22 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-03 21:22 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, David Bohman
[-- Attachment #1: Type: text/plain, Size: 2612 bytes --]
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Mon, Nov 3, 2025 at 8:49 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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.
>> [...]
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> @@ -1577,6 +1578,46 @@ 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
>> + )
>> +'
>
> What is the purpose of wrapping this code in a subshell?
>
> Same question regarding the other test added by this patch.
>
It's not needed, I first created two subshells with cd into each of
them. I let it be when I merged them. So let me remove them.
>> +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 &&
>> +
>> + ! git -C repo fetch --tags 2>out &&
>
> Should this be using `test_must_fail` rather than `!`?
>
Yes, will fix!
>> + test_grep "tag-1 (would clobber existing tag)" out &&
>> + git -C repo for-each-ref >out &&
>> + test_grep "tag-2" out
>> + )
>> +'
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-03 17:53 ` Eric Sunshine
@ 2025-11-03 20:52 ` Justin Tobler
2025-11-06 8:39 ` [PATCH v2] " Karthik Nayak
` (6 subsequent siblings)
8 siblings, 0 replies; 54+ messages in thread
From: Justin Tobler @ 2025-11-03 20:52 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, David Bohman
On 25/11/03 02:49PM, Karthik Nayak wrote:
> 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.
Ok so when fetching tags, if there is a reference conflict, we are
bailing out without committing the transaction. In such cases, we
actually want to handle the rejected reference updates and continue with
the transaction.
> Fix this by committing the transaction even when we have an error code.
> This ensures other references are applied. Do this by extracting out the
> transaction commit code into a new `commit_ref_transaction()` function
> and using that.
Makes sense.
> Add two tests to check for this regression. While here, add a missing
> cleanup from previous test.
>
> Reported-by: David Bohman <debohman@gmail.com>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> This fixes the bug reported by David Bohman [1].
>
> [1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
> ---
> builtin/fetch.c | 65 +++++++++++++++++++++++++++++++++-----------------------
> t/t5510-fetch.sh | 41 +++++++++++++++++++++++++++++++++++
> 2 files changed, 79 insertions(+), 27 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c7ff3480fb..8dea08dc74 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1686,6 +1686,38 @@ static void ref_transaction_rejection_handler(const char *refname,
> *data->retcode = 1;
> }
>
> +static int commit_ref_transaction(struct ref_transaction **transaction,
> + bool is_atomic, const char *remote_name,
> + struct strbuf *err)
nit: I think `commit_ref_transaction()` here can easily be confused with
`ref_transaction_commit()` and it's not exactly clear how they differ
from the names alone. Maybe we could explain the additional
responsibilities in a comment?
> +{
> + int retcode = ref_transaction_commit(*transaction, err);
> + if (retcode) {
> + /*
> + * Explicitly handle transaction cleanup to avoid
> + * aborting an already closed transaction.
> + */
> + ref_transaction_free(*transaction);
> + *transaction = NULL;
> + }
> +
> + if (*transaction && !is_atomic) {
> + struct ref_rejection_data data = {
> + .conflict_msg_shown = 0,
> + .remote_name = remote_name,
> + .retcode = &retcode,
> + };
> +
> + ref_transaction_for_each_rejected_update(*transaction,
> + ref_transaction_rejection_handler,
> + &data);
> +
> + ref_transaction_free(*transaction);
> + *transaction = NULL;
> + }
> +
> + return retcode;
> +}
> +
> static int do_fetch(struct transport *transport,
> struct refspec *rs,
> const struct fetch_config *config)
> @@ -1826,6 +1858,10 @@ static int do_fetch(struct transport *transport,
>
> if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
> &fetch_head, config)) {
> + /* As we're using batched updates, commit any pending updates. */
> + if (!atomic_fetch)
> + commit_ref_transaction(&transaction, false,
> + transport->remote->name, &err);
IIUC, when we encounter an error via `fetch_and_consume_refs()` we now
explicitly commit the transaction early and handle the rejected
references. At first I wondered why we wouldn't just skip the "goto
cleanup" in such cases, but I assume this is in part because we are
trying to match the pre-batched updates behavior.
Naive question: I noticed that `backfill_tags()` also invokes
`fetch_and_consume_refs()`. Do we also need to commit pending updates
there in case of reference conflicts?
> retcode = 1;
> goto cleanup;
> }
> @@ -1858,33 +1894,8 @@ static int do_fetch(struct transport *transport,
> if (retcode)
> goto cleanup;
>
> - retcode = ref_transaction_commit(transaction, &err);
> - if (retcode) {
> - /*
> - * Explicitly handle transaction cleanup to avoid
> - * aborting an already closed transaction.
> - */
> - ref_transaction_free(transaction);
> - transaction = NULL;
> - goto cleanup;
> - }
> -
> - if (!atomic_fetch) {
> - struct ref_rejection_data data = {
> - .retcode = &retcode,
> - .conflict_msg_shown = 0,
> - .remote_name = transport->remote->name,
> - };
> -
> - ref_transaction_for_each_rejected_update(transaction,
> - ref_transaction_rejection_handler,
> - &data);
> - if (retcode) {
> - ref_transaction_free(transaction);
> - transaction = NULL;
> - goto cleanup;
> - }
> - }
> + retcode = commit_ref_transaction(&transaction, atomic_fetch,
> + transport->remote->name, &err);
This is where we would normally commit the reference transaction and
handle rejected reference updates. Now we just reuse
`commit_ref_transaction()`.
Do we need to check the return value and potentially "goto cleanup"
before proceeding?
-Justin
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-03 17:53 ` Eric Sunshine
2025-11-03 20:52 ` Justin Tobler
@ 2025-11-06 8:39 ` Karthik Nayak
2025-11-06 11:50 ` Patrick Steinhardt
2025-11-06 22:10 ` Justin Tobler
2025-11-08 21:34 ` [PATCH v3 0/2] " Karthik Nayak
` (5 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-06 8:39 UTC (permalink / raw)
To: git; +Cc: jltobler, sunshine, David Bohman, Karthik Nayak
The commit 0e358de64a (fetch: use batched reference updates, 2025-05-19)
updated the 'git-fetch(1)' command to use batched updates. This batches
updates to gain performance improvements. When fetching references, each
update is added to the transaction. Finally, when committing, individual
updates are allowed to fail with reason, while the transaction itself
succeeds.
One scenario which was missed here, was fetching tags. When fetching
conflicting tags, the `fetch_and_consume_refs()` function returns '1',
which skipped committing the transaction and directly jumped to the
cleanup section. This mean that no updates were applied. This also
extends to backfilling tags when using the now deprecated 'branches/'
format for remotes.
Fix this by committing the transaction even when we have an error code.
This ensures other references are applied. Do this by extracting out the
transaction commit code into a new `commit_ref_transaction()` function
and using that.
Add tests to check for this regression. While here, add a missing
cleanup from previous test.
Reported-by: David Bohman <debohman@gmail.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
This fixes the bug reported by David Bohman [1].
[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
---
Changes in v2:
- Add a comment to explain the purpose of `commit_ref_transaction()` and
how it works.
- Also extend the same logic towards backfilling tags. While I was able
to add a test for the happy path, I couldn't figure out how to test
when `backfill_tags()` tags would fail.
Tangentially, this flow seems to only be triggered when using the now
deprecated 'branches/' remote format.
- Remove unneeded subshells from the tests.
- Link to v1: https://patch.msgid.link/20251103-fix-tags-not-fetching-v1-1-e63caeb6c113@gmail.com
---
builtin/fetch.c | 75 +++++++++++++++++++++++++++++++++++---------------------
t/t5510-fetch.sh | 61 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 108 insertions(+), 28 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c7ff3480fb..d5aee5af10 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
*data->retcode = 1;
}
+/*
+ * Commit the reference transaction. If it isn't an atomic transaction, handle
+ * rejected updates as part of using batched updates.
+ */
+static int commit_ref_transaction(struct ref_transaction **transaction,
+ bool is_atomic, const char *remote_name,
+ struct strbuf *err)
+{
+ int retcode = ref_transaction_commit(*transaction, err);
+ if (retcode) {
+ /*
+ * Explicitly handle transaction cleanup to avoid
+ * aborting an already closed transaction.
+ */
+ ref_transaction_free(*transaction);
+ *transaction = NULL;
+ }
+
+ if (*transaction && !is_atomic) {
+ struct ref_rejection_data data = {
+ .conflict_msg_shown = 0,
+ .remote_name = remote_name,
+ .retcode = &retcode,
+ };
+
+ ref_transaction_for_each_rejected_update(*transaction,
+ ref_transaction_rejection_handler,
+ &data);
+
+ ref_transaction_free(*transaction);
+ *transaction = NULL;
+ }
+
+ return retcode;
+}
+
static int do_fetch(struct transport *transport,
struct refspec *rs,
const struct fetch_config *config)
@@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,
if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
&fetch_head, config)) {
+ /* As we're using batched updates, commit any pending updates. */
+ if (!atomic_fetch)
+ commit_ref_transaction(&transaction, false,
+ transport->remote->name, &err);
retcode = 1;
goto cleanup;
}
@@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,
* the transaction and don't commit anything.
*/
if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
- &fetch_head, config))
+ &fetch_head, config)) {
+ if (!atomic_fetch)
+ commit_ref_transaction(&transaction, false,
+ transport->remote->name, &err);
retcode = 1;
+ }
}
free_refs(tags_ref_map);
@@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
- retcode = ref_transaction_commit(transaction, &err);
- if (retcode) {
- /*
- * Explicitly handle transaction cleanup to avoid
- * aborting an already closed transaction.
- */
- ref_transaction_free(transaction);
- transaction = NULL;
- goto cleanup;
- }
-
- if (!atomic_fetch) {
- struct ref_rejection_data data = {
- .retcode = &retcode,
- .conflict_msg_shown = 0,
- .remote_name = transport->remote->name,
- };
-
- ref_transaction_for_each_rejected_update(transaction,
- ref_transaction_rejection_handler,
- &data);
- if (retcode) {
- ref_transaction_free(transaction);
- transaction = NULL;
- goto cleanup;
- }
- }
+ retcode = commit_ref_transaction(&transaction, atomic_fetch,
+ transport->remote->name, &err);
commit_fetch_head(&fetch_head);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index b7059cccaa..9ff656a2bc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1552,6 +1552,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti
'
test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with lock' '
+ test_when_finished rm -rf base repo &&
(
git init --ref-format=reftable base &&
cd base &&
@@ -1577,6 +1578,66 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
)
'
+test_expect_success 'fetch --tags fetches existing tags' '
+ test_when_finished rm -rf base repo &&
+
+ git init base &&
+ git -C base commit --allow-empty -m "empty-commit" &&
+
+ git clone --bare base repo &&
+
+ git -C base tag tag-1 &&
+ git -C repo for-each-ref >out &&
+ test_grep ! "tag-1" out &&
+ git -C repo fetch --tags &&
+ git -C repo for-each-ref >out &&
+ test_grep "tag-1" out
+'
+
+test_expect_success 'fetch --tags fetches non-conflicting tags' '
+ test_when_finished rm -rf base repo &&
+
+ git init base &&
+ git -C base commit --allow-empty -m "empty-commit" &&
+ git -C base tag tag-1 &&
+
+ git clone --bare base repo &&
+
+ git -C base tag tag-2 &&
+ git -C repo for-each-ref >out &&
+ test_grep ! "tag-2" out &&
+
+ git -C base commit --allow-empty -m "second empty-commit" &&
+ git -C base tag -f tag-1 &&
+
+ test_must_fail git -C repo fetch --tags 2>out &&
+ test_grep "tag-1 (would clobber existing tag)" out &&
+ git -C repo for-each-ref >out &&
+ test_grep "tag-2" out
+'
+
+test_expect_success 'backfill tags with branches remote format' '
+ test_when_finished rm -rf base repo &&
+
+ git init base &&
+ git -C base commit --allow-empty -m "empty-commit" &&
+ git -C base tag tag1 &&
+
+ git clone --no-tags base repo &&
+
+ git -C repo remote remove origin &&
+ mkdir -p repo/.git/branches &&
+ echo "$(cd base && pwd)#master" >repo/.git/branches/origin &&
+
+ git -C base commit --allow-empty -m "second empty-commit" &&
+ git -C base tag tag2 &&
+
+ git -C repo fetch origin &&
+ git -C repo for-each-ref refs/tags >out &&
+ test_grep "tag1" out &&
+ test_grep "tag2" out
+'
+
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-06 8:39 ` [PATCH v2] " Karthik Nayak
@ 2025-11-06 11:50 ` Patrick Steinhardt
2025-11-06 18:56 ` Junio C Hamano
2025-11-07 13:15 ` Karthik Nayak
2025-11-06 22:10 ` Justin Tobler
1 sibling, 2 replies; 54+ messages in thread
From: Patrick Steinhardt @ 2025-11-06 11:50 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, sunshine, David Bohman
On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:
> 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.
Okay, this is obviously broken indeed.
> This also extends to backfilling tags when using the now deprecated
> 'branches/' format for remotes.
I'm a bit lost here -- what does backfilling have to do with the
"branches/" directory? The backfill is supposed to create tags that
point into the history that one has just fetched. So:
- With `--tags` we fetch all tags announced by the remote.
- With `--no-tags` we fetch no tags.
- Otherwise we fetch those tags that point into our history.
The last behaviour is a bit more on the esoteric side, but it's
described as such in git-fetch(1):
By default, any tag that points into the histories being fetched is
also fetched; the effect is to fetch tags that point at branches
that you are interested in. This default behavior can be changed by
using the --tags or --no-tags options or by configuring
remote.<name>.tagOpt. By using a refspec that fetches tags
explicitly, you can fetch tags that do not point into branches you
are interested in as well.
The following test demonstrates this behaviour:
test_expect_success "fetch single branch without explicit tag option" '
git init source &&
git -C source commit --allow-empty --message common &&
git clone file://"$(pwd)"/source target &&
(
cd source &&
git commit --allow-empty --message discard-me &&
git tag discard-me &&
git commit --amend --allow-empty --message fetch-me &&
git tag fetch-me
) &&
# The "discard-me" tag does not point into the history that we are
# about to fetch, so it should not have been created.
git -C target fetch origin &&
git -C target tag -l >actual &&
echo "fetch-me" >expect &&
# But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
# should now see it.
git -C target fetch origin --tags &&
git -C target tag -l >actual &&
cat >expect <<-\EOF &&
discard-me
fetch-me
EOF
test_cmp expect actual
'
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c7ff3480fb..d5aee5af10 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
> *data->retcode = 1;
> }
>
> +/*
> + * Commit the reference transaction. If it isn't an atomic transaction, handle
> + * rejected updates as part of using batched updates.
> + */
> +static int commit_ref_transaction(struct ref_transaction **transaction,
> + bool is_atomic, const char *remote_name,
> + struct strbuf *err)
> +{
> + int retcode = ref_transaction_commit(*transaction, err);
> + if (retcode) {
> + /*
> + * Explicitly handle transaction cleanup to avoid
> + * aborting an already closed transaction.
> + */
> + ref_transaction_free(*transaction);
> + *transaction = NULL;
> + }
> +
> + if (*transaction && !is_atomic) {
> + struct ref_rejection_data data = {
> + .conflict_msg_shown = 0,
> + .remote_name = remote_name,
> + .retcode = &retcode,
> + };
> +
> + ref_transaction_for_each_rejected_update(*transaction,
> + ref_transaction_rejection_handler,
> + &data);
> +
> + ref_transaction_free(*transaction);
> + *transaction = NULL;
> + }
Okay. Do we need to discern cases where this is called and we haven't
managed to even queue a single reference update?
> + return retcode;
> +}
> +
> static int do_fetch(struct transport *transport,
> struct refspec *rs,
> const struct fetch_config *config)
Nit: it might make sense to have a preparatory commit that extracts the
function but that is otherwise a no-op change.
> @@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,
>
> if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
> &fetch_head, config)) {
> + /* As we're using batched updates, commit any pending updates. */
> + if (!atomic_fetch)
> + commit_ref_transaction(&transaction, false,
> + transport->remote->name, &err);
> retcode = 1;
> goto cleanup;
> }
Hm. Don't we also have to unset the transaction now? Ah, no, you pass
the pointer to the transaction here and set it to `NULL` in
`commit_ref_transaction()`. Makes sense.
> @@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,
> * the transaction and don't commit anything.
> */
> if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
> - &fetch_head, config))
> + &fetch_head, config)) {
> + if (!atomic_fetch)
> + commit_ref_transaction(&transaction, false,
> + transport->remote->name, &err);
> retcode = 1;
> + }
> }
>
> free_refs(tags_ref_map);
We now have three different callsites where we commit the transaction.
It gets better due to the newly introduced function, but it overall
feels somewhat fragile regardless of that.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-06 11:50 ` Patrick Steinhardt
@ 2025-11-06 18:56 ` Junio C Hamano
2025-11-07 13:15 ` Karthik Nayak
1 sibling, 0 replies; 54+ messages in thread
From: Junio C Hamano @ 2025-11-06 18:56 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git, jltobler, sunshine, David Bohman
Patrick Steinhardt <ps@pks.im> writes:
> We now have three different callsites where we commit the transaction.
> It gets better due to the newly introduced function, but it overall
> feels somewhat fragile regardless of that.
Indeed.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-06 11:50 ` Patrick Steinhardt
2025-11-06 18:56 ` Junio C Hamano
@ 2025-11-07 13:15 ` Karthik Nayak
2025-11-07 14:07 ` Patrick Steinhardt
1 sibling, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-07 13:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, sunshine, David Bohman
[-- Attachment #1: Type: text/plain, Size: 8636 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:
>> 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.
>
> Okay, this is obviously broken indeed.
>
>> This also extends to backfilling tags when using the now deprecated
>> 'branches/' format for remotes.
>
> I'm a bit lost here -- what does backfilling have to do with the
> "branches/" directory? The backfill is supposed to create tags that
> point into the history that one has just fetched. So:
>
I didn't read the code well enough. Let me walk through what I read:
The block for backfilling tags, is only triggered in `do_fetch()`, if
if (tags == TAGS_DEFAULT && autotags) { ... }
This means that the autotags must be '1'. And I see at the start of the
function that:
int autotags = (transport->remote->fetch_tags == 1);
So I went into looking when `transport->remote->fetch_tags` would be set
to '1'. This is only done in `read_branches_file()` which is done when
parsing the now deprecated 'branches/' directory.
I was correct until here. But, there is something I missed.
We also pass a pointer to `autotags` to the `get_ref_map()` function. In
this function, we set `autotags` to '1' for any of the following
conditions:
- When there is a refspec specified by the user.
- We have a default branch with a remote specified.
So this means there are other scenarios we use the backfill() command.
That brings us to the second part of it, if we specify the '--tags'
flag, then we fetch all tags, even the ones which aren't part of our
history. This also happens as part of the `get_ref_map()` function. This
flow also skips the 'backfill()' function.
So in effect, we only backfill tags, when the user doesn't specify
either '--tags' or '--no-tags'.
> - With `--tags` we fetch all tags announced by the remote.
>
> - With `--no-tags` we fetch no tags.
>
> - Otherwise we fetch those tags that point into our history.
>
> The last behaviour is a bit more on the esoteric side, but it's
> described as such in git-fetch(1):
>
> By default, any tag that points into the histories being fetched is
> also fetched; the effect is to fetch tags that point at branches
> that you are interested in. This default behavior can be changed by
> using the --tags or --no-tags options or by configuring
> remote.<name>.tagOpt. By using a refspec that fetches tags
> explicitly, you can fetch tags that do not point into branches you
> are interested in as well.
>
But backfilling isn't about diverged history, no? It's about fetching
history of refs being requested.
> The following test demonstrates this behaviour:
>
> test_expect_success "fetch single branch without explicit tag option" '
> git init source &&
> git -C source commit --allow-empty --message common &&
> git clone file://"$(pwd)"/source target &&
> (
> cd source &&
> git commit --allow-empty --message discard-me &&
> git tag discard-me &&
> git commit --amend --allow-empty --message fetch-me &&
> git tag fetch-me
> ) &&
>
> # The "discard-me" tag does not point into the history that we are
> # about to fetch, so it should not have been created.
> git -C target fetch origin &&
> git -C target tag -l >actual &&
> echo "fetch-me" >expect &&
>
> # But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
> # should now see it.
> git -C target fetch origin --tags &&
Here, we don't really backfill, but rather we request all tags from the
remote, hence we end up with the 'discard-me' tag. Not because of the
diverged history. I also confirmed this by adding a breakpoint into the
`backfill_tags()` function, while running this test.
> git -C target tag -l >actual &&
> cat >expect <<-\EOF &&
> discard-me
> fetch-me
> EOF
> test_cmp expect actual
> '
But I was able to slightly modify the test to get the required affect:
test_expect_success "backfill tags when providing a refspec" '
git init source &&
git -C source commit --allow-empty --message common &&
git clone file://"$(pwd)"/source target &&
(
cd source &&
git commit --allow-empty --message history &&
git tag history &&
git commit --allow-empty --message fetch-me &&
git tag fetch-me
) &&
# The "history" tag is backfilled eventhough we requested
# to only fetch the master
git -C target fetch origin master:branch &&
git -C target tag -l >actual &&
cat >expect <<-\EOF &&
fetch-me
history
EOF
test_cmp expect actual
'
I will add this in. Thanks for the explanation, it really helped
consolidate my understanding here.
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index c7ff3480fb..d5aee5af10 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
>> *data->retcode = 1;
>> }
>>
>> +/*
>> + * Commit the reference transaction. If it isn't an atomic transaction, handle
>> + * rejected updates as part of using batched updates.
>> + */
>> +static int commit_ref_transaction(struct ref_transaction **transaction,
>> + bool is_atomic, const char *remote_name,
>> + struct strbuf *err)
>> +{
>> + int retcode = ref_transaction_commit(*transaction, err);
>> + if (retcode) {
>> + /*
>> + * Explicitly handle transaction cleanup to avoid
>> + * aborting an already closed transaction.
>> + */
>> + ref_transaction_free(*transaction);
>> + *transaction = NULL;
>> + }
>> +
>> + if (*transaction && !is_atomic) {
>> + struct ref_rejection_data data = {
>> + .conflict_msg_shown = 0,
>> + .remote_name = remote_name,
>> + .retcode = &retcode,
>> + };
>> +
>> + ref_transaction_for_each_rejected_update(*transaction,
>> + ref_transaction_rejection_handler,
>> + &data);
>> +
>> + ref_transaction_free(*transaction);
>> + *transaction = NULL;
>> + }
>
> Okay. Do we need to discern cases where this is called and we haven't
> managed to even queue a single reference update?
>
I don't see a reason. This is anyways a post-commit action, if there are
no updates, there will be no rejections. So this will be a no-op.
>> + return retcode;
>> +}
>> +
>> static int do_fetch(struct transport *transport,
>> struct refspec *rs,
>> const struct fetch_config *config)
>
> Nit: it might make sense to have a preparatory commit that extracts the
> function but that is otherwise a no-op change.
>
Let me do that. I was thinking the change is small. But perhaps that'd
be easier for reviewing.
>> @@ -1826,6 +1862,10 @@ static int do_fetch(struct transport *transport,
>>
>> if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
>> &fetch_head, config)) {
>> + /* As we're using batched updates, commit any pending updates. */
>> + if (!atomic_fetch)
>> + commit_ref_transaction(&transaction, false,
>> + transport->remote->name, &err);
>> retcode = 1;
>> goto cleanup;
>> }
>
> Hm. Don't we also have to unset the transaction now? Ah, no, you pass
> the pointer to the transaction here and set it to `NULL` in
> `commit_ref_transaction()`. Makes sense.
>
>> @@ -1848,8 +1888,12 @@ static int do_fetch(struct transport *transport,
>> * the transaction and don't commit anything.
>> */
>> if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
>> - &fetch_head, config))
>> + &fetch_head, config)) {
>> + if (!atomic_fetch)
>> + commit_ref_transaction(&transaction, false,
>> + transport->remote->name, &err);
>> retcode = 1;
>> + }
>> }
>>
>> free_refs(tags_ref_map);
>
> We now have three different callsites where we commit the transaction.
> It gets better due to the newly introduced function, but it overall
> feels somewhat fragile regardless of that.
>
Yeah I must agree with that. I could think of a cleaner way, but will
spend some time here.
> Thanks!
>
> Patrick
Thanks,
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-07 13:15 ` Karthik Nayak
@ 2025-11-07 14:07 ` Patrick Steinhardt
2025-11-07 15:13 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-11-07 14:07 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, sunshine, David Bohman
On Fri, Nov 07, 2025 at 05:15:32AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:
> > The following test demonstrates this behaviour:
> >
> > test_expect_success "fetch single branch without explicit tag option" '
> > git init source &&
> > git -C source commit --allow-empty --message common &&
> > git clone file://"$(pwd)"/source target &&
> > (
> > cd source &&
> > git commit --allow-empty --message discard-me &&
> > git tag discard-me &&
> > git commit --amend --allow-empty --message fetch-me &&
> > git tag fetch-me
> > ) &&
> >
> > # The "discard-me" tag does not point into the history that we are
> > # about to fetch, so it should not have been created.
> > git -C target fetch origin &&
> > git -C target tag -l >actual &&
> > echo "fetch-me" >expect &&
> >
> > # But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
> > # should now see it.
> > git -C target fetch origin --tags &&
>
> Here, we don't really backfill, but rather we request all tags from the
> remote, hence we end up with the 'discard-me' tag. Not because of the
> diverged history. I also confirmed this by adding a breakpoint into the
> `backfill_tags()` function, while running this test.
Oh, exactly. But there's two fetches here: the first one only fetches
"fetch-me" because we don't pass "--tags". The second one was simply as
a demonstration that we would also fetch the other tag that doesn't
point into our fetched history with "--tags".
I notice though that the first fetch forgot to `test_cmp`.
> > git -C target tag -l >actual &&
> > cat >expect <<-\EOF &&
> > discard-me
> > fetch-me
> > EOF
> > test_cmp expect actual
> > '
>
> But I was able to slightly modify the test to get the required affect:
>
> test_expect_success "backfill tags when providing a refspec" '
> git init source &&
> git -C source commit --allow-empty --message common &&
> git clone file://"$(pwd)"/source target &&
> (
> cd source &&
> git commit --allow-empty --message history &&
> git tag history &&
> git commit --allow-empty --message fetch-me &&
> git tag fetch-me
> ) &&
>
> # The "history" tag is backfilled eventhough we requested
> # to only fetch the master
> git -C target fetch origin master:branch &&
> git -C target tag -l >actual &&
> cat >expect <<-\EOF &&
> fetch-me
> history
> EOF
> test_cmp expect actual
> '
>
> I will add this in. Thanks for the explanation, it really helped
> consolidate my understanding here.
Yup, that should work, as well.
> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index c7ff3480fb..d5aee5af10 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
[snip]
> >> + if (*transaction && !is_atomic) {
> >> + struct ref_rejection_data data = {
> >> + .conflict_msg_shown = 0,
> >> + .remote_name = remote_name,
> >> + .retcode = &retcode,
> >> + };
> >> +
> >> + ref_transaction_for_each_rejected_update(*transaction,
> >> + ref_transaction_rejection_handler,
> >> + &data);
> >> +
> >> + ref_transaction_free(*transaction);
> >> + *transaction = NULL;
> >> + }
> >
> > Okay. Do we need to discern cases where this is called and we haven't
> > managed to even queue a single reference update?
> >
>
> I don't see a reason. This is anyways a post-commit action, if there are
> no updates, there will be no rejections. So this will be a no-op.
I guess the question was rather whether we fear a negative consequence
by trying to commit an empty transaction. The commit doesn't know to
short-circuit empty transactions, so we'd still end up locking data even
though we eventually end up doing nothing.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-07 14:07 ` Patrick Steinhardt
@ 2025-11-07 15:13 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-07 15:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, sunshine, David Bohman
[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Nov 07, 2025 at 05:15:32AM -0800, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > On Thu, Nov 06, 2025 at 09:39:25AM +0100, Karthik Nayak wrote:
>> > The following test demonstrates this behaviour:
>> >
>> > test_expect_success "fetch single branch without explicit tag option" '
>> > git init source &&
>> > git -C source commit --allow-empty --message common &&
>> > git clone file://"$(pwd)"/source target &&
>> > (
>> > cd source &&
>> > git commit --allow-empty --message discard-me &&
>> > git tag discard-me &&
>> > git commit --amend --allow-empty --message fetch-me &&
>> > git tag fetch-me
>> > ) &&
>> >
>> > # The "discard-me" tag does not point into the history that we are
>> > # about to fetch, so it should not have been created.
>> > git -C target fetch origin &&
>> > git -C target tag -l >actual &&
>> > echo "fetch-me" >expect &&
>> >
>> > # But with "--tags" we instruct git-fetch(1) to fetch all tags, so we
>> > # should now see it.
>> > git -C target fetch origin --tags &&
>>
>> Here, we don't really backfill, but rather we request all tags from the
>> remote, hence we end up with the 'discard-me' tag. Not because of the
>> diverged history. I also confirmed this by adding a breakpoint into the
>> `backfill_tags()` function, while running this test.
>
> Oh, exactly. But there's two fetches here: the first one only fetches
> "fetch-me" because we don't pass "--tags". The second one was simply as
> a demonstration that we would also fetch the other tag that doesn't
> point into our fetched history with "--tags".
>
Yup, even the first 'fetch' doesn't hit the backfill flow. Since it
points to the reference being fetched.
> I notice though that the first fetch forgot to `test_cmp`.
>
>> > git -C target tag -l >actual &&
>> > cat >expect <<-\EOF &&
>> > discard-me
>> > fetch-me
>> > EOF
>> > test_cmp expect actual
>> > '
>>
>> But I was able to slightly modify the test to get the required affect:
>>
>> test_expect_success "backfill tags when providing a refspec" '
>> git init source &&
>> git -C source commit --allow-empty --message common &&
>> git clone file://"$(pwd)"/source target &&
>> (
>> cd source &&
>> git commit --allow-empty --message history &&
>> git tag history &&
>> git commit --allow-empty --message fetch-me &&
>> git tag fetch-me
>> ) &&
>>
>> # The "history" tag is backfilled eventhough we requested
>> # to only fetch the master
>> git -C target fetch origin master:branch &&
>> git -C target tag -l >actual &&
>> cat >expect <<-\EOF &&
>> fetch-me
>> history
>> EOF
>> test_cmp expect actual
>> '
>>
>> I will add this in. Thanks for the explanation, it really helped
>> consolidate my understanding here.
>
> Yup, that should work, as well.
>
>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> >> index c7ff3480fb..d5aee5af10 100644
>> >> --- a/builtin/fetch.c
>> >> +++ b/builtin/fetch.c
>> >> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
> [snip]
>> >> + if (*transaction && !is_atomic) {
>> >> + struct ref_rejection_data data = {
>> >> + .conflict_msg_shown = 0,
>> >> + .remote_name = remote_name,
>> >> + .retcode = &retcode,
>> >> + };
>> >> +
>> >> + ref_transaction_for_each_rejected_update(*transaction,
>> >> + ref_transaction_rejection_handler,
>> >> + &data);
>> >> +
>> >> + ref_transaction_free(*transaction);
>> >> + *transaction = NULL;
>> >> + }
>> >
>> > Okay. Do we need to discern cases where this is called and we haven't
>> > managed to even queue a single reference update?
>> >
>>
>> I don't see a reason. This is anyways a post-commit action, if there are
>> no updates, there will be no rejections. So this will be a no-op.
>
> I guess the question was rather whether we fear a negative consequence
> by trying to commit an empty transaction. The commit doesn't know to
> short-circuit empty transactions, so we'd still end up locking data even
> though we eventually end up doing nothing.
>
> Thanks!
>
> Patrick
That's correct, but that's also an internal detail of the reference
backend.
- In the files backend, since we lock individual files, no updates
means no locks.
- The packed backend and reftable backend would lock the entire
backend.
So I guess this is something we should fix on the backends themselves.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-06 8:39 ` [PATCH v2] " Karthik Nayak
2025-11-06 11:50 ` Patrick Steinhardt
@ 2025-11-06 22:10 ` Justin Tobler
2025-11-07 14:01 ` Karthik Nayak
1 sibling, 1 reply; 54+ messages in thread
From: Justin Tobler @ 2025-11-06 22:10 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, sunshine, David Bohman
On 25/11/06 09:39AM, Karthik Nayak wrote:
> @@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,
> if (retcode)
> goto cleanup;
>
> - retcode = ref_transaction_commit(transaction, &err);
> - if (retcode) {
> - /*
> - * Explicitly handle transaction cleanup to avoid
> - * aborting an already closed transaction.
> - */
> - ref_transaction_free(transaction);
> - transaction = NULL;
> - goto cleanup;
> - }
> -
> - if (!atomic_fetch) {
> - struct ref_rejection_data data = {
> - .retcode = &retcode,
> - .conflict_msg_shown = 0,
> - .remote_name = transport->remote->name,
> - };
> -
> - ref_transaction_for_each_rejected_update(transaction,
> - ref_transaction_rejection_handler,
> - &data);
> - if (retcode) {
> - ref_transaction_free(transaction);
> - transaction = NULL;
> - goto cleanup;
> - }
> - }
> + retcode = commit_ref_transaction(&transaction, atomic_fetch,
> + transport->remote->name, &err);
It looks like previously, whenever `ref_transaction_commit()` or
`ref_transaction_rejection_handler()` returned a non-zero value, we
would "goto cleanup". Now that these operations are handled via
`commit_ref_transaction()` though, it looks like we no longer handle the
"retcode" return value and just continue. I think we still need to check
the return value here.
-Justin
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v2] fetch: fix non-conflicting tags not being committed
2025-11-06 22:10 ` Justin Tobler
@ 2025-11-07 14:01 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-07 14:01 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, sunshine, David Bohman
[-- Attachment #1: Type: text/plain, Size: 1484 bytes --]
Justin Tobler <jltobler@gmail.com> writes:
> On 25/11/06 09:39AM, Karthik Nayak wrote:
>> @@ -1858,33 +1902,8 @@ static int do_fetch(struct transport *transport,
>> if (retcode)
>> goto cleanup;
>>
>> - retcode = ref_transaction_commit(transaction, &err);
>> - if (retcode) {
>> - /*
>> - * Explicitly handle transaction cleanup to avoid
>> - * aborting an already closed transaction.
>> - */
>> - ref_transaction_free(transaction);
>> - transaction = NULL;
>> - goto cleanup;
>> - }
>> -
>> - if (!atomic_fetch) {
>> - struct ref_rejection_data data = {
>> - .retcode = &retcode,
>> - .conflict_msg_shown = 0,
>> - .remote_name = transport->remote->name,
>> - };
>> -
>> - ref_transaction_for_each_rejected_update(transaction,
>> - ref_transaction_rejection_handler,
>> - &data);
>> - if (retcode) {
>> - ref_transaction_free(transaction);
>> - transaction = NULL;
>> - goto cleanup;
>> - }
>> - }
>> + retcode = commit_ref_transaction(&transaction, atomic_fetch,
>> + transport->remote->name, &err);
>
> It looks like previously, whenever `ref_transaction_commit()` or
> `ref_transaction_rejection_handler()` returned a non-zero value, we
> would "goto cleanup". Now that these operations are handled via
> `commit_ref_transaction()` though, it looks like we no longer handle the
> "retcode" return value and just continue. I think we still need to check
> the return value here.
>
> -Justin
Good catch, will add this in. Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 0/2] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (2 preceding siblings ...)
2025-11-06 8:39 ` [PATCH v2] " Karthik Nayak
@ 2025-11-08 21:34 ` Karthik Nayak
2025-11-08 21:34 ` [PATCH v3 1/2] fetch: extract out reference committing logic Karthik Nayak
2025-11-08 21:34 ` [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-11 13:27 ` [PATCH v4 0/2] " Karthik Nayak
` (4 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-08 21:34 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, sunshine, gitster, David Bohman
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.
[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
Signed-off-by: Karthik Nayak <karthik.188@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 | 73 ++++++++++++++++++++++++++++++++++++--------------------
t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 109 insertions(+), 26 deletions(-)
Karthik Nayak (2):
fetch: extract out reference committing logic
fetch: fix non-conflicting tags not being committed
Range-diff versus v2:
1: 703593ef40 ! 1: 8a1efdd999 fetch: fix non-conflicting tags not being committed
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- fetch: fix non-conflicting tags not being committed
+ fetch: extract out reference committing logic
- 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.
+ 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
- One scenario which was missed here, was fetching tags. When fetching
- conflicting tags, the `fetch_and_consume_refs()` function returns '1',
- which skipped committing the transaction and directly jumped to the
- cleanup section. This mean that no updates were applied. This also
- extends to backfilling tags when using the now deprecated 'branches/'
- format for remotes.
+ 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.
- Fix this by committing the transaction even when we have an error code.
- This ensures other references are applied. Do this by extracting out the
- transaction commit code into a new `commit_ref_transaction()` function
- and using that.
+ 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.
- Add tests to check for this regression. While here, add a missing
- cleanup from previous test.
+ In preparation of the follow up commit, extract the committing and
+ rejection handling logic into a separate function called
+ `commit_ref_transaction()`.
- Reported-by: David Bohman <debohman@gmail.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/fetch.c ##
@@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
struct refspec *rs,
const struct fetch_config *config)
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
-
- if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
- &fetch_head, config)) {
-+ /* As we're using batched updates, commit any pending updates. */
-+ if (!atomic_fetch)
-+ commit_ref_transaction(&transaction, false,
-+ transport->remote->name, &err);
- retcode = 1;
- goto cleanup;
- }
-@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
- * the transaction and don't commit anything.
- */
- if (backfill_tags(&display_state, transport, transaction, tags_ref_map,
-- &fetch_head, config))
-+ &fetch_head, config)) {
-+ if (!atomic_fetch)
-+ commit_ref_transaction(&transaction, false,
-+ transport->remote->name, &err);
- retcode = 1;
-+ }
- }
-
- free_refs(tags_ref_map);
-@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
- */
- ref_transaction_free(transaction);
- transaction = NULL;
-- goto cleanup;
++ retcode = commit_ref_transaction(&transaction, atomic_fetch,
++ transport->remote->name, &err);
++ if (retcode)
+ goto cleanup;
- }
-
- if (!atomic_fetch) {
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
- goto cleanup;
- }
- }
-+ retcode = commit_ref_transaction(&transaction, atomic_fetch,
-+ transport->remote->name, &err);
commit_fetch_head(&fetch_head);
-
- ## t/t5510-fetch.sh ##
-@@ t/t5510-fetch.sh: 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 &&
-@@ t/t5510-fetch.sh: test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
- )
- '
-
-+test_expect_success 'fetch --tags fetches existing tags' '
-+ test_when_finished rm -rf base repo &&
-+
-+ git init base &&
-+ git -C base commit --allow-empty -m "empty-commit" &&
-+
-+ git clone --bare base repo &&
-+
-+ git -C base tag tag-1 &&
-+ git -C repo for-each-ref >out &&
-+ test_grep ! "tag-1" out &&
-+ git -C repo fetch --tags &&
-+ git -C repo for-each-ref >out &&
-+ test_grep "tag-1" out
-+'
-+
-+test_expect_success 'fetch --tags fetches non-conflicting tags' '
-+ test_when_finished rm -rf base repo &&
-+
-+ git init base &&
-+ git -C base commit --allow-empty -m "empty-commit" &&
-+ git -C base tag tag-1 &&
-+
-+ git clone --bare base repo &&
-+
-+ git -C base tag tag-2 &&
-+ git -C repo for-each-ref >out &&
-+ test_grep ! "tag-2" out &&
-+
-+ git -C base commit --allow-empty -m "second empty-commit" &&
-+ git -C base tag -f tag-1 &&
-+
-+ test_must_fail git -C repo fetch --tags 2>out &&
-+ test_grep "tag-1 (would clobber existing tag)" out &&
-+ git -C repo for-each-ref >out &&
-+ test_grep "tag-2" out
-+'
-+
-+test_expect_success 'backfill tags with branches remote format' '
-+ test_when_finished rm -rf base repo &&
-+
-+ git init base &&
-+ git -C base commit --allow-empty -m "empty-commit" &&
-+ git -C base tag tag1 &&
-+
-+ git clone --no-tags base repo &&
-+
-+ git -C repo remote remove origin &&
-+ mkdir -p repo/.git/branches &&
-+ echo "$(cd base && pwd)#master" >repo/.git/branches/origin &&
-+
-+ git -C base commit --allow-empty -m "second empty-commit" &&
-+ git -C base tag tag2 &&
-+
-+ git -C repo fetch origin &&
-+ git -C repo for-each-ref refs/tags >out &&
-+ test_grep "tag1" out &&
-+ test_grep "tag2" out
-+'
-+
- . "$TEST_DIRECTORY"/lib-httpd.sh
- start_httpd
-
-: ---------- > 2: 1de8d8b953 fetch: fix non-conflicting tags not being committed
base-commit: a99f379adf116d53eb11957af5bab5214915f91d
change-id: 20251103-fix-tags-not-fetching-0f1621a474d4
Thanks
- Karthik
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v3 1/2] fetch: extract out reference committing logic
2025-11-08 21:34 ` [PATCH v3 0/2] " Karthik Nayak
@ 2025-11-08 21:34 ` Karthik Nayak
2025-11-10 7:34 ` Patrick Steinhardt
2025-11-08 21:34 ` [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
1 sibling, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-08 21:34 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, sunshine, gitster
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()`.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/fetch.c | 65 ++++++++++++++++++++++++++++++++++-----------------------
1 file changed, 39 insertions(+), 26 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c7ff3480fb..49e195199e 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
*data->retcode = 1;
}
+/*
+ * Commit the reference transaction. If it isn't an atomic transaction, handle
+ * rejected updates as part of using batched updates.
+ */
+static int commit_ref_transaction(struct ref_transaction **transaction,
+ bool is_atomic, const char *remote_name,
+ struct strbuf *err)
+{
+ int retcode = ref_transaction_commit(*transaction, err);
+ if (retcode) {
+ /*
+ * Explicitly handle transaction cleanup to avoid
+ * aborting an already closed transaction.
+ */
+ ref_transaction_free(*transaction);
+ *transaction = NULL;
+ }
+
+ if (*transaction && !is_atomic) {
+ struct ref_rejection_data data = {
+ .conflict_msg_shown = 0,
+ .remote_name = remote_name,
+ .retcode = &retcode,
+ };
+
+ ref_transaction_for_each_rejected_update(*transaction,
+ ref_transaction_rejection_handler,
+ &data);
+
+ ref_transaction_free(*transaction);
+ *transaction = NULL;
+ }
+
+ return retcode;
+}
+
static int do_fetch(struct transport *transport,
struct refspec *rs,
const struct fetch_config *config)
@@ -1858,33 +1894,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);
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 1/2] fetch: extract out reference committing logic
2025-11-08 21:34 ` [PATCH v3 1/2] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-10 7:34 ` Patrick Steinhardt
2025-11-10 13:11 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-11-10 7:34 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, sunshine, gitster
On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c7ff3480fb..49e195199e 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
> *data->retcode = 1;
> }
>
> +/*
> + * Commit the reference transaction. If it isn't an atomic transaction, handle
> + * rejected updates as part of using batched updates.
> + */
> +static int commit_ref_transaction(struct ref_transaction **transaction,
> + bool is_atomic, const char *remote_name,
> + struct strbuf *err)
> +{
> + int retcode = ref_transaction_commit(*transaction, err);
> + if (retcode) {
> + /*
> + * Explicitly handle transaction cleanup to avoid
> + * aborting an already closed transaction.
> + */
> + ref_transaction_free(*transaction);
> + *transaction = NULL;
> + }
> +
> + if (*transaction && !is_atomic) {
This condition is somewhat weird, as we know that it won't ever execute
if `retcode` is non-zero. So wouldn't the function be way easier to
follow if you turned the above conditional into a `goto out`?
static int commit_ref_transaction(struct ref_transaction **transaction,
bool is_atomic, const char *remote_name,
struct strbuf *err)
{
int retcode;
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;
}
This feels significantly easier to read to me.
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 1/2] fetch: extract out reference committing logic
2025-11-10 7:34 ` Patrick Steinhardt
@ 2025-11-10 13:11 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-10 13:11 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, sunshine, gitster
[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Sat, Nov 08, 2025 at 10:34:43PM +0100, Karthik Nayak wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index c7ff3480fb..49e195199e 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1686,6 +1686,42 @@ static void ref_transaction_rejection_handler(const char *refname,
>> *data->retcode = 1;
>> }
>>
>> +/*
>> + * Commit the reference transaction. If it isn't an atomic transaction, handle
>> + * rejected updates as part of using batched updates.
>> + */
>> +static int commit_ref_transaction(struct ref_transaction **transaction,
>> + bool is_atomic, const char *remote_name,
>> + struct strbuf *err)
>> +{
>> + int retcode = ref_transaction_commit(*transaction, err);
>> + if (retcode) {
>> + /*
>> + * Explicitly handle transaction cleanup to avoid
>> + * aborting an already closed transaction.
>> + */
>> + ref_transaction_free(*transaction);
>> + *transaction = NULL;
>> + }
>> +
>> + if (*transaction && !is_atomic) {
>
> This condition is somewhat weird, as we know that it won't ever execute
> if `retcode` is non-zero. So wouldn't the function be way easier to
> follow if you turned the above conditional into a `goto out`?
>
I don't have any arguments here, I basically simply moved the code as is
and didn't want to make changes to reduce the review load.
> static int commit_ref_transaction(struct ref_transaction **transaction,
> bool is_atomic, const char *remote_name,
> struct strbuf *err)
> {
> int retcode;
>
> 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;
> }
>
> This feels significantly easier to read to me.
>
> Patrick
I do agree here, and since the code is small, I think it is worthwhile
making this change. Will add in. Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed
2025-11-08 21:34 ` [PATCH v3 0/2] " Karthik Nayak
2025-11-08 21:34 ` [PATCH v3 1/2] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-08 21:34 ` Karthik Nayak
2025-11-10 7:34 ` Patrick Steinhardt
1 sibling, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-08 21:34 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, sunshine, gitster, David Bohman
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 even when we have an error code.
This ensures other references are applied. Add tests to check for this
regression. While here, add a missing cleanup from previous test.
Reported-by: David Bohman <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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 49e195199e..337ca2b0af 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1963,6 +1963,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 > 0 && !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..e62190d5d7 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" '
+ git init source &&
+ git -C source commit --allow-empty --message common &&
+ git clone file://"$(pwd)"/source target &&
+ (
+ cd source &&
+ git commit --allow-empty --message history &&
+ git tag history &&
+ git commit --allow-empty --message fetch-me &&
+ git tag 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
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed
2025-11-08 21:34 ` [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-10 7:34 ` Patrick Steinhardt
2025-11-10 13:23 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-11-10 7:34 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, sunshine, gitster, David Bohman
On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 49e195199e..337ca2b0af 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1963,6 +1963,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 > 0 && !atomic_fetch && transaction)
> + commit_ref_transaction(&transaction, false,
> + transport->remote->name, &err);
I think this needs some explanation why this condition is safe. There's
quite a bunch of function calls and conditions that assign to it:
- `truncate_fetch_head()` only ever assigns negative. This will be
ignored as expected.
- `open_fetch_head()` behaves likewise.
- `prune_refs()` returns negative, but we then turn the return code
into `1`. So we'd end up calling `commit_ref_transaction()` in this
case, but we didn't in the previous iteration of this patch series.
Was this intentional?
- `fetch_and_consume_refs()` is one of the intended cases, and it sets
up a positive retcode indeed.
- `backfill_tags()` behaves likewise, and was intended.
So this looks good to me, with the only questionable one being
`prune_refs()`.
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed
2025-11-10 7:34 ` Patrick Steinhardt
@ 2025-11-10 13:23 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-10 13:23 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, sunshine, gitster, David Bohman
[-- Attachment #1: Type: text/plain, Size: 2179 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 49e195199e..337ca2b0af 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1963,6 +1963,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 > 0 && !atomic_fetch && transaction)
>> + commit_ref_transaction(&transaction, false,
>> + transport->remote->name, &err);
>
> I think this needs some explanation why this condition is safe. There's
> quite a bunch of function calls and conditions that assign to it:
>
> - `truncate_fetch_head()` only ever assigns negative. This will be
> ignored as expected.
>
> - `open_fetch_head()` behaves likewise.
>
Also the transaction isn't even defined until this stage.
> - `prune_refs()` returns negative, but we then turn the return code
> into `1`. So we'd end up calling `commit_ref_transaction()` in this
> case, but we didn't in the previous iteration of this patch series.
> Was this intentional?
Its basically the same, before batched updates, we would return the
return code of `refs_delete_refs()` from within `prune_refs()`.
The fn `refs_delete_refs()` creates a transaction within to delete all
refs, this is done because we delete refs without old OID and hence they
wouldn't ever fail.
So now, when we pass our transaction to `prune_refs()`, it is also safe
to commit it.
One scenario I didn't think of earlier was that we would now enable
partial pruning with this change. But I would argue that this is
desirable since that is how we deal with other ref updates during fetch.
>
> - `fetch_and_consume_refs()` is one of the intended cases, and it sets
> up a positive retcode indeed.
>
> - `backfill_tags()` behaves likewise, and was intended.
>
> So this looks good to me, with the only questionable one being
> `prune_refs()`.
>
> Patrick
Either ways, I would think that we should elaborate a little here.
Thanks,
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v4 0/2] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (3 preceding siblings ...)
2025-11-08 21:34 ` [PATCH v3 0/2] " Karthik Nayak
@ 2025-11-11 13:27 ` Karthik Nayak
2025-11-11 13:27 ` [PATCH v4 1/2] fetch: extract out reference committing logic Karthik Nayak
2025-11-11 13:27 ` [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-13 13:38 ` [PATCH v5 0/2] " Karthik Nayak
` (3 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, David Bohman, Patrick Steinhardt, jltobler,
gitster
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.
[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
Signed-off-by: Karthik Nayak <karthik.188@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 | 67 ++++++++++++++++++++++++++++++++++----------------------
t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+), 26 deletions(-)
Karthik Nayak (2):
fetch: extract out reference committing logic
fetch: fix non-conflicting tags not being committed
Range-diff versus v3:
1: ee20b46cc2 ! 1: 49fa9a85ef fetch: extract out reference committing logic
@@ Commit message
rejection handling logic into a separate function called
`commit_ref_transaction()`.
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/fetch.c ##
@@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
+ struct strbuf *err)
+{
+ int retcode = ref_transaction_commit(*transaction, err);
-+ if (retcode) {
-+ /*
-+ * Explicitly handle transaction cleanup to avoid
-+ * aborting an already closed transaction.
-+ */
-+ ref_transaction_free(*transaction);
-+ *transaction = NULL;
-+ }
++ if (retcode)
++ goto out;
+
-+ if (*transaction && !is_atomic) {
++ if (!is_atomic) {
+ struct ref_rejection_data data = {
+ .conflict_msg_shown = 0,
+ .remote_name = remote_name,
@@ builtin/fetch.c: static void ref_transaction_rejection_handler(const char *refna
+ ref_transaction_for_each_rejected_update(*transaction,
+ ref_transaction_rejection_handler,
+ &data);
-+
-+ ref_transaction_free(*transaction);
-+ *transaction = NULL;
+ }
+
++out:
++ ref_transaction_free(*transaction);
++ *transaction = NULL;
+ return retcode;
+}
+
2: 543b67c97c ! 2: 12c71b602d fetch: fix non-conflicting tags not being committed
@@ Commit message
extends to backfilling tags which is done when fetching specific
refspecs which contains tags in their history.
- Fix this by committing the transaction even when we have an error code.
- This ensures other references are applied. Add tests to check for this
- regression. While here, add a missing cleanup from previous test.
+ 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()` and `open_fetch_head()` both set `retcode`
+ before the transaction is created, so no commit is attempted.
+
+ - `prune_refs()` sets `retcode` after creating the transaction, so
+ the commit will now proceed. Before batched updates, `prune_refs()`
+ created its own transaction internally with all-or-nothing
+ semantics. This was done since all deletions were made without an
+ old OID, which meant they were assumed to never fail. This change
+ allows partial deletions to succeed, consistent with how other
+ reference updates behave during fetch.
+
+ - `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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
@@ builtin/fetch.c: static int do_fetch(struct transport *transport,
+ * When using batched updates, we want to commit the non-rejected
+ * updates and also handle the rejections.
+ */
-+ if (retcode > 0 && !atomic_fetch && transaction)
++ if (retcode && !atomic_fetch && transaction)
+ commit_ref_transaction(&transaction, false,
+ transport->remote->name, &err);
+
base-commit: a99f379adf116d53eb11957af5bab5214915f91d
change-id: 20251103-fix-tags-not-fetching-0f1621a474d4
Thanks
- Karthik
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v4 1/2] fetch: extract out reference committing logic
2025-11-11 13:27 ` [PATCH v4 0/2] " Karthik Nayak
@ 2025-11-11 13:27 ` Karthik Nayak
2025-11-11 13:27 ` [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
1 sibling, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, David Bohman, Patrick Steinhardt, jltobler,
gitster
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 <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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);
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed
2025-11-11 13:27 ` [PATCH v4 0/2] " Karthik Nayak
2025-11-11 13:27 ` [PATCH v4 1/2] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-11 13:27 ` Karthik Nayak
2025-11-12 6:16 ` Patrick Steinhardt
1 sibling, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-11 13:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, David Bohman, Patrick Steinhardt, jltobler,
gitster
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()` and `open_fetch_head()` both set `retcode`
before the transaction is created, so no commit is attempted.
- `prune_refs()` sets `retcode` after creating the transaction, so
the commit will now proceed. Before batched updates, `prune_refs()`
created its own transaction internally with all-or-nothing
semantics. This was done since all deletions were made without an
old OID, which meant they were assumed to never fail. This change
allows partial deletions to succeed, consistent with how other
reference updates behave during fetch.
- `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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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..e62190d5d7 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" '
+ git init source &&
+ git -C source commit --allow-empty --message common &&
+ git clone file://"$(pwd)"/source target &&
+ (
+ cd source &&
+ git commit --allow-empty --message history &&
+ git tag history &&
+ git commit --allow-empty --message fetch-me &&
+ git tag 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
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed
2025-11-11 13:27 ` [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-12 6:16 ` Patrick Steinhardt
2025-11-12 8:52 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-11-12 6:16 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, David Bohman, jltobler, gitster
On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:
> The cleanup section is reached with `retcode` set in several scenarios:
>
> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
> before the transaction is created, so no commit is attempted.
>
> - `prune_refs()` sets `retcode` after creating the transaction, so
> the commit will now proceed. Before batched updates, `prune_refs()`
> created its own transaction internally with all-or-nothing
> semantics. This was done since all deletions were made without an
> old OID, which meant they were assumed to never fail. This change
> allows partial deletions to succeed, consistent with how other
> reference updates behave during fetch.
Okay, so we do have a change in behaviour for `prune_refs()`. I guess
the reasoning is sound, but I was wondering why we don't have a test for
this.
I guess the reason is that, as you said, it should in theory always
succeed. But what if with the "files" backend one of the refs that we're
about to prune was locked? Would that be a case where we continue with
pruning the remaining refs now?
Thanks!
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed
2025-11-12 6:16 ` Patrick Steinhardt
@ 2025-11-12 8:52 ` Karthik Nayak
2025-11-12 16:34 ` Junio C Hamano
0 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-12 8:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, David Bohman, jltobler, gitster
[-- Attachment #1: Type: text/plain, Size: 1666 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:
>> The cleanup section is reached with `retcode` set in several scenarios:
>>
>> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
>> before the transaction is created, so no commit is attempted.
>>
>> - `prune_refs()` sets `retcode` after creating the transaction, so
>> the commit will now proceed. Before batched updates, `prune_refs()`
>> created its own transaction internally with all-or-nothing
>> semantics. This was done since all deletions were made without an
>> old OID, which meant they were assumed to never fail. This change
>> allows partial deletions to succeed, consistent with how other
>> reference updates behave during fetch.
>
> Okay, so we do have a change in behaviour for `prune_refs()`. I guess
> the reasoning is sound, but I was wondering why we don't have a test for
> this.
>
> I guess the reason is that, as you said, it should in theory always
> succeed. But what if with the "files" backend one of the refs that we're
> about to prune was locked? Would that be a case where we continue with
> pruning the remaining refs now?
>
I was thinking of concurrent writes to lock the reference, and didn't
think of a nice way to do this. Your solution works and is indeed better.
I started writing the test and realized that the pruning happens before
we create the batched updates transaction. So I was _wrong_ and there is
no change for `prune_refs()` either, as the transaction is never defined
at this stage. Will amend and send in a new version.
> Thanks!
>
> Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v4 2/2] fetch: fix non-conflicting tags not being committed
2025-11-12 8:52 ` Karthik Nayak
@ 2025-11-12 16:34 ` Junio C Hamano
0 siblings, 0 replies; 54+ messages in thread
From: Junio C Hamano @ 2025-11-12 16:34 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Patrick Steinhardt, git, David Bohman, jltobler
Karthik Nayak <karthik.188@gmail.com> writes:
> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:
>>> The cleanup section is reached with `retcode` set in several scenarios:
>>>
>>> - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
>>> before the transaction is created, so no commit is attempted.
>>>
>>> - `prune_refs()` sets `retcode` after creating the transaction, so
>>> the commit will now proceed. Before batched updates, `prune_refs()`
>>> created its own transaction internally with all-or-nothing
>>> semantics. This was done since all deletions were made without an
>>> old OID, which meant they were assumed to never fail. This change
>>> allows partial deletions to succeed, consistent with how other
>>> reference updates behave during fetch.
>>
>> Okay, so we do have a change in behaviour for `prune_refs()`. I guess
>> the reasoning is sound, but I was wondering why we don't have a test for
>> this.
>>
>> I guess the reason is that, as you said, it should in theory always
>> succeed. But what if with the "files" backend one of the refs that we're
>> about to prune was locked? Would that be a case where we continue with
>> pruning the remaining refs now?
>>
>
> I was thinking of concurrent writes to lock the reference, and didn't
> think of a nice way to do this. Your solution works and is indeed better.
>
> I started writing the test and realized that the pruning happens before
> we create the batched updates transaction. So I was _wrong_ and there is
> no change for `prune_refs()` either, as the transaction is never defined
> at this stage. Will amend and send in a new version.
Thanks, both. I too was wondering what was going on in that part of
the flow.
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v5 0/2] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (4 preceding siblings ...)
2025-11-11 13:27 ` [PATCH v4 0/2] " Karthik Nayak
@ 2025-11-13 13:38 ` Karthik Nayak
2025-11-13 13:38 ` [PATCH v5 1/2] fetch: extract out reference committing logic Karthik Nayak
2025-11-13 13:38 ` [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 0/3] " Karthik Nayak
` (2 subsequent siblings)
8 siblings, 2 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-13 13:38 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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.
[1]: id:CAB9xhmPcHnB2+i6WeA3doAinv7RAeGs04+n0fHLGToJq=UKUNw@mail.gmail.com
Signed-off-by: Karthik Nayak <karthik.188@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 | 67 ++++++++++++++++++++++++++++++++++----------------------
t/t5510-fetch.sh | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 103 insertions(+), 26 deletions(-)
Karthik Nayak (2):
fetch: extract out reference committing logic
fetch: fix non-conflicting tags not being committed
Range-diff versus v4:
1: ba560e030b = 1: cc187b053f fetch: extract out reference committing logic
2: 0403971a5b ! 2: 27497a1b9d fetch: fix non-conflicting tags not being committed
@@ Commit message
The cleanup section is reached with `retcode` set in several scenarios:
- - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
- before the transaction is created, so no commit is attempted.
-
- - `prune_refs()` sets `retcode` after creating the transaction, so
- the commit will now proceed. Before batched updates, `prune_refs()`
- created its own transaction internally with all-or-nothing
- semantics. This was done since all deletions were made without an
- old OID, which meant they were assumed to never fail. This change
- allows partial deletions to succeed, consistent with how other
- reference updates behave during fetch.
+ - `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
@@ t/t5510-fetch.sh: test_expect_success REFFILES 'D/F conflict on case sensitive f
+'
+
+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 &&
-+ git commit --allow-empty --message history &&
-+ git tag history &&
-+ git commit --allow-empty --message fetch-me &&
-+ git tag fetch-me
++ test_commit history &&
++ test_commit fetch-me
+ ) &&
+
+ # The "history" tag is backfilled eventhough we requested
base-commit: a99f379adf116d53eb11957af5bab5214915f91d
change-id: 20251103-fix-tags-not-fetching-0f1621a474d4
Thanks
- Karthik
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v5 1/2] fetch: extract out reference committing logic
2025-11-13 13:38 ` [PATCH v5 0/2] " Karthik Nayak
@ 2025-11-13 13:38 ` Karthik Nayak
2025-11-13 13:38 ` [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
1 sibling, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-13 13:38 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster
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 <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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);
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed
2025-11-13 13:38 ` [PATCH v5 0/2] " Karthik Nayak
2025-11-13 13:38 ` [PATCH v5 1/2] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-13 13:38 ` Karthik Nayak
2025-11-13 21:23 ` Junio C Hamano
1 sibling, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-13 13:38 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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
--
2.51.0
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed
2025-11-13 13:38 ` [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-13 21:23 ` Junio C Hamano
2025-11-15 22:16 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Junio C Hamano @ 2025-11-13 21:23 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, ps, David Bohman
Karthik Nayak <karthik.188@gmail.com> writes:
> 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.
This may be sufficient to clean out the hanging transaction that the
original code forgot to commit, but in scenarios where this change
makes a difference, i.e., where the code does "goto cleanup" before
it calls commit_ref_transaction() in the main flow of the code,
there are things that are not performed that we may still want to
perform. Namely, we do not
- call commit_fetch_head()
- run set_upstream processing
- honor do_set_head flag that was left for remote that does not
have followremotehead=never
but don't we want to do some of them at least?
If it turns out that we want to do all of them, I also wonder if the
resulting code would become easier to follow if we lose the call to
commit_ref_transaction() in the main code flow, do the above three
points before committing the ref transaction, and then after the
cleanup label, make a call to commit_ref_transaction() if we have an
open transaction and we are not atomic (regardless of the value of
retcode at that point). That call may yield another retcode that
the existing error reporting at the end of this function may have to
react to.
> 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);
IOW, something like this on top of this patch (not even compile tested).
builtin/fetch.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git c/builtin/fetch.c w/builtin/fetch.c
index b19fa8e966..d3eb65dac6 100644
--- c/builtin/fetch.c
+++ w/builtin/fetch.c
@@ -1888,11 +1888,6 @@ static int do_fetch(struct transport *transport,
if (retcode)
goto cleanup;
- retcode = commit_ref_transaction(&transaction, atomic_fetch,
- transport->remote->name, &err);
- if (retcode)
- goto cleanup;
-
commit_fetch_head(&fetch_head);
if (set_upstream) {
@@ -1957,14 +1952,9 @@ 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 (!atomic_fetch && transaction)
+ retcode = commit_ref_transaction(&transaction, false,
+ transport->remote->name, &err);
if (retcode) {
if (err.len) {
error("%s", err.buf);
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed
2025-11-13 21:23 ` Junio C Hamano
@ 2025-11-15 22:16 ` Karthik Nayak
2025-11-17 0:02 ` Junio C Hamano
0 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-15 22:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jltobler, ps, David Bohman
[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>
> This may be sufficient to clean out the hanging transaction that the
> original code forgot to commit, but in scenarios where this change
> makes a difference, i.e., where the code does "goto cleanup" before
> it calls commit_ref_transaction() in the main flow of the code,
> there are things that are not performed that we may still want to
> perform. Namely, we do not
>
> - call commit_fetch_head()
>
> - run set_upstream processing
>
> - honor do_set_head flag that was left for remote that does not
> have followremotehead=never
>
> but don't we want to do some of them at least?
>
Thanks for bringing this up. I would think we should do all of these,
but not if the '--atomic' flag is used. If the '--atomic' flag is used,
we shouldn't do anything else and simply skip to the end.
> If it turns out that we want to do all of them, I also wonder if the
> resulting code would become easier to follow if we lose the call to
> commit_ref_transaction() in the main code flow, do the above three
> points before committing the ref transaction, and then after the
> cleanup label, make a call to commit_ref_transaction() if we have an
> open transaction and we are not atomic (regardless of the value of
> retcode at that point). That call may yield another retcode that
> the existing error reporting at the end of this function may have to
> react to.
>
The issue is with '--atomic' again. I could think of this small change
over this topic, which passes the fetch tests.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b19fa8e966..df11f59f56 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1890,7 +1890,7 @@ static int do_fetch(struct transport *transport,
retcode = commit_ref_transaction(&transaction, atomic_fetch,
transport->remote->name, &err);
- if (retcode)
+ if (retcode && atomic_fetch)
goto cleanup;
commit_fetch_head(&fetch_head);
I do wonder if we can cleanup this code, it is a bit messy right now.
That said, we could either append this change as a new commit with some
additional tests and re-roll the series or send it as a separate commit
based on this series. I'd prefer the latter so that we have the fix for
fetching tags merged sooner, but happy to do either.
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed
2025-11-15 22:16 ` Karthik Nayak
@ 2025-11-17 0:02 ` Junio C Hamano
2025-11-17 15:38 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Junio C Hamano @ 2025-11-17 0:02 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, ps, David Bohman
Karthik Nayak <karthik.188@gmail.com> writes:
>> perform. Namely, we do not
>>
>> - call commit_fetch_head()
>>
>> - run set_upstream processing
>>
>> - honor do_set_head flag that was left for remote that does not
>> have followremotehead=never
>>
>> but don't we want to do some of them at least?
>>
>
> Thanks for bringing this up. I would think we should do all of these,
> but not if the '--atomic' flag is used. If the '--atomic' flag is used,
> we shouldn't do anything else and simply skip to the end.
True.
So when not "--atomic", the code with these two patches will still
misbehave, but it is not a regression these two patches causes.
Failing to do any of the above three when "--atomic" is not in
effect is a part of original regression in the previous cycle caused
by the "batched ref updates". These two patches are trying to
address the regression, but these three points are not covered. Am
I reading the situation correctly?
> That said, we could either append this change as a new commit with some
> additional tests and re-roll the series or send it as a separate commit
> based on this series. I'd prefer the latter so that we have the fix for
> fetching tags merged sooner, but happy to do either.
Either is fine, as this won't make Git 2.52, it seems. It is OK as
it is not a new regression, but it still is a recent one, and would
be nice if we have something concrete to address it soon after 2.52.
Thanks.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v5 2/2] fetch: fix non-conflicting tags not being committed
2025-11-17 0:02 ` Junio C Hamano
@ 2025-11-17 15:38 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-17 15:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jltobler, ps, David Bohman
[-- Attachment #1: Type: text/plain, Size: 1778 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> perform. Namely, we do not
>>>
>>> - call commit_fetch_head()
>>>
>>> - run set_upstream processing
>>>
>>> - honor do_set_head flag that was left for remote that does not
>>> have followremotehead=never
>>>
>>> but don't we want to do some of them at least?
>>>
>>
>> Thanks for bringing this up. I would think we should do all of these,
>> but not if the '--atomic' flag is used. If the '--atomic' flag is used,
>> we shouldn't do anything else and simply skip to the end.
>
> True.
>
> So when not "--atomic", the code with these two patches will still
> misbehave, but it is not a regression these two patches causes.
> Failing to do any of the above three when "--atomic" is not in
> effect is a part of original regression in the previous cycle caused
> by the "batched ref updates". These two patches are trying to
> address the regression, but these three points are not covered. Am
> I reading the situation correctly?
Yes you're.
>
>> That said, we could either append this change as a new commit with some
>> additional tests and re-roll the series or send it as a separate commit
>> based on this series. I'd prefer the latter so that we have the fix for
>> fetching tags merged sooner, but happy to do either.
>
> Either is fine, as this won't make Git 2.52, it seems. It is OK as
> it is not a new regression, but it still is a recent one, and would
> be nice if we have something concrete to address it soon after 2.52.
>
> Thanks.
Ah well, I have something locally already. So will include it in this
series and push a new version with the once I see greens on the CI [1].
Thanks,
Karthik
[1]: https://gitlab.com/gitlab-org/git/-/merge_requests/444
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v6 0/3] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (5 preceding siblings ...)
2025-11-13 13:38 ` [PATCH v5 0/2] " Karthik Nayak
@ 2025-11-18 11:27 ` Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 1/3] fetch: extract out reference committing logic Karthik Nayak
` (2 more replies)
2025-11-19 21:46 ` [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
8 siblings, 3 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-18 11:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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 <karthik.188@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 v5:
1: ab03acf218 = 1: 21f2518724 fetch: extract out reference committing logic
2: 8a9982ee75 = 2: 9ca27b08fa fetch: fix non-conflicting tags not being committed
-: ---------- > 3: 33a7654bfa fetch: fix failed batched updates skipping operations
base-commit: a99f379adf116d53eb11957af5bab5214915f91d
change-id: 20251103-fix-tags-not-fetching-0f1621a474d4
Thanks
- Karthik
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v6 1/3] fetch: extract out reference committing logic
2025-11-18 11:27 ` [PATCH v6 0/3] " Karthik Nayak
@ 2025-11-18 11:27 ` Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
2 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-18 11:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster
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 <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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);
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v6 2/3] fetch: fix non-conflicting tags not being committed
2025-11-18 11:27 ` [PATCH v6 0/3] " Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 1/3] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-18 11:27 ` Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
2 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-18 11:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v6 3/3] fetch: fix failed batched updates skipping operations
2025-11-18 11:27 ` [PATCH v6 0/3] " Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 1/3] fetch: extract out reference committing logic Karthik Nayak
2025-11-18 11:27 ` [PATCH v6 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-18 11:27 ` Karthik Nayak
2025-11-18 18:03 ` Junio C Hamano
2 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-18 11:27 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster
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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/fetch.c | 6 +++-
t/t5510-fetch.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 92 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..f5c87d81fe 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1639,6 +1639,93 @@ 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 &&
+ ! test -f FETCH_HEAD &&
+ git remote add origin ../base &&
+ touch 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_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 &&
+ touch 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 -f refs/remotes/origin/HEAD &&
+ mkdir -p refs/remotes/origin &&
+ touch refs/remotes/origin/branch.lock &&
+ test_must_fail git fetch origin &&
+ test -f refs/remotes/origin/HEAD
+ )
+'
+
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations
2025-11-18 11:27 ` [PATCH v6 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
@ 2025-11-18 18:03 ` Junio C Hamano
2025-11-19 8:59 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Junio C Hamano @ 2025-11-18 18:03 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, ps
Karthik Nayak <karthik.188@gmail.com> writes:
> 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.
Other than the usual "unless you care about timestamps, do not use
'touch' only to create a file" applies, but other than that the
added tests look quite sensible.
About the second new test piece, it is a bit surprising that we
didn't have test for --set-upstream on successful fetch. It does
not need REFFILES prerequisite, does it?
Thanks.
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 4b113d7c27..f5c87d81fe 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1639,6 +1639,93 @@ 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 &&
> + ! test -f FETCH_HEAD &&
> + git remote add origin ../base &&
> + touch 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_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 &&
> + touch 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 -f refs/remotes/origin/HEAD &&
> + mkdir -p refs/remotes/origin &&
> + touch refs/remotes/origin/branch.lock &&
> + test_must_fail git fetch origin &&
> + test -f refs/remotes/origin/HEAD
> + )
> +'
> +
> . "$TEST_DIRECTORY"/lib-httpd.sh
> start_httpd
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v6 3/3] fetch: fix failed batched updates skipping operations
2025-11-18 18:03 ` Junio C Hamano
@ 2025-11-19 8:59 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-19 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jltobler, ps
[-- Attachment #1: Type: text/plain, Size: 1846 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>
> Other than the usual "unless you care about timestamps, do not use
> 'touch' only to create a file" applies, but other than that the
> added tests look quite sensible.
>
Oops, will change that.
> About the second new test piece, it is a bit surprising that we
> didn't have test for --set-upstream on successful fetch. It does
> not need REFFILES prerequisite, does it?
I was surprised too, that we don't have a test covering that flag.
You're right it doesn't. The test for the conflict does, but the happy
path doesn't. Will change.
>
> Thanks.
>
Thanks for the review.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (6 preceding siblings ...)
2025-11-18 11:27 ` [PATCH v6 0/3] " Karthik Nayak
@ 2025-11-19 21:46 ` Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 1/3] fetch: extract out reference committing logic Karthik Nayak
` (2 more replies)
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
8 siblings, 3 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-19 21:46 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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 <karthik.188@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
---
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
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v7 1/3] fetch: extract out reference committing logic
2025-11-19 21:46 ` [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-19 21:46 ` Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
2 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-19 21:46 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster
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 <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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);
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v7 2/3] fetch: fix non-conflicting tags not being committed
2025-11-19 21:46 ` [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 1/3] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-19 21:46 ` Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
2 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-19 21:46 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, David Bohman
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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v7 3/3] fetch: fix failed batched updates skipping operations
2025-11-19 21:46 ` [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 1/3] fetch: extract out reference committing logic Karthik Nayak
2025-11-19 21:46 ` [PATCH v7 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-19 21:46 ` Karthik Nayak
2025-11-19 22:20 ` Eric Sunshine
2 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-19 21:46 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster
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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/fetch.c | 6 +++-
t/t5510-fetch.sh | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 92 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..cd55958bdc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1639,6 +1639,93 @@ 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 &&
+ ! test -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 -f 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 -f 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
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations
2025-11-19 21:46 ` [PATCH v7 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
@ 2025-11-19 22:20 ` Eric Sunshine
2025-11-19 23:08 ` Junio C Hamano
0 siblings, 1 reply; 54+ messages in thread
From: Eric Sunshine @ 2025-11-19 22:20 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, ps, gitster
On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak <karthik.188@gmail.com> wrote:
> 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:
> [...]
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> @@ -1639,6 +1639,93 @@ test_expect_success "backfill tags when providing a refspec" '
> +test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
> + test_when_finished rm -rf base repo &&
> + [...]
> + git init --bare repo &&
> + (
> + cd repo &&
> + ! test -f FETCH_HEAD &&
Is this supposed to be asserting that the file does not exist or that
the path is not a file? If the former, then test_path_is_missing()
would be a better choice.
> + 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 -f FETCH_HEAD
> + )
> +'
> +
> +test_expect_success REFFILES "HEAD is updated even with conflicts" '
> + test_when_finished rm -rf base repo &&
> + [...]
> + git init --bare repo &&
> + (
> + cd repo &&
> + git remote add origin ../base &&
> +
> + ! test -f refs/remotes/origin/HEAD &&
Ditto.
> + mkdir -p refs/remotes/origin &&
> + >refs/remotes/origin/branch.lock &&
> + test_must_fail git fetch origin &&
> + test -f refs/remotes/origin/HEAD
> + )
> +'
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations
2025-11-19 22:20 ` Eric Sunshine
@ 2025-11-19 23:08 ` Junio C Hamano
2025-11-21 11:00 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Junio C Hamano @ 2025-11-19 23:08 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Karthik Nayak, git, jltobler, ps
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>> 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:
>> [...]
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> @@ -1639,6 +1639,93 @@ test_expect_success "backfill tags when providing a refspec" '
>> +test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
>> + test_when_finished rm -rf base repo &&
>> + [...]
>> + git init --bare repo &&
>> + (
>> + cd repo &&
>> + ! test -f FETCH_HEAD &&
>
> Is this supposed to be asserting that the file does not exist or that
> the path is not a file? If the former, then test_path_is_missing()
> would be a better choice.
Thanks for carefully reading. Personally, I think this is not
needed, as we have just created a new repository. It might be
even better to replace it with
rm -f FETCH_HEAD &&
to clarify that we do want to see this _created_ with a failing "git
fetch", not merely left behind.
>
>> + 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 -f FETCH_HEAD
More importantly, should we inspect the contents of this file to see
what gets recorded. If we are fetching foo and bar, and we made foo
fail, do we expect foo and bar in the file? Or do we expect only bar
in the file? Something else?
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v7 3/3] fetch: fix failed batched updates skipping operations
2025-11-19 23:08 ` Junio C Hamano
@ 2025-11-21 11:00 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-21 11:00 UTC (permalink / raw)
To: Junio C Hamano, Eric Sunshine; +Cc: git, jltobler, ps
[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]
Junio C Hamano <gitster@pobox.com> writes:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Wed, Nov 19, 2025 at 4:47 PM Karthik Nayak <karthik.188@gmail.com> wrote:
>>> 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:
>>> [...]
>>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>>> ---
>>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>>> @@ -1639,6 +1639,93 @@ test_expect_success "backfill tags when providing a refspec" '
>>> +test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
>>> + test_when_finished rm -rf base repo &&
>>> + [...]
>>> + git init --bare repo &&
>>> + (
>>> + cd repo &&
>>> + ! test -f FETCH_HEAD &&
>>
>> Is this supposed to be asserting that the file does not exist or that
>> the path is not a file? If the former, then test_path_is_missing()
>> would be a better choice.
>
> Thanks for carefully reading. Personally, I think this is not
> needed, as we have just created a new repository. It might be
> even better to replace it with
>
> rm -f FETCH_HEAD &&
>
> to clarify that we do want to see this _created_ with a failing "git
> fetch", not merely left behind.
>
That's fair.
>>
>>> + 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 -f FETCH_HEAD
>
> More importantly, should we inspect the contents of this file to see
> what gets recorded. If we are fetching foo and bar, and we made foo
> fail, do we expect foo and bar in the file? Or do we expect only bar
> in the file? Something else?
I would say we should. Let me send in a version with these changes.
Thanks,
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed
2025-11-03 13:49 [PATCH] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (7 preceding siblings ...)
2025-11-19 21:46 ` [PATCH v7 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-21 11:13 ` Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 1/3] fetch: extract out reference committing logic Karthik Nayak
` (3 more replies)
8 siblings, 4 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-21 11:13 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, sunshine, David Bohman
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 <karthik.188@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
---
builtin/fetch.c | 71 ++++++++++++++++----------
t/t5510-fetch.sh | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 195 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 v7:
1: f5d1abef41 = 1: 6553f56915 fetch: extract out reference committing logic
2: fa2466f2bf = 2: d525415fbb fetch: fix non-conflicting tags not being committed
3: df59310296 ! 3: 30ddb99550 fetch: fix failed batched updates skipping operations
@@ t/t5510-fetch.sh: test_expect_success "backfill tags when providing a refspec" '
+ git init --bare repo &&
+ (
+ cd repo &&
-+ ! test -f FETCH_HEAD &&
++ 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 -f FETCH_HEAD
++ test_grep "branch ${SQ}branch${SQ} of ../base" FETCH_HEAD &&
++ test_grep "branch ${SQ}foo${SQ} of ../base" FETCH_HEAD
+ )
+'
+
@@ t/t5510-fetch.sh: test_expect_success "backfill tags when providing a refspec" '
+ cd repo &&
+ git remote add origin ../base &&
+
-+ ! test -f refs/remotes/origin/HEAD &&
++ test_path_is_missing refs/remotes/origin/HEAD &&
+ mkdir -p refs/remotes/origin &&
+ >refs/remotes/origin/branch.lock &&
+ test_must_fail git fetch origin &&
base-commit: a99f379adf116d53eb11957af5bab5214915f91d
change-id: 20251103-fix-tags-not-fetching-0f1621a474d4
Thanks
- Karthik
^ permalink raw reply [flat|nested] 54+ messages in thread* [PATCH v8 1/3] fetch: extract out reference committing logic
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-21 11:13 ` Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (2 subsequent siblings)
3 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-11-21 11:13 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, sunshine
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 <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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);
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 1/3] fetch: extract out reference committing logic Karthik Nayak
@ 2025-11-21 11:13 ` Karthik Nayak
2025-12-01 12:58 ` Patrick Steinhardt
2025-11-21 11:13 ` [PATCH v8 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
2025-11-21 19:58 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Junio C Hamano
3 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-21 11:13 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, sunshine, David Bohman
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 <debohman@gmail.com>
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed
2025-11-21 11:13 ` [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-12-01 12:58 ` Patrick Steinhardt
2025-12-02 22:26 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-12-01 12:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, gitster, sunshine, David Bohman
On Fri, Nov 21, 2025 at 12:13:46PM +0100, Karthik Nayak wrote:
> 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
> @@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
[snip]
> +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
Tiny nit, not worth a reroll: s/eventhough/even though/. Other than that
this patch looks good to me.
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed
2025-12-01 12:58 ` Patrick Steinhardt
@ 2025-12-02 22:26 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-12-02 22:26 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, gitster, sunshine, David Bohman
[-- Attachment #1: Type: text/plain, Size: 930 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> On Fri, Nov 21, 2025 at 12:13:46PM +0100, Karthik Nayak wrote:
>> 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
>> @@ -1577,6 +1578,67 @@ test_expect_success REFFILES 'D/F conflict on case sensitive filesystem with loc
> [snip]
>> +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
>
> Tiny nit, not worth a reroll: s/eventhough/even though/. Other than that
> this patch looks good to me.
>
> Patrick
Will add it in. Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v8 3/3] fetch: fix failed batched updates skipping operations
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 1/3] fetch: extract out reference committing logic Karthik Nayak
2025-11-21 11:13 ` [PATCH v8 2/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
@ 2025-11-21 11:13 ` Karthik Nayak
2025-12-01 12:58 ` Patrick Steinhardt
2025-11-21 19:58 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Junio C Hamano
3 siblings, 1 reply; 54+ messages in thread
From: Karthik Nayak @ 2025-11-21 11:13 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak, jltobler, ps, gitster, sunshine
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 <gitster@pobox.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
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
--
2.51.2
^ permalink raw reply related [flat|nested] 54+ messages in thread* Re: [PATCH v8 3/3] fetch: fix failed batched updates skipping operations
2025-11-21 11:13 ` [PATCH v8 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
@ 2025-12-01 12:58 ` Patrick Steinhardt
2025-12-02 22:35 ` Karthik Nayak
0 siblings, 1 reply; 54+ messages in thread
From: Patrick Steinhardt @ 2025-12-01 12:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, gitster, sunshine
On Fri, Nov 21, 2025 at 12:13:47PM +0100, Karthik Nayak wrote:
> 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.
This raises the question what happens when this function _does_ get
executed again. But we're guarding us:
static void commit_fetch_head(struct fetch_head *fetch_head)
{
if (!fetch_head->fp || !atomic_fetch)
return;
strbuf_write(&fetch_head->buf, fetch_head->fp);
}
And as we only `goto cleanup` in case `retcode && atomic_fetch` we know
that the above function will exit early. So this is a no-op change
indeed.
> 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.
Makes sense.
> 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 &&
Hm. Is this compatible with all supported systems? We typically write
this as:
: >refs/heads/foo.lock
But I have to acknowledge that I only do this because some people that
are more knowledgeable than I am know that we need this.
Other than that I'm happy with the current state of this patch series.
If the above turns out to be a non-issue I think it should be ready for
'next'.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 54+ messages in thread* Re: [PATCH v8 3/3] fetch: fix failed batched updates skipping operations
2025-12-01 12:58 ` Patrick Steinhardt
@ 2025-12-02 22:35 ` Karthik Nayak
0 siblings, 0 replies; 54+ messages in thread
From: Karthik Nayak @ 2025-12-02 22:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, jltobler, gitster, sunshine
[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
[snip]
>> 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 &&
>
> Hm. Is this compatible with all supported systems? We typically write
> this as:
>
> : >refs/heads/foo.lock
>
> But I have to acknowledge that I only do this because some people that
> are more knowledgeable than I am know that we need this.
>
> Other than that I'm happy with the current state of this patch series.
> If the above turns out to be a non-issue I think it should be ready for
> 'next'.
>
I didn't know about this. The CI didn't complain about this too.
A quick search through our repo shows
$ rg --stats '^\s*>[\w/.-]+' t/
...
969 matches
969 matched lines
285 files contained matches
...
$ rg --stats '^\s*:\s*>[\w/.-]+' t/
...
188 matches
188 matched lines
58 files contained matches
...
So seems like we use both, meaning, this should be okay?
> Thanks!
>
> Patrick
Thanks for the review.
Since the other comment was a nit. I will hold off on sending a new version :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed
2025-11-21 11:13 ` [PATCH v8 0/3] fetch: fix non-conflicting tags not being committed Karthik Nayak
` (2 preceding siblings ...)
2025-11-21 11:13 ` [PATCH v8 3/3] fetch: fix failed batched updates skipping operations Karthik Nayak
@ 2025-11-21 19:58 ` Junio C Hamano
3 siblings, 0 replies; 54+ messages in thread
From: Junio C Hamano @ 2025-11-21 19:58 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jltobler, ps, sunshine, David Bohman
Karthik Nayak <karthik.188@gmail.com> writes:
> builtin/fetch.c | 71 ++++++++++++++++----------
> t/t5510-fetch.sh | 150 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 195 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
Looking good. Will replace.
Thanks.
^ permalink raw reply [flat|nested] 54+ messages in thread