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

[Patrick Steinhardt <ps@pks.im>]

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