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

From: shejialuo <[email-masked]>
Date: Tue, 25 Feb 2025 20:34:06 +0800

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