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

[Patrick Steinhardt <ps@pks.im>]

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