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

[Patrick Steinhardt <ps@pks.im>]

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.
> 
> Let callers provide an optional array of expected old OIDs in parallel with
> the refname list. Delete the ref at position N only if it still points at
> the OID at position N. Treat a null OID as an unconditional deletion in
> ref_transaction_delete(), allowing callers to include broken refs whose old
> value cannot be resolved.
> 
> 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?

> diff --git a/refs.c b/refs.c
> index 92d5df5b7..13ee2d459 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1523,7 +1524,7 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>  			   struct strbuf *err)
>  {
>  	if (old_oid && is_null_oid(old_oid))
> -		BUG("delete called with old_oid set to zeros");
> +		old_oid = NULL;
>  	if (old_oid && old_target)
>  		BUG("delete called with both old_oid and old_target set");
>  	if (old_target && !(flags & REF_NO_DEREF))

I'm not a huge fan of starting to treat a null OID as something other
than "this branch should not exist". Everywhere else it still does, so
mixing this feels fishy to me.

Also, this change wouldn't have to exist if we instead started to drive
a proper transaction.

> @@ -3069,39 +3070,73 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio
>  	}
>  }
>  
> +struct delete_refs_rejection_data {
> +	int failures;
> +	struct string_list *failed_refs;
> +};
> +
> +static void delete_refs_rejection_handler(const char *refname,
> +					  const struct object_id *old_oid UNUSED,
> +					  const struct object_id *new_oid UNUSED,
> +					  const char *old_target UNUSED,
> +					  const char *new_target UNUSED,
> +					  enum ref_transaction_error err,
> +					  const char *details,
> +					  void *cb_data)
> +{
> +	struct delete_refs_rejection_data *data = cb_data;
> +
> +	warning(_("could not delete reference %s: %s"), refname,
> +		details ? details : ref_transaction_error_msg(err));
> +	data->failures++;
> +	if (data->failed_refs)
> +		string_list_insert(data->failed_refs, refname);
> +}
> +
>  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.

Patrick