[PATCH] Add an option for using any HTTP authentication scheme, not only basic

[PATCH] Add an option for using any HTTP authentication scheme, not only basic

[Martin Storsjö]

This adds the configuration option http.authAny (overridable with
the environment variable GIT_HTTP_AUTH_ANY), for instructing curl
to allow any HTTP authentication scheme, not only basic (which
sends the password in plaintext).

When this is enabled, curl has to do double requests most of the time,
in order to discover which HTTP authentication method to use, which
lowers the performance slightly. Therefore this isn't enabled by default.

One example of another authentication scheme to use is digest, which
doesn't send the password in plaintext, but uses a challenge-response
mechanism instead. Using digest authentication in practice requires
at least curl 7.18.2, due to bugs in the digest handling in earlier
versions of curl.

Signed-off-by: Martin Storsjo <martin@martin.st>
---

This is a resend of a patch I sent a few weeks ago. Now this functionality
is configurable and disabled by default.

 Documentation/config.txt |    7 +++++++
 http.c                   |   18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f3ebd2f..1515d77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1011,6 +1011,13 @@ http.noEPSV::
 	support EPSV mode. Can be overridden by the 'GIT_CURL_FTP_NO_EPSV'
 	environment variable. Default is false (curl will use EPSV).
 
+http.authAny::
+	Allow any HTTP authentication method, not only basic. Enabling
+	this lowers the performance slightly, by having to do requests
+	without any authentication to discover the authentication method
+	to use. Can be overridden by the 'GIT_HTTP_AUTH_ANY'
+	environment variable. Default is false.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/http.c b/http.c
index 2e3d649..0b18c64 100644
--- a/http.c
+++ b/http.c
@@ -26,6 +26,9 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static char *user_name, *user_pass;
+#if LIBCURL_VERSION_NUM >= 0x070a06
+static int curl_http_auth_any = 0;
+#endif
 
 static struct curl_slist *pragma_header;
 
@@ -150,6 +153,12 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (!strcmp("http.authany", var)) {
+		curl_http_auth_any = git_config_bool(var, value);
+		return 0;
+	}
+#endif
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
@@ -184,6 +193,10 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
 #endif
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (curl_http_auth_any)
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
+#endif
 
 	init_curl_http_auth(result);
 
@@ -329,6 +342,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+	if (getenv("GIT_HTTP_AUTH_ANY"))
+		curl_http_auth_any = 1;
+#endif
+
 	if (remote && remote->url && remote->url[0])
 		http_auth_init(remote->url[0]);
 
-- 
1.6.0.2

Re: [PATCH] Add an option for using any HTTP authentication scheme, not only basic

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 610 bytes --]

Hi,

On Tue, 14 Apr 2009, Martin Storsjö wrote:

> diff --git a/http.c b/http.c
> index 2e3d649..0b18c64 100644
> --- a/http.c
> +++ b/http.c
> @@ -26,6 +26,9 @@ static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
>  static char *user_name, *user_pass;
> +#if LIBCURL_VERSION_NUM >= 0x070a06
> +static int curl_http_auth_any = 0;
> +#endif

In six months from now, it might be easier to read

#if LIBCURL_VERSION_NUM >= 0x070a06
#define LIBCURL_CAN_HANDLE_ANY_AUTH
#endif

[...]

#ifdef LIBCURL_CAN_HANDLE_ANY_AUTH
[...]

Don't you agree?

Thanks,
Dscho

Re: [PATCH] Add an option for using any HTTP authentication scheme, not only basic

[Martin Storsjö]

Hi,

On Tue, 14 Apr 2009, Johannes Schindelin wrote:

> In six months from now, it might be easier to read
> 
> #if LIBCURL_VERSION_NUM >= 0x070a06
> #define LIBCURL_CAN_HANDLE_ANY_AUTH
> #endif
> 
> [...]
> 
> #ifdef LIBCURL_CAN_HANDLE_ANY_AUTH
> [...]
> 
> Don't you agree?

Yeah, absolutely.

Any comments on the naming of options and env variables? Otherwise, I'll 
resend an updated version soon.

// Martin

Re: [PATCH] Add an option for using any HTTP authentication scheme, not only basic

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 534 bytes --]

Hi,

On Wed, 15 Apr 2009, Martin Storsjö wrote:

> On Tue, 14 Apr 2009, Johannes Schindelin wrote:
> 
> > In six months from now, it might be easier to read
> > 
> > #if LIBCURL_VERSION_NUM >= 0x070a06
> > #define LIBCURL_CAN_HANDLE_ANY_AUTH
> > #endif
> > 
> > [...]
> > 
> > #ifdef LIBCURL_CAN_HANDLE_ANY_AUTH
> > [...]
> > 
> > Don't you agree?
> 
> Yeah, absolutely.
> 
> Any comments on the naming of options and env variables? Otherwise, I'll 
> resend an updated version soon.

They sound sensible enough to me!

Thanks,
Dscho

[PATCH v2] Add an option for using any HTTP authentication scheme, not only basic

[Martin Storsjö]

This adds the configuration option http.authAny (overridable with
the environment variable GIT_HTTP_AUTH_ANY), for instructing curl
to allow any HTTP authentication scheme, not only basic (which
sends the password in plaintext).

When this is enabled, curl has to do double requests most of the time,
in order to discover which HTTP authentication method to use, which
lowers the performance slightly. Therefore this isn't enabled by default.

One example of another authentication scheme to use is digest, which
doesn't send the password in plaintext, but uses a challenge-response
mechanism instead. Using digest authentication in practice requires
at least curl 7.18.1, due to bugs in the digest handling in earlier
versions of curl.

Signed-off-by: Martin Storsjo <martin@martin.st>
---

