From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 22 Jan 2024 20:13:22 GMT
Subject: Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
Message-ID: <ee977173-bc6d-48f6-9bc8-e1d84fe3d95d@gmail.com>
In-Reply-To: <20240119142705.139374-3-karthik.188@gmail.com>

Hi Karthik

On 19/01/2024 14:27, Karthik Nayak wrote:
> The `is_pseudoref_syntax()` function is used to determine if a
> particular refname follows the pseudoref syntax. The pseudoref syntax is
> loosely defined at this instance as all refs which are in caps and use
> underscores. Most of the pseudorefs also have the "HEAD" suffix.
> 
> Using this information we make the `is_pseudoref_syntax()` function
> stricter, by adding the check for "HEAD" suffix and for refs which don't
> end with the HEAD suffix, matching them with a predetermined list.
> 
> This requires fixing up t1407 to use the "HEAD" suffix for creation of
> pseudorefs.

I'm concerned that this change is a regression. is_pseudoref_syntax() is 
used by is_current_worktree_ref() and so scripts that create pseudorefs 
that do not conform to your new rules will break as git will no-longer 
consider the pseudorefs they create to be worktree specific. The list of 
hard coded exceptions also looks quite short, I can see MERGE_AUTOSTASH 
and BISECT_START are missing and there are probably others I've not 
thought of.

The commit message would be a good place to discuss why you're making 
this change, the implications of the change and any alternative 
approaches that you considered. As I understand it you're tying to get 
round the problem that the files backend stores pseudorefs mixed up with 
other non-ref files in $GIT_DIR. Another approach would be to read all 
the files whose name matches the pseudoref syntax and see if its 
contents looks like a valid ref skipping names like COMMIT_EDITMSG that 
we know are not pseudorefs.

Best Wishes

Phillip

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   refs.c                        | 21 ++++++++++++++++++---
>   t/t1407-worktree-ref-store.sh | 12 ++++++------
>   2 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 5999605230..b84e173762 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -829,6 +829,14 @@ int is_per_worktree_ref(const char *refname)
>   
>   int is_pseudoref_syntax(const char *refname)
>   {
> +	/* TODO: move these pseudorefs to have _HEAD suffix */
> +	static const char *const irregular_pseudorefs[] = {
> +		"BISECT_EXPECTED_REV",
> +		"NOTES_MERGE_PARTIAL",
> +		"NOTES_MERGE_REF",
> +		"AUTO_MERGE"
> +	};
> +	size_t i;
>   	const char *c;
>   
>   	for (c = refname; *c; c++) {
> @@ -837,10 +845,17 @@ int is_pseudoref_syntax(const char *refname)
>   	}
>   
>   	/*
> -	 * HEAD is not a pseudoref, but it certainly uses the
> -	 * pseudoref syntax.
> +	 * Most pseudorefs end with _HEAD. HEAD itself is not a
> +	 * pseudoref, but it certainly uses the pseudoref syntax.
>   	 */
> -	return 1;
> +	if (ends_with(refname, "HEAD"))
> +		return 1;
> +
> +	for (i = 0; i < ARRAY_SIZE(irregular_pseudorefs); i++)
> +		if (!strcmp(refname, irregular_pseudorefs[i]))
> +			return 1;
> +
> +	return 0;
>   }
>   
>   static int is_current_worktree_ref(const char *ref) {
> diff --git a/t/t1407-worktree-ref-store.sh b/t/t1407-worktree-ref-store.sh
> index 05b1881c59..53592c95f3 100755
> --- a/t/t1407-worktree-ref-store.sh
> +++ b/t/t1407-worktree-ref-store.sh
> @@ -61,18 +61,18 @@ test_expect_success 'create_symref(FOO, refs/heads/main)' '
>   # PSEUDO-WT and refs/bisect/random do not create reflogs by default, so it is
>   # not testing a realistic scenario.
>   test_expect_success REFFILES 'for_each_reflog()' '
> -	echo $ZERO_OID > .git/logs/PSEUDO-MAIN &&
> +	echo $ZERO_OID >.git/logs/PSEUDO_MAIN_HEAD &&
>   	mkdir -p     .git/logs/refs/bisect &&
> -	echo $ZERO_OID > .git/logs/refs/bisect/random &&
> +	echo $ZERO_OID >.git/logs/refs/bisect/random &&
>   
> -	echo $ZERO_OID > .git/worktrees/wt/logs/PSEUDO-WT &&
> +	echo $ZERO_OID >.git/worktrees/wt/logs/PSEUDO_WT_HEAD &&
>   	mkdir -p     .git/worktrees/wt/logs/refs/bisect &&
> -	echo $ZERO_OID > .git/worktrees/wt/logs/refs/bisect/wt-random &&
> +	echo $ZERO_OID >.git/worktrees/wt/logs/refs/bisect/wt-random &&
>   
>   	$RWT for-each-reflog | cut -d" " -f 2- | sort >actual &&
>   	cat >expected <<-\EOF &&
>   	HEAD 0x1
> -	PSEUDO-WT 0x0
> +	PSEUDO_WT_HEAD 0x0
>   	refs/bisect/wt-random 0x0
>   	refs/heads/main 0x0
>   	refs/heads/wt-main 0x0
> @@ -82,7 +82,7 @@ test_expect_success REFFILES 'for_each_reflog()' '
>   	$RMAIN for-each-reflog | cut -d" " -f 2- | sort >actual &&
>   	cat >expected <<-\EOF &&
>   	HEAD 0x1
> -	PSEUDO-MAIN 0x0
> +	PSEUDO_MAIN_HEAD 0x0
>   	refs/bisect/random 0x0
>   	refs/heads/main 0x0
>   	refs/heads/wt-main 0x0