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

[Patrick Steinhardt <ps@pks.im>]

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