From: Jeff King <peff@peff•net>
To: Karthik Nayak <karthik.188@gmail•com>
Cc: git@vger•kernel.org, newren@gmail•com
Subject: Re: [PATCH 2/6] refs: attach rejection details to updates
Date: Thu, 15 Jan 2026 15:29:29 -0500 [thread overview]
Message-ID: <20260115202929.GC1053259@coredump.intra.peff.net> (raw)
In-Reply-To: <CAOLa=ZSyfkb8oe=ZtkOcsGo9Dk44GZSFiaye3Vw2kDs_XqS8=Q@mail.gmail.com>
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.
> > 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
next prev parent reply other threads:[~2026-01-15 20:29 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-14 15:40 [PATCH 0/6] refs: provide detailed error messages when using batched update Karthik Nayak
2026-01-14 15:40 ` [PATCH 1/6] refs: remove unused header Karthik Nayak
2026-01-14 17:08 ` Junio C Hamano
2026-01-15 9:50 ` Karthik Nayak
2026-01-14 15:40 ` [PATCH 2/6] refs: attach rejection details to updates Karthik Nayak
2026-01-14 17:43 ` Jeff King
2026-01-15 10:02 ` Karthik Nayak
2026-01-15 20:29 ` Jeff King [this message]
2026-01-16 17:56 ` Karthik Nayak
2026-01-14 15:40 ` [PATCH 3/6] refs: add rejection detail to the callback function Karthik Nayak
2026-01-14 17:44 ` Jeff King
2026-01-15 10:10 ` Karthik Nayak
2026-01-14 15:40 ` [PATCH 4/6] update-ref: utilize rejected error details if available Karthik Nayak
2026-01-14 17:27 ` Junio C Hamano
2026-01-14 17:55 ` Jeff King
2026-01-15 11:08 ` Karthik Nayak
2026-01-14 15:40 ` [PATCH 5/6] fetch: utilize rejected ref error details Karthik Nayak
2026-01-14 17:33 ` Junio C Hamano
2026-01-15 10:54 ` Karthik Nayak
2026-01-14 18:00 ` Jeff King
2026-01-15 15:20 ` Karthik Nayak
2026-01-14 15:40 ` [PATCH 6/6] receive-pack: " Karthik Nayak
2026-01-14 18:03 ` Jeff King
2026-01-15 15:21 ` Karthik Nayak
2026-01-14 16:45 ` [PATCH 0/6] refs: provide detailed error messages when using batched update Junio C Hamano
2026-01-16 21:27 ` [PATCH v2 0/7] " Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 1/7] refs: drop unnecessary header includes Karthik Nayak
2026-01-18 12:07 ` SZEDER Gábor
2026-01-19 8:53 ` Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 2/7] refs: skip to next ref when current ref is rejected Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 3/7] refs: add rejection detail to the callback function Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 4/7] update-ref: utilize rejected error details if available Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 5/7] fetch: utilize rejected ref error details Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 6/7] receive-pack: " Karthik Nayak
2026-01-16 21:27 ` [PATCH v2 7/7] fetch: delay user information post committing of transaction Karthik Nayak
2026-01-17 13:56 ` Phillip Wood
2026-01-19 16:11 ` Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 0/6] refs: provide detailed error messages when using batched update Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 1/6] refs: skip to next ref when current ref is rejected Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 2/6] refs: add rejection detail to the callback function Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 3/6] update-ref: utilize rejected error details if available Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 4/6] fetch: utilize rejected ref error details Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 5/6] receive-pack: " Karthik Nayak
2026-01-20 9:59 ` [PATCH v3 6/6] fetch: delay user information post committing of transaction Karthik Nayak
2026-01-21 16:21 ` Phillip Wood
2026-01-21 18:43 ` Junio C Hamano
2026-01-22 9:05 ` Karthik Nayak
2026-01-21 18:12 ` [PATCH v3 0/6] refs: provide detailed error messages when using batched update Junio C Hamano
2026-01-22 12:04 ` [PATCH v4 " Karthik Nayak
2026-01-22 12:04 ` [PATCH v4 1/6] refs: skip to next ref when current ref is rejected Karthik Nayak
2026-01-22 12:04 ` [PATCH v4 2/6] refs: add rejection detail to the callback function Karthik Nayak
2026-01-22 12:04 ` [PATCH v4 3/6] update-ref: utilize rejected error details if available Karthik Nayak
2026-01-22 12:04 ` [PATCH v4 4/6] fetch: utilize rejected ref error details Karthik Nayak
2026-01-22 12:04 ` [PATCH v4 5/6] receive-pack: " Karthik Nayak
2026-01-22 12:05 ` [PATCH v4 6/6] fetch: delay user information post committing of transaction Karthik Nayak
2026-01-22 20:10 ` Junio C Hamano
2026-01-23 14:49 ` Karthik Nayak
2026-01-23 17:57 ` Junio C Hamano
2026-01-25 18:47 ` Karthik Nayak
2026-01-23 14:41 ` Phillip Wood
2026-01-23 14:50 ` Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 0/6] refs: provide detailed error messages when using batched update Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 1/6] refs: skip to next ref when current ref is rejected Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 2/6] refs: add rejection detail to the callback function Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 3/6] update-ref: utilize rejected error details if available Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 4/6] fetch: utilize rejected ref error details Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 5/6] receive-pack: " Karthik Nayak
2026-01-25 22:52 ` [PATCH v5 6/6] fetch: delay user information post committing of transaction Karthik Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260115202929.GC1053259@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=karthik.188@gmail$(echo .)com \
--cc=newren@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox