Re: [PATCH 1/3] remote-curl: refactor smart-http discovery

[Jeff King <peff@peff.net>]

On Tue, Feb 05, 2019 at 03:29:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > After making initial contact with an http server, we have to decide if
> > the server supports smart-http, and if so, which version. Our rules are
> > a bit inconsistent:
> > ...
> >
> >  - we now predicate the smart/dumb decision entirely on the presence of
> >    the correct content-type
> >
> >  - we do a real pkt-line parse before deciding how to proceed (and die
> >    if it isn't valid)
> >
> >  - use skip_prefix() for comparing service strings, instead of
> >    constructing expected output in a strbuf; this avoids dealing with
> >    memory cleanup
> >
> > Note that this _is_ tightening what the client will allow. It's all
> > according to the spec, but it's possible that other implementations
> > might violate these. However, violating these particular rules seems
> > like an odd choice for a server to make.
> >
> > [1] Documentation/technical/http-protocol.txt, l. 166-167
> > [2] Documentation/technical/protocol-v2.txt, l. 63-64
> > [3] Documentation/technical/http-protocol.txt, l. 247
> >
> > Helped-by: Josh Steadmon <steadmon@google.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> >  remote-curl.c | 93 ++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 59 insertions(+), 34 deletions(-)
> 
> It turns out that this has interactions with 01f9ec64 ("Use
> packet_reader instead of packet_read_line", 2018-12-29) on the
> ms/packet-err-check branch.  Can we get this rebased on top of
> a more recent 'master'?

Yep. Here it is.

Rather than a range-diff, which is quite large due to the code movement,
I'll include below the interesting hunk of a diff between the two
endpoints (i.e., what we would have seen applying the packet-err-check
changes on top of my code movement, which is more or less what I did to
generate it).

Josh's original 3/3 isn't needed anymore, since ms/packet-err-check
covers that case already. However, he did write tests, which
ms/packet-err-check does not have. So I've converted his final patch
into just a test addition.

  [1/3]: remote-curl: refactor smart-http discovery
  [2/3]: remote-curl: tighten "version 2" check for smart-http
  [3/3]: t5551: test server-side ERR packet

 remote-curl.c                   | 100 ++++++++++++++++++--------------
 t/lib-httpd.sh                  |   1 +
 t/lib-httpd/apache.conf         |   4 ++
 t/lib-httpd/error-smart-http.sh |   3 +
 t/t5551-http-fetch-smart.sh     |   5 ++
 5 files changed, 70 insertions(+), 43 deletions(-)
 create mode 100644 t/lib-httpd/error-smart-http.sh

Here's that hunk:

diff --git a/remote-curl.c b/remote-curl.c
index b1309f2bdc..bb7421023b 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -333,10 +334,8 @@ static int get_protocol_http_header(enum protocol_version version,
 static void check_smart_http(struct discovery *d, const char *service,
 			     struct strbuf *type)
 {
-	char *src_buf;
-	size_t src_len;
-	char *line;
 	const char *p;
+	struct packet_reader reader;
 
 	/*
 	 * If we don't see x-$service-advertisement, then it's not smart-http.
@@ -348,45 +347,42 @@ static void check_smart_http(struct discovery *d, const char *service,
 	    strcmp(p, "-advertisement"))
 		return;
 
-	/*
-	 * "Peek" at the first packet by using a separate buf/len pair; some
-	 * cases below require us leaving the originals intact.
-	 */
-	src_buf = d->buf;
-	src_len = d->len;
-	line = packet_read_line_buf(&src_buf, &src_len, NULL);
-	if (!line)
+	packet_reader_init(&reader, -1, d->buf, d->len,
+			   PACKET_READ_CHOMP_NEWLINE |
+			   PACKET_READ_DIE_ON_ERR_PACKET);
+	if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
 		die("invalid server response; expected service, got flush packet");
 
-	if (skip_prefix(line, "# service=", &p) && !strcmp(p, service)) {
+	if (skip_prefix(reader.line, "# service=", &p) && !strcmp(p, service)) {
 		/*
 		 * The header can include additional metadata lines, up
 		 * until a packet flush marker.  Ignore these now, but
 		 * in the future we might start to scan them.
 		 */
-		while (packet_read_line_buf(&src_buf, &src_len, NULL))
-			;
+		for (;;) {
+			packet_reader_read(&reader);
+			if (reader.pktlen <= 0) {
+				break;
+			}
+		}
 
 		/*
 		 * v0 smart http; callers expect us to soak up the
 		 * service and header packets
 		 */
-		d->buf = src_buf;
-		d->len = src_len;
+		d->buf = reader.src_buffer;
+		d->len = reader.src_len;
 		d->proto_git = 1;
 
-	} else if (!strcmp(line, "version 2")) {
+	} else if (!strcmp(reader.line, "version 2")) {
 		/*
 		 * v2 smart http; do not consume version packet, which will
 		 * be handled elsewhere.
 		 */
 		d->proto_git = 1;
 
-	} else if (skip_prefix(line, "ERR ", &p)) {
-		die(_("remote error: %s"), p);
-
 	} else {
-		die("invalid server response; got '%s'", line);
+		die("invalid server response; got '%s'", reader.line);
 	}
 }