On Fri Nov 15, 2024 at 23:49, Bence Ferdinandy <bence@xxxxxxxxxxxxxx> wrote:
>
> On Fri Nov 15, 2024 at 06:50, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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).
>
>>
>>> 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.
>
>>
>>> + 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?
On second thought, it would maybe make even more sense to get the reference
hash and put that into referent. In that case the output could still be
"'%s/HEAD' has changed from '%s' and now points to '%s'\n"
but with a non-symref after from.
