On Sun, Feb 11, 2024 at 07:39:19PM +0100, Karthik Nayak wrote:
> Introduce two new functions `is_pseudoref()` and `is_headref()`. This
> provides the necessary functionality for us to add pseudorefs and HEAD
> to the loose ref cache in the files backend, allowing us to build
> tooling to print these refs.
>
> The `is_pseudoref()` function internally calls `is_pseudoref_syntax()`
> but adds onto it by also checking to ensure that the pseudoref either
> ends with a "_HEAD" suffix or matches a list of exceptions. After which
> we also parse the contents of the pseudoref to ensure that it conforms
> to the ref format.
>
> 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.
>
> Keeping this in mind, we leave `is_pseudoref_syntax()` as is and create
> `is_pseudoref()` which is stricter. Ideally we'd want to move the new
> syntax checks to `is_pseudoref_syntax()` but a prerequisite for this
> would be to actually remove the exception list by converting those
> pseudorefs to also contain a '_HEAD' suffix and perhaps move bisect
> related files like 'BISECT_START' to a new directory similar to the
> 'rebase-merge' directory.
>
> Helped-by: Jeff King <peff@xxxxxxxx>
> Signed-off-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> ---
> refs.c | 41 +++++++++++++++++++++++++++++++++++++++++
> refs.h | 3 +++
> 2 files changed, 44 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index fff343c256..d8e4cf9a11 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -860,6 +860,47 @@ 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",
> + };
> + struct object_id oid;
> + size_t i;
> +
> + if (!is_pseudoref_syntax(refname))
> + return 0;
> +
> + 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);
> + }
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.
So I think this at least needs to be renamed. But I find it really hard
to come up with a proper name here because in my opinion the function
does too much. `is_existing_pseudoref()` feels much too specific to me.
Also, the "reftable" backend wouldn't need to check whether the ref
exists, but only whether a name that it encounters is a pseudoref name
or not.
> + for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> + if (!strcmp(refname, irregular_pseudorefs[i])) {
> + refs_resolve_ref_unsafe(refs, refname,
> + RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
> + &oid, NULL);
> + return !is_null_oid(&oid);
> + }
> +
> + return 0;
> +}
> +
> +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 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. 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.
Patrick
> static int is_current_worktree_ref(const char *ref) {
> return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref);
> }
> diff --git a/refs.h b/refs.h
> index 303c5fac4d..f66cdd731c 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -1023,4 +1023,7 @@ extern struct ref_namespace_info ref_namespace[NAMESPACE__COUNT];
> */
> void update_ref_namespace(enum ref_namespace namespace, char *ref);
>
> +int is_pseudoref(struct ref_store *refs, const char *refname);
> +int is_headref(struct ref_store *refs, const char *refname);
> +
> #endif /* REFS_H */
> --
> 2.43.GIT
>
Attachment:
signature.asc
Description: PGP signature