Repost with the curl version checked in only one place.

 Documentation/config.txt |    7 +++++++
 http.c                   |   22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f3ebd2f..1515d77 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1011,6 +1011,13 @@ http.noEPSV::
 	support EPSV mode. Can be overridden by the 'GIT_CURL_FTP_NO_EPSV'
 	environment variable. Default is false (curl will use EPSV).
 
+http.authAny::
+	Allow any HTTP authentication method, not only basic. Enabling
+	this lowers the performance slightly, by having to do requests
+	without any authentication to discover the authentication method
+	to use. Can be overridden by the 'GIT_HTTP_AUTH_ANY'
+	environment variable. Default is false.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/http.c b/http.c
index 2e3d649..49b8441 100644
--- a/http.c
+++ b/http.c
@@ -3,6 +3,10 @@
 int data_received;
 int active_requests;
 
+#if LIBCURL_VERSION_NUM >= 0x070a06
+#define LIBCURL_CAN_HANDLE_AUTH_ANY
+#endif
+
 #ifdef USE_CURL_MULTI
 static int max_requests = -1;
 static CURLM *curlm;
@@ -26,6 +30,9 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static char *user_name, *user_pass;
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+static int curl_http_auth_any = 0;
+#endif
 
 static struct curl_slist *pragma_header;
 
@@ -150,6 +157,12 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (!strcmp("http.authany", var)) {
+		curl_http_auth_any = git_config_bool(var, value);
+		return 0;
+	}
+#endif
 
 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
@@ -184,6 +197,10 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
 #endif
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (curl_http_auth_any)
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
+#endif
 
 	init_curl_http_auth(result);
 
@@ -329,6 +346,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;
 
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (getenv("GIT_HTTP_AUTH_ANY"))
+		curl_http_auth_any = 1;
+#endif
+
 	if (remote && remote->url && remote->url[0])
 		http_auth_init(remote->url[0]);
 
-- 
1.6.0.2

HTTP NTLM Authentication

[gsky]

Is is possible for me to pass arguments to the curl calls that git uses to
access a repository hosted over HTTP?

I am having a problem accessing the repository as it is authenticated using
NTLM, I can curl the repository using

curl --ntlm http://username:pass@machine.domain/git

How can I do the same for the git clone of the repository?  Is it possible
easily, or do I have to modify the source and recompile?

gsky
-- 
View this message in context: http://www.nabble.com/HTTP-NTLM-Authentication-tp25718488p25718488.html
Sent from the git mailing list archive at Nabble.com.

[PATCH] Use the best HTTP authentication method supported by the server

[Nicholas Miell]

Currently, libcurl is limited to using HTTP Basic authentication if a
username and password are specified. HTTP Basic passes the username
and password to the server as plaintext, which is obviously
suboptimal. Furthermore, some servers are configured to require a more
secure authentication method (e.g. Digest or NTLM), which means that
git can't talk to them at all.

This is easily solved by telling libcurl to use any HTTP
authentication method it pleases. I leave the decision as to whether
HTTP Basic (i.e. completely insecure) should be allowed at all to
somebody else.  This can be easily changed in the future by using
CURLAUTH_ANYSAFE instead of CURLAUTH_ANY.

Signed-off-by: Nicholas Miell <nmiell@gmail.com>
---
 http.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

This passes make test; but I haven't actually tested it on a real
HTTP server.

diff --git a/http.c b/http.c
index 23b2a19..1937b45 100644
--- a/http.c
+++ b/http.c
@@ -185,6 +185,7 @@ static void init_curl_http_auth(CURL *result)
 		if (!user_pass)
 			user_pass = xstrdup(getpass("Password: "));
 		strbuf_addf(&up, "%s:%s", user_name, user_pass);
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
 		curl_easy_setopt(result, CURLOPT_USERPWD,
 				 strbuf_detach(&up, NULL));
 	}
-- 
1.6.2.5

[PATCH 0/2] http: allow multi-pass authentication

[Tay Ray Chuan]

This patch series applies on top of master. It enables fetching and
pushing over http with the most suitable authentication scheme chosen
by curl when the http.authAny or GIT_HTTP_AUTH_ANY is set.

Authorization headers can also be preserved across requests, with at
least 1 curl session being preserved by default. This is especially
useful for the smart http protocol, where it is hard to rewind and re-
send a request.

Nicholas, Martin's patch should lead to similar functionality as your
patch (dated Oct 3rd) would. However, unlike your patch,
CURLOPT_HTTPAUTH is set even if the user name is not specified
explicitly in the remote url, since it's conditional on
http.c::user_name being set.

I've tested this with Digest, and I believe this should work with NTLM
too.

gsky, could you try this out with NTLM?

=?ISO-8859-15?Q?Martin_Storsj=F6?= (1):
  Add an option for using any HTTP authentication scheme, not only
    basic

Tay Ray Chuan (1):
  http: maintain curl sessions

 Documentation/config.txt |   13 +++++++++++++
 http.c                   |   41 +++++++++++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 2 deletions(-)

--
Cheers,
Ray Chuan

[PATCH 1/2] http: maintain curl sessions

[Tay Ray Chuan]

Allow curl sessions to be kept alive (ie. not ended with
curl_easy_cleanup()) even after the request is completed, the number of
which is determined by the configuration setting http.minSessions.

Add a count for curl sessions, and update it, across slots, when
starting and ending curl sessions.

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
 Documentation/config.txt |    6 ++++++
 http.c                   |   19 +++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a8e0876..b77d66d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1132,6 +1132,12 @@ http.maxRequests::
 	How many HTTP requests to launch in parallel. Can be overridden
 	by the 'GIT_HTTP_MAX_REQUESTS' environment variable. Default is 5.

+http.minSessions::
+	The number of curl sessions (counted across slots) to be kept across
+	requests. They will not be ended with curl_easy_cleanup() until
+	http_cleanup() is invoked. If USE_CURL_MULTI is not defined, this
+	value will be capped at 1. Defaults to 1.
+
 http.postBuffer::
 	Maximum size in bytes of the buffer used by smart HTTP
 	transports when POSTing data to the remote system.
diff --git a/http.c b/http.c
index ed6414a..fb0a97b 100644
--- a/http.c
+++ b/http.c
@@ -7,6 +7,8 @@ int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;

+static int min_curl_sessions = 1;
+static int curl_session_count;
 #ifdef USE_CURL_MULTI
 static int max_requests = -1;
 static CURLM *curlm;
@@ -152,6 +154,14 @@ static int http_options(const char *var, const char *value, void *cb)
 			ssl_cert_password_required = 1;
 		return 0;
 	}
+	if (!strcmp("http.minsessions", var)) {
+		min_curl_sessions = git_config_int(var, value);
+#ifndef USE_CURL_MULTI
+		if (min_curl_sessions > 1)
+			min_curl_sessions = 1;
+#endif
+		return 0;
+	}
 #ifdef USE_CURL_MULTI
 	if (!strcmp("http.maxrequests", var)) {
 		max_requests = git_config_int(var, value);
@@ -372,6 +382,7 @@ void http_init(struct remote *remote)
 	if (curl_ssl_verify == -1)
 		curl_ssl_verify = 1;

+	curl_session_count = 0;
 #ifdef USE_CURL_MULTI
 	if (max_requests < 1)
 		max_requests = DEFAULT_MAX_REQUESTS;
@@ -480,6 +491,7 @@ struct active_request_slot *get_active_slot(void)
 #else
 		slot->curl = curl_easy_duphandle(curl_default);
 #endif
+		curl_session_count++;
 	}

 	active_requests++;
@@ -558,9 +570,11 @@ void fill_active_slots(void)
 	}

 	while (slot != NULL) {
-		if (!slot->in_use && slot->curl != NULL) {
+		if (!slot->in_use && slot->curl != NULL
+			&& curl_session_count > min_curl_sessions) {
 			curl_easy_cleanup(slot->curl);
 			slot->curl = NULL;
+			curl_session_count--;
 		}
 		slot = slot->next;
 	}
@@ -633,12 +647,13 @@ static void closedown_active_slot(struct active_request_slot *slot)
 void release_active_slot(struct active_request_slot *slot)
 {
 	closedown_active_slot(slot);
-	if (slot->curl) {
+	if (slot->curl && curl_session_count > min_curl_sessions) {
 #ifdef USE_CURL_MULTI
 		curl_multi_remove_handle(curlm, slot->curl);
 #endif
 		curl_easy_cleanup(slot->curl);
 		slot->curl = NULL;
+		curl_session_count--;
 	}
 #ifdef USE_CURL_MULTI
 	fill_active_slots();
--
1.6.4.4

[PATCH 2/2] Add an option for using any HTTP authentication scheme, not only basic

[Tay Ray Chuan]

From:	=?ISO-8859-15?Q?Martin_Storsj=F6?= <martin@martin.st>

This adds the configuration option http.authAny (overridable with
the environment variable GIT_HTTP_AUTH_ANY), for instructing curl
to allow any HTTP authentication scheme, not only basic (which
sends the password in plaintext).

When this is enabled, curl has to do double requests most of the time,
in order to discover which HTTP authentication method to use, which
lowers the performance slightly. Therefore this isn't enabled by default.

One example of another authentication scheme to use is digest, which
doesn't send the password in plaintext, but uses a challenge-response
mechanism instead. Using digest authentication in practice requires
at least curl 7.18.1, due to bugs in the digest handling in earlier
versions of curl.

Signed-off-by: Martin Storsjo <martin@martin.st>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

  Martin, I've not made any changes, except to make it apply cleanly.

 Documentation/config.txt |    7 +++++++
 http.c                   |   22 ++++++++++++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b77d66d..a54ede3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1158,6 +1158,13 @@ http.noEPSV::
 	support EPSV mode. Can be overridden by the 'GIT_CURL_FTP_NO_EPSV'
 	environment variable. Default is false (curl will use EPSV).

+http.authAny::
+	Allow any HTTP authentication method, not only basic. Enabling
+	this lowers the performance slightly, by having to do requests
+	without any authentication to discover the authentication method
+	to use. Can be overridden by the 'GIT_HTTP_AUTH_ANY'
+	environment variable. Default is false.
+
 i18n.commitEncoding::
 	Character encoding the commit messages are stored in; git itself
 	does not care per se, but this information is necessary e.g. when
diff --git a/http.c b/http.c
index fb0a97b..aeb69b3 100644
--- a/http.c
+++ b/http.c
@@ -7,6 +7,10 @@ int active_requests;
 int http_is_verbose;
 size_t http_post_buffer = 16 * LARGE_PACKET_MAX;

+#if LIBCURL_VERSION_NUM >= 0x070a06
+#define LIBCURL_CAN_HANDLE_AUTH_ANY
+#endif
+
 static int min_curl_sessions = 1;
 static int curl_session_count;
 #ifdef USE_CURL_MULTI
@@ -36,6 +40,9 @@ static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
 static char *user_name, *user_pass;
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+static int curl_http_auth_any = 0;
+#endif

 #if LIBCURL_VERSION_NUM >= 0x071700
 /* Use CURLOPT_KEYPASSWD as is */
@@ -190,6 +197,12 @@ static int http_options(const char *var, const char *value, void *cb)
 			http_post_buffer = LARGE_PACKET_MAX;
 		return 0;
 	}
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (!strcmp("http.authany", var)) {
+		curl_http_auth_any = git_config_bool(var, value);
+		return 0;
+	}
+#endif

 	/* Fall back on the default ones */
 	return git_default_config(var, value, cb);
@@ -240,6 +253,10 @@ static CURL *get_curl_handle(void)
 #if LIBCURL_VERSION_NUM >= 0x070907
 	curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL);
 #endif
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (curl_http_auth_any)
+		curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_ANY);
+#endif

 	init_curl_http_auth(result);

