Subject: Re: [GSoC][PATCH v13 09/10] fsck: add ref name check for files backend

From: Patrick Steinhardt <[email-masked]>
Date: Tue, 30 Jul 2024 10:31:48 +0200

On Mon, Jul 29, 2024 at 09:27:45PM +0800, shejialuo wrote:
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index cb184953c1..0d4fc27768 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3419,6 +3419,27 @@ typedef int (*files_fsck_refs_fn)(struct fsck_options *o,
>  				  const char *refs_check_dir,
>  				  struct dir_iterator *iter);
>  
> +static int files_fsck_refs_name(struct fsck_options *o,
> +				const char *gitdir UNUSED,
> +				const char *refs_check_dir,
> +				struct dir_iterator *iter)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct fsck_refs_info info;
> +	int ret = 0;
> +
> +	if (check_refname_format(iter->basename, REFNAME_ALLOW_ONELEVEL)) {
> +		strbuf_addf(&sb, "%s/%s", refs_check_dir, iter->relative_path);
> +		info.path = sb.buf;
> +		ret = fsck_refs_report(o, NULL, &info,
> +				       FSCK_MSG_BAD_REF_NAME,
> +				       "invalid refname format");
> +	}
> +
> +	strbuf_release(&sb);
> +	return ret;
> +}
> +
>  static int files_fsck_refs_dir(struct ref_store *ref_store,
>  			       struct fsck_options *o,
>  			       const char *refs_check_dir,
> @@ -3469,6 +3490,7 @@ static int files_fsck_refs(struct ref_store *ref_store,
>  			   struct fsck_options *o)
>  {
>  	files_fsck_refs_fn fsck_refs_fns[]= {
> +		files_fsck_refs_name,
>  		NULL

Neat. I very much like that we can simply add new checks to this
function and the rest is handled for us already. Makes this whole thing
nicely extensible.

>  	};
>  
> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> new file mode 100755
> index 0000000000..b2db58d2c6
> --- /dev/null
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +
> +test_description='Test reffiles backend consistency check'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +GIT_TEST_DEFAULT_REF_FORMAT=files
> +export GIT_TEST_DEFAULT_REF_FORMAT
> +
> +. ./test-lib.sh

Is this test suite intentionally not marked with
`TEST_PASSES_SANITIZE_LEAK=true`?

> +
> +test_expect_success 'ref name should be checked' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	branch_dir_prefix=.git/refs/heads &&
> +	tag_dir_prefix=.git/refs/tags &&
> +	(
> +		cd repo &&
> +		git commit --allow-empty -m initial &&
> +		git checkout -b branch-1 &&
> +		git tag tag-1 &&
> +		git commit --allow-empty -m second &&
> +		git checkout -b branch-2 &&
> +		git tag tag-2 &&
> +		git tag multi_hierarchy/tag-2
> +	) &&

I don't quite get why you create several subshells only to cd into
`repo` in each of them. Isn't a single subshell sufficient for all of
those tests? If you want to delimit blocks, then you can simply add an
empty newline between them.

> +	(
> +		cd repo &&
> +		cp $branch_dir_prefix/branch-1 $branch_dir_prefix/.branch-1 &&
> +		test_must_fail git fsck 2>err &&
> +		cat >expect <<-EOF &&
> +		error: refs/heads/.branch-1: badRefName: invalid refname format
> +		EOF
> +		rm $branch_dir_prefix/.branch-1 &&
> +		test_cmp expect err
> +	) &&
> +	(
> +		cd repo &&
> +		cp $tag_dir_prefix/tag-1 $tag_dir_prefix/tag-1.lock &&
> +		test_must_fail git fsck 2>err &&
> +		cat >expect <<-EOF &&
> +		error: refs/tags/tag-1.lock: badRefName: invalid refname format
> +		EOF
> +		rm $tag_dir_prefix/tag-1.lock &&
> +		test_cmp expect err
> +	) &&

The other cases all make sense, but I don't think that a file ending
with ".lock" should be marked as having a "badRefName". It is expected
that concurrent writers may have such lock files.

What could make sense is to eventually mark stale lock files older than
X amount of time as errors or warnings. But I'd think that this is
outside of the scope of this patch series.

Patrick