[PATCH] fetch: commit references fetched before backfilling tags

[PATCH] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Wherein batched updates were used to fetch both
references and backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. This means in situations such as shallow
fetching the negotiation during backfilling tags, the client doesn't
have any references to report in the 'have' section. Since backfilling
doesn't use a depth limit, this can cause the server to send all the
objects present in the repository.

Fix this by committing the previous batched update and initiating a new
one for backfilling tags. Also add a test which captures this regression.

While this does make it a little slower than master, due to creation of
two transactions, It is still faster than not using batched updates:

Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
  Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
  Range (min … max):    1.427 s …  1.558 s    10 runs

Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
  Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
  Range (min … max):    81.4 ms …  88.6 ms    29 runs

Summary
  fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
   17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799
---
 builtin/fetch.c  | 21 +++++++++++++++++++++
 t/t5510-fetch.sh | 17 +++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..68b04d0f8a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
+
+		/*
+		 * Backfilling tags has no depth limit. If we don't commit
+		 * the fetched references, the backfill will report no refs
+		 * in the 'have' section of the negotiation. This can cause
+		 * the server to send all objects.
+		 */
+		if (tags_ref_map && !atomic_fetch) {
+			retcode |= commit_ref_transaction(&transaction, false,
+							  transport->remote->name,
+							  &rejected_refs, &err);
+
+			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+								  REF_TRANSACTION_ALLOW_FAILURE, &err);
+			if (!transaction) {
+				free_refs(tags_ref_map);
+				retcode = -1;
+				goto cleanup;
+			}
+		}
+
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..81865c1ecc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,23 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'shallow fetch does not fetch objects again for tags' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --depth 5 origin &&
+
+	test $(grep -c "fetch> command=fetch" trace) -gt 2 &&
+	test $(grep -c "fetch> have $HEAD_OID" trace) -eq 2
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

Re: [PATCH] fetch: commit references fetched before backfilling tags

[Patrick Steinhardt]

On Fri, Oct 09, 2026 at 12:10:37AM +0200, Karthik Nayak wrote:
> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
> fetch code was modified to use batched updates to provide a good
> performance improvement. Wherein batched updates were used to fetch both
> references and backfill tags.
> 
> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. This means in situations such as shallow
> fetching the negotiation during backfilling tags, the client doesn't
> have any references to report in the 'have' section. Since backfilling
> doesn't use a depth limit, this can cause the server to send all the
> objects present in the repository.

So in my own words: the server sends the reference, we queue them in a
transaction, but don't commit it yet. We then try to backfill tags, and
because we don't have the refs committed yet the backfill will think we
don't have any of the relevant commits that those tags point to.
Consequently, the packfile negotiation will result in way more objects
being fetched than necessary.

This makes me wonder why we even do a proper fetch. In theory, we could
basically just ask the server for the individual tagged objects without
performing any negotiation, right?

Or... well, would that work with nested annotated tags? No idea.

> Fix this by committing the previous batched update and initiating a new
> one for backfilling tags. Also add a test which captures this regression.
> 
> While this does make it a little slower than master, due to creation of
> two transactions, It is still faster than not using batched updates:
> 
> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
>   Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
>   Range (min … max):    1.427 s …  1.558 s    10 runs
> 
> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
>   Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
>   Range (min … max):    81.4 ms …  88.6 ms    29 runs
> 
> Summary
>   fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
>    17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)

I was expecting to also see HEAD~ here to back up your claim that this
is a bit slower than master.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2decc6cfd..68b04d0f8a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
> +
> +		/*
> +		 * Backfilling tags has no depth limit. If we don't commit
> +		 * the fetched references, the backfill will report no refs
> +		 * in the 'have' section of the negotiation. This can cause
> +		 * the server to send all objects.
> +		 */
> +		if (tags_ref_map && !atomic_fetch) {
> +			retcode |= commit_ref_transaction(&transaction, false,
> +							  transport->remote->name,
> +							  &rejected_refs, &err);
> +
> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +			if (!transaction) {
> +				free_refs(tags_ref_map);
> +				retcode = -1;
> +				goto cleanup;
> +			}
> +		}

Okay, makes sense. `commit_ref_transaction()` knows to already handle
the failures for us and print them. And `rejected_refs` is basically
being treated additive, so if both transactions have some failures then
the map will contain the combined set of rejected refs.

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 300bd5396d..81865c1ecc 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1942,6 +1942,23 @@ test_expect_success "backfill tags when providing a refspec" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'shallow fetch does not fetch objects again for tags' '
> +	test_when_finished rm -rf source target trace &&
> +
> +	git init source &&
> +	test_commit_bulk -C source 10 &&
> +	git -C source tag -a tag -m tag HEAD~2 &&
> +	HEAD_OID=$(git -C source rev-parse HEAD) &&
> +
> +	git init target &&
> +	git -C target remote add origin ../source &&
> +	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
> +		git -C target fetch --depth 5 origin &&
> +
> +	test $(grep -c "fetch> command=fetch" trace) -gt 2 &&
> +	test $(grep -c "fetch> have $HEAD_OID" trace) -eq 2
> +'

You don't really verify that we don't re-fetch objects, you only verify
that we provide "have" lines to the remote side. Which is ultimately the
same, but in a bit more of a roundabout way.

Thanks!

Patrick

Re: [PATCH] fetch: commit references fetched before backfilling tags

[Justin Tobler]

On 26/10/09 01:26PM, Patrick Steinhardt wrote:
> On Fri, Oct 09, 2026 at 12:10:37AM +0200, Karthik Nayak wrote:
> > In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
> > fetch code was modified to use batched updates to provide a good
> > performance improvement. Wherein batched updates were used to fetch both
> > references and backfill tags.
> > 
> > When using batched updates, the references aren't yet committed to disk
> > when we start backfilling tags. This means in situations such as shallow
> > fetching the negotiation during backfilling tags, the client doesn't
> > have any references to report in the 'have' section. Since backfilling
> > doesn't use a depth limit, this can cause the server to send all the
> > objects present in the repository.
> 
> So in my own words: the server sends the reference, we queue them in a
> transaction, but don't commit it yet. We then try to backfill tags, and
> because we don't have the refs committed yet the backfill will think we
> don't have any of the relevant commits that those tags point to.
> Consequently, the packfile negotiation will result in way more objects
> being fetched than necessary.
> 
> This makes me wonder why we even do a proper fetch. In theory, we could
> basically just ask the server for the individual tagged objects without
> performing any negotiation, right?
> 
> Or... well, would that work with nested annotated tags? No idea.

IIUC, when we backfill tags, we only fetch tags that reference objects
that we have locally. The server advertises the tag reference OID and
its recursively peeled non-tag OID so the client can figure this out:

  efe1aaafb77990c4f023cec81b198e0af55bbfb5	refs/tags/foo
  c8dd1e3bb1152844983558802a52c9e4c17652b4	refs/tags/foo^{}

So because there could be nested annontated tags, I think we would need
to fetch to get the intermediate objects.

-Justin

Re: [PATCH] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2355 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 26/10/09 01:26PM, Patrick Steinhardt wrote:
>> On Fri, Oct 09, 2026 at 12:10:37AM +0200, Karthik Nayak wrote:
>> > In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
>> > fetch code was modified to use batched updates to provide a good
>> > performance improvement. Wherein batched updates were used to fetch both
>> > references and backfill tags.
>> >
>> > When using batched updates, the references aren't yet committed to disk
>> > when we start backfilling tags. This means in situations such as shallow
>> > fetching the negotiation during backfilling tags, the client doesn't
>> > have any references to report in the 'have' section. Since backfilling
>> > doesn't use a depth limit, this can cause the server to send all the
>> > objects present in the repository.
>>
>> So in my own words: the server sends the reference, we queue them in a
>> transaction, but don't commit it yet. We then try to backfill tags, and
>> because we don't have the refs committed yet the backfill will think we
>> don't have any of the relevant commits that those tags point to.
>> Consequently, the packfile negotiation will result in way more objects
>> being fetched than necessary.
>>
>> This makes me wonder why we even do a proper fetch. In theory, we could
>> basically just ask the server for the individual tagged objects without
>> performing any negotiation, right?
>>
>> Or... well, would that work with nested annotated tags? No idea.
>
> IIUC, when we backfill tags, we only fetch tags that reference objects
> that we have locally. The server advertises the tag reference OID and
> its recursively peeled non-tag OID so the client can figure this out:
>
>   efe1aaafb77990c4f023cec81b198e0af55bbfb5	refs/tags/foo
>   c8dd1e3bb1152844983558802a52c9e4c17652b4	refs/tags/foo^{}
>
> So because there could be nested annontated tags, I think we would need
> to fetch to get the intermediate objects.
>
> -Justin

Yup exactly this. Afaik the protocol also works by transferring the set
of objects that form the closure of between have <> want.

So for:

   foo -> [tag A] -> [tag B] -> [commit C]

We cannot simply request for A without saying we have C. We have C and
we know it, but the haves are obtained from the committed refs, so
until refs are written to disk we never advertise C.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

[snip]

>> Fix this by committing the previous batched update and initiating a new
>> one for backfilling tags. Also add a test which captures this regression.
>>
>> While this does make it a little slower than master, due to creation of
>> two transactions, It is still faster than not using batched updates:
>>
>> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
>>   Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
>>   Range (min … max):    1.427 s …  1.558 s    10 runs
>>
>> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
>>   Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
>>   Range (min … max):    81.4 ms …  88.6 ms    29 runs
>>
>> Summary
>>   fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
>>    17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
>
> I was expecting to also see HEAD~ here to back up your claim that this
> is a bit slower than master.

I had it and removed it thinking it was too much information. Will add
it back in.

[snip]

>> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
>> index 300bd5396d..81865c1ecc 100755
>> --- a/t/t5510-fetch.sh
>> +++ b/t/t5510-fetch.sh
>> @@ -1942,6 +1942,23 @@ test_expect_success "backfill tags when providing a refspec" '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'shallow fetch does not fetch objects again for tags' '
>> +	test_when_finished rm -rf source target trace &&
>> +
>> +	git init source &&
>> +	test_commit_bulk -C source 10 &&
>> +	git -C source tag -a tag -m tag HEAD~2 &&
>> +	HEAD_OID=$(git -C source rev-parse HEAD) &&
>> +
>> +	git init target &&
>> +	git -C target remote add origin ../source &&
>> +	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
>> +		git -C target fetch --depth 5 origin &&
>> +
>> +	test $(grep -c "fetch> command=fetch" trace) -gt 2 &&
>> +	test $(grep -c "fetch> have $HEAD_OID" trace) -eq 2
>> +'
>
> You don't really verify that we don't re-fetch objects, you only verify
> that we provide "have" lines to the remote side. Which is ultimately the
> same, but in a bit more of a roundabout way.
>
> Thanks!
>
> Patrick

Ah, that's what I wanted and realized the trace only says 'packfile'
without listing whats in it.

Now I realize I could just check the repo with `cat-file`. Let me do
that and clean this up.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Wherein batched updates were used to fetch both
references and backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. This means in situations such as shallow
fetching the negotiation during backfilling tags, the client doesn't
have any references to report in the 'have' section. Since backfilling
doesn't use a depth limit, this can cause the server to send all the
objects present in the repository.

Fix this by committing the previous batched update and initiating a new
one for backfilling tags. Also add a test which captures this regression.

While this does make it a little slower than master, due to two
transactions:

Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)
  Time (mean ± σ):      82.1 ms ±   3.3 ms    [User: 60.2 ms, System: 24.4 ms]
  Range (min … max):    77.4 ms …  89.4 ms    33 runs

Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
  Time (mean ± σ):      82.6 ms ±   2.4 ms    [User: 59.3 ms, System: 25.2 ms]
  Range (min … max):    78.1 ms …  88.7 ms    31 runs

Summary
  fetch: many refs (refformat = reftable, refcount = 10000, revision = master) ran
   1.01 ± 0.05 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)

It is still faster than not using batched updates:

Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
  Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
  Range (min … max):    1.427 s …  1.558 s    10 runs

Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
  Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
  Range (min … max):    81.4 ms …  88.6 ms    29 runs

