Re: Possible regression: lost diagnostic message when pushing non-commit objects to refs/heads/*

> 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. ;)
>

> 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;