[PATCH 1/2] Change how HTTP response body is returned

[PATCH 1/2] Change how HTTP response body is returned

[Masaya Suzuki]

This changes the way HTTP response body is returned in
http_request_reauth and post_rpc.

1. http_request_reauth makes up to two requests; one without a
   credential and one with a credential. The first request can fail if
   it needs a credential. When the keep_error option is specified, the
   response to the first request can be written to the HTTP response
   body destination. If the response body destination is a string
   buffer, it erases the buffer before making the second request. By
   introducing http_response_dest, it can handle the case that the
   destination is a file handle.
2. post_rpc makes an HTTP request and the response body is directly
   written to a file descriptor. This makes it check the HTTP status
   code before writing it, and do not write the response body if it's an
   error response. It's ok without this check now because post_rpc makes
   a request with CURLOPT_FAILONERROR, and libcurl won't call the
   callback if the response has an error status code.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c        | 99 +++++++++++++++++++++++++++++----------------------
 remote-curl.c | 29 ++++++++++++---
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/http.c b/http.c
index eacc2a75e..d23417670 100644
--- a/http.c
+++ b/http.c
@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
  */
 static int http_schannel_use_ssl_cainfo;
 
+/*
+ * Where to store the result of http_request.
+ *
+ * At most one of buffer or file can be non-NULL. The buffer and file are not
+ * allocated by http_request, and the caller is responsible for releasing them.
+ */
+struct http_response_dest {
+	struct strbuf *buffer;
+
+	FILE *file;
+	const char *filename;
+};
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
 	curl_easy_setopt(curl, CURLOPT_RANGE, buf);
 }
 
-/* http_request() targets */
-#define HTTP_REQUEST_STRBUF	0
-#define HTTP_REQUEST_FILE	1
-
 static int http_request(const char *url,
-			void *result, int target,
+			struct http_response_dest *dest,
 			const struct http_get_options *options)
 {
 	struct active_request_slot *slot;
@@ -1812,21 +1821,23 @@ static int http_request(const char *url,
 	slot = get_active_slot();
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 
-	if (result == NULL) {
-		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
-	} else {
+	if (dest->file) {
+		off_t posn = ftello(dest->file);
 		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
-		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
-
-		if (target == HTTP_REQUEST_FILE) {
-			off_t posn = ftello(result);
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite);
-			if (posn > 0)
-				http_opt_request_remainder(slot->curl, posn);
-		} else
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite_buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->file);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite);
+		if (posn > 0)
+			http_opt_request_remainder(slot->curl, posn);
+	} else if (dest->buffer) {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite_buffer);
+	} else {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
 	}
 
 	accept_language = get_accept_language();