Summary
  fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
   17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799
---
Changes in v2:
- Add benchmarks between master and this branch to show perf regression.
- Instead of checking the trace file, simply check that we didn't fetch
  older objects in the test.
- Link to v1: https://patch.msgid.link/20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com

CC: Patrick Steinhardt <ps@pks.im>
CC: Justin Tobler <jltobler@gmail.com>
---
 builtin/fetch.c  | 21 +++++++++++++++++++++
 t/t5510-fetch.sh | 16 ++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..68b04d0f8a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
+
+		/*
+		 * Backfilling tags has no depth limit. If we don't commit
+		 * the fetched references, the backfill will report no refs
+		 * in the 'have' section of the negotiation. This can cause
+		 * the server to send all objects.
+		 */
+		if (tags_ref_map && !atomic_fetch) {
+			retcode |= commit_ref_transaction(&transaction, false,
+							  transport->remote->name,
+							  &rejected_refs, &err);
+
+			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+								  REF_TRANSACTION_ALLOW_FAILURE, &err);
+			if (!transaction) {
+				free_refs(tags_ref_map);
+				retcode = -1;
+				goto cleanup;
+			}
+		}
+
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..5ba372fbcb 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,22 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'shallow fetch does not fetch objects again for tags' '
+	test_when_finished rm -rf source target &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	root_oid=$(git -C source rev-parse HEAD~9) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	git -C target fetch --depth 5 origin &&
+
+	# Should not fetch objects beyond the pre-specified depth.
+	test_must_fail git -C target cat-file -e "$root_oid"
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

Re: [PATCH v2] fetch: commit references fetched before backfilling tags

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. This means in situations such as shallow
> fetching the negotiation during backfilling tags, the client doesn't
> have any references to report in the 'have' section. Since backfilling
> doesn't use a depth limit, this can cause the server to send all the
> objects present in the repository.

Ouch.

> Fix this by committing the previous batched update and initiating a new
> one for backfilling tags. Also add a test which captures this regression.

> @@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
> +
> +		/*
> +		 * Backfilling tags has no depth limit. If we don't commit
> +		 * the fetched references, the backfill will report no refs
> +		 * in the 'have' section of the negotiation. This can cause
> +		 * the server to send all objects.
> +		 */

Does this problem only happen when the main fetch updated some ref,
or can backfilling make such a huge negotiation even when the main
fetch transaction is empty?

> +		if (tags_ref_map && !atomic_fetch) {
> +			retcode |= commit_ref_transaction(&transaction, false,
> +							  transport->remote->name,
> +							  &rejected_refs, &err);

This block is entered only when "--atomic" is not in effect, because
we want to update all refs or nothing, and these "all refs"
presumably includes the followed tags, but does it mean those who
want atomic fetch should be prepared to see their fetch degenerate
down to a full clone?

> +
> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +			if (!transaction) {
> +				free_refs(tags_ref_map);
> +				retcode = -1;
> +				goto cleanup;

Is this jumping a bit too far?  If our primary fetch have finished
successfully, and opening the next transaction only for tag backfill
task has failed, there are tons of processing happening we skip with
this new "goto" that we would have normally performed, starting with
computing summary_width used for later reporting, writing out
FETCH_HEAD, and setting upstream for the current branch, all of
which we should be able to do as long as the primary fetch
successfully finishes, no?

> +			}
> +		}

> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 300bd5396d..5ba372fbcb 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -1942,6 +1942,22 @@ test_expect_success "backfill tags when providing a refspec" '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'shallow fetch does not fetch objects again for tags' '
> +	test_when_finished rm -rf source target &&
> +
> +	git init source &&
> +	test_commit_bulk -C source 10 &&
> +	git -C source tag -a tag -m tag HEAD~2 &&
> +	root_oid=$(git -C source rev-parse HEAD~9) &&
> +
> +	git init target &&
> +	git -C target remote add origin ../source &&
> +	git -C target fetch --depth 5 origin &&
> +
> +	# Should not fetch objects beyond the pre-specified depth.
> +	test_must_fail git -C target cat-file -e "$root_oid"
> +'
> +
>  test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
>  	test_when_finished rm -rf base repo &&
>  
>
> ---
> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
> change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0
>
>
> Thanks
> - Karthik

Re: [PATCH v2] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3422 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> When using batched updates, the references aren't yet committed to disk
>> when we start backfilling tags. This means in situations such as shallow
>> fetching the negotiation during backfilling tags, the client doesn't
>> have any references to report in the 'have' section. Since backfilling
>> doesn't use a depth limit, this can cause the server to send all the
>> objects present in the repository.
>
> Ouch.
>

Indeed.

>> Fix this by committing the previous batched update and initiating a new
>> one for backfilling tags. Also add a test which captures this regression.
>
>> @@ -2076,6 +2076,27 @@ static int do_fetch(struct transport *transport,
>>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>>
>>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
>> +
>> +		/*
>> +		 * Backfilling tags has no depth limit. If we don't commit
>> +		 * the fetched references, the backfill will report no refs
>> +		 * in the 'have' section of the negotiation. This can cause
>> +		 * the server to send all objects.
>> +		 */
>
> Does this problem only happen when the main fetch updated some ref,
> or can backfilling make such a huge negotiation even when the main
> fetch transaction is empty?
>

Only when the main fetch updates the on-disk state. If there are no refs
updated in the main transaction, then there is nothing that we could've
used in the negotiation anyway.

I will modify the comment a bit, as the 'will report no refs' is a bit
inaccurate, since it will only not report the refs we didn't yet commit.

>> +		if (tags_ref_map && !atomic_fetch) {
>> +			retcode |= commit_ref_transaction(&transaction, false,
>> +							  transport->remote->name,
>> +							  &rejected_refs, &err);
>
> This block is entered only when "--atomic" is not in effect, because
> we want to update all refs or nothing, and these "all refs"
> presumably includes the followed tags, but does it mean those who
> want atomic fetch should be prepared to see their fetch degenerate
> down to a full clone?
>

Yes, '--atomic' always had this issue. There's not much we can do there
to fix it. I will add it to my commit message though.

>> +
>> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +			if (!transaction) {
>> +				free_refs(tags_ref_map);
>> +				retcode = -1;
>> +				goto cleanup;
>
> Is this jumping a bit too far?  If our primary fetch have finished
> successfully, and opening the next transaction only for tag backfill
> task has failed, there are tons of processing happening we skip with
> this new "goto" that we would have normally performed, starting with
> computing summary_width used for later reporting, writing out
> FETCH_HEAD, and setting upstream for the current branch, all of
> which we should be able to do as long as the primary fetch
> successfully finishes, no?
>
>> +			}
>> +		}
>

Hmm. Yeah, you have a point. I think we'll have to differentiate between
`retcode` and a new `backfill_retcode`. Will add that in.

[snip]

While testing more with regards to your comments, I realized the new
test doesn't really catch the issue, since we only transfer objects we
already had, so checking for root_oid will fail with/without the fix.
Will revert the test to the older version.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v3] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Batched updates are used both for the fetched
references and to backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. This means in situations such as shallow
fetching the negotiation during backfilling tags, the newly fetched refs
are not available for advertisement in the 'haves' section. This can
cause the server to send all objects the client already has.

The issue always existed when the '--atomic' flag was used, as both the
regular ref updates and the tags need to be part of the same
transaction.

Fix this by committing the previous batched update and initiating a new
one for backfilling tags. Use a separate error variable for tracking the
reference updates: One, it ensures that errors from the regular ref
updates don't cause a total failure. Two, if starting the transaction
for backfilling tags fails, only those updates are not made to disk. The
initial error is now printed straight away to clear the buffer for
further errors.

Add two tests, one which validates that the 'haves' are filled when
backfilling tags. The other, to test that upstream is still set and tags
are backfilled even if the main ref update is rejected.

While this does make it a little slower than master, due to two
transactions:

Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)
  Time (mean ± σ):      82.1 ms ±   3.3 ms    [User: 60.2 ms, System: 24.4 ms]
  Range (min … max):    77.4 ms …  89.4 ms    33 runs

Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
  Time (mean ± σ):      82.6 ms ±   2.4 ms    [User: 59.3 ms, System: 25.2 ms]
  Range (min … max):    78.1 ms …  88.7 ms    31 runs

Summary
  fetch: many refs (refformat = reftable, refcount = 10000, revision = master) ran
   1.01 ± 0.05 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)

It is still faster than not using batched updates:

Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
  Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
  Range (min … max):    1.427 s …  1.558 s    10 runs

Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
  Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
  Range (min … max):    81.4 ms …  88.6 ms    29 runs

Summary
  fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
   17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799
---
Changes in v3:
- Fix the early exit, so a failed transaction_begin() only skips the
  backfill.
- Keep the first commit's result separately, so rejections don't skip
  the upstream and remote-HEAD setup.
- Test that the backfill advertises the fetched tip as a have, instead
  of checking that the root object isn't fetched, which passed even
  without the fix.
- Add a test for the upstream setup when a main ref is rejected and a
  tag is backfilled.
- Link to v2: https://patch.msgid.link/20261012-799-shallow-fetch-with-tags-v2-1-4211e9a13d3c@gmail.com

Changes in v2:
- Add benchmarks between master and this branch to show perf regression.
- Instead of checking the trace file, simply check that we didn't fetch
  older objects in the test.
- Link to v1: https://patch.msgid.link/20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com

