Re: [PATCH v3 4/8] packed-backend: add "packed-refs" header consistency check
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 12, 2025, 17:48 UTC
- Message-ID
- <xmqq4j0zuk9y.fsf@gitster.g>
- In-Reply-To
- <Z6xwW0N7GG4NhCzZ@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 26 quoted lines
> 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?
Show 26 quoted lines
>> 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.
Thanks.