Re: [PATCH v5 2/3] branch, tag: retain old OIDs in batched deletions

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 24, 2026, 11:08 UTC
Message-ID: <arUEo7bzFJfOTTFY@pks.im>
In-Reply-To: <6a8401c448f527fd80c162908a2736811a723096.1790196627.git.maciej.ciemborowicz@gmail.com>

On Wed, Sep 23, 2026 at 11:04:41PM +0200, Maciej Ciemborowicz wrote:

Show 13 quoted lines

> Before 8198907795 (use delete_refs when deleting tags or branches,
> 2021-01-21), branch and tag deletion passed each resolved old OID to
> delete_ref(). This prevented the command from deleting a ref that another
> process had changed after it was inspected.
> 
> The conversion to batched deletion dropped those old OIDs. Besides making
> the deletions unconditional, this causes reference-transaction hooks to
> report zero as both the old and new OID.
> 
> Both commands still resolve the old OIDs before starting the deletion. Pass
> those values to refs_delete_refs(). This restores the old race protection
> and lets hooks receive useful old values without adding ref reads. If a ref
> changes concurrently, reject its deletion and preserve the new value.

Hm. The motivation makes sense to me, but I have to wonder whether we're approaching it on the wrong level. With your proposed changes, we're now not force-deleting the refs anymore, which is a user-visible change in behaviour.

What you're after though is to always have an old object ID available when the reference-transaction hook kicks in. But if that's the goal, shouldn't we consider whether we can instead resolve the old value during the transaction and queue that for the reftx hook, regardless of whether or not the user has asked for an old object ID? That would now cover _all_ users that modify refs without us having to update every single callsite.

Sure, strictly speaking it's a backwards-incompatible change. But we've always considered the reftx hook to be exposing internals, so we aren't all that strict about retaining its behaviour and have allowed changes in behaviour in the past. So I wouldn't mind if we adapted the hook to always yield the old object ID.

Patrick