{"thread":{"id":"64676","subject":"Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","startedAt":"2025-12-24T03:32:40Z","lastAt":"2025-12-27T07:44:12Z","messageCount":6,"participants":["Elijah Newren","Junio C Hamano","Jeff King","Karthik Nayak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"532684","messageId":"CABPp-BGL2tJR4dPidQuFcp-X0_VkVTknCY-0Zgo=jHVGv_P=wA@mail.gmail.com","threadId":"64676","inReplyTo":null,"subject":"Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-12-24T03:32:28Z","receivedAt":"2025-12-24T03:32:40Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\ngit used to have better diagnostics about pushing non-commit objects\nto refs/heads/*, dating all the way back to c3b0dec509fe (Be more\ncareful about updating refs, 2008-01-15):\n\n$ git --version && git push . tagit:old\ngit version 2.50.1\nTotal 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)\nremote: error: cannot update ref 'refs/heads/old': trying to write\nnon-commit object d19968fcf0d3193147b827c9e89668d619afd01e to branch\n'refs/heads/old'\nTo .\n ! [remote rejected] tagit -> old (failed to update ref)\nerror: failed to push some refs to '.'\n\nUnfortunately, the \"trying to write non-commit object\" error is no longer shown:\n\n$ git --version && git push . tagit:old\ngit version 2.51.0\nTotal 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)\nTo .\n ! [remote rejected] tagit -> old (invalid new value provided)\nerror: failed to push some refs to '.'\n\nThe relevant error message is still part of the code:\n$ git grep \"write non-commit object\" -- '*.c'\nrefs/files-backend.c:                           \"trying to write\nnon-commit object %s to branch '%s'\",\nrefs/reftable-backend.c:                        strbuf_addf(err,\n_(\"trying to write non-commit object %s to branch '%s'\"),\n\nbut the error message isn't displayed.  Bisecting shows that this\nstarted with commit 9d2962a7c44 (\"receive-pack: use batched reference\nupdates\", 2025-05-19).  That commit message to me suggests that while\nerror handling was necessarily changed, that dropping the errors was\nnot intentional:\n\n```\nAs using batched updates requires the error handling to be moved to the\nend of the flow, create and use a 'struct strset' to track the failed\nrefs and attribute the correct errors to them.\n```\n\nBut it's possible I'm reading it wrong.  Was it intentional, or is\nthis a regression?\n"},{"id":"532685","messageId":"xmqqikdwo8i1.fsf@gitster.g","threadId":"64676","inReplyTo":"CABPp-BGL2tJR4dPidQuFcp-X0_VkVTknCY-0Zgo=jHVGv_P=wA@mail.gmail.com","subject":"Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-12-24T03:37:26Z","receivedAt":"2025-12-24T03:37:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren <newren@gmail.com> writes:\n\n> Bisecting shows that this\n> started with commit 9d2962a7c44 (\"receive-pack: use batched reference\n> updates\", 2025-05-19).  That commit message to me suggests that while\n> error handling was necessarily changed, that dropping the errors was\n> not intentional:\n>\n> ```\n> As using batched updates requires the error handling to be moved to the\n> end of the flow, create and use a 'struct strset' to track the failed\n> refs and attribute the correct errors to them.\n> ```\n>\n> But it's possible I'm reading it wrong.  Was it intentional, or is\n> this a regression?\n\nThe topic bisect found was supposed to be purely performance\noptimization, and we should take any changes in behaviour as\nregressions.\n\nThanks.\n"},{"id":"532689","messageId":"20251224081214.GA1879908@coredump.intra.peff.net","threadId":"64676","inReplyTo":"CABPp-BGL2tJR4dPidQuFcp-X0_VkVTknCY-0Zgo=jHVGv_P=wA@mail.gmail.com","subject":"Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-12-24T08:12:14Z","receivedAt":"2025-12-24T08:12:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 23, 2025 at 07:32:28PM -0800, Elijah Newren wrote:\n\n> The relevant error message is still part of the code:\n> $ git grep \"write non-commit object\" -- '*.c'\n> refs/files-backend.c:                           \"trying to write\n> non-commit object %s to branch '%s'\",\n> refs/reftable-backend.c:                        strbuf_addf(err,\n> _(\"trying to write non-commit object %s to branch '%s'\"),\n> \n> but the error message isn't displayed.  Bisecting shows that this\n> started with commit 9d2962a7c44 (\"receive-pack: use batched reference\n> updates\", 2025-05-19).  That commit message to me suggests that while\n> error handling was necessarily changed, that dropping the errors was\n> not intentional:\n> \n> ```\n> As using batched updates requires the error handling to be moved to the\n> end of the flow, create and use a 'struct strset' to track the failed\n> refs and attribute the correct errors to them.\n> ```\n> \n> But it's possible I'm reading it wrong.  Was it intentional, or is\n> this a regression?\n\nI didn't participate in this topic beyond a few memory-management\nnitpicks, but my gut feeling is that yes, this is a regression.\n\nWe still format that specific error into an \"err\" strbuf inside the refs\ncode. In the older version, we returned up the stack and eventually\nprinted the error from the failed transaction to stderr (or in the case\nof receive-pack, the sideband).\n\nBut in the new batched world that allows partial-batch failures, we\nthrow it away. The problem (at least for the files backend) is this code\nin files_transaction_prepare():\n\n          ret = lock_ref_for_update(refs, update, i, transaction,\n                                    head_ref, &refnames_to_check,\n                                    err);\n          if (ret) {\n                  if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n                          strbuf_reset(err);\n                          ret = 0;\n\n                          continue;\n                  }\n                  goto cleanup;\n          }\n\nWe see the error from lock_ref_for_update() as before, and the useful\nmessage is in \"err\" here. But ref_transaction_maybe_set_rejected() only\nlooks at \"ret\", the numeric error code, and decides that it is enough to\nrecord that.\n\nWe should do something useful with the \"err\" string that was collected,\nrather than immediately calling strbuf_reset() to throw it away.\n\nUnfortunately we can't just dump it to stderr here, since we don't know\nwhat our caller would want to do with the error (and in fact for\nreceive-pack we eventually want to call rp_error() which dumps it over\nthe sideband). So we have to return it back to the caller somehow.\n\nWe only get one \"err\" string to return for the whole transaction. We can\nplay some games to make it multi-line, like this:\n\n  diff --git a/refs/files-backend.c b/refs/files-backend.c\n  index 6f6f76a8d8..6ad57e53c0 100644\n  --- a/refs/files-backend.c\n  +++ b/refs/files-backend.c\n  @@ -2978,13 +2978,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n   \t */\n   \tfor (i = 0; i < transaction->nr; i++) {\n   \t\tstruct ref_update *update = transaction->updates[i];\n  +\t\tstruct strbuf this_err = STRBUF_INIT;\n   \n   \t\tret = lock_ref_for_update(refs, update, i, transaction,\n   \t\t\t\t\t  head_ref, &refnames_to_check,\n  -\t\t\t\t\t  err);\n  +\t\t\t\t\t  &this_err);\n   \t\tif (ret) {\n   \t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n  -\t\t\t\tstrbuf_reset(err);\n  +\t\t\t\tif (err->len)\n  +\t\t\t\t\tstrbuf_addch(err, '\\n');\n  +\t\t\t\tstrbuf_addbuf(err, &this_err);\n  +\t\t\t\tstrbuf_release(&this_err);\n   \t\t\t\tret = 0;\n   \n   \t\t\t\tcontinue;\n\nbut even that is not quite enough. We still return success from the\noverall transaction, because we allow failures within the batch! So now\nour caller does not even look at \"err\" at all.\n\nSo I guess we need to attach the failure more directly to the failed\nref. Probably ref_transaction_maybe_set_rejected() should take the error\nstring along with the ref_transaction_error enum, and attach it to the\nfailed item. We also need to get those details out to the callers, which\nuse ref_transaction_for_each_rejected_update(). So that interface needs\nto be expanded to pass out the details string.\n\nAnd then receive-pack can either dump it via rp_error(), giving the same\nbehavior as the old version. Or it can stick it into the per-ref status\nfield. The latter feels more \"right\" in the sense that the error\nmessages can be reliably attached to specific ref updates in the\nmachine-readable output (rather than appearing willy-nilly on stderr or\nsideband 2). But I'd guess it would make the output rather unwieldy.\n\nThe patch below does the sideband dumping, and gets back the message in\nthis toy example. But as the inline comments show, it would probably\nneed support in a few other spots (both generating the detailed err\nmessages, and then showing them at the right spots). Plus it has a big\nmemory leak, in that nobody ever frees the detail strings. ;)\n\nSo consider it just a sketch. I'm hoping Karthik can pick it up from\nhere.\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 288d3772ea..315e791193 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1649,6 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t\t\t\t\t      const char *old_target UNUSED,\n \t\t\t\t\t      const char *new_target UNUSED,\n \t\t\t\t\t      enum ref_transaction_error err,\n+\t\t\t\t\t      const char *details,\n \t\t\t\t\t      void *cb_data)\n {\n \tstruct ref_rejection_data *data = cb_data;\n@@ -1675,6 +1676,8 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t} else {\n \t\tconst char *reason = ref_transaction_error_msg(err);\n \n+\t\t/* probably should show \"details\" string here? */\n+\n \t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n \t}\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 9c49174616..96daf54a09 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1853,10 +1853,12 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t\t\t\t\t      const char *old_target UNUSED,\n \t\t\t\t\t      const char *new_target UNUSED,\n \t\t\t\t\t      enum ref_transaction_error err,\n+\t\t\t\t\t      const char *details,\n \t\t\t\t\t      void *cb_data)\n {\n \tstruct strmap *failed_refs = cb_data;\n \n+\trp_error(\"%s\", details);\n \tstrmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));\n }\n \ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 195437e7c6..6ef53d9886 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -573,11 +573,14 @@ static void print_rejected_refs(const char *refname,\n \t\t\t\tconst char *old_target,\n \t\t\t\tconst char *new_target,\n \t\t\t\tenum ref_transaction_error err,\n+\t\t\t\tconst char *details,\n \t\t\t\tvoid *cb_data UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *reason = ref_transaction_error_msg(err);\n \n+\t/* do something with \"details\" string here? */\n+\n \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n \t\t    new_oid ? oid_to_hex(new_oid) : new_target,\n \t\t    old_oid ? oid_to_hex(old_oid) : old_target,\ndiff --git a/refs.c b/refs.c\nindex 046b695bb2..adf01c527b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1238,7 +1238,8 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \n int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n \t\t\t\t       size_t update_idx,\n-\t\t\t\t       enum ref_transaction_error err)\n+\t\t\t\t       enum ref_transaction_error err,\n+\t\t\t\t       struct strbuf *details)\n {\n \tif (update_idx >= transaction->nr)\n \t\tBUG(\"trying to set rejection on invalid update index\");\n@@ -1264,6 +1265,9 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n \t\t\t   transaction->updates[update_idx]->refname, 0);\n \n \ttransaction->updates[update_idx]->rejection_err = err;\n+\tif (details)\n+\t\ttransaction->updates[update_idx]->rejection_details =\n+\t\t\tstrbuf_detach(details, NULL);\n \tALLOC_GROW(transaction->rejections->update_indices,\n \t\t   transaction->rejections->nr + 1,\n \t\t   transaction->rejections->alloc);\n@@ -2658,7 +2662,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n \t\t\t\t\t\t       &type, &ignore_errno))) {\n \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n \t\t\t\t\t    transaction, *update_idx,\n-\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {\n+\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n+\t\t\t\t\t    NULL)) {\n \t\t\t\t\tstrset_remove(&dirnames, dirname.buf);\n \t\t\t\t\tstrset_add(&conflicting_dirnames, dirname.buf);\n \t\t\t\t\tcontinue;\n@@ -2672,7 +2677,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n \t\t\tif (extras && string_list_has_string(extras, dirname.buf)) {\n \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n \t\t\t\t\t    transaction, *update_idx,\n-\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {\n+\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n+\t\t\t\t\t    NULL)) {\n \t\t\t\t\tstrset_remove(&dirnames, dirname.buf);\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n@@ -2709,9 +2715,12 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n \t\t\t\t    string_list_has_string(skip, iter->ref.name))\n \t\t\t\t\tcontinue;\n \n+\t\t\t\t/* should we be formatting err first here and\n+\t\t\t\t * passing it in? */\n \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n \t\t\t\t\t    transaction, *update_idx,\n-\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT))\n+\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n+\t\t\t\t\t    NULL))\n \t\t\t\t\tcontinue;\n \n \t\t\t\tstrbuf_addf(err, _(\"'%s' exists; cannot create '%s'\"),\n@@ -2725,9 +2734,10 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n \n \t\textra_refname = find_descendant_ref(dirname.buf, extras, skip);\n \t\tif (extra_refname) {\n+\t\t\t/* format err first and pass it in? */\n \t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n \t\t\t\t    transaction, *update_idx,\n-\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT))\n+\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT, NULL))\n \t\t\t\tcontinue;\n \n \t\t\tstrbuf_addf(err, _(\"cannot process '%s' and '%s' at the same time\"),\n@@ -2862,7 +2872,8 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio\n \t\t   (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n \t\t   (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n \t\t   update->old_target, update->new_target,\n-\t\t   update->rejection_err, cb_data);\n+\t\t   update->rejection_err, update->rejection_details,\n+\t\t   cb_data);\n \t}\n }\n \ndiff --git a/refs.h b/refs.h\nindex d9051bbb04..4fbe3da924 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -975,6 +975,7 @@ typedef void ref_transaction_for_each_rejected_update_fn(const char *refname,\n \t\t\t\t\t\t\t const char *old_target,\n \t\t\t\t\t\t\t const char *new_target,\n \t\t\t\t\t\t\t enum ref_transaction_error err,\n+\t\t\t\t\t\t\t const char *details,\n \t\t\t\t\t\t\t void *cb_data);\n void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n \t\t\t\t\t      ref_transaction_for_each_rejected_update_fn cb,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6f6f76a8d8..b58e3a3664 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2983,8 +2983,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t  head_ref, &refnames_to_check,\n \t\t\t\t\t  err);\n \t\tif (ret) {\n-\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n-\t\t\t\tstrbuf_reset(err);\n+\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n \t\t\t\tret = 0;\n \n \t\t\t\tcontinue;\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 4ea0c12299..b4b124a674 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1437,8 +1437,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t\t\t\t\t    update->refname);\n \t\t\t\t\tret = REF_TRANSACTION_ERROR_CREATE_EXISTS;\n \n-\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n-\t\t\t\t\t\tstrbuf_reset(err);\n+\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n \t\t\t\t\t\tret = 0;\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\t}\n@@ -1452,8 +1451,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t\t\t\t\t    oid_to_hex(&update->old_oid));\n \t\t\t\t\tret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE;\n \n-\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n-\t\t\t\t\t\tstrbuf_reset(err);\n+\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n \t\t\t\t\t\tret = 0;\n \t\t\t\t\t\tcontinue;\n \t\t\t\t\t}\n@@ -1496,8 +1494,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t\t\t\t    oid_to_hex(&update->old_oid));\n \t\t\t\tret = REF_TRANSACTION_ERROR_NONEXISTENT_REF;\n \n-\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n-\t\t\t\t\tstrbuf_reset(err);\n+\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n \t\t\t\t\tret = 0;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex c7d2a6e50b..c5c121cc1c 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -128,6 +128,7 @@ struct ref_update {\n \t * was rejected.\n \t */\n \tenum ref_transaction_error rejection_err;\n+\tchar *rejection_details;\n \n \t/*\n \t * If this ref_update was split off of a symref update via\n@@ -153,7 +154,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n  */\n int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n \t\t\t\t       size_t update_idx,\n-\t\t\t\t       enum ref_transaction_error err);\n+\t\t\t\t       enum ref_transaction_error err,\n+\t\t\t\t       struct strbuf *details);\n \n /*\n  * Add a ref_update with the specified properties to transaction, and\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 4319a4eacb..8b04a5e11c 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1401,8 +1401,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t    &refnames_to_check, head_type,\n \t\t\t\t\t    &head_referent, &referent, err);\n \t\tif (ret) {\n-\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n-\t\t\t\tstrbuf_reset(err);\n+\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n \t\t\t\tret = 0;\n \n \t\t\t\tcontinue;\n\n"},{"id":"532690","messageId":"20251224082116.GA1946629@coredump.intra.peff.net","threadId":"64676","inReplyTo":"20251224081214.GA1879908@coredump.intra.peff.net","subject":"Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-12-24T08:21:16Z","receivedAt":"2025-12-24T08:21:18Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 24, 2025 at 03:12:14AM -0500, Jeff King wrote:\n\n> But in the new batched world that allows partial-batch failures, we\n> throw it away. The problem (at least for the files backend) is this code\n> in files_transaction_prepare():\n> \n>           ret = lock_ref_for_update(refs, update, i, transaction,\n>                                     head_ref, &refnames_to_check,\n>                                     err);\n>           if (ret) {\n>                   if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n>                           strbuf_reset(err);\n>                           ret = 0;\n> \n>                           continue;\n>                   }\n>                   goto cleanup;\n>           }\n\nBTW, you found the regression via receive-pack, but as you can see here\nit is really a problem for any batched ref-update caller that sets the\nALLOW_FAILURE flag. So the original sin is not from the commit you found\nvia bisect, but 23fc8e4f61 (refs: implement batch reference update\nsupport, 2025-04-08). And it affects fetch, too:\n\n  $ git.v2.50.0 fetch . v1.0.0:refs/heads/foo\n  error: cannot update ref 'refs/heads/foo': trying to write non-commit object f665776185ad074b236c00751d666da7d1977dbe to branch 'refs/heads/foo'\n  From .\n   ! [new tag]               v1.0.0     -> foo  (unable to update local ref)\n\n  $ git.v2.51.0 fetch . v1.0.0:refs/heads/foo\n  From .\n   * [new tag]               v1.0.0     -> foo\n  error: fetching ref refs/heads/foo failed: invalid new value provided\n\nActually, I think there is another bug lurking there for fetch. We do\nnot even mark the failure in the status output anymore!\n\nAnd I guess \"update-ref --batch-updates\" suffers from the same lack of\ndetail:\n\n  $ echo create refs/heads/foo v1.0.0 | git update-ref --batch-updates --stdin\n  rejected refs/heads/foo f665776185ad074b236c00751d666da7d1977dbe 0000000000000000000000000000000000000000 invalid new value provided\n\nthough it is not technically a regression since the option to ask for\nALLOW_FAILURE did not even exist before --batch-updates. It would be\nnice if it gave more details (whether to stderr or in the\nmachine-readable output).\n\n-Peff\n"},{"id":"532766","messageId":"CAOLa=ZSOZz9aGFFeD7tiQ+PRwkMosjcoxfTSk52fQeQq0ghgaw@mail.gmail.com","threadId":"64676","inReplyTo":"20251224081214.GA1879908@coredump.intra.peff.net","subject":"Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-12-26T16:48:19Z","receivedAt":"2025-12-26T16:48:21Z","isPatch":false,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Dec 23, 2025 at 07:32:28PM -0800, Elijah Newren wrote:\n>\n>> The relevant error message is still part of the code:\n>> $ git grep \"write non-commit object\" -- '*.c'\n>> refs/files-backend.c:                           \"trying to write\n>> non-commit object %s to branch '%s'\",\n>> refs/reftable-backend.c:                        strbuf_addf(err,\n>> _(\"trying to write non-commit object %s to branch '%s'\"),\n>>\n>> but the error message isn't displayed.  Bisecting shows that this\n>> started with commit 9d2962a7c44 (\"receive-pack: use batched reference\n>> updates\", 2025-05-19).  That commit message to me suggests that while\n>> error handling was necessarily changed, that dropping the errors was\n>> not intentional:\n>>\n>> ```\n>> As using batched updates requires the error handling to be moved to the\n>> end of the flow, create and use a 'struct strset' to track the failed\n>> refs and attribute the correct errors to them.\n>> ```\n>>\n>> But it's possible I'm reading it wrong.  Was it intentional, or is\n>> this a regression?\n>\n> I didn't participate in this topic beyond a few memory-management\n> nitpicks, but my gut feeling is that yes, this is a regression.\n>\n\nDefinitely a regression. We did find a few regressions with this topic,\nthe only  bright side being fixing those also added missing tests which\nwould catch further breakages.\n\n> We still format that specific error into an \"err\" strbuf inside the refs\n> code. In the older version, we returned up the stack and eventually\n> printed the error from the failed transaction to stderr (or in the case\n> of receive-pack, the sideband).\n>\n> But in the new batched world that allows partial-batch failures, we\n> throw it away. The problem (at least for the files backend) is this code\n> in files_transaction_prepare():\n>\n>           ret = lock_ref_for_update(refs, update, i, transaction,\n>                                     head_ref, &refnames_to_check,\n>                                     err);\n>           if (ret) {\n>                   if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n>                           strbuf_reset(err);\n>                           ret = 0;\n>\n>                           continue;\n>                   }\n>                   goto cleanup;\n>           }\n>\n> We see the error from lock_ref_for_update() as before, and the useful\n> message is in \"err\" here. But ref_transaction_maybe_set_rejected() only\n> looks at \"ret\", the numeric error code, and decides that it is enough to\n> record that.\n>\n> We should do something useful with the \"err\" string that was collected,\n> rather than immediately calling strbuf_reset() to throw it away.\n>\n> Unfortunately we can't just dump it to stderr here, since we don't know\n> what our caller would want to do with the error (and in fact for\n> receive-pack we eventually want to call rp_error() which dumps it over\n> the sideband). So we have to return it back to the caller somehow.\n>\n> We only get one \"err\" string to return for the whole transaction. We can\n> play some games to make it multi-line, like this:\n>\n>   diff --git a/refs/files-backend.c b/refs/files-backend.c\n>   index 6f6f76a8d8..6ad57e53c0 100644\n>   --- a/refs/files-backend.c\n>   +++ b/refs/files-backend.c\n>   @@ -2978,13 +2978,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n>    \t */\n>    \tfor (i = 0; i < transaction->nr; i++) {\n>    \t\tstruct ref_update *update = transaction->updates[i];\n>   +\t\tstruct strbuf this_err = STRBUF_INIT;\n>\n>    \t\tret = lock_ref_for_update(refs, update, i, transaction,\n>    \t\t\t\t\t  head_ref, &refnames_to_check,\n>   -\t\t\t\t\t  err);\n>   +\t\t\t\t\t  &this_err);\n>    \t\tif (ret) {\n>    \t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n>   -\t\t\t\tstrbuf_reset(err);\n>   +\t\t\t\tif (err->len)\n>   +\t\t\t\t\tstrbuf_addch(err, '\\n');\n>   +\t\t\t\tstrbuf_addbuf(err, &this_err);\n>   +\t\t\t\tstrbuf_release(&this_err);\n>    \t\t\t\tret = 0;\n>\n>    \t\t\t\tcontinue;\n>\n> but even that is not quite enough. We still return success from the\n> overall transaction, because we allow failures within the batch! So now\n> our caller does not even look at \"err\" at all.\n>\n\nYup, that's my understanding as well, we'd want to pass the error per\nupdate being performed.\n\n> So I guess we need to attach the failure more directly to the failed\n> ref. Probably ref_transaction_maybe_set_rejected() should take the error\n> string along with the ref_transaction_error enum, and attach it to the\n> failed item. We also need to get those details out to the callers, which\n> use ref_transaction_for_each_rejected_update(). So that interface needs\n> to be expanded to pass out the details string.\n>\n\nYup, this was what I was thinking of too, and your exploration here\nseems to be on the same line.\n\n> And then receive-pack can either dump it via rp_error(), giving the same\n> behavior as the old version. Or it can stick it into the per-ref status\n> field. The latter feels more \"right\" in the sense that the error\n> messages can be reliably attached to specific ref updates in the\n> machine-readable output (rather than appearing willy-nilly on stderr or\n> sideband 2). But I'd guess it would make the output rather unwieldy.\n\nThe second option would be more useful to the user too. Since they can\nact upon that specific update.\n\n> The patch below does the sideband dumping, and gets back the message in\n> this toy example. But as the inline comments show, it would probably\n> need support in a few other spots (both generating the detailed err\n> messages, and then showing them at the right spots). Plus it has a big\n> memory leak, in that nobody ever frees the detail strings. ;)\n>\n\nThanks for putting up something to show :)\n\n> So consider it just a sketch. I'm hoping Karthik can pick it up from\n> here.\n>\n\nYeah, I can polish what you've send. I'll work on it and send something\nsoon-ish (I'm taking some time off, but its hard to stay away from the\nlaptop).\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 288d3772ea..315e791193 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1649,6 +1649,7 @@ static void ref_transaction_rejection_handler(const char *refname,\n>  \t\t\t\t\t      const char *old_target UNUSED,\n>  \t\t\t\t\t      const char *new_target UNUSED,\n>  \t\t\t\t\t      enum ref_transaction_error err,\n> +\t\t\t\t\t      const char *details,\n>  \t\t\t\t\t      void *cb_data)\n>  {\n>  \tstruct ref_rejection_data *data = cb_data;\n> @@ -1675,6 +1676,8 @@ static void ref_transaction_rejection_handler(const char *refname,\n>  \t} else {\n>  \t\tconst char *reason = ref_transaction_error_msg(err);\n>\n> +\t\t/* probably should show \"details\" string here? */\n> +\n>  \t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n>  \t}\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 9c49174616..96daf54a09 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1853,10 +1853,12 @@ static void ref_transaction_rejection_handler(const char *refname,\n>  \t\t\t\t\t      const char *old_target UNUSED,\n>  \t\t\t\t\t      const char *new_target UNUSED,\n>  \t\t\t\t\t      enum ref_transaction_error err,\n> +\t\t\t\t\t      const char *details,\n>  \t\t\t\t\t      void *cb_data)\n>  {\n>  \tstruct strmap *failed_refs = cb_data;\n>\n> +\trp_error(\"%s\", details);\n>  \tstrmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));\n>  }\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 195437e7c6..6ef53d9886 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -573,11 +573,14 @@ static void print_rejected_refs(const char *refname,\n>  \t\t\t\tconst char *old_target,\n>  \t\t\t\tconst char *new_target,\n>  \t\t\t\tenum ref_transaction_error err,\n> +\t\t\t\tconst char *details,\n>  \t\t\t\tvoid *cb_data UNUSED)\n>  {\n>  \tstruct strbuf sb = STRBUF_INIT;\n>  \tconst char *reason = ref_transaction_error_msg(err);\n>\n> +\t/* do something with \"details\" string here? */\n> +\n>  \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n>  \t\t    new_oid ? oid_to_hex(new_oid) : new_target,\n>  \t\t    old_oid ? oid_to_hex(old_oid) : old_target,\n> diff --git a/refs.c b/refs.c\n> index 046b695bb2..adf01c527b 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1238,7 +1238,8 @@ void ref_transaction_free(struct ref_transaction *transaction)\n>\n>  int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n>  \t\t\t\t       size_t update_idx,\n> -\t\t\t\t       enum ref_transaction_error err)\n> +\t\t\t\t       enum ref_transaction_error err,\n> +\t\t\t\t       struct strbuf *details)\n>  {\n>  \tif (update_idx >= transaction->nr)\n>  \t\tBUG(\"trying to set rejection on invalid update index\");\n> @@ -1264,6 +1265,9 @@ int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n>  \t\t\t   transaction->updates[update_idx]->refname, 0);\n>\n>  \ttransaction->updates[update_idx]->rejection_err = err;\n> +\tif (details)\n> +\t\ttransaction->updates[update_idx]->rejection_details =\n> +\t\t\tstrbuf_detach(details, NULL);\n>  \tALLOC_GROW(transaction->rejections->update_indices,\n>  \t\t   transaction->rejections->nr + 1,\n>  \t\t   transaction->rejections->alloc);\n> @@ -2658,7 +2662,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n>  \t\t\t\t\t\t       &type, &ignore_errno))) {\n>  \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n>  \t\t\t\t\t    transaction, *update_idx,\n> -\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {\n> +\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n> +\t\t\t\t\t    NULL)) {\n>  \t\t\t\t\tstrset_remove(&dirnames, dirname.buf);\n>  \t\t\t\t\tstrset_add(&conflicting_dirnames, dirname.buf);\n>  \t\t\t\t\tcontinue;\n> @@ -2672,7 +2677,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n>  \t\t\tif (extras && string_list_has_string(extras, dirname.buf)) {\n>  \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n>  \t\t\t\t\t    transaction, *update_idx,\n> -\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT)) {\n> +\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n> +\t\t\t\t\t    NULL)) {\n>  \t\t\t\t\tstrset_remove(&dirnames, dirname.buf);\n>  \t\t\t\t\tcontinue;\n>  \t\t\t\t}\n> @@ -2709,9 +2715,12 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n>  \t\t\t\t    string_list_has_string(skip, iter->ref.name))\n>  \t\t\t\t\tcontinue;\n>\n> +\t\t\t\t/* should we be formatting err first here and\n> +\t\t\t\t * passing it in? */\n>  \t\t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n>  \t\t\t\t\t    transaction, *update_idx,\n> -\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT))\n> +\t\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT,\n> +\t\t\t\t\t    NULL))\n>  \t\t\t\t\tcontinue;\n>\n>  \t\t\t\tstrbuf_addf(err, _(\"'%s' exists; cannot create '%s'\"),\n> @@ -2725,9 +2734,10 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs\n>\n>  \t\textra_refname = find_descendant_ref(dirname.buf, extras, skip);\n>  \t\tif (extra_refname) {\n> +\t\t\t/* format err first and pass it in? */\n>  \t\t\tif (transaction && ref_transaction_maybe_set_rejected(\n>  \t\t\t\t    transaction, *update_idx,\n> -\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT))\n> +\t\t\t\t    REF_TRANSACTION_ERROR_NAME_CONFLICT, NULL))\n>  \t\t\t\tcontinue;\n>\n>  \t\t\tstrbuf_addf(err, _(\"cannot process '%s' and '%s' at the same time\"),\n> @@ -2862,7 +2872,8 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio\n>  \t\t   (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n>  \t\t   (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n>  \t\t   update->old_target, update->new_target,\n> -\t\t   update->rejection_err, cb_data);\n> +\t\t   update->rejection_err, update->rejection_details,\n> +\t\t   cb_data);\n>  \t}\n>  }\n>\n> diff --git a/refs.h b/refs.h\n> index d9051bbb04..4fbe3da924 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -975,6 +975,7 @@ typedef void ref_transaction_for_each_rejected_update_fn(const char *refname,\n>  \t\t\t\t\t\t\t const char *old_target,\n>  \t\t\t\t\t\t\t const char *new_target,\n>  \t\t\t\t\t\t\t enum ref_transaction_error err,\n> +\t\t\t\t\t\t\t const char *details,\n>  \t\t\t\t\t\t\t void *cb_data);\n>  void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n>  \t\t\t\t\t      ref_transaction_for_each_rejected_update_fn cb,\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 6f6f76a8d8..b58e3a3664 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2983,8 +2983,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n>  \t\t\t\t\t  head_ref, &refnames_to_check,\n>  \t\t\t\t\t  err);\n>  \t\tif (ret) {\n> -\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n> -\t\t\t\tstrbuf_reset(err);\n> +\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n>  \t\t\t\tret = 0;\n>\n>  \t\t\t\tcontinue;\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 4ea0c12299..b4b124a674 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1437,8 +1437,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n>  \t\t\t\t\t\t    update->refname);\n>  \t\t\t\t\tret = REF_TRANSACTION_ERROR_CREATE_EXISTS;\n>\n> -\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n> -\t\t\t\t\t\tstrbuf_reset(err);\n> +\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n>  \t\t\t\t\t\tret = 0;\n>  \t\t\t\t\t\tcontinue;\n>  \t\t\t\t\t}\n> @@ -1452,8 +1451,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n>  \t\t\t\t\t\t    oid_to_hex(&update->old_oid));\n>  \t\t\t\t\tret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE;\n>\n> -\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n> -\t\t\t\t\t\tstrbuf_reset(err);\n> +\t\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n>  \t\t\t\t\t\tret = 0;\n>  \t\t\t\t\t\tcontinue;\n>  \t\t\t\t\t}\n> @@ -1496,8 +1494,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n>  \t\t\t\t\t    oid_to_hex(&update->old_oid));\n>  \t\t\t\tret = REF_TRANSACTION_ERROR_NONEXISTENT_REF;\n>\n> -\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n> -\t\t\t\t\tstrbuf_reset(err);\n> +\t\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n>  \t\t\t\t\tret = 0;\n>  \t\t\t\t\tcontinue;\n>  \t\t\t\t}\n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index c7d2a6e50b..c5c121cc1c 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -128,6 +128,7 @@ struct ref_update {\n>  \t * was rejected.\n>  \t */\n>  \tenum ref_transaction_error rejection_err;\n> +\tchar *rejection_details;\n>\n\nSo this is where the rejection details are added. Free'ing this can be\npart of `ref_transaction_free()` I assume.\n\n>  \t/*\n>  \t * If this ref_update was split off of a symref update via\n> @@ -153,7 +154,8 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n>   */\n>  int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,\n>  \t\t\t\t       size_t update_idx,\n> -\t\t\t\t       enum ref_transaction_error err);\n> +\t\t\t\t       enum ref_transaction_error err,\n> +\t\t\t\t       struct strbuf *details);\n>\n>  /*\n>   * Add a ref_update with the specified properties to transaction, and\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 4319a4eacb..8b04a5e11c 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1401,8 +1401,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n>  \t\t\t\t\t    &refnames_to_check, head_type,\n>  \t\t\t\t\t    &head_referent, &referent, err);\n>  \t\tif (ret) {\n> -\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret)) {\n> -\t\t\t\tstrbuf_reset(err);\n> +\t\t\tif (ref_transaction_maybe_set_rejected(transaction, i, ret, err)) {\n>  \t\t\t\tret = 0;\n>\n>  \t\t\t\tcontinue;\n\nOverall this looks good. I'll build out something and send, but probably\nin the 1st-2nd week of January!\n\nKarthik\n"},{"id":"532772","messageId":"20251227074411.GB2071715@coredump.intra.peff.net","threadId":"64676","inReplyTo":"CAOLa=ZSOZz9aGFFeD7tiQ+PRwkMosjcoxfTSk52fQeQq0ghgaw@mail.gmail.com","subject":"Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-12-27T07:44:11Z","receivedAt":"2025-12-27T07:44:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 26, 2025 at 11:48:19AM -0500, Karthik Nayak wrote:\n\n> > And then receive-pack can either dump it via rp_error(), giving the same\n> > behavior as the old version. Or it can stick it into the per-ref status\n> > field. The latter feels more \"right\" in the sense that the error\n> > messages can be reliably attached to specific ref updates in the\n> > machine-readable output (rather than appearing willy-nilly on stderr or\n> > sideband 2). But I'd guess it would make the output rather unwieldy.\n> \n> The second option would be more useful to the user too. Since they can\n> act upon that specific update.\n\nThe trouble is that the low-level code constructing the \"err\" buffer is\naimed at writing a human-readable message. So you get the whole string like:\n\n  cannot update ref 'refs/heads/foo': trying to write non-commit object\n  d19968fcf0d3193147b827c9e89668d619afd01e to branch 'refs/heads/foo'\n\nThat's already somewhat redundant by itself, because\nlock_ref_for_update(), the intermediate caller that sticks \"cannot\nupdate ref 'foo':\" on the front of the string, does not know that its\nhelper function write_ref_to_lockfile() has already put \"foo\" in the\nerror message is returned.\n\nAnd we get one layer worse when we attach that whole thing to\nmachine-readable output associated with the ref \"foo\".\n\nThere's probably some clean-up possible here, but it will have to be\ndone very carefully. If we can check that all of the callers of\nwrite_ref_to_lockfile() mention the refname in their error messages, for\nexample, then we can simplify what write_ref_to_lockfile() puts in its\nerror messages.\n\nI'll let you decide how you want to proceed, but IMHO it would be OK to\nhandle the immediate regression fix by just going back to dumping the\nerror messages to stderr. And then further cleanup can come on top.\n\n> Yeah, I can polish what you've send. I'll work on it and send something\n> soon-ish (I'm taking some time off, but its hard to stay away from the\n> laptop).\n\nSounds good. Enjoy your holiday!\n\n-Peff\n"}]}