@@ -391,6 +408,11 @@ void http_init(struct remote *remote)
 	if (getenv("GIT_CURL_FTP_NO_EPSV"))
 		curl_ftp_no_epsv = 1;

+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+	if (getenv("GIT_HTTP_AUTH_ANY"))
+		curl_http_auth_any = 1;
+#endif
+
 	if (remote && remote->url && remote->url[0]) {
 		http_auth_init(remote->url[0]);
 		if (!ssl_cert_password_required &&
--
1.6.4.4

Re: [PATCH 0/2] http: allow multi-pass authentication

[Martin Storsjö]

On Fri, 27 Nov 2009, Tay Ray Chuan wrote:

> This patch series applies on top of master. It enables fetching and
> pushing over http with the most suitable authentication scheme chosen
> by curl when the http.authAny or GIT_HTTP_AUTH_ANY is set.

I also tested this, and things generally seem to work fine.

Thanks to the "maintain curl sessions" patch, only the first request needs 
to be redone after getting the 401 error containing the authentication 
challenge, the later ones work fine on the first try. However, 
theoretically, I guess we can't be certain that the curl session really is 
initialized for the later requests (we could be given a new fresh curl 
session for some reason), or the first request could perhaps be a large, 
(currently) non-rewindable POST.


Avoiding redoing large POST requests is generally accomplished by adding a 
Expect: 100-continue header, and then waiting for a reply (either 100 
continue or 401 unauthorized) to that header before actually sending the 
POST body data. If the server doesn't support the Expect header (e.g. 
Lighttpd doesn't support it), the client starts sending the POST body 
after a timeout (1 second in libcurl).

(As a side note, chunked POST requests without a content-length header 
isn't supported by lighttpd at all at the moment, neither in the stable 
1.4 version nor in the new upcoming 1.5 branch.)


Normally, libcurl should add the Expect: 100-continue header 
automatically, but for some reason 
(http://article.gmane.org/gmane.comp.web.curl.library/25992) it doesn't, 
so that's probably why we're manually adding that header in 
remote-curl.c:371 at the moment. libcurl doesn't detect this at the moment 
(http://article.gmane.org/gmane.comp.web.curl.library/25991) so it won't 
wait for the 100 continue response before starting to send the body data. 

So, with a server supporting Expect, the 401 error response may come after 
sending a few KB of POST data (corresponding to the roundtrip delay for 
the server to respond to the header) - if the server doesn't support 
Expect at all, the whole request will be sent and may need to be rewound.

To clarify - this only happens if the curl authentication isn't 
initialized yet, for the first request of every curl session. The 
"maintain curl sessions" patch makes sure this isn't needed in the normal 
case.

I've experimented with two solutions to this, which add partial and full 
rewind solutions to the chunked POST requests - I'll send them as 
follow-ups to this mail.

// Martin

[PATCH/RFC] Allow curl to rewind the RPC read buffer

[Martin Storsjö]

When using multi-pass authentication methods, the curl library may need
to rewind the read buffers used for providing data to HTTP POST, if data
has been output before a 401 error is received.

This is needed only when the first request (when the multi-pass
authentication method isn't initialized and hasn't received its challenge
yet) for a certain curl session is a chunked HTTP POST.

As long as the current rpc read buffer is the first one, we're able to
rewind without need for additional buffering.

The curl library currently starts sending data without waiting for a
response to the Expect: 100-continue header, due to a bug in curl that
exists up to curl version 7.19.7.

If the HTTP server doesn't handle Expect: 100-continue headers properly
(e.g. Lighttpd), the library has to start sending data without knowing
if the request will be successfully authenticated. In this case, this
rewinding solution is not sufficient - the whole request will be sent
before the 401 error is received.

Signed-off-by: Martin Storsjo <martin@martin.st>
---

The curl bug is yet unconfirmed upstream, discussed here:
http://article.gmane.org/gmane.comp.web.curl.library/25991

 remote-curl.c |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index a331bae..28b2a31 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -290,6 +290,7 @@ struct rpc_state {
 	int out;
 	struct strbuf result;
 	unsigned gzip_request : 1;
+	unsigned initial_buffer : 1;
 };
 
 static size_t rpc_out(void *ptr, size_t eltsize,
@@ -300,6 +301,7 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	size_t avail = rpc->len - rpc->pos;
 
 	if (!avail) {
+		rpc->initial_buffer = 0;
 		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
 		if (!avail)
 			return 0;
@@ -314,6 +316,29 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	return avail;
 }
 
+#ifndef NO_CURL_IOCTL
+curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
+{
+	struct rpc_state *rpc = clientp;
+
+	switch (cmd) {
+	case CURLIOCMD_NOP:
+		return CURLIOE_OK;
+
+	case CURLIOCMD_RESTARTREAD:
+		if (rpc->initial_buffer) {
+			rpc->pos = 0;
+			return CURLIOE_OK;
+		}
+		fprintf(stderr, "Unable to rewind rpc post data - try increasing http.postBuffer\n");
+		return CURLIOE_FAILRESTART;
+
+	default:
+		return CURLIOE_UNKNOWNCMD;
+	}
+}
+#endif
+
 static size_t rpc_in(const void *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
@@ -370,8 +395,13 @@ static int post_rpc(struct rpc_state *rpc)
 		 */
 		headers = curl_slist_append(headers, "Expect: 100-continue");
 		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
+#ifndef NO_CURL_IOCTL
+		curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
+		curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
+#endif
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
 			fflush(stderr);
-- 
1.6.4.4

[PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Martin Storsjö]

When using multi-pass authentication methods, the curl library may
need to rewind the read buffers used for providing data to HTTP POST,
if data has been output before a 401 error is received.

This solution buffers all data read by the curl library, in order to allow
it to rewind the reading buffer at any time later.

If communicating with a HTTP server that doesn't support the
Expect: 100-continue headers, all HTTP POST data will be sent before
the server replies with the 401 error containing the authentication
challenge.

The buffering is enabled only if the rpc_service function allocates a
buffer - this should perhaps be limited to the cases where http.authAny
is enabled.

Signed-off-by: Martin Storsjo <martin@martin.st>
---
 remote-curl.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index a331bae..c1c5ccd 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -286,6 +286,10 @@ struct rpc_state {
 	size_t alloc;
 	size_t len;
 	size_t pos;
+	char *rewind_buf;
+	size_t rewind_buf_size;
+	size_t rewind_buf_write_pos;
+	size_t rewind_buf_read_pos;
 	int in;
 	int out;
 	struct strbuf result;
@@ -299,12 +303,28 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	struct rpc_state *rpc = buffer_;
 	size_t avail = rpc->len - rpc->pos;
 
+	if (rpc->rewind_buf && rpc->rewind_buf_read_pos < rpc->rewind_buf_write_pos) {
+		avail = rpc->rewind_buf_write_pos - rpc->rewind_buf_read_pos;
+		if (max < avail)
+			avail = max;
+		memcpy(ptr, rpc->rewind_buf + rpc->rewind_buf_read_pos, avail);
+		rpc->rewind_buf_read_pos += avail;
+		return avail;
+	}
+
 	if (!avail) {
 		avail = packet_read_line(rpc->out, rpc->buf, rpc->alloc);
 		if (!avail)
 			return 0;
 		rpc->pos = 0;
 		rpc->len = avail;
+
+		if (rpc->rewind_buf) {
+			ALLOC_GROW(rpc->rewind_buf, rpc->rewind_buf_write_pos + avail, rpc->rewind_buf_size);
+			memcpy(rpc->rewind_buf + rpc->rewind_buf_write_pos, rpc->buf, avail);
+			rpc->rewind_buf_write_pos += avail;
+			rpc->rewind_buf_read_pos += avail;
+		}
 	}
 
 	if (max < avail)
@@ -314,6 +334,26 @@ static size_t rpc_out(void *ptr, size_t eltsize,
 	return avail;
 }
 
+#ifndef NO_CURL_IOCTL
+curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
+{
+	struct rpc_state *rpc = clientp;
+
+	switch (cmd) {
+	case CURLIOCMD_NOP:
+		return CURLIOE_OK;
+
+	case CURLIOCMD_RESTARTREAD:
+		rpc->rewind_buf_read_pos = 0;
+		rpc->pos = rpc->len;
+		return CURLIOE_OK;
+
+	default:
+		return CURLIOE_UNKNOWNCMD;
+	}
+}
+#endif
+
 static size_t rpc_in(const void *ptr, size_t eltsize,
 		size_t nmemb, void *buffer_)
 {
@@ -370,8 +410,18 @@ static int post_rpc(struct rpc_state *rpc)
 		 */
 		headers = curl_slist_append(headers, "Expect: 100-continue");
 		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+		if (rpc->rewind_buf) {
+			ALLOC_GROW(rpc->rewind_buf, rpc->rewind_buf_write_pos + rpc->len, rpc->rewind_buf_size);
+			memcpy(rpc->rewind_buf, rpc->buf, rpc->len);
+			rpc->rewind_buf_read_pos = rpc->len;
+			rpc->rewind_buf_write_pos = rpc->len;
+		}
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
+#ifndef NO_CURL_IOCTL
+		curl_easy_setopt(slot->curl, CURLOPT_IOCTLFUNCTION, rpc_ioctl);
+		curl_easy_setopt(slot->curl, CURLOPT_IOCTLDATA, rpc);
+#endif
 		if (options.verbosity > 1) {
 			fprintf(stderr, "POST %s (chunked)\n", rpc->service_name);
 			fflush(stderr);
@@ -472,6 +522,8 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->buf = xmalloc(rpc->alloc);
 	rpc->in = client.in;
 	rpc->out = client.out;
+	rpc->rewind_buf_size = 100;
+	rpc->rewind_buf = xmalloc(rpc->rewind_buf_size);
 	strbuf_init(&rpc->result, 0);
 
 	strbuf_addf(&buf, "%s/%s", url, svc);
@@ -503,6 +555,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	free(rpc->hdr_content_type);
 	free(rpc->hdr_accept);
 	free(rpc->buf);
+	free(rpc->rewind_buf);
 	strbuf_release(&buf);
 	return err;
 }
-- 
1.6.4.4

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Shawn O. Pearce]

Martin Storsj? <martin@martin.st> wrote:
> When using multi-pass authentication methods, the curl library may need
> to rewind the read buffers used for providing data to HTTP POST, if data
> has been output before a 401 error is received.

In theory, since the cURL session stays active, we would have
received the 401 authentication error during the initial
"GET $GIT_DIR/info/refs?service=git-$service" request, and the subsequent
"POST $GIT_DIR/git-$service" requests would automatically include the
authentication data.

That's theory.  Reality doesn't always agree with my theories.  :-)
 
>  remote-curl.c |   30 ++++++++++++++++++++++++++++++
>  1 files changed, 30 insertions(+), 0 deletions(-)

Acked-by: Shawn O. Pearce <spearce@spearce.org>

-- 
Shawn.

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Tay Ray Chuan]

Hi,

On Wed, Dec 2, 2009 at 12:01 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> In theory, since the cURL session stays active, we would have
> received the 401 authentication error during the initial
> "GET $GIT_DIR/info/refs?service=git-$service" request, and the subsequent
> "POST $GIT_DIR/git-$service" requests would automatically include the
> authentication data.
>
> That's theory.  Reality doesn't always agree with my theories.  :-)

that's because the curl session where the 401 was received (and thus
successful authentication takes place) is closed.

I sent out a patch series recently which contains a patch to maintain
at least one curl session throughout a http session (from http_init()
to http_cleanup()), you can see this here:

  http://www.spinics.net/lists/git/msg118190.html

-- 
Cheers,
Ray Chuan

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Shawn O. Pearce]

Martin Storsj? <martin@martin.st> wrote:
> When using multi-pass authentication methods, the curl library may
> need to rewind the read buffers used for providing data to HTTP POST,
> if data has been output before a 401 error is received.
> 
> This solution buffers all data read by the curl library, in order to allow
> it to rewind the reading buffer at any time later.

NAK.


In the case of git-upload-pack requests, we should fit into 1 MiB
almost all of the time, and thus not need to grow the http.postBuffer
to support a rewind.  The state data plus current have list isn't
all that large.  A 1 MiB request means we have over 20,900 commits
in common with the remote and still haven't been able to find a
sufficient cut point.  Or the remote has 20,000 active, unrelated
branches we are trying to fetch.  Either way, this is a really sick
and twisted situation.

In the case of git-receive-pack requests, we might be uploading an
entire project to an empty repository on the remote side.  This could
be 8 GiB worth of data if the project was something huge like KDE.
We can't assume that we should malloc 8 GiB of memory to buffer
the payload.

The *correct* way to support an arbitrary rewind is to modify the
outgoing channel from remote-curl to its protocol engine (client.in
within the rpc_service method) to somehow request the protocol engine
(aka git-send-pack or git-fetch-pack) to stop and regenerate the
current request.


Another approach would be to modify http-backend (and the protocol)
to support an "auth ping" request prior to spooling out the entire
payload if its more than an http.postBuffer size.  Basically we
do what the "Expect: 100-continue" protocol is supposed to do,
but in the application layer rather than the HTTP/1.1 layer, so
our CGI actually gets invoked.

This unfortunately still relies on the underlying libcurl to not
discard the authentication data after that initial "auth ping".
But to be honest, I think that is a reasonable expectation.  The
#@!*@!* library should be able to generate two requests back-to-back
to the same URL without needing to rewind the 2nd request.

-- 
Shawn.

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Shawn O. Pearce]

Tay Ray Chuan <rctay89@gmail.com> wrote:
> On Wed, Dec 2, 2009 at 12:01 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > In theory, since the cURL session stays active, we would have
> > received the 401 authentication error during the initial
> > "GET $GIT_DIR/info/refs?service=git-$service" request, and the subsequent
> > "POST $GIT_DIR/git-$service" requests would automatically include the
> > authentication data.
> >
> > That's theory. ?Reality doesn't always agree with my theories. ?:-)
> 
> that's because the curl session where the 401 was received (and thus
> successful authentication takes place) is closed.
> 
> I sent out a patch series recently which contains a patch to maintain
> at least one curl session throughout a http session (from http_init()
> to http_cleanup()), you can see this here:
> 
>   http://www.spinics.net/lists/git/msg118190.html

