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

From: shejialuo <[email-masked]>
Date: Wed, 31 Jul 2024 00:14:24 +0800

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

No, I don't know this. I will add `TEST_PASSES_SANITIZE_LEAK=true` and
export this environment variable.

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

I just want to delimit, I will use newline in the next version.

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

If so, let us just ignore ".lock" situation at the moment.

> Patrick