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

[Junio C Hamano <gitster@pobox.com>]

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.

> 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.

> 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.

> 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 &&