[PATCH v2 0/5] Protocol v2 fix: http and auth

[Jonathan Tan <jonathantanmy@google.com>]

Thanks to Junio and Peff for their reviews.

The main changes are the name and comments around the stop_reading part.
I have renamed it to flush_read_but_not_sent and have added a lot of
comments - I tried to err on the side of overcommenting here, since it
is rather complicated.

Other changes:
 - used test_seq instead of seq
 - used fast_import instead of test_commit 1500 times
 - in patch 4, the BUG statement now reads "...larger than
   LARGE_PACKET_DATA_MAX; the corresponding BUG in patch 5 is
   LARGE_PACKET_MAX (this means that the total diff does not change, and
   is thus not visible in the interdiff below)

I discovered a slight issue in which the full http.postBuffer is not
used, because Git immediately switches to chunked mode if the buffer
cannot contain a maximally sized pkt-line (without first reading the
pkt-line to see if it fits). This means that I could replace "test-seq 1
1500" with "test-seq 1 2" and the test would still pass, but I'm still
using 1500 in this patch set so that the test will pass if we ever
decide to use the http.postBuffer slightly more efficiently.

Jonathan Tan (5):
  remote-curl: reduce scope of rpc_state.argv
  remote-curl: reduce scope of rpc_state.stdin_preamble
  remote-curl: reduce scope of rpc_state.result
  remote-curl: refactor reading into rpc_state's buf
  remote-curl: use post_rpc() for protocol v2 also

 pkt-line.c             |   2 +-
 pkt-line.h             |   1 +
 remote-curl.c          | 362 +++++++++++++++++++----------------------
 t/t5702-protocol-v2.sh |  33 +++-
 4 files changed, 199 insertions(+), 199 deletions(-)

Interdiff against v1:
diff --git a/remote-curl.c b/remote-curl.c
index 2394a00650..8c03c78fc6 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -526,10 +526,13 @@ struct rpc_state {
 	unsigned write_line_lengths : 1;
 
 	/*
-	 * rpc_out uses this to keep track of whether it should continue
-	 * reading to populate the current request. Initialize to 0.
+	 * Used by rpc_out; initialize to 0. This is true if a flush has been
+	 * read, but the corresponding line length (if write_line_lengths is
+	 * true) and EOF have not been sent to libcurl. Since each flush marks
+	 * the end of a request, each flush must be completely sent before any
+	 * further reading occurs.
 	 */
-	unsigned stop_reading : 1;
+	unsigned flush_read_but_not_sent : 1;
 };
 
 /*
@@ -600,26 +603,34 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 		rpc->initial_buffer = 0;
 		rpc->len = 0;
 		rpc->pos = 0;
-		if (!rpc->stop_reading) {
+		if (!rpc->flush_read_but_not_sent) {
 			if (!rpc_read_from_out(rpc, 0, &avail, &status))
 				BUG("The entire rpc->buf should be larger than LARGE_PACKET_MAX");
 			if (status == PACKET_READ_FLUSH)
-				/*
-				 * We are done reading for this request, but we
-				 * still need to send this line out (if
-				 * rpc->write_line_lengths is true) so do not
-				 * return yet.
-				 */
-				rpc->stop_reading = 1;
+				rpc->flush_read_but_not_sent = 1;
 		}
+		/*
+		 * If flush_read_but_not_sent is true, we have already read one
+		 * full request but have not fully sent it + EOF, which is why
+		 * we need to refrain from reading.
+		 */
 	}
-	if (!avail && rpc->stop_reading) {
+	if (rpc->flush_read_but_not_sent) {
+		if (!avail) {
+			/*
+			 * The line length either does not need to be sent at
+			 * all or has already been completely sent. Now we can
+			 * return 0, indicating EOF, meaning that the flush has
+			 * been fully sent.
+			 */
+			rpc->flush_read_but_not_sent = 0;
+			return 0;
+		}
 		/*
-		 * "return 0" will notify Curl that this RPC request is done,
-		 * so reset stop_reading back to 0 for the next request.
+		 * If avail is non-zerp, the line length for the flush still
+		 * hasn't been fully sent. Proceed with sending the line
+		 * length.
 		 */
-		rpc->stop_reading = 0;
-		return 0;
 	}
 
 	if (max < avail)
@@ -1290,7 +1301,7 @@ static int stateless_connect(const char *service_name)
 	rpc.gzip_request = 1;
 	rpc.initial_buffer = 0;
 	rpc.write_line_lengths = 1;
-	rpc.stop_reading = 0;
+	rpc.flush_read_but_not_sent = 0;
 
 	/*
 	 * Dump the capability listing that we got from the server earlier
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 61acf99d80..e112b6086c 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -552,10 +552,17 @@ test_expect_success 'clone big repository with http:// using protocol v2' '
 
 	git init "$HTTPD_DOCUMENT_ROOT_PATH/big" &&
 	# Ensure that the list of wants is greater than http.postbuffer below
-	for i in $(seq 1 1500)
+	for i in $(test_seq 1 1500)
 	do
-		test_commit -C "$HTTPD_DOCUMENT_ROOT_PATH/big" "commit$i"
-	done &&
+		# do not use here-doc, because it requires a process
+		# per loop iteration
+		echo "commit refs/heads/too-many-refs-$i" &&
+		echo "committer git <git@example.com> $i +0000" &&
+		echo "data 0" &&
+		echo "M 644 inline bla.txt" &&
+		echo "data 4" &&
+		echo "bla"
+	done | git -C "$HTTPD_DOCUMENT_ROOT_PATH/big" fast-import &&
 
 	GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" git \
 		-c protocol.version=2 -c http.postbuffer=65536 \
-- 
2.19.0.271.gfe8321ec05.dirty