Right, this patch looked sane to me.  It didn't touch code I recently
have touched myself, so I didn't bother to ACK, but if it helps,
Acked-by: Shawn O. Pearce <spearce@spearce.org>.

-- 
Shawn.

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Martin Storsjö]

On Tue, 1 Dec 2009, Shawn O. Pearce wrote:

> Martin Storsj? <martin@martin.st> wrote:
> > When using multi-pass authentication methods, the curl library may need
> > to rewind the read buffers used for providing data to HTTP POST, if data
> > has been output before a 401 error is received.
> 
> In theory, since the cURL session stays active, we would have
> received the 401 authentication error during the initial
> "GET $GIT_DIR/info/refs?service=git-$service" request, and the subsequent
> "POST $GIT_DIR/git-$service" requests would automatically include the
> authentication data.
> 
> That's theory.  Reality doesn't always agree with my theories.  :-)

As Tay said - his "maintain curl sessions" patch should make this 
redundant in most cases. But in case request pattern gets changed or if 
the curl session for some other reason isn't able to authenticate on the 
first try, this is a quite non-intrusive way of ensuring that these 
requests can be restarted.

// Martin

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Martin Storsjö]

On Tue, 1 Dec 2009, Shawn O. Pearce wrote:

> In the case of git-receive-pack requests, we might be uploading an
> entire project to an empty repository on the remote side.  This could
> be 8 GiB worth of data if the project was something huge like KDE.
> We can't assume that we should malloc 8 GiB of memory to buffer
> the payload.

