# [PATCH 0/6] refs: provide detailed error messages when using batched update 32 messages from 2026-01-14 to 2026-01-25. Participants: Karthik Nayak, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/64802 ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 0/6] refs: provide detailed error messages when using batched update Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501%40gmail.com ``` 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/ --- builtin/fetch.c | 9 +++++--- builtin/receive-pack.c | 9 ++++++-- builtin/update-ref.c | 13 +++++++----- refs.c | 56 ++++++++++++++++++++++++++++++------------------- refs.h | 1 + refs/files-backend.c | 3 ++- refs/packed-backend.c | 9 +++++--- refs/refs-internal.h | 4 +++- refs/reftable-backend.c | 3 ++- t/t1400-update-ref.sh | 26 +++++++++++------------ t/t5510-fetch.sh | 8 +++---- t/t5516-fetch-push.sh | 15 +++++++++++++ 12 files changed, 102 insertions(+), 54 deletions(-) Karthik Nayak (6): refs: remove unused header refs: attach rejection details to updates 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 base-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6 change-id: 20260113-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-17786b20894a Thanks - Karthik ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 1/6] refs: remove unused header Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-1-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-1-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` Some of the headers in 'refs.c' are no longer required, let's remove them. Signed-off-by: Karthik Nayak --- refs.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/refs.c b/refs.c index e06e0cb072..965b232a06 100644 --- a/refs.c +++ b/refs.c @@ -15,7 +15,6 @@ #include "iterator.h" #include "refs.h" #include "refs/refs-internal.h" -#include "run-command.h" #include "hook.h" #include "object-name.h" #include "odb.h" @@ -26,7 +25,6 @@ #include "strvec.h" #include "repo-settings.h" #include "setup.h" -#include "sigchain.h" #include "date.h" #include "commit.h" #include "wildmatch.h" -- 2.51.2 ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 2/6] refs: attach rejection details to updates Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-2-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-2-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` 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. 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: - 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 Signed-off-by: Karthik Nayak --- refs.c | 52 ++++++++++++++++++++++++++++++++----------------- refs/files-backend.c | 3 ++- refs/packed-backend.c | 9 ++++++--- refs/refs-internal.h | 4 +++- refs/reftable-backend.c | 3 ++- 5 files changed, 47 insertions(+), 24 deletions(-) diff --git a/refs.c b/refs.c index 965b232a06..991bd8e6ee 100644 --- a/refs.c +++ b/refs.c @@ -1222,6 +1222,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction->updates[i]->committer_info); free((char *)transaction->updates[i]->new_target); free((char *)transaction->updates[i]->old_target); + free((char *)transaction->updates[i]->rejection_details); free(transaction->updates[i]); } @@ -1236,7 +1237,8 @@ void ref_transaction_free(struct ref_transaction *transaction) int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err) + enum ref_transaction_error err, + const char *details) { if (update_idx >= transaction->nr) BUG("trying to set rejection on invalid update index"); @@ -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); ALLOC_GROW(transaction->rejections->update_indices, transaction->rejections->nr + 1, transaction->rejections->alloc); @@ -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; } if (extras && string_list_has_string(extras, dirname.buf)) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, dirname.buf); + 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); - continue; + strbuf_reset(err); + goto next; } - strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), - refname, dirname.buf); goto cleanup; } } @@ -2711,13 +2720,16 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs 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; + } - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - iter->ref.name, refname); goto cleanup; } @@ -2727,15 +2739,19 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, extra_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; + } - strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), - refname, extra_refname); goto cleanup; } +next:; } ret = 0; diff --git a/refs/files-backend.c b/refs/files-backend.c index 6f6f76a8d8..8d22a2e8e3 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2983,7 +2983,8 @@ static int files_transaction_prepare(struct ref_store *ref_store, head_ref, &refnames_to_check, err); if (ret) { - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err->buf)) { strbuf_reset(err); ret = 0; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 4ea0c12299..535200db01 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1437,7 +1437,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re update->refname); ret = REF_TRANSACTION_ERROR_CREATE_EXISTS; - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err->buf)) { strbuf_reset(err); ret = 0; continue; @@ -1452,7 +1453,8 @@ 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)) { + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err->buf)) { strbuf_reset(err); ret = 0; continue; @@ -1496,7 +1498,8 @@ 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)) { + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err->buf)) { strbuf_reset(err); ret = 0; continue; diff --git a/refs/refs-internal.h b/refs/refs-internal.h index c7d2a6e50b..60d9f015cf 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -128,6 +128,7 @@ struct ref_update { * was rejected. */ enum ref_transaction_error rejection_err; + const char *rejection_details; /* * If this ref_update was split off of a symref update via @@ -153,7 +154,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, */ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err); + enum ref_transaction_error err, + const char *details); /* * Add a ref_update with the specified properties to transaction, and diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4319a4eacb..a9c9ceebf3 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1401,7 +1401,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, &refnames_to_check, head_type, &head_referent, &referent, err); if (ret) { - if (ref_transaction_maybe_set_rejected(transaction, i, ret)) { + if (ref_transaction_maybe_set_rejected(transaction, i, + ret, err->buf)) { strbuf_reset(err); ret = 0; -- 2.51.2 ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 3/6] refs: add rejection detail to the callback function Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-3-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-3-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` The previous commit started storing the rejection details alongside the error code for rejected updates. Pass this along to the callback function `ref_transaction_for_each_rejected_update()`. Currently the field is unused, but will be integrated in the upcoming commits. Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/fetch.c | 1 + builtin/receive-pack.c | 1 + builtin/update-ref.c | 1 + refs.c | 2 +- refs.h | 1 + 5 files changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 288d3772ea..d427adea61 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1649,6 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data) { struct ref_rejection_data *data = cb_data; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ef1f77be8c..94d3e73cee 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1813,6 +1813,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data) { struct strmap *failed_refs = cb_data; diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 195437e7c6..0046a87c57 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -573,6 +573,7 @@ static void print_rejected_refs(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data UNUSED) { struct strbuf sb = STRBUF_INIT; diff --git a/refs.c b/refs.c index 991bd8e6ee..ad1898598f 100644 --- a/refs.c +++ b/refs.c @@ -2880,7 +2880,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL, (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL, update->old_target, update->new_target, - update->rejection_err, cb_data); + update->rejection_err, update->rejection_details, cb_data); } } diff --git a/refs.h b/refs.h index d9051bbb04..4fbe3da924 100644 --- a/refs.h +++ b/refs.h @@ -975,6 +975,7 @@ typedef void ref_transaction_for_each_rejected_update_fn(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, + const char *details, void *cb_data); void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, ref_transaction_for_each_rejected_update_fn cb, -- 2.51.2 ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 4/6] update-ref: utilize rejected error details if available Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-4-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-4-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` When git-update-ref(1) received the '--update-ref' flag, the error details generated in the refs namespace wasn't propagated with failed updates. Instead only an error code pertaining to the type of rejection was noted. This missed detailed error message which the user can act upon. The previous commits added the required code to propagate these detailed error messages from the refs namespace. Now that additional details are available, use them instead of the generic error message based of the error code. Fix the tests to also accommodate these error messages. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/update-ref.c | 14 ++++++++------ t/t1400-update-ref.sh | 26 +++++++++++++------------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0046a87c57..800e380d32 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data UNUSED) { struct strbuf sb = STRBUF_INIT; - const char *reason = ref_transaction_error_msg(err); - strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, - new_oid ? oid_to_hex(new_oid) : new_target, - old_oid ? oid_to_hex(old_oid) : old_target, - reason); + if (details) + strbuf_addf(&sb, "%s\n", details); + else + strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, + new_oid ? oid_to_hex(new_oid) : new_target, + old_oid ? oid_to_hex(old_oid) : old_target, + ref_transaction_error_msg(err)); fwrite(sb.buf, sb.len, 1, stdout); strbuf_release(&sb); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index db7f5444da..6cd6b45411 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -2100,7 +2100,7 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "invalid new value provided" stdout + test_grep -q "trying to write ref ${SQ}refs/heads/ref2${SQ} with nonexistent object" stdout ) ' @@ -2126,7 +2126,7 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "invalid new value provided" stdout + test_grep -q "trying to write non-commit object $head_tree to branch ${SQ}refs/heads/ref2${SQ}" stdout ) ' @@ -2148,7 +2148,7 @@ do git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && test_must_fail git rev-parse refs/heads/ref2 && - test_grep -q "reference does not exist" stdout + test_grep -q "cannot lock ref ${SQ}refs/heads/ref2${SQ}: unable to resolve reference ${SQ}refs/heads/ref2${SQ}" stdout ) ' @@ -2172,7 +2172,7 @@ do test_cmp expect actual && echo $head >expect && test_must_fail git rev-parse refs/heads/ref2 && - test_grep -q "reference does not exist" stdout + test_grep -q "cannot lock ref ${SQ}refs/heads/ref2${SQ}: reference is missing but expected $head" stdout ) ' @@ -2198,7 +2198,7 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "expected symref but found regular ref" stdout + test_grep -q "cannot lock ref ${SQ}refs/heads/ref2${SQ}: expected symref with target ${SQ}refs/heads/nonexistent${SQ}: but is a regular ref" stdout ) ' @@ -2223,7 +2223,7 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "reference already exists" stdout + test_grep -q "cannot lock ref ${SQ}refs/heads/ref2${SQ}: reference already exists" stdout ) ' @@ -2248,7 +2248,7 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "incorrect old value provided" stdout + test_grep -q "cannot lock ref ${SQ}refs/heads/ref2${SQ}: is at $head but expected $old_head" stdout ) ' @@ -2269,7 +2269,7 @@ do echo $old_head >expect && git rev-parse refs/heads/ref/foo >actual && test_cmp expect actual && - test_grep -q "refname conflict" stdout + test_grep -q "${SQ}refs/heads/ref/foo${SQ} exists; cannot create ${SQ}refs/heads/ref${SQ}" stdout ) ' @@ -2290,7 +2290,7 @@ do echo $old_head >expect && git rev-parse refs/heads/foo >actual && test_cmp expect actual && - test_grep -q "refname conflict" stdout + test_grep -q "${SQ}refs/heads/ref/foo${SQ} exists; cannot create ${SQ}refs/heads/ref${SQ}" stdout ) ' @@ -2316,7 +2316,7 @@ do echo $old_head >expect && git rev-parse refs/heads/ref >actual && test_cmp expect actual && - test_grep -q "reference conflict due to case-insensitive filesystem" stdout + test_grep -e "cannot lock ref ${SQ}refs/heads/Foo${SQ}: Unable to create" -e "Foo.lock" stdout ) ' @@ -2358,7 +2358,7 @@ do format_command $type "delete refs/heads/symbolic" "$head" >stdin && git update-ref $type --stdin --batch-updates stdout && - test_grep "reference does not exist" stdout + test_grep "cannot lock ref ${SQ}refs/heads/symbolic${SQ}: unable to resolve reference ${SQ}refs/heads/non-existent${SQ}" stdout ) ' @@ -2374,7 +2374,7 @@ do format_command $type "delete refs/heads/new-branch" "$head" >stdin && git update-ref $type --stdin --batch-updates stdout && - test_grep "incorrect old value provided" stdout + test_grep "cannot lock ref ${SQ}refs/heads/new-branch${SQ}: is at $(git rev-parse new-branch) but expected $head" stdout ) ' @@ -2388,7 +2388,7 @@ do format_command $type "delete refs/heads/non-existent" "$head" >stdin && git update-ref $type --stdin --batch-updates stdout && - test_grep "reference does not exist" stdout + test_grep "cannot lock ref ${SQ}refs/heads/non-existent${SQ}: unable to resolve reference ${SQ}refs/heads/non-existent${SQ}" stdout ) ' done -- 2.51.2 ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 5/6] fetch: utilize rejected ref error details Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-5-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-5-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` In 0e358de64a (fetch: use batched reference updates, 2025-05-19), git-fetch(1) switched to using batched reference updates. This also introduced a regression wherein instead of providing detailed error 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. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/fetch.c | 10 ++++++---- t/t5510-fetch.sh | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index d427adea61..49495be0b6 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1649,7 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data) { struct ref_rejection_data *data = cb_data; @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, "branches"), data->remote_name); data->conflict_msg_shown = true; } else { - const char *reason = ref_transaction_error_msg(err); - - error(_("fetching ref %s failed: %s"), refname, reason); + if (details) + error("%s", details); + else + error(_("fetching ref %s failed: %s"), + refname, ref_transaction_error_msg(err)); } *data->retcode = 1; diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ce1c23684e..c69afb5a60 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1516,7 +1516,7 @@ test_expect_success REFFILES 'existing reference lock in repo' ' git remote add origin ../base && touch refs/heads/foo.lock && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/branch >actual && test_cmp expect actual @@ -1530,7 +1530,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti cd case_insensitive && git remote add origin -- ../case_sensitive_fd && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "failed: refname conflict" err && + test_grep "cannot process ${SQ}refs/remotes/origin/foo${SQ} and ${SQ}refs/remotes/origin/foo/bar${SQ} at the same time" err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/foo/bar >actual && test_cmp expect actual @@ -1544,7 +1544,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti cd case_insensitive && git remote add origin -- ../case_sensitive_df && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "failed: refname conflict" err && + test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}./refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/Foo/bar >actual && test_cmp expect actual @@ -1658,7 +1658,7 @@ test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" ' git remote add origin ../base && >refs/heads/foo.lock && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && test_grep "branch ${SQ}branch${SQ} of ../base" FETCH_HEAD && test_grep "branch ${SQ}foo${SQ} of ../base" FETCH_HEAD ) -- 2.51.2 ``` ## Karthik Nayak, 2026-01-14 15:40 Subject: [PATCH 6/6] receive-pack: utilize rejected ref error details Message-ID: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-6-f5f8b173c501@gmail.com> URL: https://gitlist.dev/e/20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-6-f5f8b173c501%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19), git-receive-pack(1) switched to using batched reference updates. This also introduced a regression wherein instead of providing detailed error 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: ! [remote rejected] 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d -> branch (cannot update ref 'refs/heads/branch': trying to write non-commit object 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d to branch 'refs/heads/branch') Here the refname is repeated multiple times due to how error messages are propagated and filled over the code stack. This potentially can be cleaned up in a future commit. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/receive-pack.c | 10 +++++++--- t/t5516-fetch-push.sh | 15 +++++++++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 94d3e73cee..969d59ae3e 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1813,12 +1813,15 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data) { 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); + + strmap_put(failed_refs, refname, (char *)details); } static void execute_commands_non_atomic(struct command *commands, @@ -1884,6 +1887,7 @@ static void execute_commands_non_atomic(struct command *commands, } ref_transaction_for_each_rejected_update(transaction, + ref_transaction_rejection_handler, &failed_refs); @@ -1895,7 +1899,7 @@ static void execute_commands_non_atomic(struct command *commands, if (reported_error) cmd->error_string = reported_error; else if (strmap_contains(&failed_refs, cmd->ref_name)) - cmd->error_string = strmap_get(&failed_refs, cmd->ref_name); + cmd->error_string = cmd->error_string_owned = xstrdup(strmap_get(&failed_refs, cmd->ref_name)); } cleanup: diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 46926e7bbd..45595991c8 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1882,4 +1882,19 @@ test_expect_success 'push with F/D conflict with deletion and creation' ' git push testrepo :refs/heads/branch/conflict refs/heads/branch ' +test_expect_success 'pushing non-commit objects should report error' ' + test_when_finished "rm -rf dest repo" && + git init dest && + git init repo && + + ( + cd repo && + test_commit --annotate test && + + tagsha=$(git rev-parse test^{tag}) && + test_must_fail git push ../dest "$tagsha:refs/heads/branch" 2>err && + test_grep "trying to write non-commit object $tagsha to branch ${SQ}refs/heads/branch${SQ}" err + ) +' + test_done -- 2.51.2 ``` ## Junio C Hamano, 2026-01-14 16:45 Subject: Re: [PATCH 0/6] refs: provide detailed error messages when using batched update Message-ID: URL: https://gitlist.dev/e/xmqq1pjsgn2s.fsf%40gitster.g In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` Karthik Nayak writes: > 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/ Thanks, all. It is very nice to see such a collaboration going ;-) Will queue. > --- > builtin/fetch.c | 9 +++++--- > builtin/receive-pack.c | 9 ++++++-- > builtin/update-ref.c | 13 +++++++----- > refs.c | 56 ++++++++++++++++++++++++++++++------------------- > refs.h | 1 + > refs/files-backend.c | 3 ++- > refs/packed-backend.c | 9 +++++--- > refs/refs-internal.h | 4 +++- > refs/reftable-backend.c | 3 ++- > t/t1400-update-ref.sh | 26 +++++++++++------------ > t/t5510-fetch.sh | 8 +++---- > t/t5516-fetch-push.sh | 15 +++++++++++++ > 12 files changed, 102 insertions(+), 54 deletions(-) > > Karthik Nayak (6): > refs: remove unused header > refs: attach rejection details to updates > 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 > > > > base-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6 > change-id: 20260113-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-17786b20894a > > Thanks > - Karthik ``` ## Junio C Hamano, 2026-01-14 17:08 Subject: Re: [PATCH 1/6] refs: remove unused header Message-ID: URL: https://gitlist.dev/e/xmqqwm1kf7gr.fsf%40gitster.g In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-1-f5f8b173c501@gmail.com> ``` Karthik Nayak writes: > Some of the headers in 'refs.c' are no longer required, let's remove > them. > > Signed-off-by: Karthik Nayak > --- > refs.c | 2 -- > 1 file changed, 2 deletions(-) One thing to note is that The resulting file refs.c still includes hook.h and because of that, the removal of run-command.h from here has no effect. > diff --git a/refs.c b/refs.c > index e06e0cb072..965b232a06 100644 > --- a/refs.c > +++ b/refs.c > @@ -15,7 +15,6 @@ > #include "iterator.h" > #include "refs.h" > #include "refs/refs-internal.h" > -#include "run-command.h" > #include "hook.h" > #include "object-name.h" > #include "odb.h" > @@ -26,7 +25,6 @@ > #include "strvec.h" > #include "repo-settings.h" > #include "setup.h" > -#include "sigchain.h" > #include "date.h" > #include "commit.h" > #include "wildmatch.h" ``` ## Junio C Hamano, 2026-01-14 17:27 Subject: Re: [PATCH 4/6] update-ref: utilize rejected error details if available Message-ID: URL: https://gitlist.dev/e/xmqqpl7cf6kf.fsf%40gitster.g In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-4-f5f8b173c501@gmail.com> ``` Karthik Nayak writes: > @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname, > const char *old_target, > const char *new_target, > enum ref_transaction_error err, > - const char *details UNUSED, > + const char *details, > void *cb_data UNUSED) > { > struct strbuf sb = STRBUF_INIT; > - const char *reason = ref_transaction_error_msg(err); > > - strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, > - new_oid ? oid_to_hex(new_oid) : new_target, > - old_oid ? oid_to_hex(old_oid) : old_target, > - reason); > + if (details) > + strbuf_addf(&sb, "%s\n", details); > + else > + strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, > + new_oid ? oid_to_hex(new_oid) : new_target, > + old_oid ? oid_to_hex(old_oid) : old_target, > + ref_transaction_error_msg(err)); Could "details" reported from the lower layer be less detailed than what we are formulating here, like updating the value of what ref from what old object to what new object, or what the err code tells the end-user? ``` ## Junio C Hamano, 2026-01-14 17:33 Subject: Re: [PATCH 5/6] fetch: utilize rejected ref error details Message-ID: URL: https://gitlist.dev/e/xmqqldi0f6a4.fsf%40gitster.g In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-5-f5f8b173c501@gmail.com> ``` Karthik Nayak writes: > In 0e358de64a (fetch: use batched reference updates, 2025-05-19), > git-fetch(1) switched to using batched reference updates. This also > introduced a regression wherein instead of providing detailed error > 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. The same question applkies as the previous step. That is ... > @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, > "branches"), data->remote_name); > data->conflict_msg_shown = true; > } else { > - const char *reason = ref_transaction_error_msg(err); > - > - error(_("fetching ref %s failed: %s"), refname, reason); > + if (details) > + error("%s", details); > + else > + error(_("fetching ref %s failed: %s"), > + refname, ref_transaction_error_msg(err)); ... would "details" always carry enough information to cover "refname" here, plus what the err code tells us? I guess ... > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index ce1c23684e..c69afb5a60 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -1516,7 +1516,7 @@ test_expect_success REFFILES 'existing reference lock in repo' ' > git remote add origin ../base && > touch refs/heads/foo.lock && > test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && > - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && > + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && ... the error only talks about our local name, and when the command is "git fetch origin refs/heads/foo:refs/remotes/origin/bar", we only complain about refs/remotes/origin/bar without ever mentioning refs/heads/foo on the remote side, so I think "details" has enough information to replace the existing message here in this case. Thanks. ``` ## Jeff King, 2026-01-14 17:43 Subject: Re: [PATCH 2/6] refs: attach rejection details to updates Message-ID: <20260114174338.GE885771@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260114174338.GE885771%40coredump.intra.peff.net 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 ``` ## Jeff King, 2026-01-14 17:44 Subject: Re: [PATCH 3/6] refs: add rejection detail to the callback function Message-ID: <20260114174458.GF885771@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260114174458.GF885771%40coredump.intra.peff.net In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-3-f5f8b173c501@gmail.com> ``` On Wed, Jan 14, 2026 at 04:40:44PM +0100, Karthik Nayak wrote: > The previous commit started storing the rejection details alongside the > error code for rejected updates. Pass this along to the callback > function `ref_transaction_for_each_rejected_update()`. Currently the > field is unused, but will be integrated in the upcoming commits. Splitting it out like this seems reasonable. > Co-authored-by: Jeff King > Signed-off-by: Karthik Nayak In case it matters, you can add my: Signed-off-by: Jeff King to this and any other patches which were derived from my earlier attempt. -Peff ``` ## Jeff King, 2026-01-14 17:55 Subject: Re: [PATCH 4/6] update-ref: utilize rejected error details if available Message-ID: <20260114175558.GG885771@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260114175558.GG885771%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Jan 14, 2026 at 09:27:28AM -0800, Junio C Hamano wrote: > Karthik Nayak writes: > > > @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname, > > const char *old_target, > > const char *new_target, > > enum ref_transaction_error err, > > - const char *details UNUSED, > > + const char *details, > > void *cb_data UNUSED) > > { > > struct strbuf sb = STRBUF_INIT; > > - const char *reason = ref_transaction_error_msg(err); > > > > - strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, > > - new_oid ? oid_to_hex(new_oid) : new_target, > > - old_oid ? oid_to_hex(old_oid) : old_target, > > - reason); > > + if (details) > > + strbuf_addf(&sb, "%s\n", details); > > + else > > + strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, > > + new_oid ? oid_to_hex(new_oid) : new_target, > > + old_oid ? oid_to_hex(old_oid) : old_target, > > + ref_transaction_error_msg(err)); > > Could "details" reported from the lower layer be less detailed than > what we are formulating here, like updating the value of what ref > from what old object to what new object, or what the err code tells > the end-user? I wondered that, too, but also: is this supposed to be machine-readable? The "rejected ..." output looks like something that could be parsed, and it seems to be documented in git-update-ref(1). Side note: if this is meant to be a stable format, surely there should be some coverage in the test suite? There doesn't seem to be. So should we just be replacing the ref_transaction_error_msg() part? I _think_ the low-level details will usually be more informative there, but not necessarily. So possibly we'd even want to show both, though I suspect just concatenating them would be messy. Plus the "details" one has a lot of redundant information in it (it mentions "refname", even though it is already on the "rejected" line). In the short-term, I wonder if we just want: if (details && *details) error("%s", details); That gets us back to the status quo, where the details are at least available via stderr. And then we can consider how to combine them into the machine-readable format separately. -Peff ``` ## Jeff King, 2026-01-14 18:00 Subject: Re: [PATCH 5/6] fetch: utilize rejected ref error details Message-ID: <20260114180040.GH885771@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260114180040.GH885771%40coredump.intra.peff.net In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-5-f5f8b173c501@gmail.com> ``` On Wed, Jan 14, 2026 at 04:40:46PM +0100, Karthik Nayak wrote: > @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, > "branches"), data->remote_name); > data->conflict_msg_shown = true; > } else { > - const char *reason = ref_transaction_error_msg(err); > - > - error(_("fetching ref %s failed: %s"), refname, reason); > + if (details) > + error("%s", details); > + else > + error(_("fetching ref %s failed: %s"), > + refname, ref_transaction_error_msg(err)); > } OK, so here we're writing to stderr anyway, and now we'll just give the more detailed data. Makes sense (though like Junio, I do wonder if the existing message might provide more details in some cases). BTW, I think there is still a related fallout for git-fetch. Even with your patch, doing this: $ git fetch . v1.0.0:refs/heads/foo From . * [new tag] v1.0.0 -> foo error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' will not put anything in the status table. Whereas in v2.50.0 and earlier, we get: $ git.v2.50.0 fetch . v1.0.0:refs/heads/foo error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' From . ! [new tag] v1.0.0 -> foo (unable to update local ref) Note the "!" and the "unable to update local ref" message in the status table. -Peff ``` ## Jeff King, 2026-01-14 18:03 Subject: Re: [PATCH 6/6] receive-pack: utilize rejected ref error details Message-ID: <20260114180306.GI885771@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260114180306.GI885771%40coredump.intra.peff.net In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-6-f5f8b173c501@gmail.com> ``` On Wed, Jan 14, 2026 at 04:40:47PM +0100, Karthik Nayak wrote: > In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19), > git-receive-pack(1) switched to using batched reference updates. This also > introduced a regression wherein instead of providing detailed error > 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: > > ! [remote rejected] 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d -> > branch (cannot update ref 'refs/heads/branch': trying to write > non-commit object 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d to branch > 'refs/heads/branch') > > Here the refname is repeated multiple times due to how error messages > are propagated and filled over the code stack. This potentially can be > cleaned up in a future commit. If we are going to have a "potentially cleaned up in the future" state, I think I would prefer to see just: if (details) rp_error("%s", details); here. And then it comes over the stderr sideband, but the actual status-table gets the same non-verbose message. That's what happened in v2.50.0 and earlier. Later if we want to try to cram more details into the machine-readable message we can. -Peff ``` ## Karthik Nayak, 2026-01-15 09:50 Subject: Re: [PATCH 1/6] refs: remove unused header Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQfjb1OfHJp6MVkbs%3D5Wey4Gp6t-jmEQSrojOsp%3Dge-Jw%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> Some of the headers in 'refs.c' are no longer required, let's remove >> them. >> >> Signed-off-by: Karthik Nayak >> --- >> refs.c | 2 -- >> 1 file changed, 2 deletions(-) > > One thing to note is that The resulting file refs.c still includes > hook.h and because of that, the removal of run-command.h from here > has no effect. > Good point, let me modify the commit message to explain this better. Perhaps: -->8-- refs: drop unnecessary header includes 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. >> diff --git a/refs.c b/refs.c >> index e06e0cb072..965b232a06 100644 >> --- a/refs.c >> +++ b/refs.c >> @@ -15,7 +15,6 @@ >> #include "iterator.h" >> #include "refs.h" >> #include "refs/refs-internal.h" >> -#include "run-command.h" >> #include "hook.h" >> #include "object-name.h" >> #include "odb.h" >> @@ -26,7 +25,6 @@ >> #include "strvec.h" >> #include "repo-settings.h" >> #include "setup.h" >> -#include "sigchain.h" >> #include "date.h" >> #include "commit.h" >> #include "wildmatch.h" ``` ## Karthik Nayak, 2026-01-15 10:02 Subject: Re: [PATCH 2/6] refs: attach rejection details to updates Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZSyfkb8oe%3DZtkOcsGo9Dk44GZSFiaye3Vw2kDs_XqS8%3DQ%40mail.gmail.com In-Reply-To: <20260114174338.GE885771@coredump.intra.peff.net> ``` Jeff King writes: > 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). 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. >> @@ -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). That's a great idea, let me do that instead. > > 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. >> [...] > > The rest of the conversions all looked sensible to me. And you fixed my > memory leak, which is good. ;) > > -Peff ``` ## Karthik Nayak, 2026-01-15 10:10 Subject: Re: [PATCH 3/6] refs: add rejection detail to the callback function Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZTX620gT%2BRuQ44AE_f82CibrGzoNv70G8BK7Wvt5HF4Ow%40mail.gmail.com In-Reply-To: <20260114174458.GF885771@coredump.intra.peff.net> ``` Jeff King writes: > On Wed, Jan 14, 2026 at 04:40:44PM +0100, Karthik Nayak wrote: > >> The previous commit started storing the rejection details alongside the >> error code for rejected updates. Pass this along to the callback >> function `ref_transaction_for_each_rejected_update()`. Currently the >> field is unused, but will be integrated in the upcoming commits. > > Splitting it out like this seems reasonable. > >> Co-authored-by: Jeff King >> Signed-off-by: Karthik Nayak > > In case it matters, you can add my: > > Signed-off-by: Jeff King > > to this and any other patches which were derived from my earlier > attempt. > > -Peff Thanks, will add in the next version. ``` ## Karthik Nayak, 2026-01-15 10:54 Subject: Re: [PATCH 5/6] fetch: utilize rejected ref error details Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZS0i%2BYXfVHHAax699ME48YG7jXNZ3WOBYryS0hypMZO-A%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Karthik Nayak writes: > >> In 0e358de64a (fetch: use batched reference updates, 2025-05-19), >> git-fetch(1) switched to using batched reference updates. This also >> introduced a regression wherein instead of providing detailed error >> 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. > > The same question applkies as the previous step. That is ... > >> @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, >> "branches"), data->remote_name); >> data->conflict_msg_shown = true; >> } else { >> - const char *reason = ref_transaction_error_msg(err); >> - >> - error(_("fetching ref %s failed: %s"), refname, reason); >> + if (details) >> + error("%s", details); >> + else >> + error(_("fetching ref %s failed: %s"), >> + refname, ref_transaction_error_msg(err)); > > ... would "details" always carry enough information to cover > "refname" here, plus what the err code tells us? > > I guess ... > In general, yes. Here's the final detailed error we'd show: generic availability checks (files + reftable backend): - '%s' exists; cannot create '%s' - cannot process '%s' and '%s' at the same time packed-backend: - cannot update ref '%s': reference already exists - cannot update ref '%s': is at %s but expected %s - cannot update ref '%s': reference is missing but expected %s files-backend: - cannot lock ref '%s': '%s' exists; cannot create '%s' - cannot lock ref '%s': cannot process '%s' and '%s' at the same time - cannot lock ref '%s': unable to resolve reference '%s' - multiple updates for 'HEAD' (including one via its referent '%s') are not allowed - cannot lock ref '%s': Unable to create '%s.lock': %s.\n\n Another git process seems to be running in this repository, e.g.\n an editor opened by 'git commit'. Please make sure all processes\n are terminated then try again. If it still fails, a git process\n may have crashed in this repository earlier:\n remove the file manually to continue. - cannot lock ref '%s': Unable to create '%s.lock': %s - cannot lock ref '%s': dangling symref already exists - cannot lock ref '%s': expected symref with target '%s': but is a regular ref - cannot lock ref '%s': is at %s but expected %s - cannot lock ref '%s': reference already exists - cannot lock ref '%s': reference is missing but expected %s - cannot lock ref '%s': there is a non-empty directory '%s' blocking reference '%s' - cannot lock ref '%s': unable to resolve reference '%s' - cannot update ref '%s': trying to write non-commit object %s to branch '%s' - cannot update ref '%s': trying to write ref '%s' with nonexistent object %s - multiple updates for '%s' (including one via symref '%s') are not allowed - verifying symref target: '%s': is at %s but expected %s - verifying symref target: '%s': reference is missing but expected %s reftable-backend: - cannot lock ref '%s': dangling symref already exists - cannot lock ref '%s': expected symref with target '%s': but is a regular ref - cannot lock ref '%s': is at %s but expected %s - cannot lock ref '%s': reference already exists - cannot lock ref '%s': reference is missing but expected %s - cannot lock ref '%s': unable to resolve reference '%s' - multiple updates for '%s' (including one via symref '%s') are not allowed - multiple updates for 'HEAD' (including one via its referent '%s') are not allowed - trying to write non-commit object %s to branch '%s' - trying to write ref '%s' with nonexistent object %s - verifying symref target: '%s': is at %s but expected %s - verifying symref target: '%s': reference is missing but expected %s >> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh >> index ce1c23684e..c69afb5a60 100755 >> --- a/t/t5510-fetch.sh >> +++ b/t/t5510-fetch.sh >> @@ -1516,7 +1516,7 @@ test_expect_success REFFILES 'existing reference lock in repo' ' >> git remote add origin ../base && >> touch refs/heads/foo.lock && >> test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && >> - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && >> + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && > > ... the error only talks about our local name, and when the command > is "git fetch origin refs/heads/foo:refs/remotes/origin/bar", we > only complain about refs/remotes/origin/bar without ever mentioning > refs/heads/foo on the remote side, so I think "details" has enough > information to replace the existing message here in this case. > > Thanks. > Yup. I do think there is a good cleanup we could potentially do here, with some of the error messages and perhaps following a better pattern in general, perhaps a more structured error message. But I didn't want to tackle that in this series. Karthik ``` ## Karthik Nayak, 2026-01-15 11:08 Subject: Re: [PATCH 4/6] update-ref: utilize rejected error details if available Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQLPB2Tntvimpp2zt%3D6PiWhJJh_oDCrUk7F8v%2BpFhyyMA%40mail.gmail.com In-Reply-To: <20260114175558.GG885771@coredump.intra.peff.net> ``` Jeff King writes: > On Wed, Jan 14, 2026 at 09:27:28AM -0800, Junio C Hamano wrote: > >> Karthik Nayak writes: >> >> > @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname, >> > const char *old_target, >> > const char *new_target, >> > enum ref_transaction_error err, >> > - const char *details UNUSED, >> > + const char *details, >> > void *cb_data UNUSED) >> > { >> > struct strbuf sb = STRBUF_INIT; >> > - const char *reason = ref_transaction_error_msg(err); >> > >> > - strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, >> > - new_oid ? oid_to_hex(new_oid) : new_target, >> > - old_oid ? oid_to_hex(old_oid) : old_target, >> > - reason); >> > + if (details) >> > + strbuf_addf(&sb, "%s\n", details); >> > + else >> > + strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, >> > + new_oid ? oid_to_hex(new_oid) : new_target, >> > + old_oid ? oid_to_hex(old_oid) : old_target, >> > + ref_transaction_error_msg(err)); >> >> Could "details" reported from the lower layer be less detailed than >> what we are formulating here, like updating the value of what ref >> from what old object to what new object, or what the err code tells >> the end-user? > > I wondered that, too, but also: is this supposed to be machine-readable? > The "rejected ..." output looks like something that could be parsed, > and it seems to be documented in git-update-ref(1). > > Side note: if this is meant to be a stable format, surely there should > be some coverage in the test suite? There doesn't seem to be. > Good catch, the documentation does indeed promise this format, so it wouldn't be appropriate to step away from it. Ideally, we should only replace the last field, but that would be a lot of redundant information. Overall, we could also drop this patch too, since the flag was introduced with batched updates and we could better justice here once we cleanup all other error messages. > So should we just be replacing the ref_transaction_error_msg() part? I > _think_ the low-level details will usually be more informative there, > but not necessarily. So possibly we'd even want to show both, though I > suspect just concatenating them would be messy. > > Plus the "details" one has a lot of redundant information in it (it > mentions "refname", even though it is already on the "rejected" line). Indeed, I've noted all possibilities in another response [1], but there is some redundancy there and we could do a nice cleanup. > > In the short-term, I wonder if we just want: > > if (details && *details) > error("%s", details); > > That gets us back to the status quo, where the details are at least > available via stderr. And then we can consider how to combine them into > the machine-readable format separately. > That's a good compromise too. I'd say we do this for now and see how we can take it from here. > -Peff [1]: CAOLa=ZS0i+YXfVHHAax699ME48YG7jXNZ3WOBYryS0hypMZO-A@mail.gmail.com ``` ## Karthik Nayak, 2026-01-15 15:20 Subject: Re: [PATCH 5/6] fetch: utilize rejected ref error details Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZQ0ETE%2BSzRV%2BM-xzEQFRTjdhSMseHjwnJ314yqPw8BPYA%40mail.gmail.com In-Reply-To: <20260114180040.GH885771@coredump.intra.peff.net> ``` Jeff King writes: > On Wed, Jan 14, 2026 at 04:40:46PM +0100, Karthik Nayak wrote: > >> @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, >> "branches"), data->remote_name); >> data->conflict_msg_shown = true; >> } else { >> - const char *reason = ref_transaction_error_msg(err); >> - >> - error(_("fetching ref %s failed: %s"), refname, reason); >> + if (details) >> + error("%s", details); >> + else >> + error(_("fetching ref %s failed: %s"), >> + refname, ref_transaction_error_msg(err)); >> } > > OK, so here we're writing to stderr anyway, and now we'll just give the > more detailed data. Makes sense (though like Junio, I do wonder if the > existing message might provide more details in some cases). > > BTW, I think there is still a related fallout for git-fetch. Even with > your patch, doing this: > > $ git fetch . v1.0.0:refs/heads/foo > From . > * [new tag] v1.0.0 -> foo > error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' > > will not put anything in the status table. Whereas in v2.50.0 and > earlier, we get: > > $ git.v2.50.0 fetch . v1.0.0:refs/heads/foo > error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' > From . > ! [new tag] v1.0.0 -> foo (unable to update local ref) > > Note the "!" and the "unable to update local ref" message in the status > table. > > -Peff This one is a bit harder to crack, earlier we were getting reference update results right as we added individual updates. Now that information is only received at the end when it is committed, we just don't have that information. One way is to delay this output until we commit everything. But we don't want to iterate over refs unnecessarily, so probably store these in a list, and then iterate over them. I'll try and add a patch for this too. Thanks for reporting. ``` ## Karthik Nayak, 2026-01-15 15:21 Subject: Re: [PATCH 6/6] receive-pack: utilize rejected ref error details Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZSCAJ-XPWK6vg3p7TO%3D3T3y8CD%2BVY4jqn41X2wbdmoaMg%40mail.gmail.com In-Reply-To: <20260114180306.GI885771@coredump.intra.peff.net> ``` Jeff King writes: > On Wed, Jan 14, 2026 at 04:40:47PM +0100, Karthik Nayak wrote: > >> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19), >> git-receive-pack(1) switched to using batched reference updates. This also >> introduced a regression wherein instead of providing detailed error >> 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: >> >> ! [remote rejected] 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d -> >> branch (cannot update ref 'refs/heads/branch': trying to write >> non-commit object 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d to branch >> 'refs/heads/branch') >> >> Here the refname is repeated multiple times due to how error messages >> are propagated and filled over the code stack. This potentially can be >> cleaned up in a future commit. > > If we are going to have a "potentially cleaned up in the future" state, > I think I would prefer to see just: > > if (details) > rp_error("%s", details); > > here. And then it comes over the stderr sideband, but the actual > status-table gets the same non-verbose message. That's what happened > in v2.50.0 and earlier. Later if we want to try to cram more details > into the machine-readable message we can. > > -Peff Fair enough, I think that would be a better approach for now, will change. ``` ## Jeff King, 2026-01-15 20:29 Subject: Re: [PATCH 2/6] refs: attach rejection details to updates Message-ID: <20260115202929.GC1053259@coredump.intra.peff.net> URL: https://gitlist.dev/e/20260115202929.GC1053259%40coredump.intra.peff.net In-Reply-To: ``` 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 ``` ## Karthik Nayak, 2026-01-16 17:56 Subject: Re: [PATCH 2/6] refs: attach rejection details to updates Message-ID: URL: https://gitlist.dev/e/CAOLa%3DZRbYBJaoFV7eWPsMGhVjqMDj%2B5-KMcAUPzM6fyXwp3qtg%40mail.gmail.com In-Reply-To: <20260115202929.GC1053259@coredump.intra.peff.net> ``` Jeff King writes: > 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! >> > 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. > 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'. > 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 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 0/6] refs: provide detailed error messages when using batched update Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98%40gmail.com In-Reply-To: <20260114-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v1-0-f5f8b173c501@gmail.com> ``` 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 v5: - In the last commit, drop 'const *' used to indicate immutability of fields within the struct. In the project it is more common to use 'const *' to indicate ownership. Since the memory of the fields are owned by the struct, let's drop the 'const *'. - Link to v4: https://patch.msgid.link/20260122-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v4-0-2ddba0832440@gmail.com Changes in v4: - In the last commit, instead of propagating {*list, count}, propagate an array with {*list, nr, count} and use ALLOC_GROW. This simplifies the variables passed and cleanups the code. - Link to v3: https://patch.msgid.link/20260120-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v3-0-e0edb29acbef@gmail.com Changes in v3: - Drop the first commit. - For the last commit, where we delay 'git fetch' status information, delay all information to the end. Also use a list to compliment the existing strmap, this ensures that the order is maintained. - Link to v2: https://patch.msgid.link/20260116-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v2-0-925a0e9c7f32@gmail.com 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 | 255 +++++++++++++++++++++++++++++++++++++----------- builtin/receive-pack.c | 7 +- builtin/update-ref.c | 7 +- refs.c | 46 +++++---- 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 +++ 12 files changed, 312 insertions(+), 125 deletions(-) Karthik Nayak (6): 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 transaction Range-diff versus v4: 1: 3264b8c3bf = 1: 661265fb86 refs: skip to next ref when current ref is rejected 2: 3d2af7a15a = 2: 1f0f2b6224 refs: add rejection detail to the callback function 3: bc7556f4b0 = 3: 8413ca46b5 update-ref: utilize rejected error details if available 4: d114f13967 = 4: b0c4441c55 fetch: utilize rejected ref error details 5: 4606c3b991 = 5: 8aa4477f51 receive-pack: utilize rejected ref error details 6: c75ccc40f3 ! 6: c9698e06bb fetch: delay user information post committing of transaction @@ builtin/fetch.c: static void display_ref_update(struct display_state *display_st + bool failed; + char success_code; + char fail_code; -+ const char *summary; -+ const char *fail_detail; -+ const char *success_detail; -+ const char *ref; -+ const char *remote; ++ char *summary; ++ char *fail_detail; ++ char *success_detail; ++ char *ref; ++ char *remote; + struct object_id old_oid; + struct object_id new_oid; +}; @@ builtin/fetch.c: static void display_ref_update(struct display_state *display_st + +static void ref_update_display_info_free(struct ref_update_display_info *info) +{ -+ free((char *)info->summary); -+ free((char *)info->success_detail); -+ free((char *)info->fail_detail); -+ free((char *)info->remote); -+ free((char *)info->ref); ++ free(info->summary); ++ free(info->success_detail); ++ free(info->fail_detail); ++ free(info->remote); ++ free(info->ref); +} + +static void ref_update_display_info_display(struct ref_update_display_info *info, base-commit: 8745eae506f700657882b9e32b2aa00f234a6fb6 change-id: 20260113-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-17786b20894a Thanks - Karthik ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 1/6] refs: skip to next ref when current ref is rejected Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-1-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-1-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` 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. 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. Signed-off-by: Karthik Nayak --- refs.c | 44 ++++++++++++++++++++++++++------------------ refs/files-backend.c | 5 ++--- refs/packed-backend.c | 12 ++++++------ refs/refs-internal.h | 4 +++- refs/reftable-backend.c | 5 ++--- 5 files changed, 39 insertions(+), 31 deletions(-) diff --git a/refs.c b/refs.c index e06e0cb072..53919c3d22 100644 --- a/refs.c +++ b/refs.c @@ -1224,6 +1224,7 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction->updates[i]->committer_info); free((char *)transaction->updates[i]->new_target); free((char *)transaction->updates[i]->old_target); + free((char *)transaction->updates[i]->rejection_details); free(transaction->updates[i]); } @@ -1238,7 +1239,8 @@ void ref_transaction_free(struct ref_transaction *transaction) int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err) + enum ref_transaction_error err, + struct strbuf *details) { if (update_idx >= transaction->nr) BUG("trying to set rejection on invalid update index"); @@ -1264,6 +1266,7 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, transaction->updates[update_idx]->refname, 0); transaction->updates[update_idx]->rejection_err = err; + transaction->updates[update_idx]->rejection_details = strbuf_detach(details, NULL); ALLOC_GROW(transaction->rejections->update_indices, transaction->rejections->nr + 1, transaction->rejections->alloc); @@ -2659,30 +2662,33 @@ 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)) { strset_remove(&dirnames, dirname.buf); strset_add(&conflicting_dirnames, dirname.buf); - continue; + goto next_ref; } - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - dirname.buf, refname); goto cleanup; } if (extras && string_list_has_string(extras, dirname.buf)) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, dirname.buf); + if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) { + REF_TRANSACTION_ERROR_NAME_CONFLICT, err)) { strset_remove(&dirnames, dirname.buf); - continue; + goto next_ref; } - strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), - refname, dirname.buf); goto cleanup; } } @@ -2712,14 +2718,14 @@ 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)) + goto next_ref; - strbuf_addf(err, _("'%s' exists; cannot create '%s'"), - iter->ref.name, refname); goto cleanup; } @@ -2729,15 +2735,17 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs extra_refname = find_descendant_ref(dirname.buf, extras, skip); if (extra_refname) { + strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"), + refname, extra_refname); + if (transaction && ref_transaction_maybe_set_rejected( transaction, *update_idx, - REF_TRANSACTION_ERROR_NAME_CONFLICT)) - continue; + 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_ref:; } ret = 0; diff --git a/refs/files-backend.c b/refs/files-backend.c index 6f6f76a8d8..6790d8bf53 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2983,10 +2983,9 @@ static int files_transaction_prepare(struct ref_store *ref_store, head_ref, &refnames_to_check, 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)) { ret = 0; - continue; } goto cleanup; diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 4ea0c12299..59b3ecb9d6 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1437,8 +1437,8 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re update->refname); 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)) { ret = 0; continue; } @@ -1452,8 +1452,8 @@ 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)) { ret = 0; continue; } @@ -1496,8 +1496,8 @@ 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)) { ret = 0; continue; } diff --git a/refs/refs-internal.h b/refs/refs-internal.h index c7d2a6e50b..191a25683f 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -128,6 +128,7 @@ struct ref_update { * was rejected. */ enum ref_transaction_error rejection_err; + const char *rejection_details; /* * If this ref_update was split off of a symref update via @@ -153,7 +154,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname, */ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction, size_t update_idx, - enum ref_transaction_error err); + enum ref_transaction_error err, + struct strbuf *details); /* * Add a ref_update with the specified properties to transaction, and diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 4319a4eacb..0e2648e36c 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1401,10 +1401,9 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, &refnames_to_check, head_type, &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)) { ret = 0; - continue; } goto done; -- 2.52.0 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 2/6] refs: add rejection detail to the callback function Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-2-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-2-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` The previous commit started storing the rejection details alongside the error code for rejected updates. Pass this along to the callback function `ref_transaction_for_each_rejected_update()`. Currently the field is unused, but will be integrated in the upcoming commits. Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/fetch.c | 1 + builtin/receive-pack.c | 1 + builtin/update-ref.c | 1 + refs.c | 2 +- refs.h | 1 + 5 files changed, 5 insertions(+), 1 deletion(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 288d3772ea..d427adea61 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1649,6 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data) { struct ref_rejection_data *data = cb_data; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index ef1f77be8c..94d3e73cee 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1813,6 +1813,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data) { struct strmap *failed_refs = cb_data; diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 195437e7c6..0046a87c57 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -573,6 +573,7 @@ static void print_rejected_refs(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, + const char *details UNUSED, void *cb_data UNUSED) { struct strbuf sb = STRBUF_INIT; diff --git a/refs.c b/refs.c index 53919c3d22..c85c3d2c8b 100644 --- a/refs.c +++ b/refs.c @@ -2874,7 +2874,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL, (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL, update->old_target, update->new_target, - update->rejection_err, cb_data); + update->rejection_err, update->rejection_details, cb_data); } } diff --git a/refs.h b/refs.h index d9051bbb04..4fbe3da924 100644 --- a/refs.h +++ b/refs.h @@ -975,6 +975,7 @@ typedef void ref_transaction_for_each_rejected_update_fn(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, + const char *details, void *cb_data); void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction, ref_transaction_for_each_rejected_update_fn cb, -- 2.52.0 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 3/6] update-ref: utilize rejected error details if available Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-3-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-3-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` When git-update-ref(1) received the '--update-ref' flag, the error details generated in the refs namespace wasn't propagated with failed updates. Instead only an error code pertaining to the type of rejection was noted. This missed detailed error message which the user can act upon. The previous commits added the required code to propagate these detailed error messages from the refs namespace. Now that additional details are available, let's output this additional details to stderr. This allows users to have additional information over the already present machine parsable output. While we're here, improve the existing tests for the machine parsable output by checking for the entire output string and not just the rejection reason. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/update-ref.c | 8 +++--- t/t1400-update-ref.sh | 71 ++++++++++++++++++++++++++++++--------------------- 2 files changed, 47 insertions(+), 32 deletions(-) diff --git a/builtin/update-ref.c b/builtin/update-ref.c index 0046a87c57..2d68c40ecb 100644 --- a/builtin/update-ref.c +++ b/builtin/update-ref.c @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname, const char *old_target, const char *new_target, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data UNUSED) { struct strbuf sb = STRBUF_INIT; - const char *reason = ref_transaction_error_msg(err); + + if (details && *details) + error("%s", details); strbuf_addf(&sb, "rejected %s %s %s %s\n", refname, new_oid ? oid_to_hex(new_oid) : new_target, old_oid ? oid_to_hex(old_oid) : old_target, - reason); + ref_transaction_error_msg(err)); fwrite(sb.buf, sb.len, 1, stdout); strbuf_release(&sb); diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index db7f5444da..db6585b8d8 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -2093,14 +2093,15 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$(test_oid 001)" "$head" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "invalid new value provided" stdout + test_grep "rejected refs/heads/ref2 $(test_oid 001) $head invalid new value provided" stdout && + test_grep "trying to write ref ${SQ}refs/heads/ref2${SQ} with nonexistent object" err ) ' @@ -2119,14 +2120,15 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$head_tree" "$head" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "invalid new value provided" stdout + test_grep "rejected refs/heads/ref2 $head_tree $head invalid new value provided" stdout && + test_grep "trying to write non-commit object $head_tree to branch ${SQ}refs/heads/ref2${SQ}" err ) ' @@ -2143,12 +2145,13 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$old_head" "$head" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && test_must_fail git rev-parse refs/heads/ref2 && - test_grep -q "reference does not exist" stdout + test_grep "rejected refs/heads/ref2 $old_head $head reference does not exist" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/ref2${SQ}: unable to resolve reference ${SQ}refs/heads/ref2${SQ}" err ) ' @@ -2166,13 +2169,14 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$old_head" "$head" >>stdin && - git update-ref $type --no-deref --stdin --batch-updates stdout && + git update-ref $type --no-deref --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && echo $head >expect && test_must_fail git rev-parse refs/heads/ref2 && - test_grep -q "reference does not exist" stdout + test_grep "rejected refs/heads/ref2 $old_head $head reference does not exist" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/ref2${SQ}: reference is missing but expected $head" err ) ' @@ -2190,7 +2194,7 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "symref-update refs/heads/ref2" "$old_head" "ref" "refs/heads/nonexistent" >>stdin && - git update-ref $type --no-deref --stdin --batch-updates stdout && + git update-ref $type --no-deref --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && @@ -2198,7 +2202,8 @@ do echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "expected symref but found regular ref" stdout + test_grep "rejected refs/heads/ref2 $ZERO_OID $ZERO_OID expected symref but found regular ref" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/ref2${SQ}: expected symref with target ${SQ}refs/heads/nonexistent${SQ}: but is a regular ref" err ) ' @@ -2216,14 +2221,15 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$old_head" "$Z" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "reference already exists" stdout + test_grep "rejected refs/heads/ref2 $old_head $ZERO_OID reference already exists" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/ref2${SQ}: reference already exists" err ) ' @@ -2241,14 +2247,15 @@ do format_command $type "update refs/heads/ref1" "$old_head" "$head" >stdin && format_command $type "update refs/heads/ref2" "$head" "$old_head" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref1 >actual && test_cmp expect actual && echo $head >expect && git rev-parse refs/heads/ref2 >actual && test_cmp expect actual && - test_grep -q "incorrect old value provided" stdout + test_grep "rejected refs/heads/ref2 $head $old_head incorrect old value provided" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/ref2${SQ}: is at $head but expected $old_head" err ) ' @@ -2264,12 +2271,13 @@ do git update-ref refs/heads/ref/foo $head && format_command $type "update refs/heads/ref/foo" "$old_head" "$head" >stdin && - format_command $type "update refs/heads/ref" "$old_head" "" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + format_command $type "update refs/heads/ref" "$old_head" "$ZERO_OID" >>stdin && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/ref/foo >actual && test_cmp expect actual && - test_grep -q "refname conflict" stdout + test_grep "rejected refs/heads/ref $old_head $ZERO_OID refname conflict" stdout && + test_grep "${SQ}refs/heads/ref/foo${SQ} exists; cannot create ${SQ}refs/heads/ref${SQ}" err ) ' @@ -2284,13 +2292,14 @@ do head=$(git rev-parse HEAD) && git update-ref refs/heads/ref/foo $head && - format_command $type "update refs/heads/foo" "$old_head" "" >stdin && - format_command $type "update refs/heads/ref" "$old_head" "" >>stdin && - git update-ref $type --stdin --batch-updates stdout && + format_command $type "update refs/heads/foo" "$old_head" "$ZERO_OID" >stdin && + format_command $type "update refs/heads/ref" "$old_head" "$ZERO_OID" >>stdin && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $old_head >expect && git rev-parse refs/heads/foo >actual && test_cmp expect actual && - test_grep -q "refname conflict" stdout + test_grep "rejected refs/heads/ref $old_head $ZERO_OID refname conflict" stdout && + test_grep "${SQ}refs/heads/ref/foo${SQ} exists; cannot create ${SQ}refs/heads/ref${SQ}" err ) ' @@ -2309,14 +2318,15 @@ do format_command $type "create refs/heads/ref" "$old_head" && format_command $type "create refs/heads/Foo" "$old_head" } >stdin && - git update-ref $type --stdin --batch-updates stdout && + git update-ref $type --stdin --batch-updates stdout 2>err && echo $head >expect && git rev-parse refs/heads/foo >actual && echo $old_head >expect && git rev-parse refs/heads/ref >actual && test_cmp expect actual && - test_grep -q "reference conflict due to case-insensitive filesystem" stdout + test_grep "rejected refs/heads/Foo $old_head $ZERO_OID reference conflict due to case-insensitive filesystem" stdout && + test_grep -e "cannot lock ref ${SQ}refs/heads/Foo${SQ}: Unable to create" -e "Foo.lock" err ) ' @@ -2357,8 +2367,9 @@ do git symbolic-ref refs/heads/symbolic refs/heads/non-existent && format_command $type "delete refs/heads/symbolic" "$head" >stdin && - git update-ref $type --stdin --batch-updates stdout && - test_grep "reference does not exist" stdout + git update-ref $type --stdin --batch-updates stdout 2>err && + test_grep "rejected refs/heads/non-existent $ZERO_OID $head reference does not exist" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/symbolic${SQ}: unable to resolve reference ${SQ}refs/heads/non-existent${SQ}" err ) ' @@ -2373,8 +2384,9 @@ do head=$(git rev-parse HEAD) && format_command $type "delete refs/heads/new-branch" "$head" >stdin && - git update-ref $type --stdin --batch-updates stdout && - test_grep "incorrect old value provided" stdout + git update-ref $type --stdin --batch-updates stdout 2>err && + test_grep "rejected refs/heads/new-branch $ZERO_OID $head incorrect old value provided" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/new-branch${SQ}: is at $(git rev-parse new-branch) but expected $head" err ) ' @@ -2387,8 +2399,9 @@ do head=$(git rev-parse HEAD) && format_command $type "delete refs/heads/non-existent" "$head" >stdin && - git update-ref $type --stdin --batch-updates stdout && - test_grep "reference does not exist" stdout + git update-ref $type --stdin --batch-updates stdout 2>err && + test_grep "rejected refs/heads/non-existent $ZERO_OID $head reference does not exist" stdout && + test_grep "cannot lock ref ${SQ}refs/heads/non-existent${SQ}: unable to resolve reference ${SQ}refs/heads/non-existent${SQ}" err ) ' done -- 2.52.0 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 4/6] fetch: utilize rejected ref error details Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-4-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-4-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` In 0e358de64a (fetch: use batched reference updates, 2025-05-19), git-fetch(1) switched to using batched reference updates. This also introduced a regression wherein instead of providing detailed error 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. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/fetch.c | 10 ++++++---- t/t5510-fetch.sh | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index d427adea61..49495be0b6 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1649,7 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data) { struct ref_rejection_data *data = cb_data; @@ -1674,9 +1674,11 @@ static void ref_transaction_rejection_handler(const char *refname, "branches"), data->remote_name); data->conflict_msg_shown = true; } else { - const char *reason = ref_transaction_error_msg(err); - - error(_("fetching ref %s failed: %s"), refname, reason); + if (details) + error("%s", details); + else + error(_("fetching ref %s failed: %s"), + refname, ref_transaction_error_msg(err)); } *data->retcode = 1; diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index ce1c23684e..c69afb5a60 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1516,7 +1516,7 @@ test_expect_success REFFILES 'existing reference lock in repo' ' git remote add origin ../base && touch refs/heads/foo.lock && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/branch >actual && test_cmp expect actual @@ -1530,7 +1530,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'F/D conflict on case insensiti cd case_insensitive && git remote add origin -- ../case_sensitive_fd && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "failed: refname conflict" err && + test_grep "cannot process ${SQ}refs/remotes/origin/foo${SQ} and ${SQ}refs/remotes/origin/foo/bar${SQ} at the same time" err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/foo/bar >actual && test_cmp expect actual @@ -1544,7 +1544,7 @@ test_expect_success CASE_INSENSITIVE_FS,REFFILES 'D/F conflict on case insensiti cd case_insensitive && git remote add origin -- ../case_sensitive_df && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "failed: refname conflict" err && + test_grep "cannot lock ref ${SQ}refs/remotes/origin/foo${SQ}: there is a non-empty directory ${SQ}./refs/remotes/origin/foo${SQ} blocking reference ${SQ}refs/remotes/origin/foo${SQ}" err && git rev-parse refs/heads/main >expect && git rev-parse refs/heads/Foo/bar >actual && test_cmp expect actual @@ -1658,7 +1658,7 @@ test_expect_success REFFILES "FETCH_HEAD is updated even if ref updates fail" ' git remote add origin ../base && >refs/heads/foo.lock && test_must_fail git fetch -f origin "refs/heads/*:refs/heads/*" 2>err && - test_grep "error: fetching ref refs/heads/foo failed: reference already exists" err && + test_grep -e "error: cannot lock ref ${SQ}refs/heads/foo${SQ}: Unable to create" -e "refs/heads/foo.lock${SQ}: File exists." err && test_grep "branch ${SQ}branch${SQ} of ../base" FETCH_HEAD && test_grep "branch ${SQ}foo${SQ} of ../base" FETCH_HEAD ) -- 2.52.0 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 5/6] receive-pack: utilize rejected ref error details Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-5-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-5-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19), git-receive-pack(1) switched to using batched reference updates. This also introduced a regression wherein instead of providing detailed error messages for failed referenced updates, the users were provided generic error messages based on the error type. 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 non-commit object 3eaec9ccf3a53f168362a6b3fdeb73426fb9813d to branch 'refs/heads/branch') Here the refname is repeated multiple times due to how error messages are propagated and filled over the code stack. This potentially can be cleaned up in a future commit. Reported-by: Elijah Newren Co-authored-by: Jeff King Signed-off-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/receive-pack.c | 8 ++++++-- t/t5516-fetch-push.sh | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 94d3e73cee..70e04b3efb 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1813,11 +1813,14 @@ static void ref_transaction_rejection_handler(const char *refname, const char *old_target UNUSED, const char *new_target UNUSED, enum ref_transaction_error err, - const char *details UNUSED, + const char *details, void *cb_data) { struct strmap *failed_refs = cb_data; + if (details) + rp_error("%s", details); + strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err)); } @@ -1884,6 +1887,7 @@ static void execute_commands_non_atomic(struct command *commands, } ref_transaction_for_each_rejected_update(transaction, + ref_transaction_rejection_handler, &failed_refs); @@ -1895,7 +1899,7 @@ static void execute_commands_non_atomic(struct command *commands, if (reported_error) cmd->error_string = reported_error; else if (strmap_contains(&failed_refs, cmd->ref_name)) - cmd->error_string = strmap_get(&failed_refs, cmd->ref_name); + cmd->error_string = cmd->error_string_owned = xstrdup(strmap_get(&failed_refs, cmd->ref_name)); } cleanup: diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 46926e7bbd..45595991c8 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1882,4 +1882,19 @@ test_expect_success 'push with F/D conflict with deletion and creation' ' git push testrepo :refs/heads/branch/conflict refs/heads/branch ' +test_expect_success 'pushing non-commit objects should report error' ' + test_when_finished "rm -rf dest repo" && + git init dest && + git init repo && + + ( + cd repo && + test_commit --annotate test && + + tagsha=$(git rev-parse test^{tag}) && + test_must_fail git push ../dest "$tagsha:refs/heads/branch" 2>err && + test_grep "trying to write non-commit object $tagsha to branch ${SQ}refs/heads/branch${SQ}" err + ) +' + test_done -- 2.52.0 ``` ## Karthik Nayak, 2026-01-25 22:52 Subject: [PATCH v5 6/6] fetch: delay user information post committing of transaction Message-ID: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-6-d58f3a9edf98@gmail.com> URL: https://gitlist.dev/e/20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-6-d58f3a9edf98%40gmail.com In-Reply-To: <20260125-633-regression-lost-diagnostic-message-when-pushing-non-commit-objects-to-refs-heads-v5-0-d58f3a9edf98@gmail.com> ``` In Git 2.50 and earlier, we would display failure codes and error message as part of the status display: $ git fetch . v1.0.0:refs/heads/foo error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' From . ! [new tag] v1.0.0 -> foo (unable to update local ref) With the addition of batched updates, this information is no longer shown to the user: $ git fetch . v1.0.0:refs/heads/foo From . * [new tag] v1.0.0 -> foo error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo' Since reference updates are batched and processed together at the end, information around the outcome is not available during individual reference parsing. To overcome this, collate and delay the output to the end. Introduce `ref_update_display_info` which will hold individual update's information and also whether the update failed or succeeded. This finally allows us to iterate over all such updates and print them to the user. Using an dynamic array and strmap does add some overhead to 'git-fetch(1)', but from benchmarking this seems to be not too bad: Benchmark 1: fetch: many refs (refformat = files, refcount = 1000, revision = master) Time (mean ± σ): 42.6 ms ± 1.2 ms [User: 13.1 ms, System: 29.8 ms] Range (min … max): 40.1 ms … 45.8 ms 47 runs Benchmark 2: fetch: many refs (refformat = files, refcount = 1000, revision = HEAD) Time (mean ± σ): 43.1 ms ± 1.2 ms [User: 12.7 ms, System: 30.7 ms] Range (min … max): 40.5 ms … 45.8 ms 48 runs Summary fetch: many refs (refformat = files, refcount = 1000, revision = master) ran 1.01 ± 0.04 times faster than fetch: many refs (refformat = files, refcount = 1000, revision = HEAD) Another approach would be to move the status printing logic to be handled post the transaction being committed. That however would require adding an iterator to the ref transaction that tracks both the outcome (success/failure) and the original refspec information for each update, which is more involved infrastructure work compared to the strmap approach here. Helped-by: Phillip Wood Reported-by: Jeff King Signed-off-by: Karthik Nayak --- builtin/fetch.c | 246 +++++++++++++++++++++++++++++++++++++++----------- t/t5516-fetch-push.sh | 1 + 2 files changed, 193 insertions(+), 54 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index 49495be0b6..a3bc7e9380 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -861,12 +861,87 @@ static void display_ref_update(struct display_state *display_state, char code, fputs(display_state->buf.buf, f); } +struct ref_update_display_info { + bool failed; + char success_code; + char fail_code; + char *summary; + char *fail_detail; + char *success_detail; + char *ref; + char *remote; + struct object_id old_oid; + struct object_id new_oid; +}; + +struct ref_update_display_info_array { + struct ref_update_display_info *info; + size_t alloc, nr; +}; + +static struct ref_update_display_info *ref_update_display_info_append( + struct ref_update_display_info_array *array, + char success_code, + char fail_code, + const char *summary, + const char *success_detail, + const char *fail_detail, + const char *ref, + const char *remote, + const struct object_id *old_oid, + const struct object_id *new_oid) +{ + struct ref_update_display_info *info; + + ALLOC_GROW(array->info, array->nr + 1, array->alloc); + info = &array->info[array->nr++]; + + info->failed = false; + info->success_code = success_code; + info->fail_code = fail_code; + info->summary = xstrdup(summary); + info->success_detail = xstrdup_or_null(success_detail); + info->fail_detail = xstrdup_or_null(fail_detail); + info->remote = xstrdup(remote); + info->ref = xstrdup(ref); + + oidcpy(&info->old_oid, old_oid); + oidcpy(&info->new_oid, new_oid); + + return info; +} + +static void ref_update_display_info_set_failed(struct ref_update_display_info *info) +{ + info->failed = true; +} + +static void ref_update_display_info_free(struct ref_update_display_info *info) +{ + free(info->summary); + free(info->success_detail); + free(info->fail_detail); + free(info->remote); + free(info->ref); +} + +static void ref_update_display_info_display(struct ref_update_display_info *info, + struct display_state *display_state, + int summary_width) +{ + display_ref_update(display_state, + info->failed ? info->fail_code : info->success_code, + info->summary, + info->failed ? info->fail_detail : info->success_detail, + info->remote, info->ref, &info->old_oid, + &info->new_oid, summary_width); +} + static int update_local_ref(struct ref *ref, struct ref_transaction *transaction, - struct display_state *display_state, const struct ref *remote_ref, - int summary_width, - const struct fetch_config *config) + const struct fetch_config *config, + struct ref_update_display_info_array *display_array) { struct commit *current = NULL, *updated; int fast_forward = 0; @@ -877,41 +952,56 @@ static int update_local_ref(struct ref *ref, if (oideq(&ref->old_oid, &ref->new_oid)) { if (verbosity > 0) - display_ref_update(display_state, '=', _("[up to date]"), NULL, - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + ref_update_display_info_append(display_array, '=', '=', + _("[up to date]"), NULL, + NULL, ref->name, + remote_ref->name, &ref->old_oid, + &ref->new_oid); return 0; } if (!update_head_ok && !is_null_oid(&ref->old_oid) && branch_checked_out(ref->name)) { + struct ref_update_display_info *info; /* * If this is the head, and it's not okay to update * the head, and the old value of the head isn't empty... */ - display_ref_update(display_state, '!', _("[rejected]"), - _("can't fetch into checked-out branch"), - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + info = ref_update_display_info_append(display_array, '!', '!', + _("[rejected]"), NULL, + _("can't fetch into checked-out branch"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + ref_update_display_info_set_failed(info); return 1; } if (!is_null_oid(&ref->old_oid) && starts_with(ref->name, "refs/tags/")) { + struct ref_update_display_info *info; + if (force || ref->force) { int r; + r = s_update_ref("updating tag", ref, transaction, 0); - display_ref_update(display_state, r ? '!' : 't', _("[tag update]"), - r ? _("unable to update local ref") : NULL, - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + + info = ref_update_display_info_append(display_array, 't', '!', + _("[tag update]"), NULL, + _("unable to update local ref"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + if (r) + ref_update_display_info_set_failed(info); + return r; } else { - display_ref_update(display_state, '!', _("[rejected]"), - _("would clobber existing tag"), - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + info = ref_update_display_info_append(display_array, '!', '!', + _("[rejected]"), NULL, + _("would clobber existing tag"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + ref_update_display_info_set_failed(info); return 1; } } @@ -921,6 +1011,7 @@ static int update_local_ref(struct ref *ref, updated = lookup_commit_reference_gently(the_repository, &ref->new_oid, 1); if (!current || !updated) { + struct ref_update_display_info *info; const char *msg; const char *what; int r; @@ -941,10 +1032,15 @@ static int update_local_ref(struct ref *ref, } r = s_update_ref(msg, ref, transaction, 0); - display_ref_update(display_state, r ? '!' : '*', what, - r ? _("unable to update local ref") : NULL, - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + + info = ref_update_display_info_append(display_array, '*', '!', + what, NULL, + _("unable to update local ref"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + if (r) + ref_update_display_info_set_failed(info); + return r; } @@ -960,6 +1056,7 @@ static int update_local_ref(struct ref *ref, } if (fast_forward) { + struct ref_update_display_info *info; struct strbuf quickref = STRBUF_INIT; int r; @@ -967,29 +1064,46 @@ static int update_local_ref(struct ref *ref, strbuf_addstr(&quickref, ".."); strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV); r = s_update_ref("fast-forward", ref, transaction, 1); - display_ref_update(display_state, r ? '!' : ' ', quickref.buf, - r ? _("unable to update local ref") : NULL, - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + + info = ref_update_display_info_append(display_array, ' ', '!', + quickref.buf, NULL, + _("unable to update local ref"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + if (r) + ref_update_display_info_set_failed(info); + strbuf_release(&quickref); return r; } else if (force || ref->force) { + struct ref_update_display_info *info; struct strbuf quickref = STRBUF_INIT; int r; + strbuf_add_unique_abbrev(&quickref, ¤t->object.oid, DEFAULT_ABBREV); strbuf_addstr(&quickref, "..."); strbuf_add_unique_abbrev(&quickref, &ref->new_oid, DEFAULT_ABBREV); r = s_update_ref("forced-update", ref, transaction, 1); - display_ref_update(display_state, r ? '!' : '+', quickref.buf, - r ? _("unable to update local ref") : _("forced update"), - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + + info = ref_update_display_info_append(display_array, '+', '!', + quickref.buf, _("forced update"), + _("unable to update local ref"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + + if (r) + ref_update_display_info_set_failed(info); + strbuf_release(&quickref); return r; } else { - display_ref_update(display_state, '!', _("[rejected]"), _("non-fast-forward"), - remote_ref->name, ref->name, - &ref->old_oid, &ref->new_oid, summary_width); + struct ref_update_display_info *info; + info = ref_update_display_info_append(display_array, '!', '!', + _("[rejected]"), NULL, + _("non-fast-forward"), + ref->name, remote_ref->name, + &ref->old_oid, &ref->new_oid); + ref_update_display_info_set_failed(info); return 1; } } @@ -1103,17 +1217,14 @@ static int store_updated_refs(struct display_state *display_state, int connectivity_checked, struct ref_transaction *transaction, struct ref *ref_map, struct fetch_head *fetch_head, - const struct fetch_config *config) + const struct fetch_config *config, + struct ref_update_display_info_array *display_array) { int rc = 0; struct strbuf note = STRBUF_INIT; const char *what, *kind; struct ref *rm; int want_status; - int summary_width = 0; - - if (verbosity >= 0) - summary_width = transport_summary_width(ref_map); if (!connectivity_checked) { struct check_connected_options opt = CHECK_CONNECTED_INIT; @@ -1218,8 +1329,8 @@ static int store_updated_refs(struct display_state *display_state, display_state->url_len); if (ref) { - rc |= update_local_ref(ref, transaction, display_state, - rm, summary_width, config); + rc |= update_local_ref(ref, transaction, rm, + config, display_array); free(ref); } else if (write_fetch_head || dry_run) { /* @@ -1227,12 +1338,12 @@ static int store_updated_refs(struct display_state *display_state, * would be written to FETCH_HEAD, if --dry-run * is set). */ - display_ref_update(display_state, '*', - *kind ? kind : "branch", NULL, - rm->name, - "FETCH_HEAD", - &rm->new_oid, &rm->old_oid, - summary_width); + + ref_update_display_info_append(display_array, '*', '*', + *kind ? kind : "branch", + NULL, NULL, "FETCH_HEAD", + rm->name, &rm->new_oid, + &rm->old_oid); } } } @@ -1300,7 +1411,8 @@ static int fetch_and_consume_refs(struct display_state *display_state, struct ref_transaction *transaction, struct ref *ref_map, struct fetch_head *fetch_head, - const struct fetch_config *config) + const struct fetch_config *config, + struct ref_update_display_info_array *display_array) { int connectivity_checked = 1; int ret; @@ -1322,7 +1434,8 @@ static int fetch_and_consume_refs(struct display_state *display_state, trace2_region_enter("fetch", "consume_refs", the_repository); ret = store_updated_refs(display_state, connectivity_checked, - transaction, ref_map, fetch_head, config); + transaction, ref_map, fetch_head, config, + display_array); trace2_region_leave("fetch", "consume_refs", the_repository); out: @@ -1493,7 +1606,8 @@ static int backfill_tags(struct display_state *display_state, struct ref_transaction *transaction, struct ref *ref_map, struct fetch_head *fetch_head, - const struct fetch_config *config) + const struct fetch_config *config, + struct ref_update_display_info_array *display_array) { int retcode, cannot_reuse; @@ -1515,7 +1629,7 @@ static int backfill_tags(struct display_state *display_state, transport_set_option(transport, TRANS_OPT_DEPTH, "0"); transport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL); retcode = fetch_and_consume_refs(display_state, transport, transaction, ref_map, - fetch_head, config); + fetch_head, config, display_array); if (gsecondary) { transport_disconnect(gsecondary); @@ -1641,6 +1755,7 @@ struct ref_rejection_data { bool conflict_msg_shown; bool case_sensitive_msg_shown; const char *remote_name; + struct strmap *rejected_refs; }; static void ref_transaction_rejection_handler(const char *refname, @@ -1681,6 +1796,7 @@ static void ref_transaction_rejection_handler(const char *refname, refname, ref_transaction_error_msg(err)); } + strmap_put(data->rejected_refs, refname, NULL); *data->retcode = 1; } @@ -1690,6 +1806,7 @@ static void ref_transaction_rejection_handler(const char *refname, */ static int commit_ref_transaction(struct ref_transaction **transaction, bool is_atomic, const char *remote_name, + struct strmap *rejected_refs, struct strbuf *err) { int retcode = ref_transaction_commit(*transaction, err); @@ -1701,6 +1818,7 @@ static int commit_ref_transaction(struct ref_transaction **transaction, .conflict_msg_shown = 0, .remote_name = remote_name, .retcode = &retcode, + .rejected_refs = rejected_refs, }; ref_transaction_for_each_rejected_update(*transaction, @@ -1729,6 +1847,9 @@ static int do_fetch(struct transport *transport, struct fetch_head fetch_head = { 0 }; struct strbuf err = STRBUF_INIT; int do_set_head = 0; + struct ref_update_display_info_array display_array = { 0 }; + struct strmap rejected_refs = STRMAP_INIT; + int summary_width = 0; if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2) @@ -1853,7 +1974,7 @@ static int do_fetch(struct transport *transport, } if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map, - &fetch_head, config)) { + &fetch_head, config, &display_array)) { retcode = 1; goto cleanup; } @@ -1876,7 +1997,7 @@ static int do_fetch(struct transport *transport, * the transaction and don't commit anything. */ if (backfill_tags(&display_state, transport, transaction, tags_ref_map, - &fetch_head, config)) + &fetch_head, config, &display_array)) retcode = 1; } @@ -1886,8 +2007,12 @@ static int do_fetch(struct transport *transport, if (retcode) goto cleanup; + if (verbosity >= 0) + summary_width = transport_summary_width(ref_map); + retcode = commit_ref_transaction(&transaction, atomic_fetch, - transport->remote->name, &err); + transport->remote->name, + &rejected_refs, &err); /* * With '--atomic', bail out if the transaction fails. Without '--atomic', * continue to fetch head and perform other post-fetch operations. @@ -1965,7 +2090,17 @@ static int do_fetch(struct transport *transport, */ if (retcode && !atomic_fetch && transaction) commit_ref_transaction(&transaction, false, - transport->remote->name, &err); + transport->remote->name, + &rejected_refs, &err); + + for (size_t i = 0; i < display_array.nr; i++) { + struct ref_update_display_info *info = &display_array.info[i]; + + if (!info->failed && strmap_contains(&rejected_refs, info->ref)) + ref_update_display_info_set_failed(info); + ref_update_display_info_display(info, &display_state, summary_width); + ref_update_display_info_free(info); + } if (retcode) { if (err.len) { @@ -1980,6 +2115,9 @@ static int do_fetch(struct transport *transport, if (transaction) ref_transaction_free(transaction); + + free(display_array.info); + strmap_clear(&rejected_refs, 0); display_state_release(&display_state); close_fetch_head(&fetch_head); strbuf_release(&err); diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 45595991c8..29e2f17608 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1893,6 +1893,7 @@ test_expect_success 'pushing non-commit objects should report error' ' tagsha=$(git rev-parse test^{tag}) && test_must_fail git push ../dest "$tagsha:refs/heads/branch" 2>err && + test_grep "! \[remote rejected\] $tagsha -> branch (invalid new value provided)" err && test_grep "trying to write non-commit object $tagsha to branch ${SQ}refs/heads/branch${SQ}" err ) ' -- 2.52.0 ```