Re: [PATCH v13 5/9] remote set-head: better output for --auto

From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Nov 19, 2024, 11:33 UTC
Message-ID: <D5Q4FTSLI0V9.OWVEG3K8AKRN@ferdinandy.com>
In-Reply-To: <xmqqiksjcx4e.fsf@gitster.g>

On Tue Nov 19, 2024 at 11:54, Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

> "Bence Ferdinandy" <bence@ferdinandy.com> writes:
>
>> No, it is not, but it's also a mistake. It should be `updateres == 1`.
>> refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other
>> error currently. Before I touched the code it was 1 for any error, so I left
>> that as is. So we want to error out on set_head if we get a 1 and continue if
>> we get 0 or -1 (and handle the difference in the report_set_head_auto).
>>
>> Thanks for noticing, I'll get that fixed in v14.
>
> It is good that somebody noticed it (and it may have happened to be
> me), but if it is a "mistake" as you said, I wonder why none of your
> tests caught it.  Do we have a gap in test coverage?

I think there is no test that is testing this branch:

	updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf,
			"remote set-head", &b_local_head, 0);
	if (updateres == 1) {
		result |= error(_("Could not setup %s"), b_head.buf);
		goto cleanup;

Running this in t/

	grep -r "Could not setup"

also yield nothing, so that's probably true. I'm wondering what would be the best way to trigger this error, refs_update_symref needs to fail for this.