* Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
@ 2025-12-24 3:32 Elijah Newren
2025-12-24 3:37 ` Junio C Hamano
2025-12-24 8:12 ` Jeff King
0 siblings, 2 replies; 6+ messages in thread
From: Elijah Newren @ 2025-12-24 3:32 UTC (permalink / raw)
To: Git Mailing List, Karthik Nayak; +Cc: Jeff King, Patrick Steinhardt
Hi,
git used to have better diagnostics about pushing non-commit objects
to refs/heads/*, dating all the way back to c3b0dec509fe (Be more
careful about updating refs, 2008-01-15):
$ git --version && git push . tagit:old
git version 2.50.1
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
remote: error: cannot update ref 'refs/heads/old': trying to write
non-commit object d19968fcf0d3193147b827c9e89668d619afd01e to branch
'refs/heads/old'
To .
! [remote rejected] tagit -> old (failed to update ref)
error: failed to push some refs to '.'
Unfortunately, the "trying to write non-commit object" error is no longer shown:
$ git --version && git push . tagit:old
git version 2.51.0
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
To .
! [remote rejected] tagit -> old (invalid new value provided)
error: failed to push some refs to '.'
The relevant error message is still part of the code:
$ git grep "write non-commit object" -- '*.c'
refs/files-backend.c: "trying to write
non-commit object %s to branch '%s'",
refs/reftable-backend.c: strbuf_addf(err,
_("trying to write non-commit object %s to branch '%s'"),
but the error message isn't displayed. Bisecting shows that this
started with commit 9d2962a7c44 ("receive-pack: use batched reference
updates", 2025-05-19). That commit message to me suggests that while
error handling was necessarily changed, that dropping the errors was
not intentional:
```
As using batched updates requires the error handling to be moved to the
end of the flow, create and use a 'struct strset' to track the failed
refs and attribute the correct errors to them.
```
But it's possible I'm reading it wrong. Was it intentional, or is
this a regression?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
2025-12-24 3:32 Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/* Elijah Newren
@ 2025-12-24 3:37 ` Junio C Hamano
2025-12-24 8:12 ` Jeff King
1 sibling, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2025-12-24 3:37 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Karthik Nayak, Jeff King, Patrick Steinhardt
Elijah Newren <newren@gmail•com> writes:
> Bisecting shows that this
> started with commit 9d2962a7c44 ("receive-pack: use batched reference
> updates", 2025-05-19). That commit message to me suggests that while
> error handling was necessarily changed, that dropping the errors was
> not intentional:
>
> ```
> As using batched updates requires the error handling to be moved to the
> end of the flow, create and use a 'struct strset' to track the failed
> refs and attribute the correct errors to them.
> ```
>
> But it's possible I'm reading it wrong. Was it intentional, or is
> this a regression?
The topic bisect found was supposed to be purely performance
optimization, and we should take any changes in behaviour as
regressions.
Thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
2025-12-24 3:32 Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/* Elijah Newren
2025-12-24 3:37 ` Junio C Hamano
@ 2025-12-24 8:12 ` Jeff King
2025-12-24 8:21 ` Jeff King
2025-12-26 16:48 ` Karthik Nayak
1 sibling, 2 replies; 6+ messages in thread
From: Jeff King @ 2025-12-24 8:12 UTC (permalink / raw)
To: Elijah Newren; +Cc: Git Mailing List, Karthik Nayak, Patrick Steinhardt
On Tue, Dec 23, 2025 at 07:32:28PM -0800, Elijah Newren wrote:
> The relevant error message is still part of the code:
> $ git grep "write non-commit object" -- '*.c'
> refs/files-backend.c: "trying to write
> non-commit object %s to branch '%s'",
> refs/reftable-backend.c: strbuf_addf(err,
> _("trying to write non-commit object %s to branch '%s'"),
>
> but the error message isn't displayed. Bisecting shows that this
> started with commit 9d2962a7c44 ("receive-pack: use batched reference
> updates", 2025-05-19). That commit message to me suggests that while
> error handling was necessarily changed, that dropping the errors was
> not intentional:
>
> ```
> As using batched updates requires the error handling to be moved to the
> end of the flow, create and use a 'struct strset' to track the failed
> refs and attribute the correct errors to them.
> ```
>
> But it's possible I'm reading it wrong. Was it intentional, or is
> this a regression?
I didn't participate in this topic beyond a few memory-management
nitpicks, but my gut feeling is that yes, this is a regression.
We still format that specific error into an "err" strbuf inside the refs
code. In the older version, we returned up the stack and eventually
printed the error from the failed transaction to stderr (or in the case
of receive-pack, the sideband).
But in the new batched world that allows partial-batch failures, we
throw it away. The problem (at least for the files backend) is this code
in files_transaction_prepare():
ret = lock_ref_for_update(refs, update, i, transaction,
head_ref, &refnames_to_check,
err);
if (ret) {
if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
strbuf_reset(err);
ret = 0;
continue;
}
goto cleanup;
}
We see the error from lock_ref_for_update() as before, and the useful
message is in "err" here. But ref_transaction_maybe_set_rejected() only
looks at "ret", the numeric error code, and decides that it is enough to
record that.
We should do something useful with the "err" string that was collected,
rather than immediately calling strbuf_reset() to throw it away.
Unfortunately we can't just dump it to stderr here, since we don't know
what our caller would want to do with the error (and in fact for
receive-pack we eventually want to call rp_error() which dumps it over
the sideband). So we have to return it back to the caller somehow.
We only get one "err" string to return for the whole transaction. We can
play some games to make it multi-line, like this:
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6f6f76a8d8..6ad57e53c0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2978,13 +2978,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,
*/
for (i = 0; i < transaction->nr; i++) {
struct ref_update *update = transaction->updates[i];
+ struct strbuf this_err = STRBUF_INIT;
ret = lock_ref_for_update(refs, update, i, transaction,
head_ref, &refnames_to_check,
- err);
+ &this_err);
if (ret) {
if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
- strbuf_reset(err);
+ if (err->len)
+ strbuf_addch(err, '\n');
+ strbuf_addbuf(err, &this_err);
+ strbuf_release(&this_err);
ret = 0;
continue;
but even that is not quite enough. We still return success from the
overall transaction, because we allow failures within the batch! So now
our caller does not even look at "err" at all.
So I guess we need to attach the failure more directly to the failed
ref. Probably ref_transaction_maybe_set_rejected() should take the error
string along with the ref_transaction_error enum, and attach it to the
failed item. We also need to get those details out to the callers, which
use ref_transaction_for_each_rejected_update(). So that interface needs
to be expanded to pass out the details string.
And then receive-pack can either dump it via rp_error(), giving the same
behavior as the old version. Or it can stick it into the per-ref status
field. The latter feels more "right" in the sense that the error
messages can be reliably attached to specific ref updates in the
machine-readable output (rather than appearing willy-nilly on stderr or
sideband 2). But I'd guess it would make the output rather unwieldy.
The patch below does the sideband dumping, and gets back the message in
this toy example. But as the inline comments show, it would probably
need support in a few other spots (both generating the detailed err
messages, and then showing them at the right spots). Plus it has a big
memory leak, in that nobody ever frees the detail strings. ;)
So consider it just a sketch. I'm hoping Karthik can pick it up from
here.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 288d3772ea..315e791193 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,
void *cb_data)
{
struct ref_rejection_data *data = cb_data;
@@ -1675,6 +1676,8 @@ static void ref_transaction_rejection_handler(const char *refname,
} else {
const char *reason = ref_transaction_error_msg(err);
+ /* probably should show "details" string here? */
+
error(_("fetching ref %s failed: %s"), refname, reason);
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9c49174616..96daf54a09 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1853,10 +1853,12 @@ 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,
void *cb_data)
{
struct strmap *failed_refs = cb_data;
+ rp_error("%s", details);
strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
}
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 195437e7c6..6ef53d9886 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -573,11 +573,14 @@ static void print_rejected_refs(const char *refname,
const char *old_target,
const char *new_target,
enum ref_transaction_error err,
+ const char *details,
void *cb_data UNUSED)
{
struct strbuf sb = STRBUF_INIT;
const char *reason = ref_transaction_error_msg(err);
+ /* do something with "details" string here? */
+
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,
diff --git a/refs.c b/refs.c
index 046b695bb2..adf01c527b 100644
--- a/refs.c
+++ b/refs.c
@@ -1238,7 +1238,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 +1265,9 @@ 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 =
+ strbuf_detach(details, NULL);
ALLOC_GROW(transaction->rejections->update_indices,
transaction->rejections->nr + 1,
transaction->rejections->alloc);
@@ -2658,7 +2662,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
&type, &ignore_errno))) {
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
+ REF_TRANSACTION_ERROR_NAME_CONFLICT,
+ NULL)) {
strset_remove(&dirnames, dirname.buf);
strset_add(&conflicting_dirnames, dirname.buf);
continue;
@@ -2672,7 +2677,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
if (extras && string_list_has_string(extras, dirname.buf)) {
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
+ REF_TRANSACTION_ERROR_NAME_CONFLICT,
+ NULL)) {
strset_remove(&dirnames, dirname.buf);
continue;
}
@@ -2709,9 +2715,12 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
string_list_has_string(skip, iter->ref.name))
continue;
+ /* should we be formatting err first here and
+ * passing it in? */
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT))
+ REF_TRANSACTION_ERROR_NAME_CONFLICT,
+ NULL))
continue;
strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
@@ -2725,9 +2734,10 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
extra_refname = find_descendant_ref(dirname.buf, extras, skip);
if (extra_refname) {
+ /* format err first and pass it in? */
if (transaction && ref_transaction_maybe_set_rejected(
transaction, *update_idx,
- REF_TRANSACTION_ERROR_NAME_CONFLICT))
+ REF_TRANSACTION_ERROR_NAME_CONFLICT, NULL))
continue;
strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"),
@@ -2862,7 +2872,8 @@ 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,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 6f6f76a8d8..b58e3a3664 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2983,8 +2983,7 @@ 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;
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 4ea0c12299..b4b124a674 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1437,8 +1437,7 @@ 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 +1451,7 @@ 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 +1494,7 @@ 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..c5c121cc1c 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;
+ 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..8b04a5e11c 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1401,8 +1401,7 @@ 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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
2025-12-24 8:12 ` Jeff King
@ 2025-12-24 8:21 ` Jeff King
2025-12-26 16:48 ` Karthik Nayak
1 sibling, 0 replies; 6+ messages in thread
From: Jeff King @ 2025-12-24 8:21 UTC (permalink / raw)
To: Elijah Newren; +Cc: Git Mailing List, Karthik Nayak, Patrick Steinhardt
On Wed, Dec 24, 2025 at 03:12:14AM -0500, Jeff King wrote:
> But in the new batched world that allows partial-batch failures, we
> throw it away. The problem (at least for the files backend) is this code
> in files_transaction_prepare():
>
> ret = lock_ref_for_update(refs, update, i, transaction,
> head_ref, &refnames_to_check,
> err);
> if (ret) {
> if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> strbuf_reset(err);
> ret = 0;
>
> continue;
> }
> goto cleanup;
> }
BTW, you found the regression via receive-pack, but as you can see here
it is really a problem for any batched ref-update caller that sets the
ALLOW_FAILURE flag. So the original sin is not from the commit you found
via bisect, but 23fc8e4f61 (refs: implement batch reference update
support, 2025-04-08). And it affects fetch, too:
$ 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)
$ git.v2.51.0 fetch . v1.0.0:refs/heads/foo
From .
* [new tag] v1.0.0 -> foo
error: fetching ref refs/heads/foo failed: invalid new value provided
Actually, I think there is another bug lurking there for fetch. We do
not even mark the failure in the status output anymore!
And I guess "update-ref --batch-updates" suffers from the same lack of
detail:
$ echo create refs/heads/foo v1.0.0 | git update-ref --batch-updates --stdin
rejected refs/heads/foo f665776185ad074b236c00751d666da7d1977dbe 0000000000000000000000000000000000000000 invalid new value provided
though it is not technically a regression since the option to ask for
ALLOW_FAILURE did not even exist before --batch-updates. It would be
nice if it gave more details (whether to stderr or in the
machine-readable output).
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
2025-12-24 8:12 ` Jeff King
2025-12-24 8:21 ` Jeff King
@ 2025-12-26 16:48 ` Karthik Nayak
2025-12-27 7:44 ` Jeff King
1 sibling, 1 reply; 6+ messages in thread
From: Karthik Nayak @ 2025-12-26 16:48 UTC (permalink / raw)
To: Jeff King, Elijah Newren; +Cc: Git Mailing List, Patrick Steinhardt
[-- Attachment #1: Type: text/plain, Size: 15568 bytes --]
Jeff King <peff@peff•net> writes:
> On Tue, Dec 23, 2025 at 07:32:28PM -0800, Elijah Newren wrote:
>
>> The relevant error message is still part of the code:
>> $ git grep "write non-commit object" -- '*.c'
>> refs/files-backend.c: "trying to write
>> non-commit object %s to branch '%s'",
>> refs/reftable-backend.c: strbuf_addf(err,
>> _("trying to write non-commit object %s to branch '%s'"),
>>
>> but the error message isn't displayed. Bisecting shows that this
>> started with commit 9d2962a7c44 ("receive-pack: use batched reference
>> updates", 2025-05-19). That commit message to me suggests that while
>> error handling was necessarily changed, that dropping the errors was
>> not intentional:
>>
>> ```
>> As using batched updates requires the error handling to be moved to the
>> end of the flow, create and use a 'struct strset' to track the failed
>> refs and attribute the correct errors to them.
>> ```
>>
>> But it's possible I'm reading it wrong. Was it intentional, or is
>> this a regression?
>
> I didn't participate in this topic beyond a few memory-management
> nitpicks, but my gut feeling is that yes, this is a regression.
>
Definitely a regression. We did find a few regressions with this topic,
the only bright side being fixing those also added missing tests which
would catch further breakages.
> We still format that specific error into an "err" strbuf inside the refs
> code. In the older version, we returned up the stack and eventually
> printed the error from the failed transaction to stderr (or in the case
> of receive-pack, the sideband).
>
> But in the new batched world that allows partial-batch failures, we
> throw it away. The problem (at least for the files backend) is this code
> in files_transaction_prepare():
>
> ret = lock_ref_for_update(refs, update, i, transaction,
> head_ref, &refnames_to_check,
> err);
> if (ret) {
> if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> strbuf_reset(err);
> ret = 0;
>
> continue;
> }
> goto cleanup;
> }
>
> We see the error from lock_ref_for_update() as before, and the useful
> message is in "err" here. But ref_transaction_maybe_set_rejected() only
> looks at "ret", the numeric error code, and decides that it is enough to
> record that.
>
> We should do something useful with the "err" string that was collected,
> rather than immediately calling strbuf_reset() to throw it away.
>
> Unfortunately we can't just dump it to stderr here, since we don't know
> what our caller would want to do with the error (and in fact for
> receive-pack we eventually want to call rp_error() which dumps it over
> the sideband). So we have to return it back to the caller somehow.
>
> We only get one "err" string to return for the whole transaction. We can
> play some games to make it multi-line, like this:
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6f6f76a8d8..6ad57e53c0 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2978,13 +2978,17 @@ static int files_transaction_prepare(struct ref_store *ref_store,
> */
> for (i = 0; i < transaction->nr; i++) {
> struct ref_update *update = transaction->updates[i];
> + struct strbuf this_err = STRBUF_INIT;
>
> ret = lock_ref_for_update(refs, update, i, transaction,
> head_ref, &refnames_to_check,
> - err);
> + &this_err);
> if (ret) {
> if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> - strbuf_reset(err);
> + if (err->len)
> + strbuf_addch(err, '\n');
> + strbuf_addbuf(err, &this_err);
> + strbuf_release(&this_err);
> ret = 0;
>
> continue;
>
> but even that is not quite enough. We still return success from the
> overall transaction, because we allow failures within the batch! So now
> our caller does not even look at "err" at all.
>
Yup, that's my understanding as well, we'd want to pass the error per
update being performed.
> So I guess we need to attach the failure more directly to the failed
> ref. Probably ref_transaction_maybe_set_rejected() should take the error
> string along with the ref_transaction_error enum, and attach it to the
> failed item. We also need to get those details out to the callers, which
> use ref_transaction_for_each_rejected_update(). So that interface needs
> to be expanded to pass out the details string.
>
Yup, this was what I was thinking of too, and your exploration here
seems to be on the same line.
> And then receive-pack can either dump it via rp_error(), giving the same
> behavior as the old version. Or it can stick it into the per-ref status
> field. The latter feels more "right" in the sense that the error
> messages can be reliably attached to specific ref updates in the
> machine-readable output (rather than appearing willy-nilly on stderr or
> sideband 2). But I'd guess it would make the output rather unwieldy.
The second option would be more useful to the user too. Since they can
act upon that specific update.
> The patch below does the sideband dumping, and gets back the message in
> this toy example. But as the inline comments show, it would probably
> need support in a few other spots (both generating the detailed err
> messages, and then showing them at the right spots). Plus it has a big
> memory leak, in that nobody ever frees the detail strings. ;)
>
Thanks for putting up something to show :)
> So consider it just a sketch. I'm hoping Karthik can pick it up from
> here.
>
Yeah, I can polish what you've send. I'll work on it and send something
soon-ish (I'm taking some time off, but its hard to stay away from the
laptop).
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 288d3772ea..315e791193 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,
> void *cb_data)
> {
> struct ref_rejection_data *data = cb_data;
> @@ -1675,6 +1676,8 @@ static void ref_transaction_rejection_handler(const char *refname,
> } else {
> const char *reason = ref_transaction_error_msg(err);
>
> + /* probably should show "details" string here? */
> +
> error(_("fetching ref %s failed: %s"), refname, reason);
> }
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 9c49174616..96daf54a09 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1853,10 +1853,12 @@ 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,
> void *cb_data)
> {
> struct strmap *failed_refs = cb_data;
>
> + rp_error("%s", details);
> strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));
> }
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index 195437e7c6..6ef53d9886 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -573,11 +573,14 @@ static void print_rejected_refs(const char *refname,
> const char *old_target,
> const char *new_target,
> enum ref_transaction_error err,
> + const char *details,
> void *cb_data UNUSED)
> {
> struct strbuf sb = STRBUF_INIT;
> const char *reason = ref_transaction_error_msg(err);
>
> + /* do something with "details" string here? */
> +
> 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,
> diff --git a/refs.c b/refs.c
> index 046b695bb2..adf01c527b 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1238,7 +1238,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 +1265,9 @@ 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 =
> + strbuf_detach(details, NULL);
> ALLOC_GROW(transaction->rejections->update_indices,
> transaction->rejections->nr + 1,
> transaction->rejections->alloc);
> @@ -2658,7 +2662,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
> &type, &ignore_errno))) {
> if (transaction && ref_transaction_maybe_set_rejected(
> transaction, *update_idx,
> - REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
> + REF_TRANSACTION_ERROR_NAME_CONFLICT,
> + NULL)) {
> strset_remove(&dirnames, dirname.buf);
> strset_add(&conflicting_dirnames, dirname.buf);
> continue;
> @@ -2672,7 +2677,8 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
> if (extras && string_list_has_string(extras, dirname.buf)) {
> if (transaction && ref_transaction_maybe_set_rejected(
> transaction, *update_idx,
> - REF_TRANSACTION_ERROR_NAME_CONFLICT)) {
> + REF_TRANSACTION_ERROR_NAME_CONFLICT,
> + NULL)) {
> strset_remove(&dirnames, dirname.buf);
> continue;
> }
> @@ -2709,9 +2715,12 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
> string_list_has_string(skip, iter->ref.name))
> continue;
>
> + /* should we be formatting err first here and
> + * passing it in? */
> if (transaction && ref_transaction_maybe_set_rejected(
> transaction, *update_idx,
> - REF_TRANSACTION_ERROR_NAME_CONFLICT))
> + REF_TRANSACTION_ERROR_NAME_CONFLICT,
> + NULL))
> continue;
>
> strbuf_addf(err, _("'%s' exists; cannot create '%s'"),
> @@ -2725,9 +2734,10 @@ enum ref_transaction_error refs_verify_refnames_available(struct ref_store *refs
>
> extra_refname = find_descendant_ref(dirname.buf, extras, skip);
> if (extra_refname) {
> + /* format err first and pass it in? */
> if (transaction && ref_transaction_maybe_set_rejected(
> transaction, *update_idx,
> - REF_TRANSACTION_ERROR_NAME_CONFLICT))
> + REF_TRANSACTION_ERROR_NAME_CONFLICT, NULL))
> continue;
>
> strbuf_addf(err, _("cannot process '%s' and '%s' at the same time"),
> @@ -2862,7 +2872,8 @@ 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,
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6f6f76a8d8..b58e3a3664 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2983,8 +2983,7 @@ 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;
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 4ea0c12299..b4b124a674 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1437,8 +1437,7 @@ 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 +1451,7 @@ 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 +1494,7 @@ 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..c5c121cc1c 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;
> + char *rejection_details;
>
So this is where the rejection details are added. Free'ing this can be
part of `ref_transaction_free()` I assume.
> /*
> * 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..8b04a5e11c 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1401,8 +1401,7 @@ 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;
Overall this looks good. I'll build out something and send, but probably
in the 1st-2nd week of January!
Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*
2025-12-26 16:48 ` Karthik Nayak
@ 2025-12-27 7:44 ` Jeff King
0 siblings, 0 replies; 6+ messages in thread
From: Jeff King @ 2025-12-27 7:44 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Elijah Newren, Git Mailing List, Patrick Steinhardt
On Fri, Dec 26, 2025 at 11:48:19AM -0500, Karthik Nayak wrote:
> > And then receive-pack can either dump it via rp_error(), giving the same
> > behavior as the old version. Or it can stick it into the per-ref status
> > field. The latter feels more "right" in the sense that the error
> > messages can be reliably attached to specific ref updates in the
> > machine-readable output (rather than appearing willy-nilly on stderr or
> > sideband 2). But I'd guess it would make the output rather unwieldy.
>
> The second option would be more useful to the user too. Since they can
> act upon that specific update.
The trouble is that the low-level code constructing the "err" buffer is
aimed at writing a human-readable message. So you get the whole string like:
cannot update ref 'refs/heads/foo': trying to write non-commit object
d19968fcf0d3193147b827c9e89668d619afd01e to branch 'refs/heads/foo'
That's already somewhat redundant by itself, because
lock_ref_for_update(), the intermediate caller that sticks "cannot
update ref 'foo':" on the front of the string, does not know that its
helper function write_ref_to_lockfile() has already put "foo" in the
error message is returned.
And we get one layer worse when we attach that whole thing to
machine-readable output associated with the ref "foo".
There's probably some clean-up possible here, but it will have to be
done very carefully. If we can check that all of the callers of
write_ref_to_lockfile() mention the refname in their error messages, for
example, then we can simplify what write_ref_to_lockfile() puts in its
error messages.
I'll let you decide how you want to proceed, but IMHO it would be OK to
handle the immediate regression fix by just going back to dumping the
error messages to stderr. And then further cleanup can come on top.
> Yeah, I can polish what you've send. I'll work on it and send something
> soon-ish (I'm taking some time off, but its hard to stay away from the
> laptop).
Sounds good. Enjoy your holiday!
-Peff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-12-27 7:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-24 3:32 Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/* Elijah Newren
2025-12-24 3:37 ` Junio C Hamano
2025-12-24 8:12 ` Jeff King
2025-12-24 8:21 ` Jeff King
2025-12-26 16:48 ` Karthik Nayak
2025-12-27 7:44 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox