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

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 30, 2024, 08:31 UTC
Message-ID: <Zqik9EOKd43WxGXp@tanuki>
In-Reply-To: <ZqeY0eHNZjKhNvIH@ArchLinux>

On Mon, Jul 29, 2024 at 09:27:45PM +0800, shejialuo wrote:

Show 38 quoted lines

> 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.

Show 18 quoted lines

>  	};
>  
> 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`?

Show 16 quoted lines

> +
> +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.

Show 20 quoted lines

> +	(
> +		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