Re: [PATCH v4 4/8] packed-backend: add "packed-refs" header consistency check

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

shejialuo <shejialuo@gmail.com> writes:

> In "packed-backend.c::create_snapshot", if there is a header (the line
> which starts with '#'), we will check whether the line starts with "#
> pack-refs with:". Before we port this check into "packed_fsck", let's
> fix "create_snapshot" to check the prefix "# packed-ref with: " instead
> of "# packed-ref with:" due to that we will always write a single
> trailing space after the colon.

A more important reason to be more strict is not "we will always
write", but "we HAVE ALWAYS written", I think.

> However, we need to consider other situations and discuss whether we
> need to add checks.
>
> 1. If the header does not exist, we should not report an error to the
>    user. This is because in older Git version, we never write header in
>    the "packed-refs" file. Also, we do allow no header in "packed-refs"
>    in runtime.

Yes.

> 2. If the header content does not start with "# packed-ref with: ", we
>    should report an error just like what "create_snapshot" does. So,
>    create a new fsck message "badPackedRefHeader(ERROR)" for this.

OK.

> 3. If the header content is not the same as the constant string
>    "PACKED_REFS_HEADER". This is expected because we make it extensible
>    intentionally. So, there is no need to report.

Nor there is any need to check for literal equality with the
constant string.  We may want to split the traits that are recorded
on the "with:" line and see if there are ones that we do not
recognise if only for curiosity, but because create_snapshot(), which
is the only run-time consumer of this information, only uses the
ones it recognises while ignoring everything else, presence of an
unknown trait is not an error- or even warning-worthy event.  Unless
we are curious and want to emit "info" level message, there is not
much point in checking the remainder of the header.