On 01/10, Jonathan Tan wrote:

On Tue,  2 Jan 2018 16:18:28 -0800
Brandon Williams [off-list ref] wrote:

quoted

+static size_t proxy_in(void *ptr, size_t eltsize,
+		       size_t nmemb, void *buffer_)

OK, I managed to look at the Curl stuff in more detail.

I know that these parameter names are what remote_curl.c has been using
for its callbacks, but I find them confusing (in particular, some Curl
documentation rightly refer to the 1st parameter as a buffer, and the
4th parameter is actually userdata). Also, according to the Curl
documentation, the type of the first parameter is "char *". Could we
change the type of the first parameter to "char *", and the name of the
fourth parameter either to "proxy_state_" or "userdata"?

Sounds good, I'll make the change.

quoted

+{
+	size_t max = eltsize * nmemb;
+	struct proxy_state *p = buffer_;
+	size_t avail = p->request_buffer.len - p->pos;
+
+	if (!avail) {
+		if (p->seen_flush) {
+			p->seen_flush = 0;
+			return 0;
+		}
+
+		strbuf_reset(&p->request_buffer);
+		switch (packet_reader_read(&p->reader)) {
+		case PACKET_READ_EOF:
+			die("error reading request from parent process");

This should say "BUG:", I think. I'm not sure what the best way of
explaining it is, but basically connect_half_duplex is supposed to
ensure (by peeking) that there is no EOF when proxy_in() is called.

This wouldn't necessarily be a bug if the parent dies early for some
reason though right?

quoted

+		case PACKET_READ_NORMAL:
+			packet_buf_write_len(&p->request_buffer, p->reader.line,
+					     p->reader.pktlen);
+			break;
+		case PACKET_READ_DELIM:
+			packet_buf_delim(&p->request_buffer);
+			break;
+		case PACKET_READ_FLUSH:
+			packet_buf_flush(&p->request_buffer);
+			p->seen_flush = 1;
+			break;
+		}
+		p->pos = 0;
+		avail = p->request_buffer.len;
+	}
+
+	if (max < avail)
+		avail = max;
+	memcpy(ptr, p->request_buffer.buf + p->pos, avail);
+	p->pos += avail;
+	return avail;

Thanks, this looks correct. I wish that the Curl API had a way for us to
say "here are 4 more bytes, and that is all" instead of us having to
make a note (p->seen_flush) to remember to return 0 on the next call,
but that's the way it is.

quoted

+}
+static size_t proxy_out(char *ptr, size_t eltsize,
+			size_t nmemb, void *buffer_)

Add a blank line before proxy_out. Also, same comment as proxy_in()
about the function signature.

I'll change this function too.

quoted

+{
+	size_t size = eltsize * nmemb;
+	struct proxy_state *p = buffer_;
+
+	write_or_die(p->out, ptr, size);
+	return size;
+}
+
+static int proxy_post(struct proxy_state *p)
+{
+	struct active_request_slot *slot;
+	struct curl_slist *headers = http_copy_default_headers();
+	int err;
+
+	headers = curl_slist_append(headers, p->hdr_content_type);
+	headers = curl_slist_append(headers, p->hdr_accept);
+	headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+
+	slot = get_active_slot();
+
+	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
+	curl_easy_setopt(slot->curl, CURLOPT_URL, p->service_url);
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);

I looked at the Curl documentation for CURLOPT_HTTPHEADER and
curl_easy_setopt doesn't consume the argument here (in fact, it asks us
to keep "headers" around), so it might be possible to just generate the
headers once in proxy_state_init().

Yeah I'll go ahead and do that, it'll make the post function a bit
cleaner too.

quoted

+
+	/* Setup function to read request from client */
+	curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, proxy_in);
+	curl_easy_setopt(slot->curl, CURLOPT_READDATA, p);
+
+	/* Setup function to write server response to client */
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out);
+	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p);
+
+	err = run_slot(slot, NULL);
+
+	if (err != HTTP_OK)
+		err = -1;

This seems to mean that we cannot have two requests in flight at the
same time even while there is no response (from the fact that we have a
HTTP status code after returning from run_slot()).

I thought that git fetch over HTTP uses the two-requests-in-flight
optimization that it also does over other protocols like SSH, but I see
that that code path (fetch_git() in remote-curl.c) also uses run_slot()
indirectly, so maybe my assumption is wrong. Anyway, this is outside the
scope of this patch.

quoted

+
+	curl_slist_free_all(headers);
+	return err;
+}
+
+static int connect_half_duplex(const char *service_name)
+{
+	struct discovery *discover;
+	struct proxy_state p;
+
+	/*
+	 * Run the info/refs request and see if the server supports protocol
+	 * v2.  If and only if the server supports v2 can we successfully
+	 * establish a half-duplex connection, otherwise we need to tell the
+	 * client to fallback to using other transport helper functions to
+	 * complete their request.
+	 */
+	discover = discover_refs(service_name, 0);
+	if (discover->version != protocol_v2) {
+		printf("fallback\n");
+		fflush(stdout);
+		return -1;
+	} else {
+		/* Half-Duplex Connection established */
+		printf("\n");
+		fflush(stdout);
+	}
+
+	proxy_state_init(&p, service_name);
+
+	/*
+	 * Dump the capability listing that we got from the server earlier
+	 * during the info/refs request.
+	 */
+	write_or_die(p.out, discover->buf, discover->len);
+
+	/* Peek the next packet line.  Until we see EOF keep sending POSTs */
+	while (packet_reader_peek(&p.reader) != PACKET_READ_EOF) {
+		if (proxy_post(&p)) {
+			/* We would have an err here */

Probably better to comment "Error message already printed by
proxy_post".

-- 
Brandon Williams