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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 12, 2025, 09:56 UTC
Message-ID: <Z6xwW0N7GG4NhCzZ@pks.im>
In-Reply-To: <Z6RPqE64ScsjzUg7@ArchLinux>

On Thu, Feb 06, 2025 at 01:59:04PM +0800, shejialuo wrote:

Show 20 quoted lines

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

Show 21 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?

Patrick