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
