Re: [PATCH] Revert "retry request without query when info/refs?query fails"

"Shawn O. Pearce" [off-list ref] writes:

This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.

Retrying without the query parameter was added as a workaround
for a single broken HTTP server at git.debian.org[1]. The server
was misconfigured to route every request with a query parameter
into gitweb.cgi. Admins fixed the server's configuration within
16 hours of the bug report to the Git mailing list, but we still
patched Git with this fallback and have been paying for it since.

As the consequence of the above, the only two things we know about
the servers in the wild are (1) a misconfiguration that requires
this retry was once made, so it is not very unlikely others did the
same misconfiguration, and (2) those unknown number of servers have
been happily serving the current clients because the workaround
patch have been hiding the misconfiguration ever since.

But as long as the failure diagnosis from updated clients that
revert this workaround is sufficient to allow such misconfigured
servers, I think it is OK.  We might see a large number of small
people having to run around and fix the configuration as a fallout,
though.

Most Git hosting services configure the smart HTTP protocol and the
retry logic confuses users when there is a transient HTTP error as
Git dropped the real error from the smart HTTP request. Removing the
retry makes root causes easier to identify.

Does that hold true also for dumb only small people installations?
They are the ones that need more help than the large installations
staffed sufficiently and run smart http gateway.

On Wed, Sep 19, 2012 at 11:29:34PM -0700, Junio C Hamano wrote:

"Shawn O. Pearce" [off-list ref] writes:

quoted

This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7.

Retrying without the query parameter was added as a workaround
for a single broken HTTP server at git.debian.org[1]. The server
was misconfigured to route every request with a query parameter
into gitweb.cgi. Admins fixed the server's configuration within
16 hours of the bug report to the Git mailing list, but we still
patched Git with this fallback and have been paying for it since.

As the consequence of the above, the only two things we know about
the servers in the wild are (1) a misconfiguration that requires
this retry was once made, so it is not very unlikely others did the
same misconfiguration, and (2) those unknown number of servers have
been happily serving the current clients because the workaround
patch have been hiding the misconfiguration ever since.

The misconfiguration was pretty wild in this case. I'd be much more
worried about stupidly non-compliant servers that will not serve
"foo/bar" when asked for "foo/bar?key=value".

But as long as the failure diagnosis from updated clients that
revert this workaround is sufficient to allow such misconfigured
servers, I think it is OK.  We might see a large number of small
people having to run around and fix the configuration as a fallout,
though.

I think Shawn's revert is the right thing to do. But it is not complete
without the manual workaround. I'm putting that patch together now and
should have it out in a few minutes.

quoted

Most Git hosting services configure the smart HTTP protocol and the
retry logic confuses users when there is a transient HTTP error as
Git dropped the real error from the smart HTTP request. Removing the
retry makes root causes easier to identify.

Does that hold true also for dumb only small people installations?
They are the ones that need more help than the large installations
staffed sufficiently and run smart http gateway.

For the most part, yes. They will get a useful error out of the smart
request if there is a transient error, the repo does not exist, etc.
The real fallout is the people who are hitting a broken or misconfigured
server and may get a confusing error code (in the one case we know
about, it was a 404, but it really could be anything, depending on the
exact nature of the misconfiguration).

-Peff

On Thu, Sep 20, 2012 at 12:24:56PM -0400, Jeff King wrote:

I think Shawn's revert is the right thing to do. But it is not complete
without the manual workaround. I'm putting that patch together now and
should have it out in a few minutes.

And here it is. This goes on top of Shawn's revert patch (it might be
nice to mention the commit id of that in the commit message of the
second patch. I couldn't do so because it is not yet in your repo).

  [1/2]: remote-curl: rename is_http variable
  [2/2]: remote-curl: let users turn off smart http

-Peff

We don't actually care whether the connection is http or
not; what we care about is whether it might be smart http.
Rename the variable to be more accurate, which will make it
easier to later make smart-http optional.

Signed-off-by: Jeff King <redacted>
---
 remote-curl.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 2359f59..c0b98cc 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -95,7 +95,7 @@ static struct discovery* discover_refs(const char *service)
 	struct strbuf buffer = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	char *refs_url;
-	int http_ret, is_http = 0;
+	int http_ret, maybe_smart = 0;
 
 	if (last && !strcmp(service, last->service))
 		return last;

@@ -103,7 +103,7 @@ static struct discovery* discover_refs(const char *service)
 
 	strbuf_addf(&buffer, "%sinfo/refs", url);
 	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
-		is_http = 1;
+		maybe_smart = 1;
 		if (!strchr(url, '?'))
 			strbuf_addch(&buffer, '?');
 		else

@@ -131,7 +131,7 @@ static struct discovery* discover_refs(const char *service)
 	last->buf_alloc = strbuf_detach(&buffer, &last->len);
 	last->buf = last->buf_alloc;
 
