On Tue, Feb 25, 2025 at 09:27:03AM +0100, Patrick Steinhardt wrote:

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

quoted

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.

I have searched the code. The go-git implement "git pack-refs" in
`PackRefs`. go-git never writes header for "packed-refs" file.

Thanks for this wonderful suggestion.

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.

Yes, I agree that we should split out this change. Let me do this.

Patrick