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: Wed, 14 Jan 2026 12:43:38 -0500 [thread overview]
Message-ID: <20260114174338.GE885771@coredump.intra.peff.net> (raw)
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
next prev parent reply other threads:[~2026-01-14 17:43 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 [this message]
2026-01-15 10:02 ` Karthik Nayak
2026-01-15 20:29 ` Jeff King
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=20260114174338.GE885771@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