On Wed, Feb 12, 2025 at 09:48:09AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@xxxxxx> writes:
>
> > On Thu, Feb 06, 2025 at 01:59:04PM +0800, shejialuo wrote:
> >> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> >> index 6401cecd5f..683cfe78dc 100644
> >> --- a/refs/packed-backend.c
> >> +++ b/refs/packed-backend.c
> >> @@ -1749,12 +1749,76 @@ static struct ref_iterator *packed_reflog_iterator_begin(struct ref_store *ref_s
> >> +static int packed_fsck_ref_header(struct fsck_options *o,
> >> + const char *start, const char *eol)
> >> +{
> >> + if (!starts_with(start, "# pack-refs with:")) {
> >> + struct fsck_ref_report report = { 0 };
> >> + report.path = "packed-refs.header";
> >> +
> >> + return fsck_report_ref(o, &report,
> >> + FSCK_MSG_BAD_PACKED_REF_HEADER,
> >> + "'%.*s' does not start with '# pack-refs with:'",
> >> + (int)(eol - start), start);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >
> > Okay. We still complain about bad headers, but only if there is a line
> > starting with "#" and only if the prefix doesn't match. This addresses
> > Junio's comment that packfiles don't have to have a header, and that
> > they may contain capabilities that we don't understand.
>
> We'd want to also ensure that there is a single trailing whitespace
> after that colon, which we have always written after "with:", no?
>
As you have commented below, I don't add this check due to the reason
that "create_snapshot" method does _not_ check this.
> >> diff --git a/t/t0602-reffiles-fsck.sh b/t/t0602-reffiles-fsck.sh
> >> index 42c8d4ca1e..da321f16c6 100755
> >> --- a/t/t0602-reffiles-fsck.sh
> >> +++ b/t/t0602-reffiles-fsck.sh
> >> @@ -639,4 +639,29 @@ test_expect_success SYMLINKS 'the filetype of packed-refs should be checked' '
> >> )
> >> '
> >>
> >> +test_expect_success 'packed-refs header should be checked' '
> >> + test_when_finished "rm -rf repo" &&
> >> + git init repo &&
> >> + (
> >> + cd repo &&
> >> + test_commit default &&
> >> +
> >> + git refs verify 2>err &&
> >> + test_must_be_empty err &&
> >> +
> >> + for bad_header in "# pack-refs wit: peeled fully-peeled sorted " \
> >> + "# pack-refs with traits: peeled fully-peeled sorted " \
> >> + "# pack-refs with a: peeled fully-peeled"
> >
> > Instead of verifying thrice that we complain about bad header prefixes,
> > should we maybe replace two of these with instances where we check a
> > packed-refs file _without_ a header and one with capabilities that we
> > don't understand?
>
> Yup. I also notice that refs/packed-backend.c:create_snapshot()
> would accept "# pack-refs with:peeled" if I am not reading it
> correctly, which is an unrelated bug.
>
Yes, you are correct. Let me fix this in the next version.
Thanks,
Jialuo
