Re: [PATCH v3 6/8] packed-backend: add "packed-refs" entry consistency check

[shejialuo <shejialuo@gmail.com>]

On Wed, Feb 12, 2025 at 10:56:50AM +0100, Patrick Steinhardt wrote:
> On Thu, Feb 06, 2025 at 01:59:40PM +0800, shejialuo wrote:
> > diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> > index c8bb93bb18..658f6bc7da 100644
> > --- a/refs/packed-backend.c
> > +++ b/refs/packed-backend.c
> > @@ -1826,6 +1899,26 @@ static int packed_fsck_ref_content(struct fsck_options *o,
> >  		line_number++;
> >  	}
> >  
> > +	while (start < eof) {
> > +		strbuf_reset(&packed_entry);
> > +		strbuf_addf(&packed_entry, "packed-refs line %lu", line_number);
> 
> Instead of greedily computing the name of the line, can we pass in the
> line number? The motivation is that in a well-formatted packed-refs file
> we won't ever need this string at all, so it's wasteful to proactively
> compute it for every single line.
> 

I agree with you here. And I already have idea to do this. Let me
improve this in the next version.

> > diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> > index da321f16c6..3ab6b5bba5 100755
> > --- a/t/t0602-reffiles-fsck.sh
> > +++ b/t/t0602-reffiles-fsck.sh
> > @@ -664,4 +664,46 @@ test_expect_success 'packed-refs header should be checked' '
> >  	)
> >  '
> >  
> > +test_expect_success 'packed-refs content should be checked' '
> > +	test_when_finished "rm -rf repo" &&
> > +	git init repo &&
> > +	(
> > +		cd repo &&
> > +		test_commit default &&
> > +		git branch branch-1 &&
> > +		git branch branch-2 &&
> > +		git tag -a annotated-tag-1 -m tag-1 &&
> > +		git tag -a annotated-tag-2 -m tag-2 &&
> > +
> > +		branch_1_oid=$(git rev-parse branch-1) &&
> > +		branch_2_oid=$(git rev-parse branch-2) &&
> > +		tag_1_oid=$(git rev-parse annotated-tag-1) &&
> > +		tag_2_oid=$(git rev-parse annotated-tag-2) &&
> > +		tag_1_peeled_oid=$(git rev-parse annotated-tag-1^{}) &&
> > +		tag_2_peeled_oid=$(git rev-parse annotated-tag-2^{}) &&
> > +		short_oid=$(printf "%s" $tag_1_peeled_oid | cut -c 1-4) &&
> > +
> > +		printf "# pack-refs with: peeled fully-peeled sorted \n"  >.git/packed-refs &&
> > +		printf "%s\n" "$short_oid refs/heads/branch-1" >>.git/packed-refs &&
> > +		printf "%sx\n" "$branch_1_oid" >>.git/packed-refs &&
> > +		printf "%s   refs/heads/bad-branch\n" "$branch_2_oid" >>.git/packed-refs &&
> > +		printf "%s refs/heads/branch.\n" "$branch_2_oid" >>.git/packed-refs &&
> > +		printf "%s refs/tags/annotated-tag-3\n" "$tag_1_oid" >>.git/packed-refs &&
> > +		printf "^%s\n" "$short_oid" >>.git/packed-refs &&
> > +		printf "%s refs/tags/annotated-tag-4.\n" "$tag_2_oid" >>.git/packed-refs &&
> > +		printf "^%s garbage\n" "$tag_2_peeled_oid" >>.git/packed-refs &&
> 
> This can be simplified using HERE docs.
> 
>         cat >.git/packed-refs <<-EOF
>         # pack-refs with: peeled fully-peeled sorted 
>         $short_oid refs/heads/branch-1
>         ${branch_1_oid}x
>         $branch_2_oid   refs/heads/bad-branch
>         $branch_2_oid refs/heads/branch.
>         $tag_1_oid refs/tags/annotated-tag-3
>         ^$short_oid\n
>         $tag_2_oid refs/tags/annotated-tag-4.
>         ^$tag_2_peeled_oid garbage
>         EOF
> 

Thanks for the suggestion, I will improve this in the next version.

> Patrick