Re: [PATCH v4 1/5] refs: introduce `is_pseudoref()` and `is_headref()`

From: Karthik Nayak <karthik.188@gmail.com>
Date: Feb 13, 2024, 15:48 UTC
Message-ID: <CAOLa=ZSQMqhBbOUtiqRbS+ftmB=C8wn8N7pcsC0izVXLsSCQsw@mail.gmail.com>
In-Reply-To: <ZcoTbRxIaGmTd4fJ@tanuki>

Hello,

Patrick Steinhardt <ps@pks.im> writes:

Show 21 quoted lines

>> +
>> +int is_headref(struct ref_store *refs, const char *refname)
>> +{
>> +	if (!strcmp(refname, "HEAD"))
>> +		return refs_ref_exists(refs, refname);
>> +
>> +	return 0;
>> +}
>
> The same comment applies here, as well.
>
> I also worry a bit about the API we have. It becomes really hard to
> figure out which function to call now as the API surface seems to
> explode. We have:
>
>   - is_pseudoref_syntax
>   - is_pseudoref
>   - is_headref
>   - check_refname_format
>   - refname_is_safe
>

I also found `is_head()` in 'reflog.c'.

> I wonder whether we can maybe consolidate the interface into one or
> maybe even two functions where the behaviour can be tweaked with a flag
> field.
>

You do bring up an interesting point about the APIs present and I agree that it would be best to consolidate them to something much simpler and nicer.

Show 15 quoted lines

> Something like `refname_is_valid()` with a bunch of flags:
>
>   - REFNAME_ACCEPT_HEAD to accept "HEAD"
>   - REFNAME_ACCEPT_PSEUDOREF to accept all of the refs ending with
>     "_HEAD" or being one of the irregular pseudorefs.
>   - REFNAME_ACCEPT_INVALID_BUT_SAFE to accept refnames which aren't
>     valid, but which would pass `refname_is_safe()`.
>
> Another alternative could be something like `classify_refname()` that
> accepts a refname and returns an enum saying what kind of ref something
> is.
>
> Given that this topic won't be included in Git v2.44 anymore, I think
> that opening this can of worms would be sensible now.
>

Over the two I do prefer something like the former method of the callee using flags to state the requirements, this way the function only does what is necessary between the listed operations.

Junio C Hamano <gitster@pobox.com> writes:

Show 26 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
>
>> I think it's quite confusing that `is_pseudoref()` not only checks
>> whether the refname may be a pseudoref, but also whether it actually
>> exists. Furthermore, why is a pseudoref only considered to exist in case
>> it's not a symbolic ref? That sounds overly restrictive to me.
>
> I am torn on this, but in favor of the proposed naming, primarily
> because is_pseudoref_syntax() was about "does this string look like
> the fullref a pseudoref would have?", and the reason why we wanted
> to have this new function was we wanted to ask "does this string
> name a valid pseudoref?"
>
>  Q: Is CHERRY_PICK_HEAD a pseudoref?
>  A: It would have been if it existed, but I see only
>     $GIT_DIR/CHERRY_PICK_HEAD that is a symbolic link, and it cannot
>     be a pseudoref.
>
> I can certainly see a broken out set of helper functions to check
>
>  - Does this string make a good fullref for a pseudoref?
>  - Does a pseudoref with his string as its fullref exist?
>
> independently.  The first one would answer Yes and the second one
> would answer No in such a context.
>

This does work into the flags based mechanism that Patrick mentioned too. The users of this new function can then only request information as necessary.

Junio C Hamano <gitster@pobox.com> writes:

Show 19 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +	if (ends_with(refname, "_HEAD")) {
>> +		refs_resolve_ref_unsafe(refs, refname,
>> +   					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
>> +   					&oid, NULL);
>> +   		return !is_null_oid(&oid);
>> +	}
>
> FYI. I see
>
> .git/rebase-apply/patch:31: space before tab in indent.
>    					RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> .git/rebase-apply/patch:32: space before tab in indent.
>    					&oid, NULL);
> .git/rebase-apply/patch:33: space before tab in indent.
>    		return !is_null_oid(&oid);
>
> around here.

Thanks for notifying, Jeff mentioned the same and I thought I fixed it. I'll have a look at why my editor did this and add some steps to avoid this in the following patches.