@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
 }
 
 static int http_request_reauth(const char *url,
-			       void *result, int target,
+			       struct http_response_dest *dest,
 			       struct http_get_options *options)
 {
-	int ret = http_request(url, result, target, options);
+	int ret = http_request(url, dest, options);
 
 	if (ret != HTTP_OK && ret != HTTP_REAUTH)
 		return ret;
@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
 	if (ret != HTTP_REAUTH)
 		return ret;
 
-	/*
-	 * If we are using KEEP_ERROR, the previous request may have
-	 * put cruft into our output stream; we should clear it out before
-	 * making our next request. We only know how to do this for
-	 * the strbuf case, but that is enough to satisfy current callers.
-	 */
-	if (options && options->keep_error) {
-		switch (target) {
-		case HTTP_REQUEST_STRBUF:
-			strbuf_reset(result);
-			break;
-		default:
-			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+	if (dest->file) {
+		/*
+		 * At this point, the file contains the response body of the
+		 * previous request. We need to truncate the file.
+		 */
+		FILE *new_file = freopen(dest->filename, "w", dest->file);
+		if (new_file == NULL) {
+			error("Unable to open local file %s", dest->filename);
+			return HTTP_ERROR;
 		}
+		dest->file = new_file;
+	} else if (dest->buffer) {
+		strbuf_reset(dest->buffer);
 	}
 
 	credential_fill(&http_auth);
 
-	return http_request(url, result, target, options);
+	return http_request(url, dest, options);
 }
 
 int http_get_strbuf(const char *url,
-		    struct strbuf *result,
+		    struct strbuf *dest_buffer,
 		    struct http_get_options *options)
 {
-	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
+	struct http_response_dest dest;
+	dest.file = NULL;
+	dest.buffer = dest_buffer;
+	return http_request_reauth(url, &dest, options);
 }
 
 /*
@@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,
 {
 	int ret;
 	struct strbuf tmpfile = STRBUF_INIT;
-	FILE *result;
+	struct http_response_dest dest;
 
 	strbuf_addf(&tmpfile, "%s.temp", filename);
-	result = fopen(tmpfile.buf, "a");
-	if (!result) {
+	dest.buffer = NULL;
+	dest.file = fopen(tmpfile.buf, "a");
+	if (!dest.file) {
 		error("Unable to open local file %s", tmpfile.buf);
 		ret = HTTP_ERROR;
 		goto cleanup;
 	}
+	dest.filename = tmpfile.buf;
 
-	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
-	fclose(result);
+	ret = http_request_reauth(url, &dest, options);
+	fclose(dest.file);
 
 	if (ret == HTTP_OK && finalize_object_file(tmpfile.buf, filename))
 		ret = HTTP_ERROR;
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcd..48656bf18 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+struct rpc_in_data {
+	struct rpc_state *rpc;
+	struct active_request_slot *slot;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
-	struct rpc_state *rpc = buffer_;
+	struct rpc_in_data *data = buffer_;
+	long response_code;
+
+	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+			      &response_code) != CURLE_OK)
+		return size;
+	if (response_code != 200)
+		return size;
+
 	if (size)
-		rpc->any_written = 1;
-	write_or_die(rpc->in, ptr, size);
+		data->rpc->any_written = 1;
+	write_or_die(data->rpc->in, ptr, size);
 	return size;
 }
 
@@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)
 	size_t gzip_size = 0;
 	int err, large_request = 0;
 	int needs_100_continue = 0;
+	struct rpc_in_data rpc_in_data;
 
 	/* 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
@@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
-
+	rpc_in_data.rpc = rpc;
+	rpc_in_data.slot = slot;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
 
 	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
-- 
2.20.1.415.g653613c723-goog

[PATCH 2/2] Unset CURLOPT_FAILONERROR

[Masaya Suzuki]

When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
to stderr. However, if the response is an error response and
CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
won't ump the headers. Showing HTTP response headers is useful for
debugging, especially for non-OK responses.

This is substantially same as setting http_options.keep_error to all
requests. Hence, removing this option.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c                       |  4 ----
 http.h                       |  1 -
 remote-curl.c                |  1 -
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 32 ++++++++++++++++++++++++++++++++
 5 files changed, 33 insertions(+), 6 deletions(-)
 create mode 100755 t/t5581-http-curl-verbose.sh

diff --git a/http.c b/http.c
index d23417670..8f8101da3 100644
--- a/http.c
+++ b/http.c
@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
-	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
 
 	/*
@@ -1848,8 +1847,6 @@ static int http_request(const char *url,
 	strbuf_addstr(&buf, "Pragma:");
 	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options && options->keep_error)
-		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 	if (options && options->initial_request &&
 	    http_follow_config == HTTP_FOLLOW_INITIAL)
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	freq->slot = get_active_slot();
 
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
-	curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
diff --git a/http.h b/http.h
index d305ca1dc..eebf40688 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 /* Options for http_get_*() */
 struct http_get_options {
 	unsigned no_cache:1,
-		 keep_error:1,
 		 initial_request:1;
 
 	/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 48656bf18..43e7a1d80 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
-	http_options.keep_error = 1;
 
 	http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
 	switch (http_ret) {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8..cc4b87507 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 000000000..c89e06e12
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+	git config push.default matching &&
+	echo content >file &&
+	git add file &&
+	git commit -m one
+'
+
+test_expect_success 'create http-accessible bare repository' '
+	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	(cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	 git --bare init
+	) &&
+	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+	(GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
+	 true) &&
+	cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
+'
+
+stop_httpd
+
+test_done
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR

[Eric Sunshine]

On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> to stderr. However, if the response is an error response and
> CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> won't ump the headers. Showing HTTP response headers is useful for

s/ump/dump/

> debugging, especially for non-OK responses.
>
> This is substantially same as setting http_options.keep_error to all
> requests. Hence, removing this option.

s/removing/remove/

> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
> diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
> @@ -0,0 +1,32 @@
> +test_expect_success 'setup repository' '
> +       ...
> +'
> +
> +test_expect_success 'create http-accessible bare repository' '

Not a big deal, but this seems like more setup, so it could be folded
into the "setup" test above it.

> +       mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +       (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +        git --bare init
> +       ) &&

Since this is a new test script, it makes sense to format the subshell
in the modern style:

    (
        cd ... &&
        git ...
    ) &&

Alternately, use -C and drop the subshell altogether:

    git -C $BLAH/repo.git --bare init &&

> +       git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> +       git push public master:master
> +'
> +
> +test_expect_success 'failure in git-upload-pack is shown' '
> +       (GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
> +        true) &&

Using test_might_fail() would allow you to drop the subshell and the "|| true":

    test_might_fail env GIT_CURL_VERBOSE=1  git clone ... &&

> +       cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
> +'

Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR

[Masaya Suzuki]

On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> > to stderr. However, if the response is an error response and
> > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> > won't ump the headers. Showing HTTP response headers is useful for
>
> s/ump/dump/
>
> > debugging, especially for non-OK responses.
> >
> > This is substantially same as setting http_options.keep_error to all
> > requests. Hence, removing this option.
>
> s/removing/remove/
>
> > Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> > ---
> > diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
> > @@ -0,0 +1,32 @@
> > +test_expect_success 'setup repository' '
> > +       ...
> > +'
> > +
> > +test_expect_success 'create http-accessible bare repository' '
>
> Not a big deal, but this seems like more setup, so it could be folded
> into the "setup" test above it.
>
> > +       mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +       (cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +        git --bare init
> > +       ) &&
>
> Since this is a new test script, it makes sense to format the subshell
> in the modern style:
>
>     (
>         cd ... &&
>         git ...
>     ) &&
>
> Alternately, use -C and drop the subshell altogether:
>
>     git -C $BLAH/repo.git --bare init &&
>
> > +       git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> > +       git push public master:master
> > +'
> > +
> > +test_expect_success 'failure in git-upload-pack is shown' '
> > +       (GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
> > +        true) &&
>
> Using test_might_fail() would allow you to drop the subshell and the "|| true":
>
>     test_might_fail env GIT_CURL_VERBOSE=1  git clone ... &&
>
> > +       cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
> > +'

The test should success. This is a test that a log is produced after a
git command fails. The point of this test is "cat curl_log | grep ..."
part that asserts the log.

Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR

[Eric Sunshine]

On Fri, Dec 28, 2018 at 2:51 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> > > +test_expect_success 'failure in git-upload-pack is shown' '
> > > +       (GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
> > > +        true) &&
> >
> > Using test_might_fail() would allow you to drop the subshell and the "|| true":
> >
> >     test_might_fail env GIT_CURL_VERBOSE=1  git clone ... &&
> >
> > > +       cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
> > > +'
>
> The test should success. This is a test that a log is produced after a
> git command fails. The point of this test is "cat curl_log | grep ..."
> part that asserts the log.

Unfortunately, the name "test_might_fail" is confusing. It is not
saying that the entire test might or might not fail. Rather, it is
saying that the one command might or might not fail (and that you
don't care if it does fail). The idiom:

    (some-git-command || true) &&

can be replaced with:

   test_might_fail some-git-command &&

without changing its meaning, and without affecting the
success/failure status of the test overall.

So, this new test could be written like this:

--- 8< ---
test_expect_success 'failure in git-upload-pack is shown' '
   test_might_fail env GIT_CURL_VERBOSE=1 git clone --bare
"$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log &&
   cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
'
--- 8< ---

and have the same meaning.

Re: [PATCH 2/2] Unset CURLOPT_FAILONERROR

[Masaya Suzuki]

On Fri, Dec 28, 2018 at 11:58 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Fri, Dec 28, 2018 at 2:51 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> > On Fri, Dec 28, 2018 at 11:37 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > On Thu, Dec 27, 2018 at 8:47 PM Masaya Suzuki <masayasuzuki@google.com> wrote:
> > > > +test_expect_success 'failure in git-upload-pack is shown' '
> > > > +       (GIT_CURL_VERBOSE=1 git clone --bare "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log ||
> > > > +        true) &&
> > >
> > > Using test_might_fail() would allow you to drop the subshell and the "|| true":
> > >
> > >     test_might_fail env GIT_CURL_VERBOSE=1  git clone ... &&
> > >
> > > > +       cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
> > > > +'
> >
> > The test should success. This is a test that a log is produced after a
> > git command fails. The point of this test is "cat curl_log | grep ..."
> > part that asserts the log.
>
> Unfortunately, the name "test_might_fail" is confusing. It is not
> saying that the entire test might or might not fail. Rather, it is
> saying that the one command might or might not fail (and that you
> don't care if it does fail). The idiom:
>
>     (some-git-command || true) &&
>
> can be replaced with:
>
>    test_might_fail some-git-command &&
>
> without changing its meaning, and without affecting the
> success/failure status of the test overall.
>
> So, this new test could be written like this:
>
> --- 8< ---
> test_expect_success 'failure in git-upload-pack is shown' '
>    test_might_fail env GIT_CURL_VERBOSE=1 git clone --bare
> "$HTTPD_URL/error_git_upload_pack/smart/repo.git" 2>curl_log &&
>    cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
> '
> --- 8< ---
>
> and have the same meaning.

Ah. I see. It's used inside the test. Thanks.

[PATCH v2 0/2] Show HTTP headers of failed requests with GIT_CURL_VERBOSE

[Masaya Suzuki]

When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
to stderr. However, if the response is an error response and
CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
won't dump the headers. Showing HTTP response headers is useful for
debugging, especially for non-OK responses.

To this end, the caller of libcurl needs to handle HTTP request failures by
themselves. The first patch makes git prepared to handle those failures. The
second patch actually unsets CURLOPT_FAILONERROR.

Masaya Suzuki (2):
  Change how HTTP response body is returned
  Unset CURLOPT_FAILONERROR

 http.c                       | 103 +++++++++++++++++++----------------
 http.h                       |   1 -
 remote-curl.c                |  30 ++++++++--
 t/lib-httpd/apache.conf      |   1 +
 t/t5581-http-curl-verbose.sh |  28 ++++++++++
 5 files changed, 110 insertions(+), 53 deletions(-)
 create mode 100755 t/t5581-http-curl-verbose.sh

-- 
2.20.1.415.g653613c723-goog

[PATCH v2 1/2] Change how HTTP response body is returned

[Masaya Suzuki]

This changes the way HTTP response body is returned in
http_request_reauth and post_rpc.

1. http_request_reauth makes up to two requests; one without a
   credential and one with a credential. The first request can fail if
   it needs a credential. When the keep_error option is specified, the
   response to the first request can be written to the HTTP response
   body destination. If the response body destination is a string
   buffer, it erases the buffer before making the second request. By
   introducing http_response_dest, it can handle the case that the
   destination is a file handle.
2. post_rpc makes an HTTP request and the response body is directly
   written to a file descriptor. This makes it check the HTTP status
   code before writing it, and do not write the response body if it's an
   error response. It's ok without this check now because post_rpc makes
   a request with CURLOPT_FAILONERROR, and libcurl won't call the
   callback if the response has an error status code.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c        | 99 +++++++++++++++++++++++++++++----------------------
 remote-curl.c | 29 ++++++++++++---
 2 files changed, 81 insertions(+), 47 deletions(-)

diff --git a/http.c b/http.c
index eacc2a75e..d23417670 100644
--- a/http.c
+++ b/http.c
@@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
  */
 static int http_schannel_use_ssl_cainfo;
 
+/*
+ * Where to store the result of http_request.
+ *
+ * At most one of buffer or file can be non-NULL. The buffer and file are not
+ * allocated by http_request, and the caller is responsible for releasing them.
+ */
+struct http_response_dest {
+	struct strbuf *buffer;
+
+	FILE *file;
+	const char *filename;
+};
+
 size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
@@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
 	curl_easy_setopt(curl, CURLOPT_RANGE, buf);
 }
 
-/* http_request() targets */
-#define HTTP_REQUEST_STRBUF	0
-#define HTTP_REQUEST_FILE	1
-
 static int http_request(const char *url,
-			void *result, int target,
+			struct http_response_dest *dest,
 			const struct http_get_options *options)
 {
 	struct active_request_slot *slot;
@@ -1812,21 +1821,23 @@ static int http_request(const char *url,
 	slot = get_active_slot();
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 
-	if (result == NULL) {
-		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
-	} else {
+	if (dest->file) {
+		off_t posn = ftello(dest->file);
 		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
-		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
-
-		if (target == HTTP_REQUEST_FILE) {
-			off_t posn = ftello(result);
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite);
-			if (posn > 0)
-				http_opt_request_remainder(slot->curl, posn);
-		} else
-			curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
-					 fwrite_buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->file);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite);
+		if (posn > 0)
+			http_opt_request_remainder(slot->curl, posn);
+	} else if (dest->buffer) {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
+		curl_easy_setopt(slot->curl, CURLOPT_FILE,
+				 dest->buffer);
+		curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION,
+				 fwrite_buffer);
+	} else {
+		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
 	}
 
 	accept_language = get_accept_language();
@@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
 }
 
 static int http_request_reauth(const char *url,
-			       void *result, int target,
+			       struct http_response_dest *dest,
 			       struct http_get_options *options)
 {
-	int ret = http_request(url, result, target, options);
+	int ret = http_request(url, dest, options);
 
 	if (ret != HTTP_OK && ret != HTTP_REAUTH)
 		return ret;
@@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
 	if (ret != HTTP_REAUTH)
 		return ret;
 
-	/*
-	 * If we are using KEEP_ERROR, the previous request may have
-	 * put cruft into our output stream; we should clear it out before
-	 * making our next request. We only know how to do this for
-	 * the strbuf case, but that is enough to satisfy current callers.
-	 */
-	if (options && options->keep_error) {
-		switch (target) {
-		case HTTP_REQUEST_STRBUF:
-			strbuf_reset(result);
-			break;
-		default:
-			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+	if (dest->file) {
+		/*
+		 * At this point, the file contains the response body of the
+		 * previous request. We need to truncate the file.
+		 */
+		FILE *new_file = freopen(dest->filename, "w", dest->file);
+		if (new_file == NULL) {
+			error("Unable to open local file %s", dest->filename);
+			return HTTP_ERROR;
 		}
+		dest->file = new_file;
+	} else if (dest->buffer) {
+		strbuf_reset(dest->buffer);
 	}
 
 	credential_fill(&http_auth);
 
-	return http_request(url, result, target, options);
+	return http_request(url, dest, options);
 }
 
 int http_get_strbuf(const char *url,
-		    struct strbuf *result,
+		    struct strbuf *dest_buffer,
 		    struct http_get_options *options)
 {
-	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
+	struct http_response_dest dest;
+	dest.file = NULL;
+	dest.buffer = dest_buffer;
+	return http_request_reauth(url, &dest, options);
 }
 
 /*
@@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,
 {
 	int ret;
 	struct strbuf tmpfile = STRBUF_INIT;
-	FILE *result;
+	struct http_response_dest dest;
 
 	strbuf_addf(&tmpfile, "%s.temp", filename);
-	result = fopen(tmpfile.buf, "a");
-	if (!result) {
+	dest.buffer = NULL;
+	dest.file = fopen(tmpfile.buf, "a");
+	if (!dest.file) {
 		error("Unable to open local file %s", tmpfile.buf);
 		ret = HTTP_ERROR;
 		goto cleanup;
 	}
+	dest.filename = tmpfile.buf;
 
-	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
-	fclose(result);
+	ret = http_request_reauth(url, &dest, options);
+	fclose(dest.file);
 
 	if (ret == HTTP_OK && finalize_object_file(tmpfile.buf, filename))
 		ret = HTTP_ERROR;
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcd..48656bf18 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+struct rpc_in_data {
+	struct rpc_state *rpc;
+	struct active_request_slot *slot;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
-	struct rpc_state *rpc = buffer_;
+	struct rpc_in_data *data = buffer_;
+	long response_code;
+
+	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+			      &response_code) != CURLE_OK)
+		return size;
+	if (response_code != 200)
+		return size;
+
 	if (size)
-		rpc->any_written = 1;
-	write_or_die(rpc->in, ptr, size);
+		data->rpc->any_written = 1;
+	write_or_die(data->rpc->in, ptr, size);
 	return size;
 }
 
@@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)
 	size_t gzip_size = 0;
 	int err, large_request = 0;
 	int needs_100_continue = 0;
+	struct rpc_in_data rpc_in_data;
 
 	/* 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
@@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
-
+	rpc_in_data.rpc = rpc;
+	rpc_in_data.slot = slot;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
 
 	rpc->any_written = 0;
 	err = run_slot(slot, NULL);
-- 
2.20.1.415.g653613c723-goog

[PATCH v2 2/2] Unset CURLOPT_FAILONERROR

[Masaya Suzuki]

When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
to stderr. However, if the response is an error response and
CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
won't dump the headers. Showing HTTP response headers is useful for
debugging, especially for non-OK responses.

This is substantially same as setting http_options.keep_error to all
requests. Hence, remove this option.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c                       |  4 ----
 http.h                       |  1 -
 remote-curl.c                |  1 -
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
 5 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100755 t/t5581-http-curl-verbose.sh

diff --git a/http.c b/http.c
index d23417670..8f8101da3 100644
--- a/http.c
+++ b/http.c
@@ -1269,7 +1269,6 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_UPLOAD, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
-	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
 	curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
 
 	/*
@@ -1848,8 +1847,6 @@ static int http_request(const char *url,
 	strbuf_addstr(&buf, "Pragma:");
 	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options && options->keep_error)
-		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 	if (options && options->initial_request &&
 	    http_follow_config == HTTP_FOLLOW_INITIAL)
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -2415,7 +2412,6 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	freq->slot = get_active_slot();
 
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FILE, freq);
-	curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
diff --git a/http.h b/http.h
index d305ca1dc..eebf40688 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 /* Options for http_get_*() */
 struct http_get_options {
 	unsigned no_cache:1,
-		 keep_error:1,
 		 initial_request:1;
 
 	/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 48656bf18..43e7a1d80 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
-	http_options.keep_error = 1;
 
 	http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
 	switch (http_ret) {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8..cc4b87507 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 000000000..73148eeba
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+	git config push.default matching &&
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+	test_might_fail env GIT_CURL_VERBOSE=1 \
+		git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+		2>curl_log &&
+	cat curl_log | grep "< HTTP/1.1 500 Intentional Breakage"
+'
+
+stop_httpd
+
+test_done
-- 
2.20.1.415.g653613c723-goog

Re: [PATCH v2 1/2] Change how HTTP response body is returned

[Junio C Hamano]

Masaya Suzuki <masayasuzuki@google.com> writes:

> Subject: Re: [PATCH v2 1/2] Change how HTTP response body is returned

Perhaps:

    Subject: http: change the way response body is returned

but if we can state why we want to do so concisely, that would be
even better.

> This changes the way HTTP response body is returned in
> http_request_reauth and post_rpc.
>
> 1. http_request_reauth makes up to two requests; one without a
>    credential and one with a credential. The first request can fail if
>    it needs a credential. When the keep_error option is specified, the
>    response to the first request can be written to the HTTP response
>    body destination. If the response body destination is a string
>    buffer, it erases the buffer before making the second request. By
>    introducing http_response_dest, it can handle the case that the
>    destination is a file handle.
> 2. post_rpc makes an HTTP request and the response body is directly
>    written to a file descriptor. This makes it check the HTTP status
>    code before writing it, and do not write the response body if it's an
>    error response. It's ok without this check now because post_rpc makes
>    a request with CURLOPT_FAILONERROR, and libcurl won't call the
>    callback if the response has an error status code.

The above may be an accurate description of what the code will do
with this patch, but I cannot quite read the reason why we would
want the code to behave so in the first place.

>
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
>  http.c        | 99 +++++++++++++++++++++++++++++----------------------
>  remote-curl.c | 29 ++++++++++++---
>  2 files changed, 81 insertions(+), 47 deletions(-)
>
> diff --git a/http.c b/http.c
> index eacc2a75e..d23417670 100644
> --- a/http.c
> +++ b/http.c
> @@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
>   */
>  static int http_schannel_use_ssl_cainfo;
>  
> +/*
> + * Where to store the result of http_request.
> + *
> + * At most one of buffer or file can be non-NULL. The buffer and file are not
> + * allocated by http_request, and the caller is responsible for releasing them.
> + */
> +struct http_response_dest {
> +	struct strbuf *buffer;
> +
> +	FILE *file;
> +	const char *filename;
> +};
> +
>  size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> @@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
>  	curl_easy_setopt(curl, CURLOPT_RANGE, buf);
>  }
>  
> -/* http_request() targets */
> -#define HTTP_REQUEST_STRBUF	0
> -#define HTTP_REQUEST_FILE	1
> -
>  static int http_request(const char *url,
> -			void *result, int target,
> +			struct http_response_dest *dest,
>  			const struct http_get_options *options)
>  {
>  	struct active_request_slot *slot;
> @@ -1812,21 +1821,23 @@ static int http_request(const char *url,
>  	slot = get_active_slot();
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
>  
> -	if (result == NULL) {
> -		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> -	} else {
> +	if (dest->file) {
> +		off_t posn = ftello(dest->file);
>  		curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> -		curl_easy_setopt(slot->curl, CURLOPT_FILE, result);

OK, so it used to be that we can either discard the result
(i.e. NOBODY) or send the result to CURLOPT_FILE, and the latter has
two options (sent to a file, or sent to in-core buffer).  The way
these three choices are given were with the result pointer and the
target enum.

That is replaced by a struct that allows the same three choices.

Makes sense so far.

> @@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
>  }
>  
>  static int http_request_reauth(const char *url,
> -			       void *result, int target,
> +			       struct http_response_dest *dest,
>  			       struct http_get_options *options)
>  {
> -	int ret = http_request(url, result, target, options);
> +	int ret = http_request(url, dest, options);

Just adjusting the calling convention to the change we saw above.

> @@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
>  	if (ret != HTTP_REAUTH)
>  		return ret;
>  
> -	/*
> -	 * If we are using KEEP_ERROR, the previous request may have
> -	 * put cruft into our output stream; we should clear it out before
> -	 * making our next request. We only know how to do this for
> -	 * the strbuf case, but that is enough to satisfy current callers.
> -	 */
> -	if (options && options->keep_error) {
> -		switch (target) {
> -		case HTTP_REQUEST_STRBUF:
> -			strbuf_reset(result);
> -			break;
> -		default:
> -			BUG("HTTP_KEEP_ERROR is only supported with strbufs");

Now it gets interesting.  We used to allow keep-error only when
receiving to in-core buffer.

> +	if (dest->file) {
> +		/*
> +		 * At this point, the file contains the response body of the
> +		 * previous request. We need to truncate the file.
> +		 */
> +		FILE *new_file = freopen(dest->filename, "w", dest->file);

Now freopen() lets us restart the file anew with a new "FILE *".

> +		if (new_file == NULL) {
> +			error("Unable to open local file %s", dest->filename);

error_errno(), perhaps?

At this point, I presume that dest->file is closed by the failed
freopen(), but dest->file is still non-NULL and causes further calls
to http_request() with this dest would be a disaster?  As long as
the caller of this function reacts to HTTP_ERROR and kill the dest,
it would be fine.

> +			return HTTP_ERROR;
>  		}
> +		dest->file = new_file;
> +	} else if (dest->buffer) {
> +		strbuf_reset(dest->buffer);
>  	}

OK.

>  	credential_fill(&http_auth);
>  
> -	return http_request(url, result, target, options);
> +	return http_request(url, dest, options);
>  }

So far, I spotted one reason why this patch wants to exist: it used
to be that keep_error was impossible when sending to a file.  It
somehow wants to allow us to do so (even though it still is unclear
who that new caller that wants to use keep_error is, and for what
purpose it wants to use that facility).

Perhaps this step can be split into at least two steps?  The first
one is to turn <result, target> to <dest> without changing any other
behaviour, and then the second one implements keep_error handling
for the "dest->file != NULL" case.

There may be other things this patch does, in which case it may
deserve to become three or more steps, but we haven't seen enough to
decide if that is the case.  Let's read on.

>  int http_get_strbuf(const char *url,
> -		    struct strbuf *result,
> +		    struct strbuf *dest_buffer,
>  		    struct http_get_options *options)
>  {
> -	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
> +	struct http_response_dest dest;
> +	dest.file = NULL;
> +	dest.buffer = dest_buffer;
> +	return http_request_reauth(url, &dest, options);

This is merely adjusting for the updated calling convention, which
makes sense.

>  }
>  
>  /*
> @@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,
>  {
>  	int ret;
>  	struct strbuf tmpfile = STRBUF_INIT;
> -	FILE *result;
> +	struct http_response_dest dest;
>  
>  	strbuf_addf(&tmpfile, "%s.temp", filename);
> -	result = fopen(tmpfile.buf, "a");
> -	if (!result) {
> +	dest.buffer = NULL;
> +	dest.file = fopen(tmpfile.buf, "a");
> +	if (!dest.file) {
>  		error("Unable to open local file %s", tmpfile.buf);
>  		ret = HTTP_ERROR;
>  		goto cleanup;
>  	}
> +	dest.filename = tmpfile.buf;

Hmph.  I somehow expected that the presence of dest.filename field
would allow callers to just set it and let the fopen() call handled
in http_request(), e.g. at the beginning of the function, it would
do something like

	if (!dest->file && dest->filename)
		dest->file = fopen(...);

But obviously that is not within the scope of this change.  Leaving
the caller responsible for opening and reporting errors as before
like the above is preferrable.

> diff --git a/remote-curl.c b/remote-curl.c
> index 1220dffcd..48656bf18 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
>  }
>  #endif
>  
> +struct rpc_in_data {
> +	struct rpc_state *rpc;
> +	struct active_request_slot *slot;
> +};
> +
> +/*
> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
> + * from ptr.
> + */
>  static size_t rpc_in(char *ptr, size_t eltsize,
>  		size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> -	struct rpc_state *rpc = buffer_;
> +	struct rpc_in_data *data = buffer_;
> +	long response_code;
> +
> +	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
> +			      &response_code) != CURLE_OK)
> +		return size;
> +	if (response_code != 200)
> +		return size;
> +
>  	if (size)
> -		rpc->any_written = 1;
> -	write_or_die(rpc->in, ptr, size);
> +		data->rpc->any_written = 1;
> +	write_or_die(data->rpc->in, ptr, size);
>  	return size;
>  }

This is not necessarily related to the change we saw in http.c but
making it more careful in general?  That is, we avoid writing the
payload to the destination (by the way, rpc->IN being the target of
write(2) is somewhat a brain-twister).  It may deserve to become its
own step in a patch series, with separate justification (i.e. "when
rpc channel receives an error from the cURL library, we used to
write the data to the file anyway, and that caused such and such
problems, as demonstrated by the new test added by this patch.  we
fix it by checking for the error before writing to the file").

> @@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)
>  	size_t gzip_size = 0;
>  	int err, large_request = 0;
>  	int needs_100_continue = 0;
> +	struct rpc_in_data rpc_in_data;
>  
>  	/* 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
> @@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)
>  
>  	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> -	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
> -
> +	rpc_in_data.rpc = rpc;
> +	rpc_in_data.slot = slot;
> +	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
>  
>  	rpc->any_written = 0;
>  	err = run_slot(slot, NULL);

These two hunks look like mere adjustments for the new callback
type, which makes sense.

Thanks.

Re: [PATCH v2 1/2] Change how HTTP response body is returned

[Jeff King]

On Thu, Jan 03, 2019 at 11:09:02AM -0800, Junio C Hamano wrote:

> > +	if (dest->file) {
> > +		/*
> > +		 * At this point, the file contains the response body of the
> > +		 * previous request. We need to truncate the file.
> > +		 */
> > +		FILE *new_file = freopen(dest->filename, "w", dest->file);
> 
> Now freopen() lets us restart the file anew with a new "FILE *".
> 
> > +		if (new_file == NULL) {
> > +			error("Unable to open local file %s", dest->filename);
> 
> error_errno(), perhaps?
> 
> At this point, I presume that dest->file is closed by the failed
> freopen(), but dest->file is still non-NULL and causes further calls
> to http_request() with this dest would be a disaster?  As long as
> the caller of this function reacts to HTTP_ERROR and kill the dest,
> it would be fine.

I also wondered what timing guarantees freopen() gives us (i.e., is it
possible for it to open and truncate the file, and then close the old
handle, flushing some in-memory buffer). C99 says:

  The freopen function first attempts to close any file that is
  associated with the specified stream. Failure to close the file is
  ignored. The error and end-of-file indicators for the stream are
  cleared.

So I think the order is OK for my concern, but not for yours. I.e., on
an error, dest->file is now undefined.

It might be nice to set "dest->file == NULL" in that case. There's no
guarantee that the caller did not hold onto its own copy of the handle,
but since this struct is only exposed internally within http.c, that's
probably OK.

The most robust thing would perhaps be:

  fflush(dest->file);
  ftruncate(fileno(dest->file), 0);

which leaves the handle intact.

(I agree with the rest of your review, especially that it would be
easier to read if this were split into separate refactor and
change-behavior steps).

-Peff

Re: [PATCH v2 1/2] Change how HTTP response body is returned

[Jeff King]

On Sat, Dec 29, 2018 at 11:44:46AM -0800, Masaya Suzuki wrote:

> +/*
> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
> + * from ptr.
> + */
>  static size_t rpc_in(char *ptr, size_t eltsize,
>  		size_t nmemb, void *buffer_)
>  {
>  	size_t size = eltsize * nmemb;
> -	struct rpc_state *rpc = buffer_;
> +	struct rpc_in_data *data = buffer_;
> +	long response_code;
> +
> +	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
> +			      &response_code) != CURLE_OK)
> +		return size;

This hunk was unexpected to me. The function here is just writing out
the data, and I expected we'd handle the error after the whole transfer
is done. But we do that anyway eventually via run_slot() (which uses
handle_curl_result). I guess the goal here is to start throwing away
data when we see an error, rather than storing it?

That makes some sense, though I do wonder if there's any case where curl
would call our WRITEFUNCTION before it knows the HTTP status. That
implies a body before our header, which seems impossible, though.

> +	if (response_code != 200)
> +		return size;

The current behavior with CURLOPT_FAILONERROR treats codes >= 400 as an
error. And in handle_curl_result(), we treat >= 300 as an error (since
we only see 3xx for a disabled redirect). I suppose it's unlikely for us
to see any success code besides 200, but we probably ought to be
following the same rules here.

-Peff

Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR

[Jeff King]

On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:

> When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> to stderr. However, if the response is an error response and
> CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> won't dump the headers. Showing HTTP response headers is useful for
> debugging, especially for non-OK responses.

Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply
that curl closes the handle when it sees the bad response code, and
nobody ever gets to see the rest of the data?

> This is substantially same as setting http_options.keep_error to all
> requests. Hence, remove this option.

The assumption here is that every code path using FAILONERROR is
prepared to handle the failing http response codes itself (since we no
longer set it at all in get_active_slot()). Is that so?

Anything that uses handle_curl_result() is OK. That means run_one_slot()
is fine, which in turn covers run_slot() for RPCs, and http_request()
for normal one-at-a-time requests. But what about the parallel multiple
requests issued by the dumb-http walker code?

There I think we end up in step_active_slots(), which calls into
finish_active_slot() for completed requests. I think that
unconditionally fetches the http code without bothering to look at
whether curl reported success or not.

So I _think_ that's probably all of the users of the curl handles
provided by get_active_slot(). Though given the tangled mess of our HTTP
code, I won't be surprised if there's a corner case I missed in that
analysis.

-Peff

Re: [PATCH v2 1/2] Change how HTTP response body is returned

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The most robust thing would perhaps be:
>
>   fflush(dest->file);
>   ftruncate(fileno(dest->file), 0);
>
> which leaves the handle intact.

An added benefit of that approach is that there is no need for
the filename field in the dest structure.

Having a separate filename field could be a positive flexibility (it
could be used to open a file, store its FILE* in dest->file, while
storing the name of another file in dest->filename), but also a
negative flexibility (dest->file possibly pointing to a file
different from dest->filename is a source of confusion).  As I do
not think any current caller that wants such a flexibility, or
callers in any immediate future, it probably is an overall plus if
we do not have to add the dest->filename field.

> (I agree with the rest of your review, especially that it would be
> easier to read if this were split into separate refactor and
> change-behavior steps).
>
> -Peff

Re: [PATCH v2 2/2] Unset CURLOPT_FAILONERROR

[Masaya Suzuki]

On Fri, Jan 4, 2019 at 2:49 AM Jeff King <peff@peff.net> wrote:
>
> On Sat, Dec 29, 2018 at 11:44:47AM -0800, Masaya Suzuki wrote:
>
> > When GIT_CURL_VERBOSE is set, libcurl produces request/response headers
> > to stderr. However, if the response is an error response and
> > CURLOPT_FAILONERROR is set, libcurl stops parsing the response, and it
> > won't dump the headers. Showing HTTP response headers is useful for
> > debugging, especially for non-OK responses.
>
> Out of curiosity, does GIT_TRACE_CURL do any better? Or is it simply
> that curl closes the handle when it sees the bad response code, and
> nobody ever gets to see the rest of the data?

curl disregards the rest of the contents when it sees a bad response
code when CURLOPT_FAILONERROR is set
(https://github.com/curl/curl/blob/dea3f94298ac0859464768959488938c4e104545/lib/http.c#L3691).
So nobody gets the rest of the data.

>
> > This is substantially same as setting http_options.keep_error to all
> > requests. Hence, remove this option.
>
> The assumption here is that every code path using FAILONERROR is
> prepared to handle the failing http response codes itself (since we no
> longer set it at all in get_active_slot()). Is that so?

I was thinking that I covered the all code paths using FAILONERROR,
but it seems it's not the case. http-walker.c and http-push.c also
calls get_active_slot(). I'll narrow down the scope on removing
FAILONERROR.

>
> Anything that uses handle_curl_result() is OK. That means run_one_slot()
> is fine, which in turn covers run_slot() for RPCs, and http_request()
> for normal one-at-a-time requests. But what about the parallel multiple
> requests issued by the dumb-http walker code?

Right. I'll keep FAILONERROR in get_active_slot and remove it only for
the code paths that can handle HTTP errors.

>
> There I think we end up in step_active_slots(), which calls into
> finish_active_slot() for completed requests. I think that
> unconditionally fetches the http code without bothering to look at
> whether curl reported success or not.
>
> So I _think_ that's probably all of the users of the curl handles
> provided by get_active_slot(). Though given the tangled mess of our HTTP
> code, I won't be surprised if there's a corner case I missed in that
> analysis.
>
> -Peff

[PATCH v3 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE

[Masaya Suzuki]

Diff from v2[1]:

*   Remove http_resonse_dest

    This was introduced to have a filename for freopen. Jeff King proposed [2]
    using fflush and ftruncate and this makes this struct not needed.

*   Unset CURLOPT_FAILONERROR only when it's necessary.

    Previously, CURLOPT_FAILONERROR was unset for everything. This patch series
    does so only when it's necessary. This is from the observation in [3] that
    pointed out there are other possible code paths that hit http.c.

*   Split the patches for easier review

[1]: https://public-inbox.org/git/20181229194447.157763-1-masayasuzuki@google.com/
[2]: https://public-inbox.org/git/20190104101149.GA26185@sigill.intra.peff.net/
[3]: https://public-inbox.org/git/20190104104907.GC26185@sigill.intra.peff.net/

Masaya Suzuki (5):
  http: support file handles for HTTP_KEEP_ERROR
  http: enable keep_error for HTTP requests
  remote-curl: define struct for CURLOPT_WRITEFUNCTION
  remote-curl: unset CURLOPT_FAILONERROR
  test: test GIT_CURL_VERBOSE=1 shows an error

 http.c                       | 27 +++++++++++++--------------
 http.h                       |  1 -
 remote-curl.c                | 29 ++++++++++++++++++++++++-----
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
 5 files changed, 66 insertions(+), 20 deletions(-)
 create mode 100755 t/t5581-http-curl-verbose.sh

-- 
2.20.1.97.g81188d93c3-goog

[PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR

[Masaya Suzuki]

HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
be supported.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 0b6807cef9..06450da96e 100644
--- a/http.c
+++ b/http.c
@@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,
 	/*
 	 * If we are using KEEP_ERROR, the previous request may have
 	 * put cruft into our output stream; we should clear it out before
-	 * making our next request. We only know how to do this for
-	 * the strbuf case, but that is enough to satisfy current callers.
+	 * making our next request.
 	 */
 	if (options && options->keep_error) {
 		switch (target) {
 		case HTTP_REQUEST_STRBUF:
 			strbuf_reset(result);
 			break;
+		case HTTP_REQUEST_FILE:
+			fflush(result);
+			ftruncate(fileno(result), 0);
+			break;
 		default:
-			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+			BUG("Unknown http_request target");
 		}
 	}
 
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v3 2/5] http: enable keep_error for HTTP requests

[Masaya Suzuki]

curl stops parsing a response when it sees a bad HTTP status code and it
has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP
headers on error.

keep_error is an option to receive the HTTP response body for those
error responses. By enabling this option, curl will process the HTTP
response headers, and they're shown if GIT_CURL_VERBOSE is set.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c        | 30 +++++++++++++-----------------
 http.h        |  1 -
 remote-curl.c |  1 -
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/http.c b/http.c
index 06450da96e..a72db87723 100644
--- a/http.c
+++ b/http.c
@@ -1876,8 +1876,6 @@ static int http_request(const char *url,
 	strbuf_addstr(&buf, "Pragma:");
 	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options && options->keep_error)
-		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 	if (options && options->initial_request &&
 	    http_follow_config == HTTP_FOLLOW_INITIAL)
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -1895,6 +1893,7 @@ static int http_request(const char *url,
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 	ret = run_one_slot(slot, &results);
 
@@ -1989,22 +1988,19 @@ static int http_request_reauth(const char *url,
 		return ret;
 
 	/*
-	 * If we are using KEEP_ERROR, the previous request may have
-	 * put cruft into our output stream; we should clear it out before
-	 * making our next request.
+	 * The previous request may have put cruft into our output stream; we
+	 * should clear it out before making our next request.
 	 */
-	if (options && options->keep_error) {
-		switch (target) {
-		case HTTP_REQUEST_STRBUF:
-			strbuf_reset(result);
-			break;
-		case HTTP_REQUEST_FILE:
-			fflush(result);
-			ftruncate(fileno(result), 0);
-			break;
-		default:
-			BUG("Unknown http_request target");
-		}
+	switch (target) {
+	case HTTP_REQUEST_STRBUF:
+		strbuf_reset(result);
+		break;
+	case HTTP_REQUEST_FILE:
+		fflush(result);
+		ftruncate(fileno(result), 0);
+		break;
+	default:
+		BUG("Unknown http_request target");
 	}
 
 	credential_fill(&http_auth);
diff --git a/http.h b/http.h
index d305ca1dc7..eebf40688c 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 /* Options for http_get_*() */
 struct http_get_options {
 	unsigned no_cache:1,
-		 keep_error:1,
 		 initial_request:1;
 
 	/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..d8eda2380a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
-	http_options.keep_error = 1;
 
 	http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
 	switch (http_ret) {
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v3 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION

[Masaya Suzuki]

In order to pass more values for rpc_in, define a struct and pass it as
an additional value.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 remote-curl.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index d8eda2380a..d4673b6e8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+struct rpc_in_data {
+	struct rpc_state *rpc;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
-	struct rpc_state *rpc = buffer_;
+	struct rpc_in_data *data = buffer_;
 	if (size)
-		rpc->any_written = 1;
-	write_or_die(rpc->in, ptr, size);
+		data->rpc->any_written = 1;
+	write_or_die(data->rpc->in, ptr, size);
 	return size;
 }
 
@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)
 	size_t gzip_size = 0;
 	int err, large_request = 0;
 	int needs_100_continue = 0;
+	struct rpc_in_data rpc_in_data;
 
 	/* 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
@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+	rpc_in_data.rpc = rpc;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
 
 
 	rpc->any_written = 0;
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v3 4/5] remote-curl: unset CURLOPT_FAILONERROR

[Masaya Suzuki]

By not setting CURLOPT_FAILONERROR, curl parses the HTTP response
headers even if the response is an error. This makes GIT_CURL_VERBOSE to
show the HTTP headers, which is useful for debugging.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 remote-curl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index d4673b6e8c..91b39ca098 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 
 struct rpc_in_data {
 	struct rpc_state *rpc;
+	struct active_request_slot *slot;
 };
 
 /*
@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_in_data *data = buffer_;
+	long response_code;
+
+	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+			      &response_code) != CURLE_OK)
+		return size;
+	if (response_code >= 300)
+		return size;
 	if (size)
 		data->rpc->any_written = 1;
 	write_or_die(data->rpc->in, ptr, size);
@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	rpc_in_data.rpc = rpc;
+	rpc_in_data.slot = slot;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 
 	rpc->any_written = 0;
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v3 5/5] test: test GIT_CURL_VERBOSE=1 shows an error

[Masaya Suzuki]

This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This
exercises the code in remote_curl.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100755 t/t5581-http-curl-verbose.sh

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..cc4b87507e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 0000000000..cd9283eeec
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+	git config push.default matching &&
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+	test_might_fail env GIT_CURL_VERBOSE=1 \
+		git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+		2>curl_log &&
+	grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+'
+
+stop_httpd
+
+test_done
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH v3 1/5] http: support file handles for HTTP_KEEP_ERROR

[SZEDER Gábor]

On Mon, Jan 07, 2019 at 06:47:37PM -0800, Masaya Suzuki wrote:
> HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
> to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
> be supported.
> 
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
>  http.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/http.c b/http.c
> index 0b6807cef9..06450da96e 100644
> --- a/http.c
> +++ b/http.c
> @@ -1991,16 +1991,19 @@ static int http_request_reauth(const char *url,
>  	/*
>  	 * If we are using KEEP_ERROR, the previous request may have
>  	 * put cruft into our output stream; we should clear it out before
> -	 * making our next request. We only know how to do this for
> -	 * the strbuf case, but that is enough to satisfy current callers.
> +	 * making our next request.
>  	 */
>  	if (options && options->keep_error) {
>  		switch (target) {
>  		case HTTP_REQUEST_STRBUF:
>  			strbuf_reset(result);
>  			break;
> +		case HTTP_REQUEST_FILE:
> +			fflush(result);
> +			ftruncate(fileno(result), 0);

Some GCC versions complain about the above line:

  http.c: In function ‘http_request_reauth’:
  http.c:1961:3: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result]
     ftruncate(fileno(result), 0);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      CC shell.o
      CC remote-testsvn.o
      CC vcs-svn/line_buffer.o
  cc1: all warnings being treated as errors
  make: *** [http.o] Error 1
  make: *** Waiting for unfinished jobs....

> +			break;
>  		default:
> -			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
> +			BUG("Unknown http_request target");
>  		}
>  	}
>  
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

[PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE

[Masaya Suzuki]

Diff from v3[1]:

*   Handle ftruncate and fflush return values
*   Call rewind to set the position back

[1]: https://public-inbox.org/git/20190108024741.62176-1-masayasuzuki@google.com/

Masaya Suzuki (5):
  http: support file handles for HTTP_KEEP_ERROR
  http: enable keep_error for HTTP requests
  remote-curl: define struct for CURLOPT_WRITEFUNCTION
  remote-curl: unset CURLOPT_FAILONERROR
  test: test GIT_CURL_VERBOSE=1 shows an error

 http.c                       | 32 +++++++++++++++++++-------------
 http.h                       |  1 -
 remote-curl.c                | 29 ++++++++++++++++++++++++-----
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+), 19 deletions(-)
 create mode 100755 t/t5581-http-curl-verbose.sh

-- 
2.20.1.97.g81188d93c3-goog

[PATCH v4 1/5] http: support file handles for HTTP_KEEP_ERROR

[Masaya Suzuki]

HTTP_KEEP_ERROR makes it easy to debug HTTP transport errors. In order
to make HTTP_KEEP_ERROR enabled for all requests, file handles need to
be supported.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/http.c b/http.c
index 0b6807cef9..4eccf4c5d8 100644
--- a/http.c
+++ b/http.c
@@ -1991,16 +1991,26 @@ static int http_request_reauth(const char *url,
 	/*
 	 * If we are using KEEP_ERROR, the previous request may have
 	 * put cruft into our output stream; we should clear it out before
-	 * making our next request. We only know how to do this for
-	 * the strbuf case, but that is enough to satisfy current callers.
+	 * making our next request.
 	 */
 	if (options && options->keep_error) {
 		switch (target) {
 		case HTTP_REQUEST_STRBUF:
 			strbuf_reset(result);
 			break;
+		case HTTP_REQUEST_FILE:
+			if (fflush(result)) {
+				error_errno("unable to flush a file");
+				return HTTP_START_FAILED;
+			}
+			rewind(result);
+			if (ftruncate(fileno(result), 0) < 0) {
+				error_errno("unable to truncate a file");
+				return HTTP_START_FAILED;
+			}
+			break;
 		default:
-			BUG("HTTP_KEEP_ERROR is only supported with strbufs");
+			BUG("Unknown http_request target");
 		}
 	}
 
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v4 2/5] http: enable keep_error for HTTP requests

[Masaya Suzuki]

curl stops parsing a response when it sees a bad HTTP status code and it
has CURLOPT_FAILONERROR set. This prevents GIT_CURL_VERBOSE to show HTTP
headers on error.

keep_error is an option to receive the HTTP response body for those
error responses. By enabling this option, curl will process the HTTP
response headers, and they're shown if GIT_CURL_VERBOSE is set.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 http.c        | 42 +++++++++++++++++++-----------------------
 http.h        |  1 -
 remote-curl.c |  1 -
 3 files changed, 19 insertions(+), 25 deletions(-)

diff --git a/http.c b/http.c
index 4eccf4c5d8..954bebf684 100644
--- a/http.c
+++ b/http.c
@@ -1876,8 +1876,6 @@ static int http_request(const char *url,
 	strbuf_addstr(&buf, "Pragma:");
 	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options && options->keep_error)
-		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 	if (options && options->initial_request &&
 	    http_follow_config == HTTP_FOLLOW_INITIAL)
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
@@ -1895,6 +1893,7 @@ static int http_request(const char *url,
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 	ret = run_one_slot(slot, &results);
 
@@ -1989,29 +1988,26 @@ static int http_request_reauth(const char *url,
 		return ret;
 
 	/*
-	 * If we are using KEEP_ERROR, the previous request may have
-	 * put cruft into our output stream; we should clear it out before
-	 * making our next request.
+	 * The previous request may have put cruft into our output stream; we
+	 * should clear it out before making our next request.
 	 */
-	if (options && options->keep_error) {
-		switch (target) {
-		case HTTP_REQUEST_STRBUF:
-			strbuf_reset(result);
-			break;
-		case HTTP_REQUEST_FILE:
-			if (fflush(result)) {
-				error_errno("unable to flush a file");
-				return HTTP_START_FAILED;
-			}
-			rewind(result);
-			if (ftruncate(fileno(result), 0) < 0) {
-				error_errno("unable to truncate a file");
-				return HTTP_START_FAILED;
-			}
-			break;
-		default:
-			BUG("Unknown http_request target");
+	switch (target) {
+	case HTTP_REQUEST_STRBUF:
+		strbuf_reset(result);
+		break;
+	case HTTP_REQUEST_FILE:
+		if (fflush(result)) {
+			error_errno("unable to flush a file");
+			return HTTP_START_FAILED;
 		}
+		rewind(result);
+		if (ftruncate(fileno(result), 0) < 0) {
+			error_errno("unable to truncate a file");
+			return HTTP_START_FAILED;
+		}
+		break;
+	default:
+		BUG("Unknown http_request target");
 	}
 
 	credential_fill(&http_auth);
diff --git a/http.h b/http.h
index d305ca1dc7..eebf40688c 100644
--- a/http.h
+++ b/http.h
@@ -146,7 +146,6 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 /* Options for http_get_*() */
 struct http_get_options {
 	unsigned no_cache:1,
-		 keep_error:1,
 		 initial_request:1;
 
 	/* If non-NULL, returns the content-type of the response. */
diff --git a/remote-curl.c b/remote-curl.c
index 1220dffcdc..d8eda2380a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -380,7 +380,6 @@ static struct discovery *discover_refs(const char *service, int for_push)
 	http_options.extra_headers = &extra_headers;
 	http_options.initial_request = 1;
 	http_options.no_cache = 1;
-	http_options.keep_error = 1;
 
 	http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
 	switch (http_ret) {
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v4 3/5] remote-curl: define struct for CURLOPT_WRITEFUNCTION

[Masaya Suzuki]

In order to pass more values for rpc_in, define a struct and pass it as
an additional value.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 remote-curl.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index d8eda2380a..d4673b6e8c 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -545,14 +545,22 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 }
 #endif
 
+struct rpc_in_data {
+	struct rpc_state *rpc;
+};
+
+/*
+ * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
+ * from ptr.
+ */
 static size_t rpc_in(char *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
 	size_t size = eltsize * nmemb;
-	struct rpc_state *rpc = buffer_;
+	struct rpc_in_data *data = buffer_;
 	if (size)
-		rpc->any_written = 1;
-	write_or_die(rpc->in, ptr, size);
+		data->rpc->any_written = 1;
+	write_or_die(data->rpc->in, ptr, size);
 	return size;
 }
 
@@ -632,6 +640,7 @@ static int post_rpc(struct rpc_state *rpc)
 	size_t gzip_size = 0;
 	int err, large_request = 0;
 	int needs_100_continue = 0;
+	struct rpc_in_data rpc_in_data;
 
 	/* 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
@@ -764,7 +773,8 @@ static int post_rpc(struct rpc_state *rpc)
 
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
-	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
+	rpc_in_data.rpc = rpc;
+	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
 
 
 	rpc->any_written = 0;
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v4 4/5] remote-curl: unset CURLOPT_FAILONERROR

[Masaya Suzuki]

By not setting CURLOPT_FAILONERROR, curl parses the HTTP response
headers even if the response is an error. This makes GIT_CURL_VERBOSE to
show the HTTP headers, which is useful for debugging.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 remote-curl.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/remote-curl.c b/remote-curl.c
index d4673b6e8c..91b39ca098 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -547,6 +547,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
 
 struct rpc_in_data {
 	struct rpc_state *rpc;
+	struct active_request_slot *slot;
 };
 
 /*
@@ -558,6 +559,13 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 {
 	size_t size = eltsize * nmemb;
 	struct rpc_in_data *data = buffer_;
+	long response_code;
+
+	if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
+			      &response_code) != CURLE_OK)
+		return size;
+	if (response_code >= 300)
+		return size;
 	if (size)
 		data->rpc->any_written = 1;
 	write_or_die(data->rpc->in, ptr, size);
@@ -774,7 +782,9 @@ static int post_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	rpc_in_data.rpc = rpc;
+	rpc_in_data.slot = slot;
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
+	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 
 	rpc->any_written = 0;
-- 
2.20.1.97.g81188d93c3-goog

[PATCH v4 5/5] test: test GIT_CURL_VERBOSE=1 shows an error

[Masaya Suzuki]

This tests GIT_CURL_VERBOSE shows an error when an URL returns 500. This
exercises the code in remote_curl.

Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
---
 t/lib-httpd/apache.conf      |  1 +
 t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)
 create mode 100755 t/t5581-http-curl-verbose.sh

diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 581c010d8f..cc4b87507e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -115,6 +115,7 @@ Alias /auth/dumb/ www/auth/dumb/
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 	SetEnv GIT_HTTP_EXPORT_ALL
 </LocationMatch>
+ScriptAliasMatch /error_git_upload_pack/(.*)/git-upload-pack error.sh/
 ScriptAliasMatch /smart_*[^/]*/(.*) ${GIT_EXEC_PATH}/git-http-backend/$1
 ScriptAlias /broken_smart/ broken-smart-http.sh/
 ScriptAlias /error/ error.sh/
diff --git a/t/t5581-http-curl-verbose.sh b/t/t5581-http-curl-verbose.sh
new file mode 100755
index 0000000000..cd9283eeec
--- /dev/null
+++ b/t/t5581-http-curl-verbose.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='test GIT_CURL_VERBOSE'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
+
+test_expect_success 'setup repository' '
+	mkdir "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" --bare init &&
+	git config push.default matching &&
+	echo content >file &&
+	git add file &&
+	git commit -m one &&
+	git remote add public "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+	git push public master:master
+'
+
+test_expect_success 'failure in git-upload-pack is shown' '
+	test_might_fail env GIT_CURL_VERBOSE=1 \
+		git clone "$HTTPD_URL/error_git_upload_pack/smart/repo.git" \
+		2>curl_log &&
+	grep "< HTTP/1.1 500 Intentional Breakage" curl_log
+'
+
+stop_httpd
+
+test_done
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH v4 0/5] Show HTTP headers of failed requests with GIT_CURL_VERBOSE

[Junio C Hamano]

Masaya Suzuki <masayasuzuki@google.com> writes:

> Diff from v3[1]:
>
> *   Handle ftruncate and fflush return values
> *   Call rewind to set the position back
>
> [1]: https://public-inbox.org/git/20190108024741.62176-1-masayasuzuki@google.com/

Thanks.  Adding the error checking even to 1/5 and moving the
updated code in 2/5 makes the range-diff a bit noisy, but the
resulting series makes lot of sense.

Will queue.

>
> Masaya Suzuki (5):
>   http: support file handles for HTTP_KEEP_ERROR
>   http: enable keep_error for HTTP requests
>   remote-curl: define struct for CURLOPT_WRITEFUNCTION
>   remote-curl: unset CURLOPT_FAILONERROR
>   test: test GIT_CURL_VERBOSE=1 shows an error
>
>  http.c                       | 32 +++++++++++++++++++-------------
>  http.h                       |  1 -
>  remote-curl.c                | 29 ++++++++++++++++++++++++-----
>  t/lib-httpd/apache.conf      |  1 +
>  t/t5581-http-curl-verbose.sh | 28 ++++++++++++++++++++++++++++
>  5 files changed, 72 insertions(+), 19 deletions(-)
>  create mode 100755 t/t5581-http-curl-verbose.sh