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

From: Junio C Hamano <[email-masked]>
Date: Wed, 12 Nov 2025 08:34:05 -0800

Karthik Nayak <[email-masked]> writes:

> Patrick Steinhardt <[email-masked]> writes:
>
>> On Tue, Nov 11, 2025 at 02:27:08PM +0100, Karthik Nayak wrote:
>>> The cleanup section is reached with `retcode` set in several scenarios:
>>>
>>>    - `truncate_fetch_head()` and `open_fetch_head()` both set `retcode`
>>>      before the transaction is created, so no commit is attempted.
>>>
>>>    - `prune_refs()` sets `retcode` after creating the transaction, so
>>>      the commit will now proceed. Before batched updates, `prune_refs()`
>>>      created its own transaction internally with all-or-nothing
>>>      semantics. This was done since all deletions were made without an
>>>      old OID, which meant they were assumed to never fail. This change
>>>      allows partial deletions to succeed, consistent with how other
>>>      reference updates behave during fetch.
>>
>> Okay, so we do have a change in behaviour for `prune_refs()`. I guess
>> the reasoning is sound, but I was wondering why we don't have a test for
>> this.
>>
>> I guess the reason is that, as you said, it should in theory always
>> succeed. But what if with the "files" backend one of the refs that we're
>> about to prune was locked? Would that be a case where we continue with
>> pruning the remaining refs now?
>>
>
> I was thinking of concurrent writes to lock the reference, and didn't
> think of a nice way to do this. Your solution works and is indeed better.
>
> I started writing the test and realized that the pruning happens before
> we create the batched updates transaction. So I was _wrong_ and there is
> no change for `prune_refs()` either, as the transaction is never defined
> at this stage. Will amend and send in a new version.

Thanks, both.  I too was wondering what was going on in that part of
the flow.