True, fair enough. This was mostly a proof of concept of how this could be 
implemented, but with these comments for you, it's clear that this isn't a 
feasible solution at all. There's no acute need for it either.

> The *correct* way to support an arbitrary rewind is to modify the
> outgoing channel from remote-curl to its protocol engine (client.in
> within the rpc_service method) to somehow request the protocol engine
> (aka git-send-pack or git-fetch-pack) to stop and regenerate the
> current request.

That's a good idea!

> Another approach would be to modify http-backend (and the protocol)
> to support an "auth ping" request prior to spooling out the entire
> payload if its more than an http.postBuffer size.  Basically we
> do what the "Expect: 100-continue" protocol is supposed to do,
> but in the application layer rather than the HTTP/1.1 layer, so
> our CGI actually gets invoked.

That's also quite a good idea, especially if it would be done in a way so 
that it's certain that the same curl session will be reused, instead of 
getting a potentially new curl session when using get_active_slot().

> This unfortunately still relies on the underlying libcurl to not
> discard the authentication data after that initial "auth ping".
> But to be honest, I think that is a reasonable expectation.  The
> #@!*@!* library should be able to generate two requests back-to-back
> to the same URL without needing to rewind the 2nd request.

Yeah, as long as the same curl session is preserved, this should be no 
problem.

