Re: [PATCH v4 1/3] packed-backend: fsck should warn when "packed-refs" file is empty

From: Junio C Hamano <gitster@pobox.com>
Date: May 13, 2025, 16:30 UTC
Message-ID: <xmqqplgch3r2.fsf@gitster.g>
In-Reply-To: <aCMn_Ktrg4GY8jHe@ArchLinux>

shejialuo <shejialuo@gmail.com> writes:

> During fsck, an empty "packed-refs" gives an error; this is unwarranted.
> The runtime code paths would accept an empty "packed-refs" file, such as
> "create_snapshot" would simply return the "snapshot" without checking
> the content of "packed-refs".

Perhaps "unwarranted" is now too strong a word; we still want to consider it an anomaly (that is why emptyPackedRefsFile warning is introduced after all). I think the problem description you want to here in the above pragraph is that fsck giving an error and runtime completely silent is inconsistent.

    Side note: and you'd probably want to say what "an error"
    reported here is.  The problem, if I understand correctly, is
    that the code assumes the file won't be empty and instead has at
    least one line in it (even when there are no refs packed, there
    is the file header line) and insists that all lines must be well
    terminated---if we tolerate an empty file, of course such a
    check will fail, as there is no terminating LF in a file with 0
    lines in it.

And because versions of Git that are not too ancient never wrote an empty packed-refs file, and often having an empty file there is/was a sign of a filesystem-level issue, the way we want resolve this inconsistency is not make everybody totally silent but notice and report the anomaly.

> But we need to consider the fsck message type carefully, it is not
> appropriate that we use "FSCK_ERROR". This is because we would
> definitely break the compatibility. Let's create a "FSCK_INFO" message
> id EMPTY_PACKED_REFS_FILE" to indicate that "packed-refs" is empty.

OK.

Show 21 quoted lines

> Signed-off-by: shejialuo <shejialuo@gmail.com>
> ---
>  Documentation/fsck-msgids.adoc |  6 ++++++
>  fsck.h                         |  1 +
>  refs/packed-backend.c          |  9 +++++++++
>  t/t0602-reffiles-fsck.sh       | 17 +++++++++++++++++
>  4 files changed, 33 insertions(+)
>
> diff --git a/Documentation/fsck-msgids.adoc b/Documentation/fsck-msgids.adoc
> index 9601fff228..0ba4f9a27e 100644
> --- a/Documentation/fsck-msgids.adoc
> +++ b/Documentation/fsck-msgids.adoc
> @@ -59,6 +59,12 @@
>  `emptyName`::
>  	(WARN) A path contains an empty name.
>  
> +`emptyPackedRefsFile`::
> +	(INFO) "packed-refs" file is empty. Report to the
> +	git@vger.kernel.org mailing list if you see this error. As only
> +	very early versions of Git would create such an empty
> +	"packed_refs" file, we might tighten this rule in the future.

I am not too happy to see "Report to ..." and everything after that here, primarily because it takes one extra step for the user to find it out when they see such an informational message. There are other existing error classes, like refMissingNewline, etc., that have the same problem. One thing to make it easier for the users to report is to put it in the error/info messages themselves, but I think it is OK to make such a clean-up (including the existing offenders) after the dust settles from this topic.

Show 16 quoted lines

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 3ad1ed0787..fb91833e76 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -2103,6 +2103,15 @@ static int packed_fsck(struct ref_store *ref_store,
>  		goto cleanup;
>  	}
>  
> +	if (!st.st_size) {
> +		struct fsck_ref_report report = { 0 };
> +		report.path = "packed-refs";
> +		ret = fsck_report_ref(o, &report,
> +				      FSCK_MSG_EMPTY_PACKED_REFS_FILE,
> +				      "file is empty");
> +		goto cleanup;
> +	}

OK.

Show 28 quoted lines

> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> index 9d1dc2144c..f671ac4d3a 100755
> --- a/t/t0602-reffiles-fsck.sh
> +++ b/t/t0602-reffiles-fsck.sh
> @@ -647,6 +647,23 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
>  	)
>  '
>  
> +test_expect_success 'empty packed-refs should be reported' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit default &&
> +
> +		>.git/packed-refs &&
> +		git refs verify 2>err &&
> +		cat >expect <<-EOF &&
> +		warning: packed-refs: emptyPackedRefsFile: file is empty
> +		EOF
> +		rm .git/packed-refs &&
> +		test_cmp expect err
> +	)
> +'
> +
>  test_expect_success 'packed-refs header should be checked' '
>  	test_when_finished "rm -rf repo" &&
>  	git init repo &&