Re: [PATCH v2 4/5] refs/reftable: add fsck check for trailing newline

[Patrick Steinhardt <ps@pks.im>]

On Tue, Sep 02, 2025 at 03:38:33PM -0700, Junio C Hamano wrote:
> Karthik Nayak <karthik.188@gmail.com> writes:
> 
> > Introduce a fsck check for the reftable backend, which checks if the
> > 'tables.list' contains a newline. The reftable backend writes a trailing
> > newline when writing the 'tables.list', but it doesn't check for it when
> > reading the file. A missing newline however indicates that the file was
> > manually tampered with, so let's raise this as an error to the user.
> 
> Hmph, how does the code react to other kinds of "manual tampering"?
> For example, if an empty line is inserted between two existing lines
> (or at the beginning of the file, for that matter), would the parser
> detect it as a corrupt file and die?
> 
> If so, it makes me strongly suspect that we are better off enforcing
> that the file does not end in an incomplete line at runtime and barf
> just the same way, instead of "most of the anomalies that the write
> codepath would never produce would cause error on the read codepath,
> but only this one that the read codepath is happy with is caught by
> the fsck", which does not sound quite right.

Fair, I'm also of the opinion that we should tighten the parser logic to
detect and reject any invalid files. For previous checks where we verify
that the table names are sane I think it's fair to live with it and warn
about those, as the actual names don't really matter. But as soon as we
hit actually-broken formats I also think that we're better of rejecting
those altogether.

Patrick