Re: [PATCH 1/2] http: add option to enable 100 Continue responses

From: Jeff King <peff@peff.net>
Date: Oct 9, 2013, 21:37 UTC
Message-ID: <20131009213742.GA8362@sigill.intra.peff.net>
In-Reply-To: <CAJo=hJvyorMjFYZnVwz4iZr88ewor6LuqOE-mpt4LsPyoddBqg@mail.gmail.com>

On Wed, Oct 09, 2013 at 02:19:36PM -0700, Shawn O. Pearce wrote:

Show 7 quoted lines

> > I'd be more comfortable defaulting this to "on" if I understood more
> > about the original problem that led to 959dfcf and 206b099. It sounds
> > like enabling this all the time will cause annoying stalls in the
> > protocol, unless the number of non-crappy proxy implementations has
> > gotten smaller over the past few years.
> 
> It actually hasn't, not significantly.

Thanks for update; my pessimistic side assumed this was the case, but it is always good to check. :)

Show 7 quoted lines

> 206b099 was written because the Google web servers for
> android.googlesource.com and code.google.com do not support
> 100-continue semantics. This caused the client to stall a full 1
> second before each POST exchange. If ancestor negotiation required
> O(128) have lines to be advertised I think this was 2 or 4 POSTs,
> resulting in 2-4 second stalls above the other latency of the network
> and the server.

Yuck.

Show 5 quoted lines

> If "Expect: 100-continue" is required for GSS-Negotiate to work then
> Git should only enable the option if the server is demanding
> GSS-Negotiate for authentication. Per 206b099 the default should still
> be off for anonymous and HTTP basic, digest, and SSL certificate
> authentication.

Part of the problem is that curl is the one handling the negotiation. When we get a 401, I think we can ask curl_easy_getinfo to tell us which auth methods are available (via CURLINFO_HTTPAUTH_AVAIL). But I don't know how we decide that GSS is what's going to be used. I guess if it is the only option, and there is no basic-auth offered?

And then in that case turn on "Expect" (or more accurately, stop disabling it).

I don't have a GSS-enabled server to test on. Brian, can you try the patch at the end of this message on your non-working server and see what it outputs?

Show 12 quoted lines

> >> +     headers = curl_slist_append(headers, http_use_100_continue ?
> >> +             "Expect: 100-continue" : "Expect:");
> >
> > Is there any point in sending the Expect: header in cases where curl
> > would not send it, though? It seems like we should assume curl does the
> > right thing most of the time, and have our option only be to override
> > curl in the negative direction.
> 
> Adding a header of "Expect:" causes curl to disable the header and
> never use it. Always supplying the header with no value prevents
> libcurl from using 100-continue on its own, which is what I fixed in
> 959dfcf.

Right. What I meant is that we do not want to unconditionally send the "Expect: 100-continue" in the other case. IOW, we would just want:

  if (!http_use_100_continue)
          headers = curl_slist_append(headers, "Expect:");

-Peff

-- >8 --

diff --git a/http.c b/http.c
index f3e1439..e7257d7 100644
--- a/http.c
+++ b/http.c
@@ -889,6 +889,12 @@ static int http_request(const char *url, struct strbuf *type,
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
 		ret = handle_curl_result(&results);
+		if (ret == HTTP_REAUTH) {
+			long auth_avail;
+			curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+					  &auth_avail);
+			fprintf(stderr, "offered auth: %ld\n", auth_avail);
+		}
 	} else {
 		snprintf(curl_errorstr, sizeof(curl_errorstr),
 			 "failed to start HTTP request");