Re: [PATCH v2 1/4] refs: introduce `is_pseudoref()` and `is_headref()`

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> We cannot directly add the new syntax checks to `is_pseudoref_syntax()`
>> because the function is also used by `is_current_worktree_ref()` and
>> making it stricter to match only known pseudorefs might have unintended
>> consequences due to files like 'BISECT_START' which isn't a pseudoref
>> but sometimes contains object ID.
>
> Well described.
>
>> diff --git a/refs.c b/refs.c
>> index 20e8f1ff1f..4b6bfc66fb 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -859,6 +859,38 @@ static int is_pseudoref_syntax(const char *refname)
>>  	return 1;
>>  }
>>
>> +int is_pseudoref(struct ref_store *refs, const char *refname)
>> +{
>> +	static const char *const irregular_pseudorefs[] = {
>> +		"AUTO_MERGE",
>> +		"BISECT_EXPECTED_REV",
>> +		"NOTES_MERGE_PARTIAL",
>> +		"NOTES_MERGE_REF",
>> +		"MERGE_AUTOSTASH"
>
> Let's end an array's initializer with a trailing comma, to help
> future patches to add entries to this array without unnecessary
> patch noise.

>> +	};
>> +	size_t i;
>> +
>> +	if (!is_pseudoref_syntax(refname))
>> +		return 0;
>> +
>> +	if (ends_with(refname, "_HEAD"))
>> +		return refs_ref_exists(refs, refname);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
>> +		 if (!strcmp(refname, irregular_pseudorefs[i]))
>> +			 return refs_ref_exists(refs, refname);
>> +
>> +	return 0;
>> +}
>
> The above uses refs_ref_exists() because we want these to
> successfully resolve for reading.
>
>> +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/
>