[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 --]