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

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 25, 2025, 08:27 UTC
Message-ID: <Z71-1xa_o39IAo6A@pks.im>
In-Reply-To: <Z7NVdhM61rhjAHtW@ArchLinux>

On Mon, Feb 17, 2025 at 11:27:50PM +0800, shejialuo wrote:

Show 13 quoted lines

> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 8140a31d07..09eb3886c3 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -694,7 +694,7 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs)
>  
>  		tmp = xmemdupz(snapshot->buf, eol - snapshot->buf);
>  
> -		if (!skip_prefix(tmp, "# pack-refs with:", (const char **)&p))
> +		if (!skip_prefix(tmp, "# pack-refs with: ", (const char **)&p))
>  			die_invalid_line(refs->path,
>  					 snapshot->buf,
>  					 snapshot->eof - snapshot->buf);

I know that Junio pointed out that we should check for a trailing space after the colon. But do we really feel comfortable to tighten the check like this now? If there was any broken writer of the format that does not include the whitespace we'd now be unable to parse their output.

I scanned through a couple of third-party clients:

  - libgit2 is fine and always writes the space. It also expects the
    whitespace to exist.

  - JGit does not expect the header to have a trailing space, but
    expects the "peeled" capability to have a leading space, which is
    mostly equivalent because that capability is typically the first one
    we write. It always writes the space.

  - gitoxide expects the space to exist and writes it.

  - go-git doesn't even seem to care about the header? Dunno, maybe I
    was just not able to locate the relevant code.

So yes, we should be fine, and the fact that other implementations expect the space to exist indicates that being more thorough here is a good thing. It might be a good idea though to split out this change into a separate commit and then provide more reasoning _why_ it is fine, including the above info about alternate implementations.

Patrick