Re: [PATCH 2/6] refs: attach rejection details to updates
- From
Jeff King <peff@peff.net>
- Date
- Jan 14, 2026, 17:43 UTC
- Message-ID
- <20260114174338.GE885771@coredump.intra.peff.net>
- In-Reply-To
- <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-2-f5f8b173c501@gmail.com>
On Wed, Jan 14, 2026 at 04:40:43PM +0100, Karthik Nayak wrote:
Show 6 quoted lines
> @@ -1262,6 +1264,8 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, > transaction->updates[update_idx]->refname, 0); > > transaction->updates[update_idx]->rejection_err = err; > + if (details) > + transaction->updates[update_idx]->rejection_details = xstrdup(details);
I guess this could use xstrdup_or_null(), but probably doesn't matter much either way. I do wonder if anybody actually passes a NULL value. I think in my hacky patch there were some spots that did, but here you're always setting the "err" buf (which is good, as we'll always have details then).
Show 25 quoted lines
> @@ -2657,30 +2661,35 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
> if (!initial_transaction &&
> (strset_contains(&conflicting_dirnames, dirname.buf) ||
> !refs_read_raw_ref(refs, dirname.buf, &oid, &referent,
> - &type, &ignore_errno))) {
> + &type, &ignore_errno))) {
> +
> + strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
> + dirname.buf, refname);
> +
> if (transaction && ref_transaction_maybe_set_rejected(
> transaction, *update_idx,
> - REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
> + REF_TRANSACTION_ERROR_NAME_CONFLICT, err->buf)) {
> strset_remove(&dirnames, dirname.buf);
> strset_add(&conflicting_dirnames, dirname.buf);
> - continue;
> + strbuf_reset(err);
> + goto next;
> }
>
> - strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
> - dirname.buf, refname);
> goto cleanup;
> }OK, so this is a case where we re-ordered the "err" handling so that it's available for the non-atomic case. Makes sense. We end up formatting into err, then copying it via xstrdup(), and then resetting the buffer, which is an extra copy. I think you could probably get around that by passing in the strbuf to set_rejected() and using strbuf_detach() to pull the value out. It's probably not worth worrying about optimizing out the copy for an error path like this, but I wonder if it would be more ergonomic (the caller does not have to remember to strbuf_reset() then).
I notice that you "goto next" now instead of "continue". So I was curious what happens in "next" now, but...
> +next:; > }
...the answer is nothing. ;) I guess maybe you were going to strbuf_reset() down here at one point? If the 'next' label remains empty, I think I'd prefer to keep these as 'continue'. But maybe you use it later in the series. I'll read on.
> [...]
The rest of the conversions all looked sensible to me. And you fixed my memory leak, which is good. ;)
-Peff