On Mon, Feb 17, 2025 at 11:27:50PM +0800, shejialuo wrote:
> 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
