Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> On Fri, Jan 19, 2024 at 9:57 PM Junio C Hamano <gitster@xxxxxxxxx> 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.