CC: Patrick Steinhardt <ps@pks.im>
CC: Justin Tobler <jltobler@gmail.com>
---
 builtin/fetch.c  | 38 +++++++++++++++++++++++++++++++++++---
 t/t5510-fetch.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..34fc37b58a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1923,6 +1923,7 @@ static int do_fetch(struct transport *transport,
 	struct strmap rejected_refs = STRMAP_INIT;
 	int summary_width = 0;
 	int follow_remote_head;
+	int commit_ref_ret = 0;
 
 	if (tags == TAGS_DEFAULT) {
 		if (transport->remote->fetch_tags == 2)
@@ -2076,6 +2077,30 @@ static int do_fetch(struct transport *transport,
 		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
+
+		/*
+		 * As negotiation only reads references from disk, commit the references
+		 * fetched so far, so that the backfill negotiation can advertise them as
+		 * 'have's. Otherwise the server may re-send objects.
+		 */
+		if (tags_ref_map && !atomic_fetch) {
+			commit_ref_ret = commit_ref_transaction(&transaction, false,
+								transport->remote->name,
+								&rejected_refs, &err);
+			if (err.len) {
+				error("%s", err.buf);
+				strbuf_reset(&err);
+			}
+
+			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
+								  REF_TRANSACTION_ALLOW_FAILURE, &err);
+			if (!transaction) {
+				free_refs(tags_ref_map);
+				tags_ref_map = NULL;
+				commit_ref_ret = -1;
+			}
+		}
+
 		if (tags_ref_map) {
 			/*
 			 * If backfilling of tags fails then we want to tell
@@ -2099,9 +2124,16 @@ static int do_fetch(struct transport *transport,
 	if (verbosity >= 0)
 		summary_width = transport_summary_width(ref_map);
 
-	retcode = commit_ref_transaction(&transaction, atomic_fetch,
-					 transport->remote->name,
-					 &rejected_refs, &err);
+	/*
+	 * The transaction may have failed to initialize, meaning there is nothing
+	 * to commit. Carry forward with other operations in such cases.
+	 */
+	if (transaction)
+		retcode = commit_ref_transaction(&transaction, atomic_fetch,
+						 transport->remote->name,
+						 &rejected_refs, &err);
+	retcode |= commit_ref_ret;
+
 	/*
 	 * With '--atomic', bail out if the transaction fails. Without '--atomic',
 	 * continue to fetch head and perform other post-fetch operations.
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..0baf18e01e 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,23 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'backfilling tags after shallow fetch advertises fetched refs as haves' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 
@@ -2005,6 +2022,31 @@ test_expect_success REFFILES "upstream tracking info is added even with conflict
 	)
 '
 
+test_expect_success REFFILES "upstream tracking info is added with conflicts and backfilled tags" '
+	test_when_finished rm -rf base repo &&
+
+	git init --initial-branch=main base &&
+	test_commit -C base one &&
+	test_commit -C base two &&
+	git -C base branch foo &&
+	# Backfilling only takes place for tags which are not pointing at tips.
+	git -C base tag -a annotated -m annotated HEAD~1 &&
+
+	git init --bare --initial-branch=main repo &&
+	(
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir -p refs/remotes/origin &&
+		>refs/remotes/origin/foo.lock &&
+		test_must_fail git fetch origin --set-upstream main \
+			"+refs/heads/*:refs/remotes/origin/*" &&
+		git config get branch.main.remote >actual &&
+		echo "origin" >expect &&
+		test_cmp expect actual &&
+		git rev-parse --verify refs/tags/annotated
+	)
+'
+
 test_expect_success REFFILES "HEAD is updated even with conflicts" '
 	test_when_finished rm -rf base repo &&
 

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Justin Tobler]

On 26/10/15 11:24PM, Karthik Nayak wrote:
> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
> fetch code was modified to use batched updates to provide a good
> performance improvement. Batched updates are used both for the fetched
> references and to backfill tags.
> 
> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. This means in situations such as shallow
> fetching the negotiation during backfilling tags, the newly fetched refs
> are not available for advertisement in the 'haves' section. This can
> cause the server to send all objects the client already has.

Ok, without the earlier fetched references committed, the backfill fetch
may ultimately ask for objects it already got. Make sense.

> The issue always existed when the '--atomic' flag was used, as both the
> regular ref updates and the tags need to be part of the same
> transaction.

So IIUC, you are saying that this problem already existed prior to
0e358de64a, but only for atomic fetches. Furthermore, this patch doesn't
fix anything for atomic fetches as they can only use a single
transaction.

> Fix this by committing the previous batched update and initiating a new
> one for backfilling tags. Use a separate error variable for tracking the
> reference updates: One, it ensures that errors from the regular ref
> updates don't cause a total failure. Two, if starting the transaction
> for backfilling tags fails, only those updates are not made to disk. The
> initial error is now printed straight away to clear the buffer for
> further errors.

Naively, I think it would be nice if it were possible supplement the
backfill fetch "haves" with the non-committed references in the pending
transaction directly. That would avoid having to commit the references
in the middle and probably also fix it for atomic fetches too, but I'm
uncertain if that would be a simple or reasonable thing to do.

Committing the intermediate state here does in part fix the issue, so it
may be a good first step regardless.

> Add two tests, one which validates that the 'haves' are filled when
> backfilling tags. The other, to test that upstream is still set and tags
> are backfilled even if the main ref update is rejected.
> 
> While this does make it a little slower than master, due to two
> transactions:
> 
> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)
>   Time (mean ± σ):      82.1 ms ±   3.3 ms    [User: 60.2 ms, System: 24.4 ms]
>   Range (min … max):    77.4 ms …  89.4 ms    33 runs
> 
> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
>   Time (mean ± σ):      82.6 ms ±   2.4 ms    [User: 59.3 ms, System: 25.2 ms]
>   Range (min … max):    78.1 ms …  88.7 ms    31 runs
> 
> Summary
>   fetch: many refs (refformat = reftable, refcount = 10000, revision = master) ran
>    1.01 ± 0.05 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
> 
> It is still faster than not using batched updates:
> 
> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
>   Time (mean ± σ):      1.468 s ±  0.041 s    [User: 0.839 s, System: 0.587 s]
>   Range (min … max):    1.427 s …  1.558 s    10 runs
> 
> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)
>   Time (mean ± σ):      84.4 ms ±   1.7 ms    [User: 60.9 ms, System: 25.8 ms]
>   Range (min … max):    81.4 ms …  88.6 ms    29 runs
> 
> Summary
>   fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran
>    17.38 ± 0.61 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = 0e358de64a9e014575d11ef884bfc9beb931e37f~1)
> 
> Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
> The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].
> 
> [1]: https://gitlab.com/gitlab-org/git/-/work_items/799
> ---
>  builtin/fetch.c  | 38 +++++++++++++++++++++++++++++++++++---
>  t/t5510-fetch.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2decc6cfd..34fc37b58a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1923,6 +1923,7 @@ static int do_fetch(struct transport *transport,
>  	struct strmap rejected_refs = STRMAP_INIT;
>  	int summary_width = 0;
>  	int follow_remote_head;
> +	int commit_ref_ret = 0;
>  
>  	if (tags == TAGS_DEFAULT) {
>  		if (transport->remote->fetch_tags == 2)
> @@ -2076,6 +2077,30 @@ static int do_fetch(struct transport *transport,
>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
> +
> +		/*
> +		 * As negotiation only reads references from disk, commit the references
> +		 * fetched so far, so that the backfill negotiation can advertise them as
> +		 * 'have's. Otherwise the server may re-send objects.
> +		 */
> +		if (tags_ref_map && !atomic_fetch) {
> +			commit_ref_ret = commit_ref_transaction(&transaction, false,
> +								transport->remote->name,
> +								&rejected_refs, &err);
> +			if (err.len) {
> +				error("%s", err.buf);
> +				strbuf_reset(&err);
> +			}

It looks like we log the error and continue even if committing the
transaction failed. Is there a reason we would want to still try to
start the next transaction?

> +
> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
> +			if (!transaction) {
> +				free_refs(tags_ref_map);
> +				tags_ref_map = NULL;
> +				commit_ref_ret = -1;
> +			}
> +		}
> +
>  		if (tags_ref_map) {
>  			/*
>  			 * If backfilling of tags fails then we want to tell
> @@ -2099,9 +2124,16 @@ static int do_fetch(struct transport *transport,
>  	if (verbosity >= 0)
>  		summary_width = transport_summary_width(ref_map);
>  
> -	retcode = commit_ref_transaction(&transaction, atomic_fetch,
> -					 transport->remote->name,
> -					 &rejected_refs, &err);
> +	/*
> +	 * The transaction may have failed to initialize, meaning there is nothing
> +	 * to commit. Carry forward with other operations in such cases.
> +	 */
> +	if (transaction)
> +		retcode = commit_ref_transaction(&transaction, atomic_fetch,
> +						 transport->remote->name,
> +						 &rejected_refs, &err);
> +	retcode |= commit_ref_ret;

hmmm, the error paths here are confusing me a bit. If the transaction
failed to be created in the first place, wouldn't we want to stop
earlier on?

Thanks,
-Justin

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Patrick Steinhardt]

On Tue, Oct 20, 2026 at 06:05:22PM -0500, Justin Tobler wrote:
> On 26/10/15 11:24PM, Karthik Nayak wrote:
[snip]
> > Fix this by committing the previous batched update and initiating a new
> > one for backfilling tags. Use a separate error variable for tracking the
> > reference updates: One, it ensures that errors from the regular ref
> > updates don't cause a total failure. Two, if starting the transaction
> > for backfilling tags fails, only those updates are not made to disk. The
> > initial error is now printed straight away to clear the buffer for
> > further errors.
> 
> Naively, I think it would be nice if it were possible supplement the
> backfill fetch "haves" with the non-committed references in the pending
> transaction directly. That would avoid having to commit the references
> in the middle and probably also fix it for atomic fetches too, but I'm
> uncertain if that would be a simple or reasonable thing to do.

Hmm, I think that's a sensible idea. We should know about the ref
updates we have already scheduled into the transaction, so in theory we
could use their new object IDs in the negotiation. If that's possible it
would definitely be a much better fix.

> Committing the intermediate state here does in part fix the issue, so it
> may be a good first step regardless.

Well, if the above suggestions works we'd want to revert this step here,
so we'd first have to go back a step again.

If the above is infeasible then that's fine. But if it is feasible
without too much effort I'd go for it directly.

Patrick

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2013 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Oct 20, 2026 at 06:05:22PM -0500, Justin Tobler wrote:
>> On 26/10/15 11:24PM, Karthik Nayak wrote:
> [snip]
>> > Fix this by committing the previous batched update and initiating a new
>> > one for backfilling tags. Use a separate error variable for tracking the
>> > reference updates: One, it ensures that errors from the regular ref
>> > updates don't cause a total failure. Two, if starting the transaction
>> > for backfilling tags fails, only those updates are not made to disk. The
>> > initial error is now printed straight away to clear the buffer for
>> > further errors.
>>
>> Naively, I think it would be nice if it were possible supplement the
>> backfill fetch "haves" with the non-committed references in the pending
>> transaction directly. That would avoid having to commit the references
>> in the middle and probably also fix it for atomic fetches too, but I'm
>> uncertain if that would be a simple or reasonable thing to do.
>
> Hmm, I think that's a sensible idea. We should know about the ref
> updates we have already scheduled into the transaction, so in theory we
> could use their new object IDs in the negotiation. If that's possible it
> would definitely be a much better fix.
>
>> Committing the intermediate state here does in part fix the issue, so it
>> may be a good first step regardless.
>
> Well, if the above suggestions works we'd want to revert this step here,
> so we'd first have to go back a step again.
>
> If the above is infeasible then that's fine. But if it is feasible
> without too much effort I'd go for it directly.
>
> Patrick

I'm a bit wary of going down ad we'd be exposing un-committed references
which could fail as 'haves' to the server. This doesn't seem to be
conceptually correct as we now expect GC to not clear the objects needed
by relying on the .keep lock instead of durable references.

But seems like we have negotiation_include_tips in the transport
structure which seems like the right fit?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 5167 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 26/10/15 11:24PM, Karthik Nayak wrote:
>> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
>> fetch code was modified to use batched updates to provide a good
>> performance improvement. Batched updates are used both for the fetched
>> references and to backfill tags.
>>
>> When using batched updates, the references aren't yet committed to disk
>> when we start backfilling tags. This means in situations such as shallow
>> fetching the negotiation during backfilling tags, the newly fetched refs
>> are not available for advertisement in the 'haves' section. This can
>> cause the server to send all objects the client already has.
>
> Ok, without the earlier fetched references committed, the backfill fetch
> may ultimately ask for objects it already got. Make sense.
>
>> The issue always existed when the '--atomic' flag was used, as both the
>> regular ref updates and the tags need to be part of the same
>> transaction.
>
> So IIUC, you are saying that this problem already existed prior to
> 0e358de64a, but only for atomic fetches. Furthermore, this patch doesn't
> fix anything for atomic fetches as they can only use a single
> transaction.
>

Yup, there's nothing we can do for atomic fetches with the current
approach.

>> Fix this by committing the previous batched update and initiating a new
>> one for backfilling tags. Use a separate error variable for tracking the
>> reference updates: One, it ensures that errors from the regular ref
>> updates don't cause a total failure. Two, if starting the transaction
>> for backfilling tags fails, only those updates are not made to disk. The
>> initial error is now printed straight away to clear the buffer for
>> further errors.
>
> Naively, I think it would be nice if it were possible supplement the
> backfill fetch "haves" with the non-committed references in the pending
> transaction directly. That would avoid having to commit the references
> in the middle and probably also fix it for atomic fetches too, but I'm
> uncertain if that would be a simple or reasonable thing to do.
>
> Committing the intermediate state here does in part fix the issue, so it
> may be a good first step regardless.
>

I've replied to this with Patrick's email, let's continue there.

[snip]

>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index b2decc6cfd..34fc37b58a 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1923,6 +1923,7 @@ static int do_fetch(struct transport *transport,
>>  	struct strmap rejected_refs = STRMAP_INIT;
>>  	int summary_width = 0;
>>  	int follow_remote_head;
>> +	int commit_ref_ret = 0;
>>
>>  	if (tags == TAGS_DEFAULT) {
>>  		if (transport->remote->fetch_tags == 2)
>> @@ -2076,6 +2077,30 @@ static int do_fetch(struct transport *transport,
>>  		struct ref *tags_ref_map = NULL, **tail = &tags_ref_map;
>>
>>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
>> +
>> +		/*
>> +		 * As negotiation only reads references from disk, commit the references
>> +		 * fetched so far, so that the backfill negotiation can advertise them as
>> +		 * 'have's. Otherwise the server may re-send objects.
>> +		 */
>> +		if (tags_ref_map && !atomic_fetch) {
>> +			commit_ref_ret = commit_ref_transaction(&transaction, false,
>> +								transport->remote->name,
>> +								&rejected_refs, &err);
>> +			if (err.len) {
>> +				error("%s", err.buf);
>> +				strbuf_reset(&err);
>> +			}
>
> It looks like we log the error and continue even if committing the
> transaction failed. Is there a reason we would want to still try to
> start the next transaction?
>

Since we use batched updates, some refs could fail, but we continue as
we may have still applied some updates and we can continue doing the
rest of the work.

>> +
>> +			transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
>> +								  REF_TRANSACTION_ALLOW_FAILURE, &err);
>> +			if (!transaction) {
>> +				free_refs(tags_ref_map);
>> +				tags_ref_map = NULL;
>> +				commit_ref_ret = -1;
>> +			}
>> +		}
>> +
>>  		if (tags_ref_map) {
>>  			/*
>>  			 * If backfilling of tags fails then we want to tell
>> @@ -2099,9 +2124,16 @@ static int do_fetch(struct transport *transport,
>>  	if (verbosity >= 0)
>>  		summary_width = transport_summary_width(ref_map);
>>
>> -	retcode = commit_ref_transaction(&transaction, atomic_fetch,
>> -					 transport->remote->name,
>> -					 &rejected_refs, &err);
>> +	/*
>> +	 * The transaction may have failed to initialize, meaning there is nothing
>> +	 * to commit. Carry forward with other operations in such cases.
>> +	 */
>> +	if (transaction)
>> +		retcode = commit_ref_transaction(&transaction, atomic_fetch,
>> +						 transport->remote->name,
>> +						 &rejected_refs, &err);
>> +	retcode |= commit_ref_ret;
>
> hmmm, the error paths here are confusing me a bit. If the transaction
> failed to be created in the first place, wouldn't we want to stop
> earlier on?
>
> Thanks,
> -Justin

Not really, since we use batched updates, the backfilling tags
transaction could fail to initialize but we still want to set upstreams
and set HEAD.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Justin Tobler]

On 26/10/21 06:01AM, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, Oct 20, 2026 at 06:05:22PM -0500, Justin Tobler wrote:
> >> Naively, I think it would be nice if it were possible supplement the
> >> backfill fetch "haves" with the non-committed references in the pending
> >> transaction directly. That would avoid having to commit the references
> >> in the middle and probably also fix it for atomic fetches too, but I'm
> >> uncertain if that would be a simple or reasonable thing to do.
> >
> > Hmm, I think that's a sensible idea. We should know about the ref
> > updates we have already scheduled into the transaction, so in theory we
> > could use their new object IDs in the negotiation. If that's possible it
> > would definitely be a much better fix.
>
> I'm a bit wary of going down ad we'd be exposing un-committed references
> which could fail as 'haves' to the server. This doesn't seem to be
> conceptually correct as we now expect GC to not clear the objects needed
> by relying on the .keep lock instead of durable references.

I'm not quite sure I follow why this would be conceptually problematic.
Should it really matter in this particular scenario whether the tips the
client advertises are committed or not? The required objects should
already be present in the repository from the initial fetch.

Regarding the continued use of ".keep" lockfiles to prevent newly
fetched packfiles from being GC'd, that still sounds reasonable to me.
It is expected that the ".keep" lockfiles remain in place until the
reference transaction is committed any way. To me, conceptually all we
are doing is allowing the current inflight state of the transaction to
be used to affect another operation being performed within the very same
transaction.

> But seems like we have negotiation_include_tips in the transport
> structure which seems like the right fit?

At a quick glance, this seems like it could be a good way for the client
to advertise to the sever the tips of the uncommitted references it has.

-Justin

Re: [PATCH v3] fetch: commit references fetched before backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2567 bytes --]

Justin Tobler <jltobler@gmail.com> writes:

> On 26/10/21 06:01AM, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > On Tue, Oct 20, 2026 at 06:05:22PM -0500, Justin Tobler wrote:
>> >> Naively, I think it would be nice if it were possible supplement the
>> >> backfill fetch "haves" with the non-committed references in the pending
>> >> transaction directly. That would avoid having to commit the references
>> >> in the middle and probably also fix it for atomic fetches too, but I'm
>> >> uncertain if that would be a simple or reasonable thing to do.
>> >
>> > Hmm, I think that's a sensible idea. We should know about the ref
>> > updates we have already scheduled into the transaction, so in theory we
>> > could use their new object IDs in the negotiation. If that's possible it
>> > would definitely be a much better fix.
>>
>> I'm a bit wary of going down ad we'd be exposing un-committed references
>> which could fail as 'haves' to the server. This doesn't seem to be
>> conceptually correct as we now expect GC to not clear the objects needed
>> by relying on the .keep lock instead of durable references.
>
> I'm not quite sure I follow why this would be conceptually problematic.
> Should it really matter in this particular scenario whether the tips the
> client advertises are committed or not? The required objects should
> already be present in the repository from the initial fetch.
>
> Regarding the continued use of ".keep" lockfiles to prevent newly
> fetched packfiles from being GC'd, that still sounds reasonable to me.
> It is expected that the ".keep" lockfiles remain in place until the
> reference transaction is committed any way. To me, conceptually all we
> are doing is allowing the current inflight state of the transaction to
> be used to affect another operation being performed within the very same
> transaction.
>

I was getting at it from the point that '.keep' lockfiles are created
for the reference transaction to commit. So while they keep the objects
in place, we should only rely on a committed reference to keep objects
in place.

But, you're right, the transaction here keeps the lockfiles in place
until we commit anyways. So I guess that's okay. Let me send in a new
version and we can see what everyone thinks. Thanks!

>> But seems like we have negotiation_include_tips in the transport
>> structure which seems like the right fit?
>
> At a quick glance, this seems like it could be a good way for the client
> to advertise to the sever the tips of the uncommitted references it has.
>
> -Justin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v4] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Batched updates are used both for the fetched
references and to backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. For shallow fetches, the negotiation
phase while backfilling tags does not advertise the newly fetched refs.
This can cause the server to send objects the client already has.

The issue always existed when the '--atomic' flag was used, as both the
regular ref updates and the tags need to be part of the same
transaction.

Fix this by advertising the already fetched refs as 'haves' during the
negotiation. Do this by simply adding the tips to the transport's
`negotiation_include_tips` field. This also fixes the issue for
`--atomic` fetches since it follows the same route.

Add three tests, two which validates that the 'haves' are filled when
backfilling tags for atomic and non-atomic flows. The third, to test
that upstream is still set and tags are backfilled even if a sibling ref
is rejected.

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799
---
Changes in v4:
- Instead of committing the first transaction and starting a new one,
  advertise the refs already fetched when backfilling tags. Thanks to
  Justin for the suggestion. This also fixes the `--atomic` flow.
- Link to v3: https://patch.msgid.link/20261015-799-shallow-fetch-with-tags-v3-1-90b6e38b98d2@gmail.com

Changes in v3:
- Fix the early exit, so a failed transaction_begin() only skips the
  backfill.
- Keep the first commit's result separately, so rejections don't skip
  the upstream and remote-HEAD setup.
- Test that the backfill advertises the fetched tip as a have, instead
  of checking that the root object isn't fetched, which passed even
  without the fix.
- Add a test for the upstream setup when a main ref is rejected and a
  tag is backfilled.
- Link to v2: https://patch.msgid.link/20261012-799-shallow-fetch-with-tags-v2-1-4211e9a13d3c@gmail.com

Changes in v2:
- Add benchmarks between master and this branch to show perf regression.
- Instead of checking the trace file, simply check that we didn't fetch
  older objects in the test.
- Link to v1: https://patch.msgid.link/20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com

CC: Patrick Steinhardt <ps@pks.im>
CC: Justin Tobler <jltobler@gmail.com>
---
 builtin/fetch.c  | 41 +++++++++++++++++++++++++++++++++++++--
 t/t5510-fetch.sh | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..9867e8c2ef 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1676,8 +1676,10 @@ static int backfill_tags(struct display_state *display_state,
 			 struct fetch_head *fetch_head,
 			 const struct fetch_config *config,
 			 struct ref_update_display_info_array *display_array,
-			 struct list_objects_filter_options *filter_options)
+			 struct list_objects_filter_options *filter_options,
+			 const struct oid_array *fetched_tips)
 {
+	struct oid_array *to_free = NULL;
 	int retcode, cannot_reuse;
 
 	/*
@@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
+
+	/*
+	 * References fetched before the tag backfill aren't yet committed
+	 * to disk, so negotiation cannot discover them. Advertise them to
+	 * the server, so that it does not re-send objects we already have.
+	 */
+	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
+		struct oid_array **include_tips =
+			&transport->smart_options->negotiation_include_tips;
+		if (!*include_tips)
+			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
+		for (size_t i = 0; i < fetched_tips->nr; i++)
+			oid_array_append(*include_tips, &fetched_tips->oid[i]);
+	}
+
 	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
 					 fetch_head, config, display_array);
 
+	if (to_free) {
+		oid_array_clear(to_free);
+		free(to_free);
+		transport->smart_options->negotiation_include_tips = NULL;
+	}
+
 	if (gsecondary) {
 		transport_disconnect(gsecondary);
 		gsecondary = NULL;
@@ -2077,6 +2100,17 @@ static int do_fetch(struct transport *transport,
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
 		if (tags_ref_map) {
+			struct oid_array fetched_tips = OID_ARRAY_INIT;
+			struct ref *rm;
+
+			/*
+			 * Collect the tips we just fetched so the tag backfill
+			 * can advertise them to the server as 'haves'.
+			 */
+			for (rm = ref_map; rm; rm = rm->next)
+				if (!is_null_oid(&rm->old_oid))
+					oid_array_append(&fetched_tips, &rm->old_oid);
+
 			/*
 			 * If backfilling of tags fails then we want to tell
 			 * the user so, but we have to continue regardless to
@@ -2086,8 +2120,11 @@ 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, &display_array, filter_options))
+					  &fetch_head, config, &display_array, filter_options,
+					  &fetched_tips))
 				retcode = 1;
+
+			oid_array_clear(&fetched_tips);
 		}
 
 		free_refs(tags_ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..ca5f9642fc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,40 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'backfilling tags advertises fetched refs as haves' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
+test_expect_success 'backfilling tags advertises fetched refs as haves even with atomic' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --atomic --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 
@@ -2005,6 +2039,31 @@ test_expect_success REFFILES "upstream tracking info is added even with conflict
 	)
 '
 
+test_expect_success REFFILES "upstream tracking info is added with conflicts and backfilled tags" '
+	test_when_finished rm -rf base repo &&
+
+	git init --initial-branch=main base &&
+	test_commit -C base one &&
+	test_commit -C base two &&
+	git -C base branch foo &&
+	# Backfilling only takes place for tags which are not pointing at tips.
+	git -C base tag -a annotated -m annotated HEAD~1 &&
+
+	git init --bare --initial-branch=main repo &&
+	(
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir -p refs/remotes/origin &&
+		>refs/remotes/origin/foo.lock &&
+		test_must_fail git fetch origin --set-upstream main \
+			"+refs/heads/*:refs/remotes/origin/*" &&
+		git config get branch.main.remote >actual &&
+		echo "origin" >expect &&
+		test_cmp expect actual &&
+		git rev-parse --verify refs/tags/annotated
+	)
+'
+
 test_expect_success REFFILES "HEAD is updated even with conflicts" '
 	test_when_finished rm -rf base repo &&
 

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

Re: [PATCH v4] fetch: advertise fetched refs when backfilling tags

[Patrick Steinhardt]

on thu, oct 22, 2026 at 11:00:16pm +0200, karthik nayak wrote:
> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
> fetch code was modified to use batched updates to provide a good
> performance improvement. Batched updates are used both for the fetched
> references and to backfill tags.
> 
> When using batched updates, the references aren't yet committed to disk
> when we start backfilling tags. For shallow fetches, the negotiation
> phase while backfilling tags does not advertise the newly fetched refs.
> This can cause the server to send objects the client already has.
> 
> The issue always existed when the '--atomic' flag was used, as both the
> regular ref updates and the tags need to be part of the same
> transaction.
> 
> Fix this by advertising the already fetched refs as 'haves' during the
> negotiation. Do this by simply adding the tips to the transport's
> `negotiation_include_tips` field. This also fixes the issue for
> `--atomic` fetches since it follows the same route.

Okay. Would the client have advertised _all_ its local references to the
server before the batch updates? And with the new logic, would we now
start to advertise all of them? Mostly trying to figure out whether
there is a potential change in behaviour here where we may now advertise
millions of haves whereas we previously didn't.

And even if that was the case it _may_ be the right thing to do. I
mostly just want to see this discussed in the commit message :)

> Add three tests, two which validates that the 'haves' are filled when
> backfilling tags for atomic and non-atomic flows. The third, to test
> that upstream is still set and tags are backfilled even if a sibling ref
> is rejected.

The third test is curious. Does it have anything to do with the fix
itself? If not, why add it now?

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b2decc6cfd..9867e8c2ef 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
>  	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
>  	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>  	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
> +
> +	/*
> +	 * References fetched before the tag backfill aren't yet committed
> +	 * to disk, so negotiation cannot discover them. Advertise them to
> +	 * the server, so that it does not re-send objects we already have.
> +	 */
> +	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
> +		struct oid_array **include_tips =
> +			&transport->smart_options->negotiation_include_tips;
> +		if (!*include_tips)
> +			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
> +		for (size_t i = 0; i < fetched_tips->nr; i++)
> +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
> +	}

Curious. Why don't we have `struct oid_array` as a local variable and
then simply `transport->smart_options->negotiation_include_tips =
&extra_tips`?

>  	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
>  					 fetch_head, config, display_array);
>  
> +	if (to_free) {
> +		oid_array_clear(to_free);
> +		free(to_free);
> +		transport->smart_options->negotiation_include_tips = NULL;
> +	}

This could then become unconditional.

> @@ -2077,6 +2100,17 @@ static int do_fetch(struct transport *transport,
>  
>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
>  		if (tags_ref_map) {
> +			struct oid_array fetched_tips = OID_ARRAY_INIT;
> +			struct ref *rm;

This is just the loop variable, so it could move into the loop.

> +			/*
> +			 * Collect the tips we just fetched so the tag backfill
> +			 * can advertise them to the server as 'haves'.
> +			 */
> +			for (rm = ref_map; rm; rm = rm->next)
> +				if (!is_null_oid(&rm->old_oid))
> +					oid_array_append(&fetched_tips, &rm->old_oid);

Makes sense.

Overall this version looks much saner to me, thanks!

Patrick

Re: [PATCH v4] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4757 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> on thu, oct 22, 2026 at 11:00:16pm +0200, karthik nayak wrote:
>> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
>> fetch code was modified to use batched updates to provide a good
>> performance improvement. Batched updates are used both for the fetched
>> references and to backfill tags.
>>
>> When using batched updates, the references aren't yet committed to disk
>> when we start backfilling tags. For shallow fetches, the negotiation
>> phase while backfilling tags does not advertise the newly fetched refs.
>> This can cause the server to send objects the client already has.
>>
>> The issue always existed when the '--atomic' flag was used, as both the
>> regular ref updates and the tags need to be part of the same
>> transaction.
>>
>> Fix this by advertising the already fetched refs as 'haves' during the
>> negotiation. Do this by simply adding the tips to the transport's
>> `negotiation_include_tips` field. This also fixes the issue for
>> `--atomic` fetches since it follows the same route.
>
> Okay. Would the client have advertised _all_ its local references to the
> server before the batch updates? And with the new logic, would we now
> start to advertise all of them? Mostly trying to figure out whether
> there is a potential change in behaviour here where we may now advertise
> millions of haves whereas we previously didn't.
>

Yeah, we would, In the end, we only negotiation the OIDs, so duplicates
would be removed, but it is O(n) for the number of refs fetched.

> And even if that was the case it _may_ be the right thing to do. I
> mostly just want to see this discussed in the commit message :)
>

That's fair, I'll add that in.

>> Add three tests, two which validates that the 'haves' are filled when
>> backfilling tags for atomic and non-atomic flows. The third, to test
>> that upstream is still set and tags are backfilled even if a sibling ref
>> is rejected.
>
> The third test is curious. Does it have anything to do with the fix
> itself? If not, why add it now?
>

It was a missing test which ensures that even if committing references
fail, we still update the upstream information. I can move it out into a
separate patch.

>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index b2decc6cfd..9867e8c2ef 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
>>  	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
>>  	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>>  	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
>> +
>> +	/*
>> +	 * References fetched before the tag backfill aren't yet committed
>> +	 * to disk, so negotiation cannot discover them. Advertise them to
>> +	 * the server, so that it does not re-send objects we already have.
>> +	 */
>> +	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
>> +		struct oid_array **include_tips =
>> +			&transport->smart_options->negotiation_include_tips;
>> +		if (!*include_tips)
>> +			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
>> +		for (size_t i = 0; i < fetched_tips->nr; i++)
>> +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
>> +	}
>
> Curious. Why don't we have `struct oid_array` as a local variable and
> then simply `transport->smart_options->negotiation_include_tips =
> &extra_tips`?
>

Because `transport->smart_options->negotiation_include_tips` may already
be set when this function is called, so we only free it when we are the
exclusive setters of the field.

>>  	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
>>  					 fetch_head, config, display_array);
>>
>> +	if (to_free) {
>> +		oid_array_clear(to_free);
>> +		free(to_free);
>> +		transport->smart_options->negotiation_include_tips = NULL;
>> +	}
>
> This could then become unconditional.
>
>> @@ -2077,6 +2100,17 @@ static int do_fetch(struct transport *transport,
>>
>>  		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
>>  		if (tags_ref_map) {
>> +			struct oid_array fetched_tips = OID_ARRAY_INIT;
>> +			struct ref *rm;
>
> This is just the loop variable, so it could move into the loop.
>

Yeah, I can inline it.

>> +			/*
>> +			 * Collect the tips we just fetched so the tag backfill
>> +			 * can advertise them to the server as 'haves'.
>> +			 */
>> +			for (rm = ref_map; rm; rm = rm->next)
>> +				if (!is_null_oid(&rm->old_oid))
>> +					oid_array_append(&fetched_tips, &rm->old_oid);
>
> Makes sense.
>
> Overall this version looks much saner to me, thanks!
>
> Patrick

Thanks, I'll wait a day for any responses and send in a new version with
the fixes.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4] fetch: advertise fetched refs when backfilling tags

[Patrick Steinhardt]

On Fri, Oct 23, 2026 at 03:58:11AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > on thu, oct 22, 2026 at 11:00:16pm +0200, karthik nayak wrote:
> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
> >> index b2decc6cfd..9867e8c2ef 100644
> >> --- a/builtin/fetch.c
> >> +++ b/builtin/fetch.c
> >> @@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
> >>  	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
> >>  	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
> >>  	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
> >> +
> >> +	/*
> >> +	 * References fetched before the tag backfill aren't yet committed
> >> +	 * to disk, so negotiation cannot discover them. Advertise them to
> >> +	 * the server, so that it does not re-send objects we already have.
> >> +	 */
> >> +	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
> >> +		struct oid_array **include_tips =
> >> +			&transport->smart_options->negotiation_include_tips;
> >> +		if (!*include_tips)
> >> +			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
> >> +		for (size_t i = 0; i < fetched_tips->nr; i++)
> >> +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
> >> +	}
> >
> > Curious. Why don't we have `struct oid_array` as a local variable and
> > then simply `transport->smart_options->negotiation_include_tips =
> > &extra_tips`?
> >
> 
> Because `transport->smart_options->negotiation_include_tips` may already
> be set when this function is called, so we only free it when we are the
> exclusive setters of the field.

Ah, okay. But if it was set already, what does it contain? And is it
okay that we essentially leak the fetched tips to our callers now if it
was set?

Patrick

Re: [PATCH v4] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2552 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Fri, Oct 23, 2026 at 03:58:11AM -0700, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>> > on thu, oct 22, 2026 at 11:00:16pm +0200, karthik nayak wrote:
>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> >> index b2decc6cfd..9867e8c2ef 100644
>> >> --- a/builtin/fetch.c
>> >> +++ b/builtin/fetch.c
>> >> @@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
>> >>  	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
>> >>  	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>> >>  	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
>> >> +
>> >> +	/*
>> >> +	 * References fetched before the tag backfill aren't yet committed
>> >> +	 * to disk, so negotiation cannot discover them. Advertise them to
>> >> +	 * the server, so that it does not re-send objects we already have.
>> >> +	 */
>> >> +	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
>> >> +		struct oid_array **include_tips =
>> >> +			&transport->smart_options->negotiation_include_tips;
>> >> +		if (!*include_tips)
>> >> +			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
>> >> +		for (size_t i = 0; i < fetched_tips->nr; i++)
>> >> +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
>> >> +	}
>> >
>> > Curious. Why don't we have `struct oid_array` as a local variable and
>> > then simply `transport->smart_options->negotiation_include_tips =
>> > &extra_tips`?
>> >
>>
>> Because `transport->smart_options->negotiation_include_tips` may already
>> be set when this function is called, so we only free it when we are the
>> exclusive setters of the field.
>
> Ah, okay. But if it was set already, what does it contain? And is it
> okay that we essentially leak the fetched tips to our callers now if it
> was set?
>
> Patrick