-	if (is_http && 5 <= last->len && last->buf[4] == '#') {
+	if (maybe_smart && 5 <= last->len && last->buf[4] == '#') {
 		/* smart HTTP response; validate that the service
 		 * pkt-line matches our request.
 		 */

-- 
1.7.11.7.15.g085c6bd

Usually there is no need for users to specify whether an
http remote is smart or dumb; the protocol is designed so
that a single initial request is made, and the client can
determine the server's capability from the response.

However, some misconfigured dumb-only servers may not like
the initial request by a smart client, as it contains a
query string. Until recently, commit 703e6e7 worked around
this by making a second request. However, that commit was
recently reverted due to its side effect of masking the
initial request's error code.

This patch takes a different approach to the workaround. We
assume that the common case is that the server is either
smart-http or a reasonably configured dumb-http. If that is
not the case, we provide both a per-remote config option and
an environment variable with which the user can manually
work around the issue.

Signed-off-by: Jeff King <redacted>
---
I added the config item as remote.foo.smarthttp. You could also allow
"http.$url.smart" (and just "http.smart", for that matter), which could
be more flexible if you have multiple remotes pointing to the same
broken server. However, it is also more complex to use, and is a lot
more code. Since we don't know if any such servers even exist, I tried
to give the minimal escape hatch, and we can easily build more features
on it later if people complain.

 Documentation/config.txt | 11 +++++++++++
 remote-curl.c            |  3 ++-
 remote.c                 |  3 +++
 remote.h                 |  1 +
 t/t5551-http-fetch.sh    | 17 +++++++++++++++++
 5 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6416cae..651b23c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt

@@ -1871,6 +1871,17 @@ remote.<name>.uploadpack::
 	The default program to execute on the remote side when fetching.  See
 	option \--upload-pack of linkgit:git-fetch-pack[1].
 
+remote.<name>.smartHttp::
+	If true, this remote will attempt to use git's smart http
+	protocol when making remote http requests. Normally git sends an
+	initial smart-http request, and falls back to the older "dumb"
+	protocol if the server does not claim to support the smart
+	protocol. However, some misconfigured dumb-only servers may
+	produce confusing results for the initial request. Setting this
+	option to false disables the initial smart request, which can
+	workaround problems with such servers. You should not generally
+	need to set this. Defaults to `true`.
+
 remote.<name>.tagopt::
 	Setting this value to \--no-tags disables automatic tag following when
 	fetching from remote <name>. Setting it to \--tags will fetch every

diff --git a/remote-curl.c b/remote-curl.c
index c0b98cc..8829bfb 100644
--- a/remote-curl.c
+++ b/remote-curl.c

@@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)
 	free_discovery(last);
 
 	strbuf_addf(&buffer, "%sinfo/refs", url);
-	if (!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) {
+	if ((!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) &&
+	     git_env_bool("GIT_SMART_HTTP", remote->smart_http)) {
 		maybe_smart = 1;
 		if (!strchr(url, '?'))
 			strbuf_addch(&buffer, '?');

diff --git a/remote.c b/remote.c
index 04fd9ea..a334390 100644
--- a/remote.c
+++ b/remote.c

@@ -152,6 +152,7 @@ static struct remote *make_remote(const char *name, int len)
 		ret->name = xstrndup(name, len);
 	else
 		ret->name = xstrdup(name);
+	ret->smart_http = 1;
 	return ret;
 }
 

@@ -453,6 +454,8 @@ static int handle_config(const char *key, const char *value, void *cb)
 					 key, value);
 	} else if (!strcmp(subkey, ".vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
+	} else if (!strcmp(subkey, ".smarthttp")) {
+		remote->smart_http = git_config_bool(key, value);
 	}
 	return 0;
 }

diff --git a/remote.h b/remote.h
index 251d8fd..9031d18 100644
--- a/remote.h
+++ b/remote.h

@@ -40,6 +40,7 @@ struct remote {
 	int fetch_tags;
 	int skip_default_update;
 	int mirror;
+	int smart_http;
 
 	const char *receivepack;
 	const char *uploadpack;

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 2db5c35..48173ed 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh

@@ -129,6 +129,23 @@ test_expect_success 'clone from auth-only-for-push repository' '
 	test_cmp expect actual
 '
 
+test_expect_success 'disable dumb http on server' '
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
+		config http.getanyfile false
+'
+
+test_expect_success 'GIT_SMART_HTTP can disable smart http' '
+	(GIT_SMART_HTTP=0 &&
+	 export GIT_SMART_HTTP &&
+	 cd clone &&
+	 test_must_fail git fetch)
+'
+
+test_expect_success 'remote.*.smartHTTP can disable smart http' '
+	(cd clone &&
+	 test_must_fail git -c remote.origin.smartHTTP=false fetch)
+'
+
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
 
 test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '

-- 
1.7.11.7.15.g085c6bd