| Message ID | Z73D5XLzzg-OPmdT@ArchLinux (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add more ref consistency checks | expand |
On Tue, Feb 25, 2025 at 09:21:41PM +0800, shejialuo wrote: > We always write a space after "# pack-refs with:". However, when > creating the packed-ref snapshot, we only check whether the header > starts with "# pack-refs with:". However, we need to make sure that we > would not break compatibility by tightening the rule. The following is > how some third-party libraries handle the header of "packed-ref" file. > > 1. libgit2 is fine and always writes the space. It also expects the > whitespace to exist. > 2. JGit does not expect th 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. > 3. gitoxide expects the space t exist and writes it. > 4. go-git doesn't create the header by default. > > So, we are safe to tighten the rule by checking whether the header > starts with "# pack-refs with: ". The commit message nicely describes why it's safe to do the change, but it doesn't describe why it's something we _want_ to do. Ideally, we'd be able to argue with a technical spec of the format, but unless I'm mistaken such a document does not exist. The next-best thing is to do what everyone can agree on, and that seems to be to both write and expect a space after the colon. By not following consensus that exists in other libraries we're being more loose. So if we for example started to stop writing the space due to a bug, we'd still continue to parse the header alright and thus not notice the problem, but now we have broken other implementations. That may be a good enough justification for the change itself. Patrick
On Wed, Feb 26, 2025 at 09:08:32AM +0100, Patrick Steinhardt wrote: > On Tue, Feb 25, 2025 at 09:21:41PM +0800, shejialuo wrote: > > We always write a space after "# pack-refs with:". However, when > > creating the packed-ref snapshot, we only check whether the header > > starts with "# pack-refs with:". However, we need to make sure that we > > would not break compatibility by tightening the rule. The following is > > how some third-party libraries handle the header of "packed-ref" file. > > > > 1. libgit2 is fine and always writes the space. It also expects the > > whitespace to exist. > > 2. JGit does not expect th 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. > > 3. gitoxide expects the space t exist and writes it. > > 4. go-git doesn't create the header by default. > > > > So, we are safe to tighten the rule by checking whether the header > > starts with "# pack-refs with: ". > > The commit message nicely describes why it's safe to do the change, but > it doesn't describe why it's something we _want_ to do. > Yes, as you have said below. We don't have document about the header format. It's an internal implementation of Git. > Ideally, we'd be able to argue with a technical spec of the format, but > unless I'm mistaken such a document does not exist. The next-best thing > is to do what everyone can agree on, and that seems to be to both write > and expect a space after the colon. By not following consensus that > exists in other libraries we're being more loose. > > So if we for example started to stop writing the space due to a bug, > we'd still continue to parse the header alright and thus not notice the > problem, but now we have broken other implementations. That may be a > good enough justification for the change itself. > Thanks, I will improve the commit message in the next version. > Patrick
diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 6c118119a0..9dabb5e556 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);
We always write a space after "# pack-refs with:". However, when creating the packed-ref snapshot, we only check whether the header starts with "# pack-refs with:". However, we need to make sure that we would not break compatibility by tightening the rule. The following is how some third-party libraries handle the header of "packed-ref" file. 1. libgit2 is fine and always writes the space. It also expects the whitespace to exist. 2. JGit does not expect th 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. 3. gitoxide expects the space t exist and writes it. 4. go-git doesn't create the header by default. So, we are safe to tighten the rule by checking whether the header starts with "# pack-refs with: ". Mentored-by: Patrick Steinhardt <ps@pks.im> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: shejialuo <shejialuo@gmail.com> --- refs/packed-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)