From: Jeff King <peff@peff.net>
Date: Wed, 14 Jan 2026 17:43:38 GMT
Subject: Re: [PATCH 2/6] refs: attach rejection details to updates
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:

> @@ -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).

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