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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Sep 18, 2021 at 06:05:17PM -0400, Jeff King wrote:
> 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.

> 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