So it is set when the user uses the '--negotiation-include' option and
provides their own tips.

When I wrote this code, I didn't include the free'ing that you see here.
But it triggered the leak sanitizer for 't5509-fetch-push-namespaces'. I
assumed it was due to my code and added this logic, without digging too
much into why my path specifically triggered it.

So, the transport does free the `negotiation_include_tips`, but only for
transports which supports smart_options. Meaning, we should only add the
tips for transports with smart options. As the other route doesn't use
it and as such doesn't free the array.

Thanks for the pushback, I'll fix it up in the next version.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 3116 bytes --]

Karthik Nayak <karthik.188@gmail.com> writes:

> Patrick Steinhardt <ps@pks.im> writes:
>
>> On Fri, Oct 23, 2026 at 03:58:11AM -0700, Karthik Nayak wrote:
>>> Patrick Steinhardt <ps@pks.im> writes:
>>> > on thu, oct 22, 2026 at 11:00:16pm +0200, karthik nayak wrote:
>>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c
>>> >> index b2decc6cfd..9867e8c2ef 100644
>>> >> --- a/builtin/fetch.c
>>> >> +++ b/builtin/fetch.c
>>> >> @@ -1697,9 +1699,30 @@ static int backfill_tags(struct display_state *display_state,
>>> >>  	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
>>> >>  	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>>> >>  	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
>>> >> +
>>> >> +	/*
>>> >> +	 * References fetched before the tag backfill aren't yet committed
>>> >> +	 * to disk, so negotiation cannot discover them. Advertise them to
>>> >> +	 * the server, so that it does not re-send objects we already have.
>>> >> +	 */
>>> >> +	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
>>> >> +		struct oid_array **include_tips =
>>> >> +			&transport->smart_options->negotiation_include_tips;
>>> >> +		if (!*include_tips)
>>> >> +			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
>>> >> +		for (size_t i = 0; i < fetched_tips->nr; i++)
>>> >> +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
>>> >> +	}
>>> >
>>> > Curious. Why don't we have `struct oid_array` as a local variable and
>>> > then simply `transport->smart_options->negotiation_include_tips =
>>> > &extra_tips`?
>>> >
>>>
>>> Because `transport->smart_options->negotiation_include_tips` may already
>>> be set when this function is called, so we only free it when we are the
>>> exclusive setters of the field.
>>
>> Ah, okay. But if it was set already, what does it contain? And is it
>> okay that we essentially leak the fetched tips to our callers now if it
>> was set?
>>
>> Patrick
>
> So it is set when the user uses the '--negotiation-include' option and
> provides their own tips.
>
> When I wrote this code, I didn't include the free'ing that you see here.
> But it triggered the leak sanitizer for 't5509-fetch-push-namespaces'. I
> assumed it was due to my code and added this logic, without digging too
> much into why my path specifically triggered it.
>
> So, the transport does free the `negotiation_include_tips`, but only for
> transports which supports smart_options. Meaning, we should only add the
> tips for transports with smart options. As the other route doesn't use
> it and as such doesn't free the array.
>
> Thanks for the pushback, I'll fix it up in the next version.
>

