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

> 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?

>
> 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