Re: [PATCH 4/6] update-ref: utilize rejected error details if available

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jan 15, 2026, 11:08 UTC
Message-ID: <CAOLa=ZQLPB2Tntvimpp2zt=6PiWhJJh_oDCrUk7F8v+pFhyyMA@mail.gmail.com>
In-Reply-To: <20260114175558.GG885771@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 39 quoted lines

> On Wed, Jan 14, 2026 at 09:27:28AM -0800, Junio C Hamano wrote:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>> > @@ -573,16 +573,18 @@ static void print_rejected_refs(const char *refname,
>> >  				const char *old_target,
>> >  				const char *new_target,
>> >  				enum ref_transaction_error err,
>> > -				const char *details UNUSED,
>> > +				const char *details,
>> >  				void *cb_data UNUSED)
>> >  {
>> >  	struct strbuf sb = STRBUF_INIT;
>> > -	const char *reason = ref_transaction_error_msg(err);
>> >
>> > -	strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
>> > -		    new_oid ? oid_to_hex(new_oid) : new_target,
>> > -		    old_oid ? oid_to_hex(old_oid) : old_target,
>> > -		    reason);
>> > +	if (details)
>> > +		strbuf_addf(&sb, "%s\n", details);
>> > +	else
>> > +		strbuf_addf(&sb, "rejected %s %s %s %s\n", refname,
>> > +			    new_oid ? oid_to_hex(new_oid) : new_target,
>> > +			    old_oid ? oid_to_hex(old_oid) : old_target,
>> > +			    ref_transaction_error_msg(err));
>>
>> Could "details" reported from the lower layer be less detailed than
>> what we are formulating here, like updating the value of what ref
>> from what old object to what new object, or what the err code tells
>> the end-user?
>
> I wondered that, too, but also: is this supposed to be machine-readable?
> The "rejected ..." output looks like something that could be parsed,
> and it seems to be documented in git-update-ref(1).
>
>   Side note: if this is meant to be a stable format, surely there should
>   be some coverage in the test suite? There doesn't seem to be.
>

Good catch, the documentation does indeed promise this format, so it wouldn't be appropriate to step away from it. Ideally, we should only replace the last field, but that would be a lot of redundant information.

Overall, we could also drop this patch too, since the flag was introduced with batched updates and we could better justice here once we cleanup all other error messages.

Show 7 quoted lines

> So should we just be replacing the ref_transaction_error_msg() part? I
> _think_ the low-level details will usually be more informative there,
> but not necessarily. So possibly we'd even want to show both, though I
> suspect just concatenating them would be messy.
>
> Plus the "details" one has a lot of redundant information in it (it
> mentions "refname", even though it is already on the "rejected" line).

Indeed, I've noted all possibilities in another response [1], but there is some redundancy there and we could do a nice cleanup.

Show 10 quoted lines

>
> In the short-term, I wonder if we just want:
>
>   if (details && *details)
> 	error("%s", details);
>
> That gets us back to the status quo, where the details are at least
> available via stderr. And then we can consider how to combine them into
> the machine-readable format separately.
>

That's a good compromise too. I'd say we do this for now and see how we can take it from here.

> -Peff

[1]: CAOLa=ZS0i+YXfVHHAax699ME48YG7jXNZ3WOBYryS0hypMZO-A@mail.gmail.com