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

From: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
Date: Sep 24, 2026, 19:43 UTC
Message-ID: <CACQ=SRH2ZRgy9RY=kXcJFwBwkHdoqCHuCturg4B1XqRWCSD_eg@mail.gmail.com>
In-Reply-To: <CAOLa=ZTWq6eiqCwUyUhCffTn1=f9pdAip7nsYJMnaPPUuccB8g@mail.gmail.com>

On Thu, Sep 24, 2026 at 12:05 PM Karthik Nayak <karthik.188@gmail.com> wrote:

> This also completes the conversion that was missed when
> batched transaction failure support was introduced.

Agreed, that sentence is unclear. I will try to remove the batch deletion API changes, along with this sentence and the caller-specific approach.

Show 5 quoted lines

> This change is totally different from the rest of the commit, I think it
> should be a precursor with adequate explanation regarding why this is
> done and why that's okay.
>
> I'm also still of the opinion that this shouldn't be done.

I will try to leave ref_transaction_delete() and its null_oid BUG() unchanged. Instead, store the value observed for the hook in separate fields that do not set REF_HAVE_OLD and therefore do not constrain the transaction.

> I'm assuming failures is to count the number of refs which failed,
> wouldn't failed_refs->nr give us the same result?

failed_refs is optional, which is why a separate counter is needed. If I move the solution into the common transaction layer, both the failed_refs parameter and the counter should no longer be needed.

> Nit: we could inline the size_t here.

I will fix this in the next version.

Thanks for the review.
- Maciej Ciemborowicz