I need to not send emails in the middle of the night. Seems like I'm
still wrong here.

The issue seems to be that for 't5509-fetch-push-namespaces' the tests
which trigger the leaks are those which use a remote helper and seems
like the transport with remote-helpers don't clean up correctly. So
while they support smart options, they don't do the necessary cleanup.

Seems like an existing bug, will add a precursor commit to fix it.

Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v5 0/3] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

When fetching, git-fetch(1) performs the reference updates in two
steps:
1. Fetches the regular references
2. Backfills any tags

With batched updates, both of these operations came under the same
reference transaction. Unfortunately, this meant that the initial
references fetched were not committed and couldn't be used as 'haves'
during the negotiation when backfilling tags. In shallow repositories,
this could mean that the server was sending objects the client already
possessed. This series fixes this regression.

Earlier versions (1-3) tried to fix this by splitting the transaction
into two. Which works, but makes the code more complex and is a bit
messy overall as we have to deal with errors and also jumps in the
codeflow.

Justin suggested that we could use the references already available for
negotiation instead, that is what v4 onwards does.

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v5:
- Add a patch fixing a pre-existing leak in the remote-helper
  transport's cleanup. This allows the patch to drop its manual cleanup.
- Move the upstream-info test into its own commit.
- Some cleanpus around the commit message and grammar
- Link to v4: https://patch.msgid.link/20261022-799-shallow-fetch-with-tags-v4-1-8a1c9ef399ed@gmail.com

