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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 16, 2026, 17:56 UTC
Message-ID: <CAOLa=ZRbYBJaoFV7eWPsMGhVjqMDj+5-KMcAUPzM6fyXwp3qtg@mail.gmail.com>
In-Reply-To: <20260115202929.GC1053259@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 20 quoted lines

> On Thu, Jan 15, 2026 at 02:02:15AM -0800, Karthik Nayak wrote:
>
>> >> +	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.
>
> I don't mind the extra defensiveness here, but I was wondering whether
> this would also mean that ref_transaction_for_each_rejected_update_fn
> callbacks could assume that "details" is always non-NULL. But maybe it
> is better to be defensive there, too.
>

Since I've moved to passing in the strbuf instead of the 'char *', this is now removed!

Show 29 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.
>
> Ah, thanks, I totally missed that it was jumping to the outer loop.
>
> It's curious that the original did a "continue" from that inner loop,
> rather than a "break". Once we see that "refs/heads/foo" is a conflict
> for a particular update and mark it as failed, there is no point in
> looking at "refs/heads/foo/bar" at all. So I suspect we were wasting
> a tiny bit of processing in this error case before, but never doing the
> wrong thing.
>

With the previous situation of not resetting strbuf after rejecting an update, we ended up adding more errors to the same strbuf and since we kept rejecting the same reference again and again, this causes the last rejection with multiple messages appended to the strbuf to be displayed to the user.

This is moot now, considering we reset the strbuf, but that's how I noticed it.

Show 6 quoted lines

> Likewise, if we did "break" from the loop, shouldn't we "continue" to
> the next ref immediately? There is no need to do further checks.
>
> Your new goto solves both of those; it's just subtle. So two possible
> suggestions for making this more clear:
>

Yes, I agree with it not being explicit.

>   - if we are going to use a label, call it next_ref or something, to
>     make it clear we are jumping to the outer loop over the refs.
>

This is a good suggestion, makes it much nicer to comprehend.

>   - switch to the goto as a preparatory patch. It's the right thing even
>     before changing the "err" handling, and the change will be more
>     obvious that way.
>

This is a fair point too, I'll do this.

> There is another way of writing it, which is to break out of the inner
> loop, and then notice that we did so. Either with an explicit flag, or
> in this case we can do it by checking slash. Like this:
>

This is an interesting approach, but it is very a bit harder to read in my opinion since the final logic is collation of 'break' + 'continue if slash'.

Show 43 quoted lines

> diff --git a/refs.c b/refs.c
> index 965b232a06..a3dafdb58b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2663,7 +2663,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
>  					    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
>  					strset_remove(&dirnames, dirname.buf);
>  					strset_add(&conflicting_dirnames, dirname.buf);
> -					continue;
> +					break;
>  				}
>
>  				strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
> @@ -2676,7 +2676,7 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
>  					    transaction, *update_idx,
>  					    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
>  					strset_remove(&dirnames, dirname.buf);
> -					continue;
> +					break;
>  				}
>
>  				strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"),
> @@ -2685,6 +2685,13 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
>  			}
>  		}
>
> +		/*
> +		 * We didn't finish our loop over the components, which means
> +		 * we hit a conflict. Bail to the next ref now.
> +		 */
> +		if (slash)
> +			continue;
> +
>  		/*
>  		 * We are at the leaf of our refname (e.g., "refs/foo/bar").
>  		 * There is no point in searching for a reference with that
>
>
> That's more "structured" in that we avoid the goto. But I'm not sure it
> is any easier to understand than a "next_ref" label. So I'm happy with
> either approach. ;)
>
> -Peff

I think the 'next_ref' approach with a separate commit chalked out for it, seems like the best approach. Thanks