From: Maciej Ciemborowicz Date: Thu, 24 Sep 2026 19:56:49 GMT Subject: Re: [PATCH v5 1/3] refs: allow callers to supply old OIDs for batch deletion Message-ID: In-Reply-To: > 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? Agreed. Extending refs_delete_refs() seems to be the wrong layer for this. I will try to rework the patch so that refs_delete_refs(), ref_transaction_delete(), and callers remain unchanged. Instead, the common transaction hook layer could record a separate observed old value for updates whose callers did not supply one. That value would be used only as hook input and would not set REF_HAVE_OLD or otherwise constrain the update. This should also allow me to remove the failed_refs interface and the special handling of null_oid from the current version. Thanks, - Maciej Ciemborowicz On Thu, Sep 24, 2026 at 1:07 PM Patrick Steinhardt wrote: > > 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