Changes in v4:
- Instead of committing the first transaction and starting a new one,
  advertise the refs already fetched when backfilling tags. Thanks to
  Justin for the suggestion. This also fixes the `--atomic` flow.
- Link to v3: https://patch.msgid.link/20261015-799-shallow-fetch-with-tags-v3-1-90b6e38b98d2@gmail.com

Changes in v3:
- Fix the early exit, so a failed transaction_begin() only skips the
  backfill.
- Keep the first commit's result separately, so rejections don't skip
  the upstream and remote-HEAD setup.
- Test that the backfill advertises the fetched tip as a have, instead
  of checking that the root object isn't fetched, which passed even
  without the fix.
- Add a test for the upstream setup when a main ref is rejected and a
  tag is backfilled.
- Link to v2: https://patch.msgid.link/20261012-799-shallow-fetch-with-tags-v2-1-4211e9a13d3c@gmail.com

Changes in v2:
- Add benchmarks between master and this branch to show perf regression.
- Instead of checking the trace file, simply check that we didn't fetch
  older objects in the test.
- Link to v1: https://patch.msgid.link/20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com

CC: Patrick Steinhardt <ps@pks.im>
CC: Justin Tobler <jltobler@gmail.com>

---
Karthik Nayak (3):
      transport: add `transport_options_release()`
      fetch: advertise fetched refs when backfilling tags
      t5510: cover upstream info update on fetch failure

 builtin/fetch.c    | 33 ++++++++++++++++++++++++++++--
 t/t5510-fetch.sh   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c |  1 +
 transport.c        | 24 +++++++++++++---------
 transport.h        |  2 ++
 5 files changed, 107 insertions(+), 12 deletions(-)

Range-diff versus v4:

-:  ---------- > 1:  583369ce44 transport: add `transport_options_release()`
1:  8e21cf3377 ! 2:  3a53029bb1 fetch: advertise fetched refs when backfilling tags
    @@ Commit message
         `negotiation_include_tips` field. This also fixes the issue for
         `--atomic` fetches since it follows the same route.
     
    -    Add three tests, two which validates that the 'haves' are filled when
    -    backfilling tags for atomic and non-atomic flows. The third, to test
    -    that upstream is still set and tags are backfilled even if a sibling ref
    -    is rejected.
    +    This does mean that all references fetched before the backfill will be
    +    used for the negotiation. While duplicate OIDs are trimmed before
    +    negotiation, we could still end up with a lot of distinct OIDs used in
    +    the 'haves' section. However, this is still better than the server
    +    transferring already existing objects to the client, which can take a
    +    lot more time.
    +
    +    Add two tests, which validate that the 'haves' are filled when
    +    backfilling tags for atomic and non-atomic flows.
     
         Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ builtin/fetch.c: static int backfill_tags(struct display_state *display_state,
     +			 struct list_objects_filter_options *filter_options,
     +			 const struct oid_array *fetched_tips)
      {
    -+	struct oid_array *to_free = NULL;
      	int retcode, cannot_reuse;
      
    - 	/*
     @@ builtin/fetch.c: static int backfill_tags(struct display_state *display_state,
      	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
      	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
    @@ builtin/fetch.c: static int backfill_tags(struct display_state *display_state,
     +		struct oid_array **include_tips =
     +			&transport->smart_options->negotiation_include_tips;
     +		if (!*include_tips)
    -+			*include_tips = to_free = xcalloc(1, sizeof(**include_tips));
    ++			*include_tips = xcalloc(1, sizeof(**include_tips));
     +		for (size_t i = 0; i < fetched_tips->nr; i++)
     +			oid_array_append(*include_tips, &fetched_tips->oid[i]);
     +	}
    @@ builtin/fetch.c: static int backfill_tags(struct display_state *display_state,
      	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
      					 fetch_head, config, display_array);
      
    -+	if (to_free) {
    -+		oid_array_clear(to_free);
    -+		free(to_free);
    -+		transport->smart_options->negotiation_include_tips = NULL;
    -+	}
    -+
    - 	if (gsecondary) {
    - 		transport_disconnect(gsecondary);
    - 		gsecondary = NULL;
     @@ builtin/fetch.c: static int do_fetch(struct transport *transport,
      
      		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
      		if (tags_ref_map) {
     +			struct oid_array fetched_tips = OID_ARRAY_INIT;
    -+			struct ref *rm;
     +
     +			/*
     +			 * Collect the tips we just fetched so the tag backfill
     +			 * can advertise them to the server as 'haves'.
     +			 */
    -+			for (rm = ref_map; rm; rm = rm->next)
    ++			for (struct ref *rm = ref_map; rm; rm = rm->next)
     +				if (!is_null_oid(&rm->old_oid))
     +					oid_array_append(&fetched_tips, &rm->old_oid);
     +
    @@ t/t5510-fetch.sh: 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 &&
      
    -@@ t/t5510-fetch.sh: test_expect_success REFFILES "upstream tracking info is added even with conflict
    - 	)
    - '
    - 
    -+test_expect_success REFFILES "upstream tracking info is added with conflicts and backfilled tags" '
    -+	test_when_finished rm -rf base repo &&
    -+
    -+	git init --initial-branch=main base &&
    -+	test_commit -C base one &&
    -+	test_commit -C base two &&
    -+	git -C base branch foo &&
    -+	# Backfilling only takes place for tags which are not pointing at tips.
    -+	git -C base tag -a annotated -m annotated HEAD~1 &&
    -+
    -+	git init --bare --initial-branch=main repo &&
    -+	(
    -+		cd repo &&
    -+		git remote add origin ../base &&
    -+		mkdir -p refs/remotes/origin &&
    -+		>refs/remotes/origin/foo.lock &&
    -+		test_must_fail git fetch origin --set-upstream main \
    -+			"+refs/heads/*:refs/remotes/origin/*" &&
    -+		git config get branch.main.remote >actual &&
    -+		echo "origin" >expect &&
    -+		test_cmp expect actual &&
    -+		git rev-parse --verify refs/tags/annotated
    -+	)
    -+'
    -+
    - test_expect_success REFFILES "HEAD is updated even with conflicts" '
    - 	test_when_finished rm -rf base repo &&
    - 
-:  ---------- > 3:  e8a5ce9e5e t5510: cover upstream info update on fetch failure

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

[PATCH v5 1/3] transport: add `transport_options_release()`

[Karthik Nayak]

The `git_transport_options` structure is used both in 'transport.c' and
'transport-helper.c'. But while the former specifically clears any
associated data in the structure during the cleanup of the transport,
the latter misses this.

This means when using remote-helpers, data allocated for the structure
leaks. Introduce a new `transport_options_release()`, which acts as a
common path to release memory allocated with the structure and use it in
both files.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 transport-helper.c |  1 +
 transport.c        | 24 ++++++++++++++----------
 transport.h        |  2 ++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index d5a064d386..14d179e0f6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -401,6 +401,7 @@ static int release_helper(struct transport *transport)
 	refspec_clear(&data->rs);
 	free(data->import_marks);
 	free(data->export_marks);
+	transport_options_release(&data->transport_options);
 	res = disconnect_helper(transport);
 	free(transport->data);
 	return res;
diff --git a/transport.c b/transport.c
index 25e2c14a7b..4491443b28 100644
--- a/transport.c
+++ b/transport.c
@@ -1017,6 +1017,19 @@ static int connect_git(struct transport *transport,
 	return 0;
 }
 
+void transport_options_release(struct git_transport_options *options)
+{
+	if (options->negotiation_restrict_tips) {
+		oid_array_clear(options->negotiation_restrict_tips);
+		free(options->negotiation_restrict_tips);
+	}
+	if (options->negotiation_include_tips) {
+		oid_array_clear(options->negotiation_include_tips);
+		free(options->negotiation_include_tips);
+	}
+	list_objects_filter_release(&options->filter_options);
+}
+
 static int disconnect_git(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
@@ -1028,16 +1041,7 @@ static int disconnect_git(struct transport *transport)
 			close(data->fd[1]);
 		finish_connect(data->conn);
 	}
-
-	if (data->options.negotiation_restrict_tips) {
-		oid_array_clear(data->options.negotiation_restrict_tips);
-		free(data->options.negotiation_restrict_tips);
-	}
-	if (data->options.negotiation_include_tips) {
-		oid_array_clear(data->options.negotiation_include_tips);
-		free(data->options.negotiation_include_tips);
-	}
-	list_objects_filter_release(&data->options.filter_options);
+	transport_options_release(&data->options);
 	oid_array_clear(&data->extra_have);
 	oid_array_clear(&data->shallow);
 	free(data);
diff --git a/transport.h b/transport.h
index 39193d0077..cb262ee3da 100644
--- a/transport.h
+++ b/transport.h
@@ -59,6 +59,8 @@ struct git_transport_options {
 	struct oidset *acked_commits;
 };
 
+void transport_options_release(struct git_transport_options *options);
+
 enum transport_family {
 	TRANSPORT_FAMILY_ALL = 0,
 	TRANSPORT_FAMILY_IPV4,

-- 
2.56.GIT

[PATCH v5 2/3] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Batched updates are used both for the fetched
references and to backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. For shallow fetches, the negotiation
phase while backfilling tags does not advertise the newly fetched refs.
This can cause the server to send objects the client already has.

The issue always existed when the '--atomic' flag was used, as both the
regular ref updates and the tags need to be part of the same
transaction.

Fix this by advertising the already fetched refs as 'haves' during the
negotiation. Do this by simply adding the tips to the transport's
`negotiation_include_tips` field. This also fixes the issue for
`--atomic` fetches since it follows the same route.

This does mean that all references fetched before the backfill will be
used for the negotiation. While duplicate OIDs are trimmed before
negotiation, we could still end up with a lot of distinct OIDs used in
the 'haves' section. However, this is still better than the server
transferring already existing objects to the client, which can take a
lot more time.

Add two tests, which validate that the 'haves' are filled when
backfilling tags for atomic and non-atomic flows.

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c  | 33 +++++++++++++++++++++++++++++++--
 t/t5510-fetch.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..d3974faf6b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1676,7 +1676,8 @@ static int backfill_tags(struct display_state *display_state,
 			 struct fetch_head *fetch_head,
 			 const struct fetch_config *config,
 			 struct ref_update_display_info_array *display_array,
-			 struct list_objects_filter_options *filter_options)
+			 struct list_objects_filter_options *filter_options,
+			 const struct oid_array *fetched_tips)
 {
 	int retcode, cannot_reuse;
 
@@ -1697,6 +1698,21 @@ static int backfill_tags(struct display_state *display_state,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
+
+	/*
+	 * References fetched before the tag backfill aren't yet committed
+	 * to disk, so negotiation cannot discover them. Advertise them to
+	 * the server, so that it does not re-send objects we already have.
+	 */
+	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
+		struct oid_array **include_tips =
+			&transport->smart_options->negotiation_include_tips;
+		if (!*include_tips)
+			*include_tips = xcalloc(1, sizeof(**include_tips));
+		for (size_t i = 0; i < fetched_tips->nr; i++)
+			oid_array_append(*include_tips, &fetched_tips->oid[i]);
+	}
+
 	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
 					 fetch_head, config, display_array);
 
