Karthik Nayak[PATCH v2 0/7] refs: provide detailed error messages when using batched update
The refs namespace uses an error buffer to capture details about failed reference updates. However when we added batched update support to reference transactions, these messages were never propagated, instead only an error code pertaining to the type of failure was propagated.
Currently, there are three regions which utilize batched updates:
- git update-ref --batch-updates - git fetch - git receive-pack
While 'git update-ref --batch-updates' was a newly introduced flag, both 'git fetch' and 'git receive-pack' were pre-existing. Before using batched updates, they provided more detailed error messages to the user, but this changed with the introduction of batched updates. This is a regression in their workings.
This patch series fixes this, by passing the detailed error message and utilizing it whenever available. The regression was reported by Elijah Newren [1] and based on the patch submitted by Jeff King [2].
[1]: https://lore.kernel.org/all/CABPp-BGL2tJR4dPidQuFcp-X0_VkVTknCY-0Zgo=jHVGv_P=wA@mail.gmail.com/ [2]: https://lore.kernel.org/all/20251224081214.GA1879908@coredump.intra.peff.net/
--- Changes in v2: - Updates to the commit messages to be more descriptive. - Instead of passing the char pointer for the error description, pass the 'strbuf' itself. This makes the API a lot cleaner to deal with. Also avoids having to remember to reset the strbuf after usage. - Chalk out a separate commit for using a 'goto next_ref' in `refs_verify_refnames_available()`. This makes the intention much clearer. - For git-update-ref(1), keep the existing implementation as is and only output the detailed error message to stderr. - For git-receive-pack(1), use 'rp_error()' for detailed error message while keeping the current implementation as is. - Added a separate patch to handle missing information in git-fetch(1)'s status table. This involves delaying updates to the end, where update success/failure information is available. I'm not too confident about this approach though, we could also drop it from the series and I could pick that up independently. This is still 1.19 ± 0.02 times faster than non-batched version (v2.50.0) in the files backend. - Link to v1: https://patch.msgid.link/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com
--- builtin/fetch.c | 188 ++++++++++++++++++++++++++++++++++++++++-------- builtin/receive-pack.c | 7 +- builtin/update-ref.c | 7 +- refs.c | 48 +++++++------ refs.h | 1 + refs/files-backend.c | 5 +- refs/packed-backend.c | 12 ++-- refs/refs-internal.h | 4 +- refs/reftable-backend.c | 5 +- t/t1400-update-ref.sh | 71 ++++++++++-------- t/t5510-fetch.sh | 8 +-- t/t5516-fetch-push.sh | 16 +++++ t/t5574-fetch-output.sh | 16 ++--- 13 files changed, 280 insertions(+), 108 deletions(-)
Karthik Nayak (7):
refs: drop unnecessary header includes
refs: skip to next ref when current ref is rejected
refs: add rejection detail to the callback function
update-ref: utilize rejected error details if available
fetch: utilize rejected ref error details
receive-pack: utilize rejected ref error details
fetch: delay user information post committing of transactionRange-diff versus v1:
1: 806ec3de6e ! 1: 75b7b2f83d refs: remove unused header
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- refs: remove unused header
+ refs: drop unnecessary header includes
- Some of the headers in 'refs.c' are no longer required, let's remove
- them.
+ The 'sigchain.h' header isn't being used and can be removed.
+
+ Similarly, 'run-command.h' serves no direct purpose here. While it gets pulled in transitively through 'hook.h', we can still drop the explicit include for clarity.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
2: 6ba3b9da56 ! 2: 507906091c refs: attach rejection details to updates
@@ Metadata
Author: Karthik Nayak <karthik.188@gmail.com>
## Commit message ##
- refs: attach rejection details to updates
+ refs: skip to next ref when current ref is rejected
- The implementation of batched updates in 23fc8e4f61 (refs: implement
- batch reference update support, 2025-04-08) added rejection error codes
- to each reference update. This allowed batching of updates, however
- while each rejection is linked to a rejection code, the already present
- user readable error message is simply dropped.
+ In `refs_verify_refnames_available()` we have two nested loops: the
+ outer loop iterates over all references to check, while the inner loop
+ checks for filesystem conflicts for a given ref by breaking down its
+ path.
- Make necessary changes to ensure that the rejection detail is also added
- to the reference update. In upcoming commits, we'll utilize this field
- to provide better error message to users, namely in:
+ With batched updates, when we detect a filesystem conflict, we mark the
+ update as rejected and execute 'continue'. However, this only skips to
+ the next iteration of the inner loop, not the outer loop as intended.
+ This causes the same reference to be repeatedly rejected. Fix this by
+ using a goto statement to skip to the next reference in the outer loop.
- - git update-ref --batch-updates
- - git fetch
- - git receive-pack
-
- We move the error message creation right above
- `ref_transaction_maybe_set_rejected()`, so that the error message is
- available and also reset the error message if utilized to avoid
- un-expected concatination.
-
- Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## refs.c ##
@@ refs.c: void ref_transaction_free(struct ref_transaction *transaction)
size_t update_idx,
- enum ref_transaction_error err)
+ enum ref_transaction_error err,
-+ const char *details)
++ struct strbuf *details)
{
if (update_idx >= transaction->nr)
BUG("trying to set rejection on invalid update index");
@@ refs.c: int ref_transaction_maybe_set_rejected(struct ref_transaction *transacti
transaction->updates[update_idx]->refname, 0);
transaction->updates[update_idx]->rejection_err = err;
-+ if (details)
-+ transaction->updates[update_idx]->rejection_details = xstrdup(details);
++ transaction->updates[update_idx]->rejection_details = strbuf_detach(details, NULL);
ALLOC_GROW(transaction->rejections->update_indices,
transaction->rejections->nr + 1,
transaction->rejections->alloc);
@@ refs.c: enum ref_transaction_error refs_verify_refnames_available(struct ref_sto
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
-+ REF_TRANSACTION_ERROR_NAME_CONFLICT, err->buf)) {
++ REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) {
strset_remove(&dirnames, dirname.buf);
strset_add(&conflicting_dirnames, dirname.buf);
- continue;
-+ strbuf_reset(err);
-+ goto next;
++ goto next_ref;
}
- strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
@@ refs.c: enum ref_transaction_error refs_verify_refnames_available(struct ref_sto
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
-+ REF_TRANSACTION_ERROR_NAME_CONFLICT, err->buf)) {
++ REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) {
strset_remove(&dirnames, dirname.buf);
- continue;
-+ strbuf_reset(err);
-+ goto next;
++ goto next_ref;
}
- strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"),
@@ refs.c: enum ref_transaction_error refs_verify_refnames_available(struct ref_sto
}
}
@@ refs.c: enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
+ if (skip &&
string_list_has_string(skip, iter->ref.name))
continue;
-
+ strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
+ iter->ref.name, refname);
-+
+
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT))
- continue;
-+ REF_TRANSACTION_ERROR_NAME_CONFLICT, err->buf)) {
-+ strbuf_reset(err);
-+ goto next;
-+ }
++ REF_TRANSACTION_ERROR_NAME_CONFLICT, err))
++ goto next_ref;
- strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
- iter->ref.name, refname);
@@ refs.c: enum ref_transaction_error refs_verify_refnames_available(struct ref_sto
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT))
- continue;
-+ REF_TRANSACTION_ERROR_NAME_CONFLICT, err->buf)) {
-+ strbuf_reset(err);
-+ goto next;
-+ }
++ REF_TRANSACTION_ERROR_NAME_CONFLICT, err))
++ goto next_ref;
- strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"),
- refname, extra_refname);
goto cleanup;
}
-+next:;
++next_ref:;
}
ret = 0;
@@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref
err);
if (ret) {
- if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+- strbuf_reset(err);
+ if (ref_transaction_maybe_set_rejected(transaction, i,
-+ ret, err->buf)) {
- strbuf_reset(err);
++ ret, err)) {
ret = 0;
-
+-
+ continue;
+ }
+ goto cleanup;
## refs/packed-backend.c ##
@@ refs/packed-backend.c: static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
@@ refs/packed-backend.c: static enum ref_transaction_error write_with_updates(stru
ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
- if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+- strbuf_reset(err);
+ if (ref_transaction_maybe_set_rejected(transaction, i,
-+ ret, err->buf)) {
- strbuf_reset(err);
++ ret, err)) {
ret = 0;
continue;
+ }
@@ refs/packed-backend.c: static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
oid_to_hex(&update->old_oid));
ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE;
- if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+- strbuf_reset(err);
+ if (ref_transaction_maybe_set_rejected(transaction, i,
-+ ret, err->buf)) {
- strbuf_reset(err);
++ ret, err)) {
ret = 0;
continue;
+ }
@@ refs/packed-backend.c: static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
oid_to_hex(&update->old_oid));
ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF;
- if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+- strbuf_reset(err);
+ if (ref_transaction_maybe_set_rejected(transaction, i,
-+ ret, err->buf)) {
- strbuf_reset(err);
++ ret, err)) {
ret = 0;
continue;
+ }
## refs/refs-internal.h ##
@@ refs/refs-internal.h: struct ref_update {
@@ refs/refs-internal.h: int refs_read_raw_ref(struct ref_store *ref_store, const c
size_t update_idx,
- enum ref_transaction_error err);
+ enum ref_transaction_error err,
-+ const char *details);
++ struct strbuf *details);
/*
* Add a ref_update with the specified properties to transaction, and
@@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s
&head_referent, &referent, err);
if (ret) {
- if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+- strbuf_reset(err);
+ if (ref_transaction_maybe_set_rejected(transaction, i,
-+ ret, err->buf)) {
- strbuf_reset(err);
++ ret, err)) {
ret = 0;
-
+-
+ continue;
+ }
+ goto done;
3: 76f199b434 ! 3: 78d6220027 refs: add rejection detail to the callback function
@@ Commit message
field is unused, but will be integrated in the upcoming commits.
Co-authored-by: Jeff King <peff@peff.net>
+ Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/fetch.c ##
4: c05216bf9c < -: ---------- update-ref: utilize rejected error details if available
-: ---------- > 4: 6ca8a03f74 update-ref: utilize rejected error details if available
5: bdfef1b20f = 5: 289282031d fetch: utilize rejected ref error details
6: 08b74e8077 ! 6: d555777da0 receive-pack: utilize rejected ref error details
@@ Commit message
messages for failed referenced updates, the users were provided generic
error messages based on the error type.
- Similar to the previous commit, switch to using detailed error messages
- if present for failed reference updates to fix this regression.
-
- One downside of this is that the messages can be very verbose, for e.g.
- in the files backend, when trying to write a non-commit object to a
- branch, you would see:
+ Now that the updates also contain detailed error message, propagate
+ those to the client via 'rp_error'. The detailed error messages can be
+ very verbose, for e.g. in the files backend, when trying to write a
+ non-commit object to a branch, you would see:
! [remote rejected] 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d ->
branch (cannot update ref 'refs/heads/branch': trying to write
@@ Commit message
Reported-by: Elijah Newren <newren@gmail.com>
Co-authored-by: Jeff King <peff@peff.net>
+ Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
## builtin/receive-pack.c ##
@@ builtin/receive-pack.c: static void ref_transaction_rejection_handler(const char
{
struct strmap *failed_refs = cb_data;
-- strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
-+ if (!details)
-+ details = ref_transaction_error_msg(err);
++ if (details)
++ rp_error("%s", details);
+
-+ strmap_put(failed_refs, refname, (char *)details);
+ strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
}
- static void execute_commands_non_atomic(struct command *commands,
@@ builtin/receive-pack.c: static void execute_commands_non_atomic(struct command *commands,
}
-: ---------- > 7: 640d09d408 fetch: delay user information post committing of transactionbase-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6 change-id: 20260113-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-17786b20894a
Thanks - Karthik
