Re: [PATCH 2/5] refs: make `is_pseudoref_syntax()` stricter
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jan 22, 2024, 20:13 UTC
- 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:
Show 11 quoted lines
> 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
Show 83 quoted lines
> 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