* 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