Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Wed, Sep 23, 2026 at 11:04:40PM +0200, Maciej Ciemborowicz wrote:
>> refs_delete_refs() performs unconditional deletions, so callers cannot
>> preserve old values that they have already resolved. Consequently,
>> reference-transaction hooks see a null old OID.

I wasn't paying attention when I gave my reviews, but the above
puzzles me.

"callers cannot preserve", meaning "after deletion the values cannot
be read anymore"?  Of course, but then callers can read them
beforehand and use the stored value when calling hooks later.

Patrick, do you understand these three lines above?  I don't, and I
am asking you because below what you say mostly seems to make sense.

>> refs_delete_refs() has always promised best-effort deletion. Always use
>> REF_TRANSACTION_ALLOW_FAILURE and report rejected updates so one failure
>> does not prevent independent refs in the batch from being deleted. Let
>> callers request the exact set of failed refs when they need to report
>> partial results. This also completes the conversion that was missed when
>> batched transaction failure support was introduced.
>
> Taking a step back though... the only reason that this function really
> exists is to provide a convenience wrapper that deletes references while
> we don't care for the old state. If we want to not do that anymore and
> instead want to expect a specific old OID, is this function still the
> right function to use?
>
> In other words, shouldn't the callers instead be updated to drive their
> own transaction if they want more complex behaviour?

That is a valid question to ask.

I think the bulk deletion of refs is done via this function, so you
certainly can update those callers of it that wants to protect
references that are being updated from getting deleted with their
own transaction and remember what refs are and are not removed, but
I am not so convinced as you seem to be that adding an optional
transaction support to the existing function they all call to do so,
as the amount of the necessary call would be more or less the same.

>>  int refs_delete_refs(struct ref_store *refs, const char *logmsg,
>> -		     struct string_list *refnames, unsigned int flags)
>> +		     struct string_list *refnames,
>> +		     const struct oid_array *old_oids,
>> +		     struct string_list *failed_refs,
>> +		     unsigned int flags)
>
> And here we also have to yield failed refs now because we don't have a
> better mechanism. Same as before though, if we used a ref transaction
> we'd already have that mechanism.
>
> So overall I'm not quite on board with this change, as I think it's going
> down the wrong route. If you want more complex behaviour when deleting
> refs you should use a ref transaction, as it would already handle all of
> what you're trying to do here.

I am neutral and would need to see what the code would look like to
decide which one is more reasonable.

Thanks.