Re: [PATCH 2/6] refs: attach rejection details to updates

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 15, 2026, 10:02 UTC
Message-ID: <CAOLa=ZSyfkb8oe=ZtkOcsGo9Dk44GZSFiaye3Vw2kDs_XqS8=Q@mail.gmail.com>
In-Reply-To: <20260114174338.GE885771@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 14 quoted lines

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

That's correct, I did ensure that there were no NULLs passed through, we could definitely drop the check. But I was being defensive. I think `xstrdup_or_null()` is the better option here.

Show 35 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).

That's a great idea, let me do that instead.

Show 12 quoted lines

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

I should have explained this, there are two loops here in play. An outer loop going through refnames to check availability for. An inner loop to breakdown the path of each refname to check for path conflicts.

With continue, we'd skip the inner loop, but would still perform other checks for the refname, this can lead to error details being overridden. So while we could replace s/goto next/continue for the code in the outer loop, it would still be needed for the inner loop.

Show 6 quoted lines

>> [...]
>
> The rest of the conversions all looked sensible to me. And you fixed my
> memory leak, which is good. ;)
>
> -Peff