Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 25, 2024, 16:28 UTC
- Message-ID
- <xmqqa5otxtqy.fsf@gitster.g>
- In-Reply-To
- <CAOLa=ZQxMZo2Y6x6GmVw=df_xS4tkF4D1_tZOeLb7jL5d5bKXA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
Show 28 quoted lines
>>> +int is_headref(struct ref_store *refs, const char *refname)
>>> +{
>>> + if (!strcmp(refname, "HEAD"))
>>> + return refs_ref_exists(refs, refname);
>>
>> Given that "git for-each-ref refs/remotes" does not show
>> "refs/remotes/origin/HEAD" in the output when we do not have the
>> remote-tracking branch that symref points at, we probably do want
>> to omit "HEAD" from the output when the HEAD symref points at an
>> unborn branch. If refs_ref_exists() says "no, it does not exist"
>> in such a case, we are perfectly fine with this code.
>>
>> We do not have to worry about the unborn state for pseudorefs
>> because they would never be symbolic. But that in turn makes me
>> suspect that the check done with refs_ref_exists() in the
>> is_pseudoref() helper is a bit too lenient by allowing it to be a
>> symbolic ref. Shouldn't we be using a check based on
>> read_ref_full(), like we did in another topic recently [*]?
>>
>>
>> [Reference]
>>
>> * https://lore.kernel.org/git/xmqqzfxa9usx.fsf@gitster.g/
>>
>
> Thanks, this makes sense and the link is helpful. I'll do something
> similar, but since HEAD can be a symref, I'll drop the
> `RESOLVE_REF_NO_RECURSE` flag and only use `RESOLVE_REF_READING`.Just to make sure there is no misunderstanding, I think how is_headref() does what it does in the patch is perfectly fine, including its use of refs_ref_exists(). The side I was referring to with "in turn makes me suspect" is the other helper function that will never have to deal with a symref. Use of refs_ref_exists() in that function is too loose.