Re: [PATCH v12 4/8] remote set-head: better output for --auto

From: Bence Ferdinandy <bence@ferdinandy.com>
Date: Nov 15, 2024, 22:49 UTC
Message-ID: <D5N4B2SSZAG4.1O5CERCP825X2@ferdinandy.com>
In-Reply-To: <xmqqcyix11w8.fsf@gitster.g>

On Fri Nov 15, 2024 at 06:50, Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

> Bence Ferdinandy <bence@ferdinandy.com> writes:
>
>> Currently, set-head --auto will print a message saying "remote/HEAD set
>> to branch", which implies something was changed.
>>
>> Change the output of --auto, so the output actually reflects what was
>> done: a) set a previously unset HEAD, b) change HEAD because remote
>> changed or c) no updates. As a fourth output, if HEAD is changed from
>> a previous value that was not a remote branch, explicitly call attention
>> to this fact.
>
> OK.  That's sensible.
>
> There is a slight variant of the fourth case.  HEAD may have been a
> symbolic ref that pointed at an unexpected place (which you
> addressed), or HEAD may have been a non-symbolic ref (which the new
> code would mistakenly say "HEAD is now created", if I am reading the
> patch correctly).

Good point, and yes, that is what happens. (Although I'm not quite sure how valid that state is where a remote's HEAD is not a branch).

Show 21 quoted lines

>
>> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
>> index 9b50276646..0ea86d51a4 100755
>> --- a/t/t5505-remote.sh
>> +++ b/t/t5505-remote.sh
>> @@ -432,12 +432,51 @@ test_expect_success 'set-head --auto' '
>>  	)
>>  '
>>  
>> +test_expect_success 'set-head --auto detects creation' '
>> +	(
>> +		cd test &&
>> +		git symbolic-ref -d refs/remotes/origin/HEAD &&
>
> Are we sure refs/remotes/origin/HEAD exists at this point in the
> test, regardless of which earlier tests were skipped or failed?  If
> not, perhaps
>
> 		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
>
> is a better alternative.

Ack.

Show 31 quoted lines

>
>> +		git remote set-head --auto origin >output &&
>> +		echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect &&
>> +		test_cmp expect output
>> +	)
>> +'
>
> Here, we could insert another one:
>
> test_expect_success 'set-head --auto to update a non symbolic ref' '
> 	(
> 		cd test &&
> 		git update-ref --no-deref -d refs/remotes/origin/HEAD &&
> 		git update-ref refs/remotes/origin/HEAD HEAD &&
> 		git remote set-head --auto origin >output &&
>
> I'd imagine "output" should at least say that we are setting up a
> symref origin/HEAD to point at some ref the --auto option figured
> out, and if we wanted to report its previous state, it was a non
> symbolic ref that pointed at some commit.  In any case,
>
> 		echo "${SQ}origin/HEAD${SQ} is now created and points to ${SQ}main${SQ}" >expect &&
>
> is not what we want to see here, I suspect.
>
> Can we detect the case where we overwrite a non symref with a symref
> without going back to step 2/8 and doing a major surgery?
>
> 		test_cmp expect output
> 	)
> '

I agree, adding this makes sense. And this also takes us back to the question of what we should do in 2/8 when refs_read_symbolic_ref exits with 1. I now tested the behaviour and if origin/HEAD is gibberish, git already dies before with

error: cannot lock ref 'refs/remotes/origin/HEAD': unable to resolve reference 'refs/remotes/origin/HEAD': reference broken

so refs_read_symbolic_ref -> 1 only happens if there's a valid non-symbolic ref in origin/HEAD. So maybe if we put "Not a symbolic reference." in the referent (which should be an invalid symref), the caller could check for that and then should be able to distinguish this special case?

Thanks, Bence

-- 
bence.ferdinandy.com