@@ -2077,6 +2093,16 @@ static int do_fetch(struct transport *transport,
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
 		if (tags_ref_map) {
+			struct oid_array fetched_tips = OID_ARRAY_INIT;
+
+			/*
+			 * Collect the tips we just fetched so the tag backfill
+			 * can advertise them to the server as 'haves'.
+			 */
+			for (struct ref *rm = ref_map; rm; rm = rm->next)
+				if (!is_null_oid(&rm->old_oid))
+					oid_array_append(&fetched_tips, &rm->old_oid);
+
 			/*
 			 * If backfilling of tags fails then we want to tell
 			 * the user so, but we have to continue regardless to
@@ -2086,8 +2112,11 @@ 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, &display_array, filter_options))
+					  &fetch_head, config, &display_array, filter_options,
+					  &fetched_tips))
 				retcode = 1;
+
+			oid_array_clear(&fetched_tips);
 		}
 
 		free_refs(tags_ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..7c1f8405df 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,40 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'backfilling tags advertises fetched refs as haves' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
+test_expect_success 'backfilling tags advertises fetched refs as haves even with atomic' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --atomic --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 

-- 
2.56.GIT

[PATCH v5 3/3] t5510: cover upstream info update on fetch failure

[Karthik Nayak]

git-fetch(1) performs reference updates in a single batched transaction,
so individual updates can be rejected while the rest succeed. Even in
that partial-failure case the command still performs the remainder of
its work, such as writing the upstream tracking configuration and
backfilling tags.

We have no test asserting that the upstream tracking information is
written when one of the reference updates is rejected. Add one.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 t/t5510-fetch.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 7c1f8405df..ca5f9642fc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -2039,6 +2039,31 @@ test_expect_success REFFILES "upstream tracking info is added even with conflict
 	)
 '
 
+test_expect_success REFFILES "upstream tracking info is added with conflicts and backfilled tags" '
+	test_when_finished rm -rf base repo &&
+
+	git init --initial-branch=main base &&
+	test_commit -C base one &&
+	test_commit -C base two &&
+	git -C base branch foo &&
+	# Backfilling only takes place for tags which are not pointing at tips.
+	git -C base tag -a annotated -m annotated HEAD~1 &&
+
+	git init --bare --initial-branch=main repo &&
+	(
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir -p refs/remotes/origin &&
+		>refs/remotes/origin/foo.lock &&
+		test_must_fail git fetch origin --set-upstream main \
+			"+refs/heads/*:refs/remotes/origin/*" &&
+		git config get branch.main.remote >actual &&
+		echo "origin" >expect &&
+		test_cmp expect actual &&
+		git rev-parse --verify refs/tags/annotated
+	)
+'
+
 test_expect_success REFFILES "HEAD is updated even with conflicts" '
 	test_when_finished rm -rf base repo &&
 

-- 
2.56.GIT

Re: [PATCH v5 1/3] transport: add `transport_options_release()`

[Patrick Steinhardt]

On Wed, Oct 28, 2026 at 10:47:59AM +0100, Karthik Nayak wrote:
> The `git_transport_options` structure is used both in 'transport.c' and
> 'transport-helper.c'. But while the former specifically clears any
> associated data in the structure during the cleanup of the transport,
> the latter misses this.
> 
> This means when using remote-helpers, data allocated for the structure
> leaks. Introduce a new `transport_options_release()`, which acts as a

Nit: "a new transport_options_release" reads a bit weird. I'd either add
"function", or drop "a new".

> common path to release memory allocated with the structure and use it in
> both files.

Why didn't our CI detect this leak? Don't we have tests exercising this?

The patch itself looks obviously good to me, thanks.

Patrick

Re: [PATCH v5 3/3] t5510: cover upstream info update on fetch failure

[Patrick Steinhardt]

On Wed, Oct 28, 2026 at 10:48:01AM +0100, Karthik Nayak wrote:
> git-fetch(1) performs reference updates in a single batched transaction,
> so individual updates can be rejected while the rest succeed. Even in
> that partial-failure case the command still performs the remainder of
> its work, such as writing the upstream tracking configuration and
> backfilling tags.
> 
> We have no test asserting that the upstream tracking information is
> written when one of the reference updates is rejected. Add one.

Very pedantic nit: I would have ordered this commit before the preceding
one to demonstrate that this already worked beforehand.

Patrick

Re: [PATCH v5 1/3] transport: add `transport_options_release()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Oct 28, 2026 at 10:47:59AM +0100, Karthik Nayak wrote:
>> The `git_transport_options` structure is used both in 'transport.c' and
>> 'transport-helper.c'. But while the former specifically clears any
>> associated data in the structure during the cleanup of the transport,
>> the latter misses this.
>>
>> This means when using remote-helpers, data allocated for the structure
>> leaks. Introduce a new `transport_options_release()`, which acts as a
>
> Nit: "a new transport_options_release" reads a bit weird. I'd either add
> "function", or drop "a new".
>

Fair enough, I'll drop the 'a new'.

>> common path to release memory allocated with the structure and use it in
>> both files.
>
> Why didn't our CI detect this leak? Don't we have tests exercising this?
>
> The patch itself looks obviously good to me, thanks.
>
> Patrick

We don't have any tests which exercise this it seems. The tests in the
next patch do, which is why it hit the bug!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v5 3/3] t5510: cover upstream info update on fetch failure

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Oct 28, 2026 at 10:48:01AM +0100, Karthik Nayak wrote:
>> git-fetch(1) performs reference updates in a single batched transaction,
>> so individual updates can be rejected while the rest succeed. Even in
>> that partial-failure case the command still performs the remainder of
>> its work, such as writing the upstream tracking configuration and
>> backfilling tags.
>>
>> We have no test asserting that the upstream tracking information is
>> written when one of the reference updates is rejected. Add one.
>
> Very pedantic nit: I would have ordered this commit before the preceding
> one to demonstrate that this already worked beforehand.
>
> Patrick

Fair. Seeing that both comments are nits and minor, I'll make the
changes locally and not re-roll unless there are other comments.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v5 1/3] transport: add `transport_options_release()`

[Patrick Steinhardt]

On Wed, Oct 28, 2026 at 04:18:11PM -0400, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > On Wed, Oct 28, 2026 at 10:47:59AM +0100, Karthik Nayak wrote:
> >> The `git_transport_options` structure is used both in 'transport.c' and
> >> 'transport-helper.c'. But while the former specifically clears any
> >> associated data in the structure during the cleanup of the transport,
> >> the latter misses this.
> >>
> >> This means when using remote-helpers, data allocated for the structure
> >> leaks. Introduce a new `transport_options_release()`, which acts as a
> >
> > Nit: "a new transport_options_release" reads a bit weird. I'd either add
> > "function", or drop "a new".
> >
> 
> Fair enough, I'll drop the 'a new'.
> 
> >> common path to release memory allocated with the structure and use it in
> >> both files.
> >
> > Why didn't our CI detect this leak? Don't we have tests exercising this?
> >
> > The patch itself looks obviously good to me, thanks.
> >
> > Patrick
> 
> We don't have any tests which exercise this it seems. The tests in the
> next patch do, which is why it hit the bug!

That would have been great to point out in the commit message :) Thanks
for clarifying!

Patrick

Re: [PATCH v5 1/3] transport: add `transport_options_release()`

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Oct 28, 2026 at 04:18:11PM -0400, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > On Wed, Oct 28, 2026 at 10:47:59AM +0100, Karthik Nayak wrote:
>> >> The `git_transport_options` structure is used both in 'transport.c' and
>> >> 'transport-helper.c'. But while the former specifically clears any
>> >> associated data in the structure during the cleanup of the transport,
>> >> the latter misses this.
>> >>
>> >> This means when using remote-helpers, data allocated for the structure
>> >> leaks. Introduce a new `transport_options_release()`, which acts as a
>> >
>> > Nit: "a new transport_options_release" reads a bit weird. I'd either add
>> > "function", or drop "a new".
>> >
>>
>> Fair enough, I'll drop the 'a new'.
>>
>> >> common path to release memory allocated with the structure and use it in
>> >> both files.
>> >
>> > Why didn't our CI detect this leak? Don't we have tests exercising this?
>> >
>> > The patch itself looks obviously good to me, thanks.
>> >
>> > Patrick
>>
>> We don't have any tests which exercise this it seems. The tests in the
>> next patch do, which is why it hit the bug!
>
> That would have been great to point out in the commit message :) Thanks
> for clarifying!
>
> Patrick

Let me push a new version with all these fixes. Thanks for the review

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v6 0/3] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

The issue was reported by Mitja Bezenšek on GitLab's Git repo [1].

When fetching, git-fetch(1) performs the reference updates in two
steps:
1. Fetches the regular references
2. Backfills any tags

With batched updates, both of these operations came under the same
reference transaction. Unfortunately, this meant that the initial
references fetched were not committed and couldn't be used as 'haves'
during the negotiation when backfilling tags. In shallow repositories,
this could mean that the server was sending objects the client already
possessed. This series fixes this regression.

Earlier versions (1-3) tried to fix this by splitting the transaction
into two. Which works, but makes the code more complex and is a bit
messy overall as we have to deal with errors and also jumps in the
codeflow.

Justin suggested that we could use the references already available for
negotiation instead, that is what v4 onwards does.

[1]: https://gitlab.com/gitlab-org/git/-/work_items/799

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v6:
- Move the last commit to the front, since it adds a test to exercise
  existing behavior.
- Fix grammar in the commit message and mention how the next commit
  would catch the leak fixed in 2/3.
- Link to v5: https://patch.msgid.link/20261028-799-shallow-fetch-with-tags-v5-0-7c43cc9ae810@gmail.com

Changes in v5:
- Add a patch fixing a pre-existing leak in the remote-helper
  transport's cleanup. This allows the patch to drop its manual cleanup.
- Move the upstream-info test into its own commit.
- Some cleanpus around the commit message and grammar
- Link to v4: https://patch.msgid.link/20261022-799-shallow-fetch-with-tags-v4-1-8a1c9ef399ed@gmail.com

Changes in v4:
- Instead of committing the first transaction and starting a new one,
  advertise the refs already fetched when backfilling tags. Thanks to
  Justin for the suggestion. This also fixes the `--atomic` flow.
- Link to v3: https://patch.msgid.link/20261015-799-shallow-fetch-with-tags-v3-1-90b6e38b98d2@gmail.com

Changes in v3:
- Fix the early exit, so a failed transaction_begin() only skips the
  backfill.
- Keep the first commit's result separately, so rejections don't skip
  the upstream and remote-HEAD setup.
- Test that the backfill advertises the fetched tip as a have, instead
  of checking that the root object isn't fetched, which passed even
  without the fix.
- Add a test for the upstream setup when a main ref is rejected and a
  tag is backfilled.
- Link to v2: https://patch.msgid.link/20261012-799-shallow-fetch-with-tags-v2-1-4211e9a13d3c@gmail.com

Changes in v2:
- Add benchmarks between master and this branch to show perf regression.
- Instead of checking the trace file, simply check that we didn't fetch
  older objects in the test.
- Link to v1: https://patch.msgid.link/20261009-799-shallow-fetch-with-tags-v1-1-379d61504af5@gmail.com

CC: Patrick Steinhardt <ps@pks.im>
CC: Justin Tobler <jltobler@gmail.com>

---
Karthik Nayak (3):
      t5510: cover upstream info update on fetch failure
      transport: add `transport_options_release()`
      fetch: advertise fetched refs when backfilling tags

 builtin/fetch.c    | 33 ++++++++++++++++++++++++++++--
 t/t5510-fetch.sh   | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 transport-helper.c |  1 +
 transport.c        | 24 +++++++++++++---------
 transport.h        |  2 ++
 5 files changed, 107 insertions(+), 12 deletions(-)

Range-diff versus v5:

3:  ed24854913 = 1:  6aafee69c1 t5510: cover upstream info update on fetch failure
1:  f15e69e2e6 ! 2:  a22b348247 transport: add `transport_options_release()`
    @@ Commit message
         the latter misses this.
     
         This means when using remote-helpers, data allocated for the structure
    -    leaks. Introduce a new `transport_options_release()`, which acts as a
    -    common path to release memory allocated with the structure and use it in
    -    both files.
    +    leaks. None of the existing tests capture this leak, but the tests
    +    introduced in the next patch do. Fix the leak by introducing
    +    `transport_options_release()`, which acts as a common path to release
    +    memory allocated with the structure and use it in both files.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
2:  dc7024fafb = 3:  ebe210d2df fetch: advertise fetched refs when backfilling tags

---
base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
change-id: 20261007-799-shallow-fetch-with-tags-d62b3efb29f0


Thanks
- Karthik

[PATCH v6 1/3] t5510: cover upstream info update on fetch failure

[Karthik Nayak]

git-fetch(1) performs reference updates in a single batched transaction,
so individual updates can be rejected while the rest succeed. Even in
that partial-failure case the command still performs the remainder of
its work, such as writing the upstream tracking configuration and
backfilling tags.

We have no test asserting that the upstream tracking information is
written when one of the reference updates is rejected. Add one.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 t/t5510-fetch.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 300bd5396d..30871617f9 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -2005,6 +2005,31 @@ test_expect_success REFFILES "upstream tracking info is added even with conflict
 	)
 '
 
+test_expect_success REFFILES "upstream tracking info is added with conflicts and backfilled tags" '
+	test_when_finished rm -rf base repo &&
+
+	git init --initial-branch=main base &&
+	test_commit -C base one &&
+	test_commit -C base two &&
+	git -C base branch foo &&
+	# Backfilling only takes place for tags which are not pointing at tips.
+	git -C base tag -a annotated -m annotated HEAD~1 &&
+
+	git init --bare --initial-branch=main repo &&
+	(
+		cd repo &&
+		git remote add origin ../base &&
+		mkdir -p refs/remotes/origin &&
+		>refs/remotes/origin/foo.lock &&
+		test_must_fail git fetch origin --set-upstream main \
+			"+refs/heads/*:refs/remotes/origin/*" &&
+		git config get branch.main.remote >actual &&
+		echo "origin" >expect &&
+		test_cmp expect actual &&
+		git rev-parse --verify refs/tags/annotated
+	)
+'
+
 test_expect_success REFFILES "HEAD is updated even with conflicts" '
 	test_when_finished rm -rf base repo &&
 

-- 
2.56.GIT

[PATCH v6 2/3] transport: add `transport_options_release()`

[Karthik Nayak]

The `git_transport_options` structure is used both in 'transport.c' and
'transport-helper.c'. But while the former specifically clears any
associated data in the structure during the cleanup of the transport,
the latter misses this.

This means when using remote-helpers, data allocated for the structure
leaks. None of the existing tests capture this leak, but the tests
introduced in the next patch do. Fix the leak by introducing
`transport_options_release()`, which acts as a common path to release
memory allocated with the structure and use it in both files.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 transport-helper.c |  1 +
 transport.c        | 24 ++++++++++++++----------
 transport.h        |  2 ++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index d5a064d386..14d179e0f6 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -401,6 +401,7 @@ static int release_helper(struct transport *transport)
 	refspec_clear(&data->rs);
 	free(data->import_marks);
 	free(data->export_marks);
+	transport_options_release(&data->transport_options);
 	res = disconnect_helper(transport);
 	free(transport->data);
 	return res;
diff --git a/transport.c b/transport.c
index 25e2c14a7b..4491443b28 100644
--- a/transport.c
+++ b/transport.c
@@ -1017,6 +1017,19 @@ static int connect_git(struct transport *transport,
 	return 0;
 }
 
+void transport_options_release(struct git_transport_options *options)
+{
+	if (options->negotiation_restrict_tips) {
+		oid_array_clear(options->negotiation_restrict_tips);
+		free(options->negotiation_restrict_tips);
+	}
+	if (options->negotiation_include_tips) {
+		oid_array_clear(options->negotiation_include_tips);
+		free(options->negotiation_include_tips);
+	}
+	list_objects_filter_release(&options->filter_options);
+}
+
 static int disconnect_git(struct transport *transport)
 {
 	struct git_transport_data *data = transport->data;
@@ -1028,16 +1041,7 @@ static int disconnect_git(struct transport *transport)
 			close(data->fd[1]);
 		finish_connect(data->conn);
 	}
-
-	if (data->options.negotiation_restrict_tips) {
-		oid_array_clear(data->options.negotiation_restrict_tips);
-		free(data->options.negotiation_restrict_tips);
-	}
-	if (data->options.negotiation_include_tips) {
-		oid_array_clear(data->options.negotiation_include_tips);
-		free(data->options.negotiation_include_tips);
-	}
-	list_objects_filter_release(&data->options.filter_options);
+	transport_options_release(&data->options);
 	oid_array_clear(&data->extra_have);
 	oid_array_clear(&data->shallow);
 	free(data);
diff --git a/transport.h b/transport.h
index 39193d0077..cb262ee3da 100644
--- a/transport.h
+++ b/transport.h
@@ -59,6 +59,8 @@ struct git_transport_options {
 	struct oidset *acked_commits;
 };
 
+void transport_options_release(struct git_transport_options *options);
+
 enum transport_family {
 	TRANSPORT_FAMILY_ALL = 0,
 	TRANSPORT_FAMILY_IPV4,

-- 
2.56.GIT

[PATCH v6 3/3] fetch: advertise fetched refs when backfilling tags

[Karthik Nayak]

In 0e358de64a (fetch: use batched reference updates, 2025-05-19), the
fetch code was modified to use batched updates to provide a good
performance improvement. Batched updates are used both for the fetched
references and to backfill tags.

When using batched updates, the references aren't yet committed to disk
when we start backfilling tags. For shallow fetches, the negotiation
phase while backfilling tags does not advertise the newly fetched refs.
This can cause the server to send objects the client already has.

The issue always existed when the '--atomic' flag was used, as both the
regular ref updates and the tags need to be part of the same
transaction.

Fix this by advertising the already fetched refs as 'haves' during the
negotiation. Do this by simply adding the tips to the transport's
`negotiation_include_tips` field. This also fixes the issue for
`--atomic` fetches since it follows the same route.

This does mean that all references fetched before the backfill will be
used for the negotiation. While duplicate OIDs are trimmed before
negotiation, we could still end up with a lot of distinct OIDs used in
the 'haves' section. However, this is still better than the server
transferring already existing objects to the client, which can take a
lot more time.

Add two tests, which validate that the 'haves' are filled when
backfilling tags for atomic and non-atomic flows.

Reported-by: Mitja Bezenšek <mitja.bezensek@login5.org>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 builtin/fetch.c  | 33 +++++++++++++++++++++++++++++++--
 t/t5510-fetch.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b2decc6cfd..d3974faf6b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1676,7 +1676,8 @@ static int backfill_tags(struct display_state *display_state,
 			 struct fetch_head *fetch_head,
 			 const struct fetch_config *config,
 			 struct ref_update_display_info_array *display_array,
-			 struct list_objects_filter_options *filter_options)
+			 struct list_objects_filter_options *filter_options,
+			 const struct oid_array *fetched_tips)
 {
 	int retcode, cannot_reuse;
 
@@ -1697,6 +1698,21 @@ static int backfill_tags(struct display_state *display_state,
 	transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
 	transport_set_option(transport, TRANS_OPT_DEPTH, "0");
 	transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);
+
+	/*
+	 * References fetched before the tag backfill aren't yet committed
+	 * to disk, so negotiation cannot discover them. Advertise them to
+	 * the server, so that it does not re-send objects we already have.
+	 */
+	if (fetched_tips && fetched_tips->nr && transport->smart_options) {
+		struct oid_array **include_tips =
+			&transport->smart_options->negotiation_include_tips;
+		if (!*include_tips)
+			*include_tips = xcalloc(1, sizeof(**include_tips));
+		for (size_t i = 0; i < fetched_tips->nr; i++)
+			oid_array_append(*include_tips, &fetched_tips->oid[i]);
+	}
+
 	retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map,
 					 fetch_head, config, display_array);
 
@@ -2077,6 +2093,16 @@ static int do_fetch(struct transport *transport,
 
 		find_non_local_tags(remote_refs, transaction, &tags_ref_map, &tail);
 		if (tags_ref_map) {
+			struct oid_array fetched_tips = OID_ARRAY_INIT;
+
+			/*
+			 * Collect the tips we just fetched so the tag backfill
+			 * can advertise them to the server as 'haves'.
+			 */
+			for (struct ref *rm = ref_map; rm; rm = rm->next)
+				if (!is_null_oid(&rm->old_oid))
+					oid_array_append(&fetched_tips, &rm->old_oid);
+
 			/*
 			 * If backfilling of tags fails then we want to tell
 			 * the user so, but we have to continue regardless to
@@ -2086,8 +2112,11 @@ 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, &display_array, filter_options))
+					  &fetch_head, config, &display_array, filter_options,
+					  &fetched_tips))
 				retcode = 1;
+
+			oid_array_clear(&fetched_tips);
 		}
 
 		free_refs(tags_ref_map);
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
index 30871617f9..ca5f9642fc 100755
--- a/t/t5510-fetch.sh
+++ b/t/t5510-fetch.sh
@@ -1942,6 +1942,40 @@ test_expect_success "backfill tags when providing a refspec" '
 	test_cmp expect actual
 '
 
+test_expect_success 'backfilling tags advertises fetched refs as haves' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
+test_expect_success 'backfilling tags advertises fetched refs as haves even with atomic' '
+	test_when_finished rm -rf source target trace &&
+
+	git init source &&
+	test_commit_bulk -C source 10 &&
+	git -C source tag -a tag -m tag HEAD~2 &&
+	HEAD_OID=$(git -C source rev-parse HEAD) &&
+
+	git init target &&
+	git -C target remote add origin ../source &&
+	GIT_TEST_PROTOCOL_VERSION=2 GIT_TRACE_PACKET=$(pwd)/trace \
+		git -C target fetch --atomic --depth 5 origin &&
+
+	test_grep "fetch> have $HEAD_OID" trace &&
+	git -C target rev-parse --verify refs/tags/tag
+'
+
 test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" '
 	test_when_finished rm -rf base repo &&
 

-- 
2.56.GIT

Re: [PATCH v6 0/3] fetch: advertise fetched refs when backfilling tags

[Patrick Steinhardt]

On Thu, Oct 29, 2026 at 01:45:36PM +0100, Karthik Nayak wrote:
> Changes in v6:
> - Move the last commit to the front, since it adds a test to exercise
>   existing behavior.
> - Fix grammar in the commit message and mention how the next commit
>   would catch the leak fixed in 2/3.
> - Link to v5: https://patch.msgid.link/20261028-799-shallow-fetch-with-tags-v5-0-7c43cc9ae810@gmail.com

Thanks, I'm happy with these changes.

Patrick