Re: [PATCH 4/5] refs: introduce `refs_for_each_all_refs()`

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 22, 2024, 17:45 UTC
Message-ID: <xmqqfrypuurw.fsf@gitster.g>
In-Reply-To: <CAOLa=ZTbtqvejpvNVY5MHU=Adx3tWQ=FqVJdRLG1gaxYu4BG7A@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 17 quoted lines

> On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> This looks more like add_pseudoref_entries() given that the general
>> direction is to have an "allow" list of pseudo refs (at this point
>> after the previous step of the series, is_pseudoref_syntax() is the
>> is_pseudoref() function, and uses ends_with("_HEAD") as a mere
>> optimization to avoid listing all the possible pseudo refs that
>> exists or will be added in the future whose name ends with "_HEAD").
>>
>> Other than the naming, I think these two steps make sense.
>
> I think overall the naming is correct, I would change the comments in
> `is_pseudoref_syntax()`.
>
> Because, apart from pseudorefs, we also want to print HEAD. This is also
> why the pattern matches "HEAD" instead of "_HEAD". I'll add some more
> comments to clarify this.

With the hardcoded "these are definitely pseudorefs" list in the function, it no longer is is_pseudoref_SYNTAX() at all. I would rather prefer to see is_pseudoref() that says no to HEAD and have the callers check

	-	if (is_pseudoref_syntax(foo))
	+	if (is_pseudoref(foo) || is_headref(foo))

than keeping the messy semantics we have. My second preference is to call it is_pseudoref_or_head() that says yes to "HEAD" and pseudorefs, even though I like it much less.

Similarly, between giving the function under discussion a more descriptive name add_pseudoref_and_head_entries(), or adding a new function add_head_entry() to make the callers call add_head_entry() and add_pseudoref_entries() separately, I have a slight preference for the latter.

Thanks.