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

[shejialuo <shejialuo@gmail.com>]

On Tue, May 13, 2025 at 09:30:57AM -0700, Junio C Hamano wrote:
> 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.
> 

Good idea, I will improve the commit message.

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

That's right. I should talk about this problem.

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

That's right. Actually we introduce redirection here. I think I will
improve this in the next release cycle. I'll add this into my TODO list.