Subject: Re: [PATCH v2 2/4] ref: add regular ref content check for files backend

From: shejialuo <[email-masked]>
Date: Wed, 28 Aug 2024 22:41:32 +0800

On Wed, Aug 28, 2024 at 02:50:01PM +0200, Patrick Steinhardt wrote:
> On Wed, Aug 28, 2024 at 12:07:58AM +0800, shejialuo wrote:
> > @@ -170,6 +173,12 @@
> >  `nullSha1`::
> >  	(WARN) Tree contains entries pointing to a null sha1.
> >  
> > +`refMissingNewline`::
> > +	(INFO) A valid ref does not end with newline.
> 
> This reads a bit funny to me. If the ref is valid, why do we complain?
> 
> Maybe this would read better if you said "An otherwise valid ref does
> not end with a newline".
> 

I think we should just drop the "valid" here. Because for symref, it
may miss newline and is NOT valid.

I will improve this in the next version.

> > @@ -3430,6 +3434,65 @@ typedef int (*files_fsck_refs_fn)(struct ref_store *ref_store,
> >  				  const char *refs_check_dir,
> >  				  struct dir_iterator *iter);
> >  
> > +static int files_fsck_refs_content(struct ref_store *ref_store,
> > +				   struct fsck_options *o,
> > +				   const char *refs_check_dir,
> > +				   struct dir_iterator *iter)
> > +{
> > +	struct strbuf ref_content = STRBUF_INIT;
> > +	struct strbuf referent = STRBUF_INIT;
> > +	struct strbuf refname = STRBUF_INIT;
> > +	struct fsck_ref_report report = {0};
> > +	const char *trailing = NULL;
> > +	unsigned int type = 0;
> > +	int failure_errno = 0;
> > +	struct object_id oid;
> > +	int ret = 0;
> > +
> > +	strbuf_addf(&refname, "%s/%s", refs_check_dir, iter->relative_path);
> > +	report.path = refname.buf;
> > +
> > +	if (S_ISREG(iter->st.st_mode)) {
> 
> This is still indenting the whole body. You mentioned that you don't
> want to use `goto`, but in our codebase it's actually quite idiomatic.
> And you already use it anyway.
> 

I think so, indenting is noisy. Will use "goto" to avoid indenting.

> > diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> > index 71a4d1a5ae..7c1910d784 100755
> > --- a/t/t0602-reffiles-fsck.sh
> > +++ b/t/t0602-reffiles-fsck.sh
> > @@ -89,4 +89,91 @@ test_expect_success 'ref name check should be adapted into fsck messages' '
> >  	test_must_be_empty err
> >  '
> >  
> > +test_expect_success 'regular ref content 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 checkout -b a/b/tag-2 &&
> 
> Wouldn't it be sufficient to only create a single commit, e.g. via
> `test_commit`? From all I can see all you need is some object ID, so
> creating the tags and second commit doesn't seem to be necessary.
> 

I agree with this. I will clean the code for the next version.

> > +	printf "%s" "$(git rev-parse branch-1)" > $branch_dir_prefix/branch-1-no-newline &&
> 
> We don't typically have spaces after the redirect. So you should remove
> them here and in all the subsequent instances.
> 

I will clean the code style here.

> > +	git refs verify 2>err &&
> > +	cat >expect <<-EOF &&
> > +	warning: refs/heads/branch-1-no-newline: refMissingNewline: missing newline
> > +	EOF
> > +	rm $branch_dir_prefix/branch-1-no-newline &&
> > +	test_cmp expect err &&
> 
> I was wondering whether each of these cases should be a separate test,
> but that may be a bit wasteful. Alternatively, can we maybe set up a
> single repository with all the garbage that we want to verify and then
> double check that executing `git refs verify` surfaces them all in a
> single invocation?
> 

Actually, I have also thought about separating the tests which may
clear and I dropped this idea due to the reason the same as yours. I DO
agree that we should set up a single repository with all the garbage
that we want to verify. This is necessary.

Thanks,
Jialuo