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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5289 bytes --]

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@peff.net>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]