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?
Thanks,
Bence
--
bence.ferdinandy.com