// Martin

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Junio C Hamano]

Martin Storsjö <martin@martin.st> writes:

> As long as the current rpc read buffer is the first one, we're able to
> rewind without need for additional buffering.

... and if the current buffer isn't the first one, what do we do?

> +#ifndef NO_CURL_IOCTL
> +curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> +{
> +	struct rpc_state *rpc = clientp;
> +
> +	switch (cmd) {
> +	case CURLIOCMD_NOP:
> +		return CURLIOE_OK;
> +
> +	case CURLIOCMD_RESTARTREAD:
> +		if (rpc->initial_buffer) {
> +			rpc->pos = 0;
> +			return CURLIOE_OK;
> +		}
> +		fprintf(stderr, "Unable to rewind rpc post data - try increasing http.postBuffer\n");
> +		return CURLIOE_FAILRESTART;
> +
> +	default:
> +		return CURLIOE_UNKNOWNCMD;
> +	}
> +}
> +#endif

What will this result in?  A failed request, then the user increases
http.postBuffer, and re-runs the entire command?  I am not suggesting the
code should do it differently (e.g.  retry with a larger buffer without
having the user to help it).  At least not yet.  That is why my first
question above was "what do we do?" and not "what should we do?".

I am primarily interested in _documenting_ the expected user experience in
the failure case, so that people can notice the message, run "git grep" to
find the above line and then run "git blame" to find the commit to read
its log message to understand what is going on.

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Daniel Stenberg]

On Tue, 1 Dec 2009, Shawn O. Pearce wrote:

> The #@!*@!* library should be able to generate two requests back-to-back to 
> the same URL without needing to rewind the 2nd request.

If '#@!*@!*' is your pattern for matching libcurl or curl, then sure libcurl 
certainly has no problem at all to send as many requests you like 
back-to-back.

The rewinding business is only really necessary for multipass authentication 
when Expect: 100-continue doesn't work (and thus libcurl has started to send 
data that the server will discard and thus is needed to get sent again). And 
that's not something you can blame "the #@!*@!* library" for, but rather your 
server end and/or how HTTP is defined to work.

-- 

  / daniel.haxx.se

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Tay Ray Chuan]

Hi,

On Wed, Dec 2, 2009 at 2:18 AM, Daniel Stenberg <daniel@haxx.se> wrote:
> If '#@!*@!*' is your pattern for matching libcurl or curl, then sure libcurl
> certainly has no problem at all to send as many requests you like
> back-to-back.

I have a feeling Shawn's referring to the git http library on top of that. ;)

> The rewinding business is only really necessary for multipass authentication
> when Expect: 100-continue doesn't work (and thus libcurl has started to send
> data that the server will discard and thus is needed to get sent again). And
> that's not something you can blame "the #@!*@!* library" for, but rather
> your server end and/or how HTTP is defined to work.

According to Martin, Expect: 100-continue is not working due to libcurl.

I quote him:

Date: Tue, 1 Dec 2009 12:28:26 +0200 (EET)
Subject: Re: [PATCH 0/2] http: allow multi-pass authentication

On Tue, Dec 1, 2009 at 6:28 PM, Martin Storsjö <martin@martin.st> wrote:
> Normally, libcurl should add the Expect: 100-continue header
> automatically, but for some reason
> (http://article.gmane.org/gmane.comp.web.curl.library/25992) it doesn't,
> so that's probably why we're manually adding that header in
> remote-curl.c:371 at the moment. libcurl doesn't detect this at the moment
> (http://article.gmane.org/gmane.comp.web.curl.library/25991) so it won't
> wait for the 100 continue response before starting to send the body data.

But, again, don't read my blaming of libcurl for this 100 business as
a criticism of curl.

-- 
Cheers,
Ray Chuan

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Tay Ray Chuan]

Hi,

On Wed, Dec 2, 2009 at 1:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ... and if the current buffer isn't the first one, what do we do?
> [snip]
> What will this result in?  A failed request, then the user increases
> http.postBuffer, and re-runs the entire command?  I am not suggesting the
> code should do it differently (e.g.  retry with a larger buffer without
> having the user to help it).  At least not yet.  That is why my first
> question above was "what do we do?" and not "what should we do?".

I guess that by "we" you're referring to the "normal" users of git?

> I am primarily interested in _documenting_ the expected user experience in
> the failure case, so that people can notice the message, run "git grep" to
> find the above line and then run "git blame" to find the commit to read
> its log message to understand what is going on.

Yes, the code will just fail. As you might suspect, the code won't
attempt to mitigate the failure by doing anything, and would require
intervention on the part of the user.

What the user could do to make this work:

1. Turn off multi-pass authentication and just go with Basic.

2. Allow for persistent curl sessions. In theory, we get a 401 the
first time when we send a GET for info/refs; subsequently, curl knows
what authentication to use, so the POST request *should* take place
without the need for rewinding. In theory.

3. Increase http.postBuffer size in the config.

-- 
Cheers,
Ray Chuan

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Tay Ray Chuan]

Hi,

