[PATCH] http: display the response body on POST failure

["Boris Mbarga via GitGitGadget" <gitgitgadget@gmail.com>]

From: elhmn <elhmn@github.com>

When Git sends a GET request and receives an HTTP code indicating
failure (and that failure does not indicate an authentication problem),
it shows the body of the response, i.e. the error message.
The same is not true for POST requests. However, it would be good to show
those error messages e.g. in the case of "429 Too many requests", because
the user might otherwise be left puzzled about the reason why their clone
did not work.

This patch aligns the way POST requests are handled with the GET request
handling.

Signed-off-by: elhmn <elhmn@github.com>
---
    http: display response body on POST failure

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1722%2Felhmn%2Fdisplay-rpc-post-err-message-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1722/elhmn/display-rpc-post-err-message-v1
Pull-Request: https://github.com/git/git/pull/1722

 http.c                               |  1 +
 http.h                               |  1 +
 remote-curl.c                        | 11 ++++++++++-
 t/lib-httpd.sh                       |  1 +
 t/lib-httpd/apache.conf              |  8 ++++++++
 t/lib-httpd/wrap-git-http-backend.sh |  7 +++++++
 t/t5551-http-fetch-smart.sh          |  5 +++++
 7 files changed, 33 insertions(+), 1 deletion(-)
 create mode 100755 t/lib-httpd/wrap-git-http-backend.sh

diff --git a/http.c b/http.c
index 3d80bd6116e..2017e909054 100644
--- a/http.c
+++ b/http.c
@@ -1419,6 +1419,7 @@ struct active_request_slot *get_active_slot(void)
 		newslot->curl = NULL;
 		newslot->in_use = 0;
 		newslot->next = NULL;
+		newslot->errstr = NULL;
 
 		slot = active_queue_head;
 		if (!slot) {
diff --git a/http.h b/http.h
index 3af19a8bf53..cb542c62933 100644
--- a/http.h
+++ b/http.h
@@ -30,6 +30,7 @@ struct active_request_slot {
 	void *callback_data;
 	void (*callback_func)(void *data);
 	struct active_request_slot *next;
+	struct strbuf *errstr;
 };
 
 struct buffer {
diff --git a/remote-curl.c b/remote-curl.c
index 0b6d7815fdd..9b2a41b2451 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -804,8 +804,11 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
 			      &response_code) != CURLE_OK)
 		return size;
-	if (response_code >= 300)
+	if (response_code >= 300) {
+		strbuf_reset(data->slot->errstr);
+		strbuf_add(data->slot->errstr, ptr, size);
 		return size;
+	}
 	if (size)
 		data->rpc->any_written = 1;
 	if (data->check_pktline)
@@ -837,6 +840,8 @@ static int run_slot(struct active_request_slot *slot,
 				strbuf_addch(&msg, ' ');
 				strbuf_addstr(&msg, curl_errorstr);
 			}
+			if (slot->errstr && slot->errstr->len)
+				error(_("%s"), slot->errstr->buf);
 		}
 		error(_("RPC failed; %s"), msg.buf);
 		strbuf_release(&msg);
@@ -896,6 +901,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 	int err, large_request = 0;
 	int needs_100_continue = 0;
 	struct rpc_in_data rpc_in_data;
+	struct strbuf errstr = STRBUF_INIT;
 
 	/* Try to load the entire request, if we can fit it into the
 	 * allocated buffer space we can use HTTP/1.0 and avoid the
@@ -1030,6 +1036,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
+	slot->errstr = &errstr;
 	rpc_in_data.rpc = rpc;
 	rpc_in_data.slot = slot;
 	rpc_in_data.check_pktline = stateless_connect;
@@ -1040,6 +1047,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 
 	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
+	slot->errstr = NULL;
 	if (err == HTTP_REAUTH && !large_request) {
 		credential_fill(&http_auth);
 		goto retry;
@@ -1060,6 +1068,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 
 	curl_slist_free_all(headers);
 	free(gzip_body);
+	strbuf_release(&errstr);
 	return err;
 }
 
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index d83bafeab32..8f5b6357176 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -164,6 +164,7 @@ prepare_httpd() {
 	install_script error-no-report.sh
 	install_script broken-smart-http.sh
 	install_script error-smart-http.sh
+	install_script wrap-git-http-backend.sh
 	install_script error.sh
 	install_script apply-one-time-perl.sh
 	install_script nph-custom-auth.sh
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 022276a6b9a..0ca58f54d72 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -108,6 +108,10 @@ SetEnv PERL_PATH ${PERL_PATH}
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
+<LocationMatch /smart_fail_on_post_with_body/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+</LocationMatch>
 <LocationMatch /smart_noexport/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 </LocationMatch>
@@ -155,6 +159,7 @@ ScriptAlias /smart/incomplete_length/git-upload-pack incomplete-length-upload-pa
 ScriptAlias /smart/incomplete_body/git-upload-pack incomplete-body-upload-pack-v2-http.sh/
 ScriptAlias /smart/no_report/git-receive-pack error-no-report.sh/
 ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
+ScriptAliasMatch /smart_fail_on_post_with_body/(.*) wrap-git-http-backend.sh/$1
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error_smart/ error-smart-http.sh/
@@ -182,6 +187,9 @@ ScriptAliasMatch /custom_auth/(.*) nph-custom-auth.sh/$1
 <Files error.sh>
   Options ExecCGI
 </Files>
+<Files wrap-git-http-backend.sh>
+  Options ExecCGI
+</Files>
 <Files apply-one-time-perl.sh>
 	Options ExecCGI
 </Files>
diff --git a/t/lib-httpd/wrap-git-http-backend.sh b/t/lib-httpd/wrap-git-http-backend.sh
new file mode 100755
index 00000000000..d1fc540330e
--- /dev/null
+++ b/t/lib-httpd/wrap-git-http-backend.sh
@@ -0,0 +1,7 @@
+if [ "$REQUEST_METHOD" = "GET" ]; then
+	"$GIT_EXEC_PATH"/git-http-backend "$@"
+elif [ "$REQUEST_METHOD" = "POST" ]; then
+	printf "Status: 429 Too Many Requests\n"
+	echo
+	printf "Too many requests"
+fi
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index a623a1058cd..badf37b4d68 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -302,6 +302,11 @@ test_expect_success 'dumb clone via http-backend respects namespace' '
 	test_cmp expect actual
 '
 
+test_expect_success 'smart http should display an error message on POST request failure' '
+	test_must_fail git clone --single-branch $HTTPD_URL/smart_fail_on_post_with_body/repo.git 2>actual &&
+	grep -q "error: Too many requests" actual
+'
+
 test_expect_success 'cookies stored in http.cookiefile when http.savecookies set' '
 	cat >cookies.txt <<-\EOF &&
 	127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue

base-commit: 786a3e4b8d754d2b14b1208b98eeb0a554ef19a8
-- 
gitgitgadget