Re: [PATCH] connect: also update offset for features without values

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 18, 2021, 22:35 UTC
Message-ID: <YUZpwi2HhflICd4Z@nand.local>
In-Reply-To: <YUZinXsGdL19l/tQ@coredump.intra.peff.net>

On Sat, Sep 18, 2021 at 06:05:17PM -0400, Jeff King wrote:

Show 20 quoted lines

> On Sat, Sep 18, 2021 at 11:53:00AM -0400, Taylor Blau wrote:
>
> > > +test_expect_success 'bogus symref in v0 capabilities' '
> > > +	test_commit foo &&
> > > +	oid=$(git rev-parse HEAD) &&
> > > +	{
> > > +		printf "%s HEAD\0symref object-format=%s\n" "$oid" "$GIT_DEFAULT_HASH" |
> > > +			test-tool pkt-line pack-raw-stdin &&
> >
> > I'm actually really happy with this modification to add the non-empty
> > object-format after the broken "symref" part, since it ensures that your
> > offset calculation is right (and that we can continue to parse features
> > with or without values after a value-less one).
>
> I don't think it quite does that, though. If I understand the parsing
> code correctly, it walks through the list looking for entries for a
> _particular_ capability. I.e., it will look for any "symref" entries,
> advancing the offset counter. And then separately it will start again
> looking for any object-format entries, with a brand-new offset counter
> starting at 0.

Ah; you're absolutely right. We call next_server_feature_value from annotate_refs_with_symref_info() and server_supports_hash(), each of which initializes their own offset from zero.

Show 5 quoted lines

> So if you want to confirm that the parsing continues after the
> unexpected entry, you'd want a second symref entry, and then to make
> sure it was correctly parsed.  Perhaps something like this:
>
> [...]

Yeah, I agree that would exercise it, and I also agree that it isn't hugely important. But this patch does make an effort to handle that case, so it's probably worth testing.

Thanks, Taylor