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

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:

quoted

quoted

+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