Re: [PATCH 3/3] remote-curl: sanity check ref advertisement from server

Jeff King wrote:

On Sun, Feb 17, 2013 at 03:05:34AM -0800, Jonathan Nieder wrote:

quoted

Jeff King wrote:

quoted

quoted

+		if (verify_ref_advertisement(last->buf, last->len) < 0)
+			die("ref advertisement is invalid at %s", refs_url);

Won't this error out with

	protocol error: bad line length character: ERR

instead of the current more helpful behavior for ERR lines?

I don't think so. Don't ERR lines appear inside their own packets?

Yes, I misread get_remote_heads for some reason.  Thanks for checking.

[...]

The one thing we do also check, though, is that we end with a flush
packet. So depending on what servers produce, it may mean we trigger
this complaint instead of passing the ERR along to fetch-pack.

Rather than doing this fake syntactic verification, I wonder if we
should simply call get_remote_heads, which does a more thorough check

I'm not sure whether servers are expected to send a flush after an
ERR packet.  The only codepath I know of in git itself that sends
such packets is git-daemon, which does not flush after the error (but
is not used in the stateless-rpc case).  http-backend uses HTTP error
codes for its errors.

If I am reading get_remote_heads correctly, calling it with the
following tweak should work ok.  The extra thread is just to feed a
string into a fd-based interface and could be avoided for "list", too,
if it costs too much.

diff --git i/connect.c w/connect.c
index 49e56ba3..55ee7de7 100644
--- i/connect.c
+++ w/connect.c

@@ -68,7 +68,8 @@ struct ref **get_remote_heads(int in, struct ref **list,
 {
 	int got_at_least_one_head = 0;
 
-	*list = NULL;
+	if (list)
+		*list = NULL;
 	for (;;) {
 		struct ref *ref;
 		unsigned char old_sha1[20];

@@ -92,6 +93,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			die("protocol error: expected sha/ref, got '%s'", buffer);
 		name = buffer + 41;
 
+		if (!list)
+			continue;
+
 		name_len = strlen(name);
 		if (len != name_len + 41) {
 			free(server_capabilities);