Re: [PATCH 1/2] show-ref --verify: accept pseudorefs

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 7, 2024, 17:12 UTC
Message-ID: <xmqq5xz0b3lu.fsf@gitster.g>
In-Reply-To: <4dedc5704c3af6ab4ec8cc7503dc826480775b8e.1707324277.git.gitgitgadget@gmail.com>

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
> of comparing the refname to "HEAD" we can accept all one-level refs that
> contain only uppercase ascii letters and underscores.

Geez. We have at least three implementations to determine if a ref is a valid name?

Show 10 quoted lines

> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 79955c2856e..1c15421e600 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
>  	while (*refs) {
>  		struct object_id oid;
>  
> -		if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
> +		if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&

I think the helper you picked is the most sensible one, modulo a few nits.

 - We would want to teach refname_is_safe() to honor is_pseudoref()
   from Karthik's series to make rules more consistent.

 - The refname_is_safe() helper is not just about the stuff at the
   root level.  While starts_with("refs/") is overly lenient, it
   rejects nonsense like "refs/../trash".  We would want to lose
   "starts_with() ||" part of the condition from here.

These are minor non-blocking nits that we should keep in mind only for longer term maintenance, #leftoverbits after the dust settles.

Will queue.

Thanks.