[RFC PATCH] http-backend: write error packet if backend command fails

[Denton Liu <liu.denton@gmail.com>]

If one tries to fetch packs with an incorrectly formatted parameter
while using the smart HTTP protocol v2, the client will block forever.
This is seen with the following command which does not terminate:

	$ git -c protocol.version=2 clone https://github.com/git/git.git --shallow-since=20151012
	Cloning into 'git'...

Meanwhile, if one uses v1, the command will terminate as expected:

	$ git -c protocol.version=1 clone https://github.com/git/git.git --shallow-since=20151012
	Cloning into 'git'...
	fatal: the remote end hung up unexpectedly

This happens because when upload-pack detects invalid parameters, it
will die(). When http-backend calls finish_command() and detects a
non-zero exit code, it will simply call exit(1). Since there is no way
in a CGI script to force a connection to terminate, the client keeps
blocking on the read(), expecting more output.

Write an error packet if the backend command fails to start or finishes
with an error so that the client can terminate the connection.

Note that if the included test case is run without the remainder of the
patch, it will run forever and fail to terminate.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---

Notes:
    So this is the first time I've touched the networking code of Git. There
    are a few problems with this patch that I'd appreciate some help
    addressing.
    
    First of all, is this even the right approach? I tried to find some
    other way to force a CGI script to terminate a connection but I don't
    think that it's possible so I had to get the client to do it instead.
    
    Next, with this patch applied it seems like t5539-fetch-http-shallow
    fails on 'no shallow lines after receiving ACK ready'. I've been trying
    to dig into why this happens but I can't quite figure it out.
    
    Finally, I'd like the test case I added in t5702 to be guarded by some
    timeout in case we regress instead of blocking the test suite forever
    but I'm not sure if that's even available functionality. It seems like
    we don't use the `timeout` program yet so I'm not sure if it's a good
    idea to introduce it here.
    
    Thanks,
    
    Denton

 http-backend.c         | 10 +++++++---
 t/t5702-protocol-v2.sh | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index ec3144b444..7ea3a688fd 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -488,10 +488,11 @@ static void run_service(const char **argv, int buffer_input)
 	cld.git_cmd = 1;
 	cld.clean_on_exit = 1;
 	cld.wait_after_clean = 1;
-	if (start_command(&cld))
+	if (start_command(&cld)) {
+		packet_write_fmt(1, "ERR %s failed to start\n", argv[0]);
 		exit(1);
+	}
 
-	close(1);
 	if (gzipped_request)
 		inflate_request(argv[0], cld.in, buffer_input, req_len);
 	else if (buffer_input)
@@ -501,8 +502,11 @@ static void run_service(const char **argv, int buffer_input)
 	else
 		close(0);
 
-	if (finish_command(&cld))
+	if (finish_command(&cld)) {
+		packet_write_fmt(1, "ERR %s failed\n", argv[0]);
 		exit(1);
+	}
+	close(1);
 }
 
 static int show_text_ref(const char *name, const struct object_id *oid,
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5039e66dc4..69a920a34b 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -586,6 +586,23 @@ test_expect_success 'clone with http:// using protocol v2' '
 	! grep "Send header: Transfer-Encoding: chunked" log
 '
 
+test_expect_success 'clone with http:// using protocol v2 and invalid parameters' '
+	test_when_finished "rm -f log" &&
+
+	test_must_fail env GIT_TRACE_PACKET="$(pwd)/log" GIT_TRACE_CURL="$(pwd)/log" \
+		git -c protocol.version=2 \
+		clone --shallow-since=20151012 "$HTTPD_URL/smart/http_parent" http_child_invalid &&
+
+	# Client requested to use protocol v2
+	grep "Git-Protocol: version=2" log &&
+	# Server responded using protocol v2
+	grep "git< version 2" log &&
+	# Verify that we sucessfully errored out
+	grep -F "ERR upload-pack failed" log &&
+	# Verify that the chunked encoding sending codepath is NOT exercised
+	! grep "Send header: Transfer-Encoding: chunked" log
+'
+
 test_expect_success 'clone big repository with http:// using protocol v2' '
 	test_when_finished "rm -f log" &&
 
-- 
2.26.0.159.g23e2136ad0