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

[Jeff King <peff@peff.net>]

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.

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:

diff --git a/t/t5704-protocol-violations.sh b/t/t5704-protocol-violations.sh
index 34538cebf0..98d7f4981a 100755
--- a/t/t5704-protocol-violations.sh
+++ b/t/t5704-protocol-violations.sh
@@ -35,13 +35,15 @@ test_expect_success 'extra delim packet in v2 fetch args' '
 test_expect_success 'bogus symref in v0 capabilities' '
 	test_commit foo &&
 	oid=$(git rev-parse HEAD) &&
+	dst=refs/heads/foo &&
 	{
-		printf "%s HEAD\0symref object-format=%s\n" "$oid" "$GIT_DEFAULT_HASH" |
+		printf "%s HEAD\0symref object-format=%s symref=HEAD:%s\n" \
+			"$oid" "$GIT_DEFAULT_HASH" "$dst" |
 			test-tool pkt-line pack-raw-stdin &&
 		printf "0000"
 	} >input &&
-	git ls-remote --upload-pack="cat input ;:" . >actual &&
-	printf "%s\tHEAD\n" "$oid" >expect &&
+	git ls-remote --symref --upload-pack="cat input ;:" . >actual &&
+	printf "ref: %s\tHEAD\n%s\tHEAD\n" "$dst" "$oid" >expect &&
 	test_cmp expect actual
 '
 

I don't think it's hugely important (after all, this is something that
the server isn't supposed to send in the first place). But given that we
did make it work correctly (and the original on the security list
didn't), it's not too bad to test it on top.

Swapping out the "printf >expect" for a here-doc might make it a bit
more readable. I used printf because of the tab handling, but:

  tab=$(printf "\t")
  cat >expect <<-EOF
  ref: ${dst}${tab}HEAD
  ${oid}${tab}HEAD
  EOF

isn't too bad.

-Peff