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".
> @@ -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.
> 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.
> + 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.
> + 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?
Patrick
