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

From: Jeff King <peff@peff.net>
Date: Jan 15, 2026, 20:29 UTC
Message-ID: <20260115202929.GC1053259@coredump.intra.peff.net>
In-Reply-To: <CAOLa=ZSyfkb8oe=ZtkOcsGo9Dk44GZSFiaye3Vw2kDs_XqS8=Q@mail.gmail.com>

On Thu, Jan 15, 2026 at 02:02:15AM -0800, Karthik Nayak wrote:

Show 12 quoted lines

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

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

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:

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

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

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:

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