Re: [PATCH v3 2/2] fetch: fix non-conflicting tags not being committed

From: Karthik Nayak <karthik.188@gmail.com>
Date: Nov 10, 2025, 13:23 UTC
Message-ID: <CAOLa=ZS4wJnsCffg6EcECFEzqBo_xV+dyNi5L=4iaLqcMwPphA@mail.gmail.com>
In-Reply-To: <aRGVd7L2DV4DNM-h@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 25 quoted lines

> On Sat, Nov 08, 2025 at 10:34:44PM +0100, Karthik Nayak wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 49e195199e..337ca2b0af 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -1963,6 +1963,14 @@ static int do_fetch(struct transport *transport,
>>  	}
>>
>>  cleanup:
>> +	/*
>> +	 * When using batched updates, we want to commit the non-rejected
>> +	 * updates and also handle the rejections.
>> +	 */
>> +	if (retcode > 0 && !atomic_fetch && transaction)
>> +		commit_ref_transaction(&transaction, false,
>> +				       transport->remote->name, &err);
>
> I think this needs some explanation why this condition is safe. There's
> quite a bunch of function calls and conditions that assign to it:
>
>   - `truncate_fetch_head()` only ever assigns negative. This will be
>     ignored as expected.
>
>   - `open_fetch_head()` behaves likewise.
>

Also the transaction isn't even defined until this stage.

>   - `prune_refs()` returns negative, but we then turn the return code
>     into `1`. So we'd end up calling `commit_ref_transaction()` in this
>     case, but we didn't in the previous iteration of this patch series.
>     Was this intentional?

Its basically the same, before batched updates, we would return the return code of `refs_delete_refs()` from within `prune_refs()`.

The fn `refs_delete_refs()` creates a transaction within to delete all refs, this is done because we delete refs without old OID and hence they wouldn't ever fail.

So now, when we pass our transaction to `prune_refs()`, it is also safe to commit it.

One scenario I didn't think of earlier was that we would now enable partial pruning with this change. But I would argue that this is desirable since that is how we deal with other ref updates during fetch.

Show 10 quoted lines

>
>   - `fetch_and_consume_refs()` is one of the intended cases, and it sets
>     up a positive retcode indeed.
>
>   - `backfill_tags()` behaves likewise, and was intended.
>
> So this looks good to me, with the only questionable one being
> `prune_refs()`.
>
> Patrick

Either ways, I would think that we should elaborate a little here.

Thanks, Karthik