On Wed, Dec 2, 2009 at 12:59 AM, Martin Storsjö <martin@martin.st> wrote:
> On Tue, 1 Dec 2009, Shawn O. Pearce wrote:
>> The *correct* way to support an arbitrary rewind is to modify the
>> outgoing channel from remote-curl to its protocol engine (client.in
>> within the rpc_service method) to somehow request the protocol engine
>> (aka git-send-pack or git-fetch-pack) to stop and regenerate the
>> current request.
>
> That's a good idea!
>
>> Another approach would be to modify http-backend (and the protocol)
>> to support an "auth ping" request prior to spooling out the entire
>> payload if its more than an http.postBuffer size.  Basically we
>> do what the "Expect: 100-continue" protocol is supposed to do,
>> but in the application layer rather than the HTTP/1.1 layer, so
>> our CGI actually gets invoked.
>
> That's also quite a good idea, especially if it would be done in a way so
> that it's certain that the same curl session will be reused, instead of
> getting a potentially new curl session when using get_active_slot().

I think restarting the read by killing the protocol engine/client and
starting again would be the easier of the two.

Not just that, it would be neater than storing everything that the
protocol engine has spewed out, like Martin's patch does.

-- 
Cheers,
Ray Chuan

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer

[Martin Storsjö]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2715 bytes --]

On Wed, 2 Dec 2009, Tay Ray Chuan wrote:

> > What will this result in?  A failed request, then the user increases
> > http.postBuffer, and re-runs the entire command?  I am not suggesting the
> > code should do it differently (e.g.  retry with a larger buffer without
> > having the user to help it).  At least not yet.  That is why my first
> > question above was "what do we do?" and not "what should we do?".
> 
> I guess that by "we" you're referring to the "normal" users of git?
> 
> > I am primarily interested in _documenting_ the expected user experience in
> > the failure case, so that people can notice the message, run "git grep" to
> > find the above line and then run "git blame" to find the commit to read
> > its log message to understand what is going on.
> 
> Yes, the code will just fail. As you might suspect, the code won't
> attempt to mitigate the failure by doing anything, and would require
> intervention on the part of the user.
> 
> What the user could do to make this work:
> 
> 1. Turn off multi-pass authentication and just go with Basic.
> 
> 2. Allow for persistent curl sessions. In theory, we get a 401 the
> first time when we send a GET for info/refs; subsequently, curl knows
> what authentication to use, so the POST request *should* take place
> without the need for rewinding. In theory.

I'd actually put this as number 1 - if this error message pops up for some 
reason, the first thing would be to find out why reusing the previous curl 
sessions didn't work.

Other options are:

- Switch to a HTTP server that handles Expect: 100-continue properly
- Try pushing the data in smaller chunks, e.g. if populating a new repo 
from scratch, don't push the whole history in one single run, or populate 
through some other mechanism and just do the incremental pushs over HTTP.

And possibly: Update curl to a version post 7.19.7, which detects the 
Expect header set by git and tries to await a response from the server 
before proceeding. (The problem that would solve is if we start sending 
and manage to send the whole initial 1 MB buffer before the 401 reply from 
the server is received. But it doesn't solve the case if the server 
doesn't understand the Expect header at all.)

> 3. Increase http.postBuffer size in the config.

As Shawn pointed out, if the whole request would have to be buffered, the 
needed size may be prohibitively large, so I guess this isn't a good hint 
to include in the error message after all. But if the request is sensibly 
sized (e.g. on the order of tens of MBs), this may be a stopgap solution.


So, should we change the error message to something a bit more 
descriptive, and add this discussion into the commit message?

// Martin

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Daniel Stenberg]

On Wed, 2 Dec 2009, Tay Ray Chuan wrote:

> According to Martin, Expect: 100-continue is not working due to libcurl.

Right, that is/was a bug in how libcurl behaves when the application itself 
has set the "Expect: 100-continue" header. Martin has provided a fix for that 
for the next libcurl version though, but that won't make a lot of existing 
users happy.

Thinking about this particular problem, what is the motivation for git to 
forcily add that header in the first place? I mean, libcurl does add the 
header by itself when it thinks it is necessary and then it handles it 
correctly.

I'm just suggesting (and speculating widely since I don't know git internals) 
that a possible way to work around this particular bug may be to reconsider 
how git adds the Expect header.

It's just an idea. Please ignore it if it is totally crazy.

-- 

  / daniel.haxx.se

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Martin Storsjö]

On Wed, 2 Dec 2009, Daniel Stenberg wrote:

> On Wed, 2 Dec 2009, Tay Ray Chuan wrote:
> 
> > According to Martin, Expect: 100-continue is not working due to libcurl.
> 
> Right, that is/was a bug in how libcurl behaves when the application itself
> has set the "Expect: 100-continue" header. Martin has provided a fix for that
> for the next libcurl version though, but that won't make a lot of existing
> users happy.
> 
> Thinking about this particular problem, what is the motivation for git to
> forcily add that header in the first place? I mean, libcurl does add the
> header by itself when it thinks it is necessary and then it handles it
> correctly.

As far as I saw, the reason for it being manually added is that curl 
actually didn't add it automatically in that case. That was the reason for 
the second patch/rfc thread that I sent to curl-library (where postsize == 
0, as in unknown, didn't trigger the addition of any Expect header).

// Martin

Re: [PATCH/RFC] Allow curl to rewind the RPC read buffer at any time

[Daniel Stenberg]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 692 bytes --]

On Wed, 2 Dec 2009, Martin Storsjö wrote:

>> Thinking about this particular problem, what is the motivation for git to 
>> forcily add that header in the first place? I mean, libcurl does add the 
>> header by itself when it thinks it is necessary and then it handles it 
>> correctly.
>
> As far as I saw, the reason for it being manually added is that curl 
> actually didn't add it automatically in that case. That was the reason for 
> the second patch/rfc thread that I sent to curl-library (where postsize == 
> 0, as in unknown, didn't trigger the addition of any Expect header).

Ah right, thanks for the clarification. An unfortunate combination then... :-(

-- 

  / daniel.haxx.se