[PATCH v4] remote-curl: fix large pushes with GSSAPI

[PATCH v4] remote-curl: fix large pushes with GSSAPI

[brian m. carlson]

Due to an interaction between the way libcurl handles GSSAPI authentication over
HTTP and the way git uses libcurl, large pushes (those over http.postBuffer
bytes) would fail due to an authentication failure requiring a rewind of the
curl buffer.  Such a rewind was not possible because the data did not fit into
the entire buffer.

Enable the use of the Expect: 100-continue header for large requests where the
server offers GSSAPI authentication to avoid this issue, since the request would
otherwise fail.  This allows git to get the authentication data right before
sending the pack contents.  Existing cases where pushes would succeed, including
small requests using GSSAPI, still disable the use of 100 Continue, as it causes
problems for some remote HTTP implementations (servers and proxies).

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 remote-curl.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index c9b891a..2c276de 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -449,6 +449,7 @@ static int post_rpc(struct rpc_state *rpc)
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
 	int err, large_request = 0;
+	int needs_100_continue = 0;
 
 	/* 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
@@ -472,6 +473,8 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
+		long authtype = 0;
+
 		do {
 			err = probe_rpc(rpc);
 			if (err == HTTP_REAUTH)
@@ -479,11 +482,19 @@ static int post_rpc(struct rpc_state *rpc)
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
+
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		slot = get_active_slot();
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
+		if (authtype & CURLAUTH_GSSNEGOTIATE)
+			needs_100_continue = 1;
+#endif
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+	headers = curl_slist_append(headers, needs_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();
-- 
1.8.4.1.635.g55556a5

Re: [PATCH v4] remote-curl: fix large pushes with GSSAPI

[Jeff King]

On Tue, Oct 29, 2013 at 02:36:37AM +0000, brian m. carlson wrote:

>  	if (large_request) {
> +		long authtype = 0;
> +

Minor nit, but this will produce an unused variable warning if the code
in the #if below doesn't get compiled. I don't know how much we care.

> +#if LIBCURL_VERSION_NUM >= 0x070a08
> +		slot = get_active_slot();
> +		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
> +		if (authtype & CURLAUTH_GSSNEGOTIATE)
> +			needs_100_continue = 1;
> +#endif

I didn't notice this in the last round, but this is somewhat of an abuse
of get_active_slot. It is intended to give you a new, pristine curl slot
that you can use to make a request, and it is yours until you call
finish_active_slot. You're not supposed to look at it after that.

However, we do reuse the curl handles. And in the case of rpc case, we
are only doing one request at a time, so the handle you get is
guaranteed to be the last one used.  So it works in practice, but it
would break if the curl handle code breaks any of these assumptions.

I think the clean way to do it would be to teach the slot code to pull
out the available auth methods, and pass them up through the call chain.
Like this on top of your patch:

diff --git a/http.c b/http.c
index 0ddb164..32fa998 100644
--- a/http.c
+++ b/http.c
@@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+				  &slot->results->authtype);
+#else
+		slot->results->authtype = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */
diff --git a/http.h b/http.h
index d77c1b5..4b32b9b 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
+	long authtype;
 };
 
 struct active_request_slot {
diff --git a/remote-curl.c b/remote-curl.c
index eaa286c..d026f05 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	return size;
 }
 
-static int run_slot(struct active_request_slot *slot)
+static int run_slot(struct active_request_slot *slot,
+		    struct slot_results *results)
 {
 	int err;
-	struct slot_results results;
+	struct slot_results results_buf;
 
-	slot->results = &results;
+	if (!results)
+		results = &results_buf;
+
+	slot->results = results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	err = handle_curl_result(&results);
+	err = handle_curl_result(results);
 	if (err != HTTP_OK && err != HTTP_REAUTH) {
 		error("RPC failed; result=%d, HTTP code = %ld",
-		      results.curl_result, results.http_code);
+		      results->curl_result, results->http_code);
 	}
 
 	return err;
 }
 
-static int probe_rpc(struct rpc_state *rpc)
+static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
 	struct curl_slist *headers = NULL;
@@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
-	err = run_slot(slot);
+	err = run_slot(slot, results);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
@@ -462,20 +466,16 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
-		long authtype = 0;
+		struct slot_results results;
 
 		do {
-			err = probe_rpc(rpc);
+			err = probe_rpc(rpc, &results);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
 
-#if LIBCURL_VERSION_NUM >= 0x070a08
-		slot = get_active_slot();
-		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
-		if (authtype & CURLAUTH_GSSNEGOTIATE)
+		if (results.authtype & CURLAUTH_GSSNEGOTIATE)
 			needs_100_continue = 1;
-#endif
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
@@ -572,7 +572,7 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request)
 		goto retry;
 	if (err != HTTP_OK)

That's note tested beyond compiling, but I think it should work. Feel
free to squash it into your patch, or if you'd like, I can split out the
refactoring steps with a commit message for you.

-Peff

Re: [PATCH v4] remote-curl: fix large pushes with GSSAPI

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 4457 bytes --]

On Wed, Oct 30, 2013 at 04:45:10AM -0400, Jeff King wrote:
> However, we do reuse the curl handles. And in the case of rpc case, we
> are only doing one request at a time, so the handle you get is
> guaranteed to be the last one used.  So it works in practice, but it
> would break if the curl handle code breaks any of these assumptions.
> 
> I think the clean way to do it would be to teach the slot code to pull
> out the available auth methods, and pass them up through the call chain.
> Like this on top of your patch:
> 
> diff --git a/http.c b/http.c
> index 0ddb164..32fa998 100644
> --- a/http.c
> +++ b/http.c
> @@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
>  	if (slot->results != NULL) {
>  		slot->results->curl_result = slot->curl_result;
>  		slot->results->http_code = slot->http_code;
> +#if LIBCURL_VERSION_NUM >= 0x070a08
> +		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
> +				  &slot->results->authtype);
> +#else
> +		slot->results->authtype = 0;
> +#endif
>  	}
>  
>  	/* Run callback if appropriate */
> diff --git a/http.h b/http.h
> index d77c1b5..4b32b9b 100644
> --- a/http.h
> +++ b/http.h
> @@ -54,6 +54,7 @@
>  struct slot_results {
>  	CURLcode curl_result;
>  	long http_code;
> +	long authtype;
>  };
>  
>  struct active_request_slot {
> diff --git a/remote-curl.c b/remote-curl.c
> index eaa286c..d026f05 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
>  	return size;
>  }
>  
> -static int run_slot(struct active_request_slot *slot)
> +static int run_slot(struct active_request_slot *slot,
> +		    struct slot_results *results)
>  {
>  	int err;
> -	struct slot_results results;
> +	struct slot_results results_buf;
>  
> -	slot->results = &results;
> +	if (!results)
> +		results = &results_buf;
> +
> +	slot->results = results;
>  	slot->curl_result = curl_easy_perform(slot->curl);
>  	finish_active_slot(slot);
>  
> -	err = handle_curl_result(&results);
> +	err = handle_curl_result(results);
>  	if (err != HTTP_OK && err != HTTP_REAUTH) {
>  		error("RPC failed; result=%d, HTTP code = %ld",
> -		      results.curl_result, results.http_code);
> +		      results->curl_result, results->http_code);
>  	}
>  
>  	return err;
>  }
>  
> -static int probe_rpc(struct rpc_state *rpc)
> +static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
>  {
>  	struct active_request_slot *slot;
>  	struct curl_slist *headers = NULL;
> @@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
>  	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
>  
> -	err = run_slot(slot);
> +	err = run_slot(slot, results);
>  
>  	curl_slist_free_all(headers);
>  	strbuf_release(&buf);
> @@ -462,20 +466,16 @@ static int post_rpc(struct rpc_state *rpc)
>  	}
>  
>  	if (large_request) {
> -		long authtype = 0;
> +		struct slot_results results;
>  
>  		do {
> -			err = probe_rpc(rpc);
> +			err = probe_rpc(rpc, &results);
>  		} while (err == HTTP_REAUTH);
>  		if (err != HTTP_OK)
>  			return -1;
>  
> -#if LIBCURL_VERSION_NUM >= 0x070a08
> -		slot = get_active_slot();
> -		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL, &authtype);
> -		if (authtype & CURLAUTH_GSSNEGOTIATE)
> +		if (results.authtype & CURLAUTH_GSSNEGOTIATE)
>  			needs_100_continue = 1;
> -#endif
>  	}
>  
>  	headers = curl_slist_append(headers, rpc->hdr_content_type);
> @@ -572,7 +572,7 @@ retry:
>  	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>  	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
>  
> -	err = run_slot(slot);
> +	err = run_slot(slot, NULL);
>  	if (err == HTTP_REAUTH && !large_request)
>  		goto retry;
>  	if (err != HTTP_OK)
> 
> That's note tested beyond compiling, but I think it should work. Feel
> free to squash it into your patch, or if you'd like, I can split out the
> refactoring steps with a commit message for you.

If you would split it out, that would be great.  Then I'll simply rebase
my patch on top of yours and go from there.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] remote-curl: fix large pushes with GSSAPI

[Jeff King]

On Wed, Oct 30, 2013 at 10:40:30PM +0000, brian m. carlson wrote:

> If you would split it out, that would be great.  Then I'll simply rebase
> my patch on top of yours and go from there.

I just included your patch on top, since it was the residue left over
after committing my refactoring. Please read over the result to make
sure I am not defaming you. :)

I noticed while committing the first patch that we do not actually
follow the "do not look at curl after finish_active_slot" rule for the
content-type. Again, we get away with it because we are not running
multiple slots at the time (we only check content-type during the
initial discovery).

I think the refactoring here is the cleanest thing by the existing
rules, but I also think we could get away with the somewhat simpler
patch of just teaching probe_rpc to grab the AUTHAVAIL (because it still
has the old slot and does not need to call get_active_slot again, and
because we know we are only using a single slot).

Going through all of this, I can't help but be annoyed at how much http
baggage we are carrying around for the curl_multi code for parallel
fetches, which is only used for dumb http. The smart-http code would be
happy with a single curl handle we used each time. But I imagine there
are still people relying on dumb http, and dropping the parallel fetch
would be a pretty severe regression for them.

  [1/3]: http: return curl's AUTHAVAIL via slot_results
  [2/3]: remote-curl: pass curl slot_results back through run_slot
  [3/3]: remote-curl: fix large pushes with GSSAPI

-Peff

[PATCH 1/3] http: return curl's AUTHAVAIL via slot_results

[Jeff King]

Callers of the http code may want to know which auth types
were available for the previous request. But after finishing
with the curl slot, they are not supposed to look at the
curl handle again. We already handle returning other
information via the slot_results struct; let's add a flag to
check the available auth.

Note that older versions of curl did not support this, so we
simply return 0 (something like "-1" would be worse, as the
value is a bitflag and we might accidentally set a flag).
This is sufficient for the callers planned in this series,
who only trigger some optional behavior if particular bits
are set, and can live with a fake "no bits" answer.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 6 ++++++
 http.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/http.c b/http.c
index 0ddb164..5c51865 100644
--- a/http.c
+++ b/http.c
@@ -761,6 +761,12 @@ void finish_active_slot(struct active_request_slot *slot)
 	if (slot->results != NULL) {
 		slot->results->curl_result = slot->curl_result;
 		slot->results->http_code = slot->http_code;
+#if LIBCURL_VERSION_NUM >= 0x070a08
+		curl_easy_getinfo(slot->curl, CURLINFO_HTTPAUTH_AVAIL,
+				  &slot->results->auth_avail);
+#else
+		slot->results->auth_avail = 0;
+#endif
 	}
 
 	/* Run callback if appropriate */
diff --git a/http.h b/http.h
index d77c1b5..81d4843 100644
--- a/http.h
+++ b/http.h
@@ -54,6 +54,7 @@
 struct slot_results {
 	CURLcode curl_result;
 	long http_code;
+	long auth_avail;
 };
 
 struct active_request_slot {
-- 
1.8.4.1.898.g8bf8a41.dirty

[PATCH 2/3] remote-curl: pass curl slot_results back through run_slot

[Jeff King]

Some callers may want to know more than just the integer
error code we return. Let them optionally pass a
slot_results struct to fill in (or NULL if they do not
care). In either case we continue to return the integer
code.

We can also give probe_rpc the same treatment (since it
builds directly on run_slot).

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..79db21e 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -383,25 +383,29 @@ static size_t rpc_in(char *ptr, size_t eltsize,
 	return size;
 }
 
-static int run_slot(struct active_request_slot *slot)
+static int run_slot(struct active_request_slot *slot,
+		    struct slot_results *results)
 {
 	int err;
-	struct slot_results results;
+	struct slot_results results_buf;
 
-	slot->results = &results;
+	if (!results)
+		results = &results_buf;
+
+	slot->results = results;
 	slot->curl_result = curl_easy_perform(slot->curl);
 	finish_active_slot(slot);
 
-	err = handle_curl_result(&results);
+	err = handle_curl_result(results);
 	if (err != HTTP_OK && err != HTTP_REAUTH) {
 		error("RPC failed; result=%d, HTTP code = %ld",
-		      results.curl_result, results.http_code);
+		      results->curl_result, results->http_code);
 	}
 
 	return err;
 }
 
-static int probe_rpc(struct rpc_state *rpc)
+static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
 	struct curl_slist *headers = NULL;
@@ -423,7 +427,7 @@ static int probe_rpc(struct rpc_state *rpc)
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, &buf);
 
-	err = run_slot(slot);
+	err = run_slot(slot, results);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
@@ -462,7 +466,7 @@ static int post_rpc(struct rpc_state *rpc)
 
 	if (large_request) {
 		do {
-			err = probe_rpc(rpc);
+			err = probe_rpc(rpc, NULL);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
@@ -561,7 +565,7 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
-	err = run_slot(slot);
+	err = run_slot(slot, NULL);
 	if (err == HTTP_REAUTH && !large_request)
 		goto retry;
 	if (err != HTTP_OK)
-- 
1.8.4.1.898.g8bf8a41.dirty

[PATCH 3/3] remote-curl: fix large pushes with GSSAPI

[Jeff King]

From: brian m. carlson <sandals@crustytoothpaste.net>

Due to an interaction between the way libcurl handles GSSAPI
authentication over HTTP and the way git uses libcurl, large
pushes (those over http.postBuffer bytes) would fail due to
an authentication failure requiring a rewind of the curl
buffer.  Such a rewind was not possible because the data did
not fit into the entire buffer.

Enable the use of the Expect: 100-continue header for large
requests where the server offers GSSAPI authentication to
avoid this issue, since the request would otherwise fail.
This allows git to get the authentication data right before
sending the pack contents.  Existing cases where pushes
would succeed, including small requests using GSSAPI, still
disable the use of 100 Continue, as it causes problems for
some remote HTTP implementations (servers and proxies).

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 79db21e..f646b5f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -442,6 +442,7 @@ static int post_rpc(struct rpc_state *rpc)
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
 	int err, large_request = 0;
+	int needs_100_continue = 0;
 
 	/* 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
@@ -465,16 +466,22 @@ static int post_rpc(struct rpc_state *rpc)
 	}
 
 	if (large_request) {
+		struct slot_results results;
+
 		do {
-			err = probe_rpc(rpc, NULL);
+			err = probe_rpc(rpc, &results);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
+
+		if (results.auth_avail & CURLAUTH_GSSNEGOTIATE)
+			needs_100_continue = 1;
 	}
 
 	headers = curl_slist_append(headers, rpc->hdr_content_type);
 	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, "Expect:");
+	headers = curl_slist_append(headers, needs_100_continue ?
+		"Expect: 100-continue" : "Expect:");
 
 retry:
 	slot = get_active_slot();
-- 
1.8.4.1.898.g8bf8a41.dirty

[BUG?] "error: cache entry has null sha1"

[Junio C Hamano]

This is totally unrelated to this thread, but I am seeing a strange
error from the "let's make sure we won't write null sha1" code we
added recently.  This reproduces reliably for me, even in a freshly
cloned repository without previous rerere records:

 - Check out v1.8.4.1

   $ git checkout v1.8.4.1^0

   at this point your HEAD should be detached.

 - Apply the first patch from this series

   $ git am -3 20131031063530.GA5812@sigill.intra.peff.net

 - Apply the second patch from this series

   $ git am -3 20131031063626.GB5812@sigill.intra.peff.net

   Up to this point things should advance smoothly

 - Attempt to apply the second patch _again_.

   $ git am -3 20131031063626.GB5812@sigill.intra.peff.net

   This should detect a no-op, but somehow leaves things in a
   conflicted state.  I haven't looked into what is wrong, but this
   message is not about this failure.

 - Try to discard

   $ git am --abort
   error: cache entry has null sha1: remote-curl.c
   fatal: unable to write new index file

   This should not happen, no?

"git reset --hard" will remove the funnies, but still...

Re: [BUG?] "error: cache entry has null sha1"

[Jeff King]

On Thu, Oct 31, 2013 at 10:11:49AM -0700, Junio C Hamano wrote:

>  - Try to discard
> 
>    $ git am --abort
>    error: cache entry has null sha1: remote-curl.c
>    fatal: unable to write new index file
> 
>    This should not happen, no?
> 
> "git reset --hard" will remove the funnies, but still...

I ran into this recently, too. Isn't it just the twoway_merge bug we
were discussing here:

  http://thread.gmane.org/gmane.comp.version-control.git/202440/focus=212316

I don't think we ever actually applied a fix.

-Peff

Re: [BUG?] "error: cache entry has null sha1"

[Jeff King]

On Thu, Oct 31, 2013 at 01:15:39PM -0400, Jeff King wrote:

> On Thu, Oct 31, 2013 at 10:11:49AM -0700, Junio C Hamano wrote:
> 
> >  - Try to discard
> > 
> >    $ git am --abort
> >    error: cache entry has null sha1: remote-curl.c
> >    fatal: unable to write new index file
> > 
> >    This should not happen, no?
> > 
> > "git reset --hard" will remove the funnies, but still...
> 
> I ran into this recently, too. Isn't it just the twoway_merge bug we
> were discussing here:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/202440/focus=212316
> 
> I don't think we ever actually applied a fix.

Also, the followup:

  http://thread.gmane.org/gmane.comp.version-control.git/217172

-Peff

Re: [BUG?] "error: cache entry has null sha1"

[Junio C Hamano]

Thanks for the pointers.

Re: [PATCH 3/3] remote-curl: fix large pushes with GSSAPI

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Thu, Oct 31, 2013 at 02:36:51AM -0400, Jeff King wrote:
> From: brian m. carlson <sandals@crustytoothpaste.net>
> 
> Due to an interaction between the way libcurl handles GSSAPI
> authentication over HTTP and the way git uses libcurl, large
> pushes (those over http.postBuffer bytes) would fail due to
> an authentication failure requiring a rewind of the curl
> buffer.  Such a rewind was not possible because the data did
> not fit into the entire buffer.
> 
> Enable the use of the Expect: 100-continue header for large
> requests where the server offers GSSAPI authentication to
> avoid this issue, since the request would otherwise fail.
> This allows git to get the authentication data right before
> sending the pack contents.  Existing cases where pushes
> would succeed, including small requests using GSSAPI, still
> disable the use of 100 Continue, as it causes problems for
> some remote HTTP implementations (servers and proxies).
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Jeff King <peff@peff.net>

The entire series looks fine by me.  Thanks for fixing this up.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re* [BUG?] "error: cache entry has null sha1"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Oct 31, 2013 at 01:15:39PM -0400, Jeff King wrote:
>
>> On Thu, Oct 31, 2013 at 10:11:49AM -0700, Junio C Hamano wrote:
>> 
>> >  - Try to discard
>> > 
>> >    $ git am --abort
>> >    error: cache entry has null sha1: remote-curl.c
>> >    fatal: unable to write new index file
>> > 
>> >    This should not happen, no?
>> > 
>> > "git reset --hard" will remove the funnies, but still...
>> 
>> I ran into this recently, too. Isn't it just the twoway_merge bug we
>> were discussing here:
>> 
>>   http://thread.gmane.org/gmane.comp.version-control.git/202440/focus=212316
>> 
>> I don't think we ever actually applied a fix.
>
> Also, the followup:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/217172

OK, so it appears that we sort of agreed that the pieces of patches
in the thread was good at $gmane/217300, but somehow I ended up
veering into a tangent from there and forgot about the topic X-<?

Here is a proposed endgame for the topic in a patch form, then.

I've added a test for low-level "read-tree --reset -u A B", and
tried the "am --abort" I saw the problem with manually, but other
than that, I haven't (re)thought about the issue hard enough to be
comfortable with this change yet.

To be applied on top of c479d14, but the result should be mergeable
cleanly all the way up to 'pu'.

Jeff King (1):
  unpack-trees: fix "read-tree -u --reset A B" with conflicted index

Junio C Hamano (2):
  t1005: reindent
  t1005: add test for "read-tree --reset -u A B"

 t/lib-read-tree.sh         |  52 ++++++++---------
 t/t1005-read-tree-reset.sh | 141 +++++++++++++++++++++++++--------------------
 unpack-trees.c             |  25 +++++---
 3 files changed, 121 insertions(+), 97 deletions(-)

-- 
1.8.5-rc0-281-g8951339

[PATCH 1/3] unpack-trees: fix "read-tree -u --reset A B" with conflicted index

[Junio C Hamano]

From: Jeff King <peff@peff.net>

When we call "read-tree --reset -u HEAD ORIG_HEAD", the first thing we
do with the index is to call read_cache_unmerged.  Originally that
would read the index, leaving aside any unmerged entries.  However, as
of d1a43f2 (reset --hard/read-tree --reset -u: remove unmerged new
paths, 2008-10-15), it actually creates a new cache entry to serve as
a placeholder, so that we later know to update the working tree.

However, we later noticed that the sha1 of that unmerged entry was
just copied from some higher stage, leaving you with random content in
the index.  That was fixed by e11d7b5 ("reset --merge": fix unmerged
case, 2009-12-31), which instead puts the null sha1 into the newly
created entry, and sets a CE_CONFLICTED flag. At the same time, it
teaches the unpack-trees machinery to pay attention to this flag, so
that oneway_merge throws away the current value.

However, it did not update the code paths for twoway_merge, which is
where we end up in the two-way read-tree with --reset. We notice that
the HEAD and ORIG_HEAD versions are the same, and say "oh, we can just
reuse the current version". But that's not true. The current version
is bogus.

Notice this case and make sure we do not keep the bogus entry; either
we do not have that path in the tree we are moving to (i.e. remove
it), or we want to have the cache entry we created for the tree we are
moving to (i.e. resolve by explicitly saying the "newtree" version is
what we want).

[jc: this is from the almost year-old $gmane/212316; the credit goes
to Peff, but we need his sign-off]

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 unpack-trees.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 7c9ecf6..bf978e1 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1728,14 +1728,23 @@ int twoway_merge(struct cache_entry **src, struct unpack_trees_options *o)
 		newtree = NULL;
 
 	if (current) {
-		if ((!oldtree && !newtree) || /* 4 and 5 */
-		    (!oldtree && newtree &&
-		     same(current, newtree)) || /* 6 and 7 */
-		    (oldtree && newtree &&
-		     same(oldtree, newtree)) || /* 14 and 15 */
-		    (oldtree && newtree &&
-		     !same(oldtree, newtree) && /* 18 and 19 */
-		     same(current, newtree))) {
+		if (current->ce_flags & CE_CONFLICTED) {
+			if (same(oldtree, newtree) || o->reset) {
+				if (!newtree)
+					return deleted_entry(current, current, o);
+				else
+					return merged_entry(newtree, current, o);
+			}
+			return o->gently ? -1 : reject_merge(current, o);
+		}
+		else if ((!oldtree && !newtree) || /* 4 and 5 */
+			 (!oldtree && newtree &&
+			  same(current, newtree)) || /* 6 and 7 */
+			 (oldtree && newtree &&
+			  same(oldtree, newtree)) || /* 14 and 15 */
+			 (oldtree && newtree &&
+			  !same(oldtree, newtree) && /* 18 and 19 */
+			  same(current, newtree))) {
 			return keep_entry(current, o);
 		}
 		else if (oldtree && !newtree && same(current, oldtree)) {
-- 
1.8.5-rc0-281-g8951339

[PATCH 2/3] t1005: reindent

[Junio C Hamano]

Just to update the style of this ancient test script to match
our house style.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-read-tree.sh         |  52 +++++++++----------
 t/t1005-read-tree-reset.sh | 126 ++++++++++++++++++++++-----------------------
 2 files changed, 89 insertions(+), 89 deletions(-)

diff --git a/t/lib-read-tree.sh b/t/lib-read-tree.sh
index abc2c6f..ef079af 100644
--- a/t/lib-read-tree.sh
+++ b/t/lib-read-tree.sh
@@ -5,39 +5,39 @@
 # write the index and that together with -u it doesn't touch the work tree.
 #
 read_tree_must_succeed () {
-    git ls-files -s >pre-dry-run &&
-    git read-tree -n "$@" &&
-    git ls-files -s >post-dry-run &&
-    test_cmp pre-dry-run post-dry-run &&
-    git read-tree "$@"
+	git ls-files -s >pre-dry-run &&
+	git read-tree -n "$@" &&
+	git ls-files -s >post-dry-run &&
+	test_cmp pre-dry-run post-dry-run &&
+	git read-tree "$@"
 }
 
 read_tree_must_fail () {
-    git ls-files -s >pre-dry-run &&
-    test_must_fail git read-tree -n "$@" &&
-    git ls-files -s >post-dry-run &&
-    test_cmp pre-dry-run post-dry-run &&
-    test_must_fail git read-tree "$@"
+	git ls-files -s >pre-dry-run &&
+	test_must_fail git read-tree -n "$@" &&
+	git ls-files -s >post-dry-run &&
+	test_cmp pre-dry-run post-dry-run &&
+	test_must_fail git read-tree "$@"
 }
 
 read_tree_u_must_succeed () {
-    git ls-files -s >pre-dry-run &&
-    git diff-files -p >pre-dry-run-wt &&
-    git read-tree -n "$@" &&
-    git ls-files -s >post-dry-run &&
-    git diff-files -p >post-dry-run-wt &&
-    test_cmp pre-dry-run post-dry-run &&
-    test_cmp pre-dry-run-wt post-dry-run-wt &&
-    git read-tree "$@"
+	git ls-files -s >pre-dry-run &&
+	git diff-files -p >pre-dry-run-wt &&
+	git read-tree -n "$@" &&
+	git ls-files -s >post-dry-run &&
+	git diff-files -p >post-dry-run-wt &&
+	test_cmp pre-dry-run post-dry-run &&
+	test_cmp pre-dry-run-wt post-dry-run-wt &&
+	git read-tree "$@"
 }
 
 read_tree_u_must_fail () {
-    git ls-files -s >pre-dry-run &&
-    git diff-files -p >pre-dry-run-wt &&
-    test_must_fail git read-tree -n "$@" &&
-    git ls-files -s >post-dry-run &&
-    git diff-files -p >post-dry-run-wt &&
-    test_cmp pre-dry-run post-dry-run &&
-    test_cmp pre-dry-run-wt post-dry-run-wt &&
-    test_must_fail git read-tree "$@"
+	git ls-files -s >pre-dry-run &&
+	git diff-files -p >pre-dry-run-wt &&
+	test_must_fail git read-tree -n "$@" &&
+	git ls-files -s >post-dry-run &&
+	git diff-files -p >post-dry-run-wt &&
+	test_cmp pre-dry-run post-dry-run &&
+	test_cmp pre-dry-run-wt post-dry-run-wt &&
+	test_must_fail git read-tree "$@"
 }
diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index f53de79..e29cf63 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -8,84 +8,84 @@ test_description='read-tree -u --reset'
 # two-tree test
 
 test_expect_success 'setup' '
-  git init &&
-  mkdir df &&
-  echo content >df/file &&
-  git add df/file &&
-  git commit -m one &&
-  git ls-files >expect &&
-  rm -rf df &&
-  echo content >df &&
-  git add df &&
-  echo content >new &&
-  git add new &&
-  git commit -m two
+	git init &&
+	mkdir df &&
+	echo content >df/file &&
+	git add df/file &&
+	git commit -m one &&
+	git ls-files >expect &&
+	rm -rf df &&
+	echo content >df &&
+	git add df &&
+	echo content >new &&
+	git add new &&
+	git commit -m two
 '
 
 test_expect_success 'reset should work' '
-  read_tree_u_must_succeed -u --reset HEAD^ &&
-  git ls-files >actual &&
-  test_cmp expect actual
+	read_tree_u_must_succeed -u --reset HEAD^ &&
+	git ls-files >actual &&
+	test_cmp expect actual
 '
 
 test_expect_success 'reset should remove remnants from a failed merge' '
-  read_tree_u_must_succeed --reset -u HEAD &&
-  git ls-files -s >expect &&
-  sha1=$(git rev-parse :new) &&
-  (
-	echo "100644 $sha1 1	old"
-	echo "100644 $sha1 3	old"
-  ) | git update-index --index-info &&
-  >old &&
-  git ls-files -s &&
-  read_tree_u_must_succeed --reset -u HEAD &&
-  git ls-files -s >actual &&
-  ! test -f old
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >expect &&
+	sha1=$(git rev-parse :new) &&
+	(
+		echo "100644 $sha1 1	old"
+		echo "100644 $sha1 3	old"
+	) | git update-index --index-info &&
+	>old &&
+	git ls-files -s &&
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >actual &&
+	! test -f old
 '
 
 test_expect_success 'Porcelain reset should remove remnants too' '
-  read_tree_u_must_succeed --reset -u HEAD &&
-  git ls-files -s >expect &&
-  sha1=$(git rev-parse :new) &&
-  (
-	echo "100644 $sha1 1	old"
-	echo "100644 $sha1 3	old"
-  ) | git update-index --index-info &&
-  >old &&
-  git ls-files -s &&
-  git reset --hard &&
-  git ls-files -s >actual &&
-  ! test -f old
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >expect &&
+	sha1=$(git rev-parse :new) &&
+	(
+		echo "100644 $sha1 1	old"
+		echo "100644 $sha1 3	old"
+	) | git update-index --index-info &&
+	>old &&
+	git ls-files -s &&
+	git reset --hard &&
+	git ls-files -s >actual &&
+	! test -f old
 '
 
 test_expect_success 'Porcelain checkout -f should remove remnants too' '
-  read_tree_u_must_succeed --reset -u HEAD &&
-  git ls-files -s >expect &&
-  sha1=$(git rev-parse :new) &&
-  (
-	echo "100644 $sha1 1	old"
-	echo "100644 $sha1 3	old"
-  ) | git update-index --index-info &&
-  >old &&
-  git ls-files -s &&
-  git checkout -f &&
-  git ls-files -s >actual &&
-  ! test -f old
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >expect &&
+	sha1=$(git rev-parse :new) &&
+	(
+		echo "100644 $sha1 1	old"
+		echo "100644 $sha1 3	old"
+	) | git update-index --index-info &&
+	>old &&
+	git ls-files -s &&
+	git checkout -f &&
+	git ls-files -s >actual &&
+	! test -f old
 '
 
 test_expect_success 'Porcelain checkout -f HEAD should remove remnants too' '
-  read_tree_u_must_succeed --reset -u HEAD &&
-  git ls-files -s >expect &&
-  sha1=$(git rev-parse :new) &&
-  (
-	echo "100644 $sha1 1	old"
-	echo "100644 $sha1 3	old"
-  ) | git update-index --index-info &&
-  >old &&
-  git ls-files -s &&
-  git checkout -f HEAD &&
-  git ls-files -s >actual &&
-  ! test -f old
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >expect &&
+	sha1=$(git rev-parse :new) &&
+	(
+		echo "100644 $sha1 1	old"
+		echo "100644 $sha1 3	old"
+	) | git update-index --index-info &&
+	>old &&
+	git ls-files -s &&
+	git checkout -f HEAD &&
+	git ls-files -s >actual &&
+	! test -f old
 '
 
 test_done
-- 
1.8.5-rc0-281-g8951339

[PATCH 3/3] t1005: add test for "read-tree --reset -u A B"

[Junio C Hamano]

With a conflicted index, this used to give us an error.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t1005-read-tree-reset.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t1005-read-tree-reset.sh b/t/t1005-read-tree-reset.sh
index e29cf63..0745685 100755
--- a/t/t1005-read-tree-reset.sh
+++ b/t/t1005-read-tree-reset.sh
@@ -43,6 +43,21 @@ test_expect_success 'reset should remove remnants from a failed merge' '
 	! test -f old
 '
 
+test_expect_success 'two-way reset should remove remnants too' '
+	read_tree_u_must_succeed --reset -u HEAD &&
+	git ls-files -s >expect &&
+	sha1=$(git rev-parse :new) &&
+	(
+		echo "100644 $sha1 1	old"
+		echo "100644 $sha1 3	old"
+	) | git update-index --index-info &&
+	>old &&
+	git ls-files -s &&
+	read_tree_u_must_succeed --reset -u HEAD HEAD &&
+	git ls-files -s >actual &&
+	! test -f old
+'
+
 test_expect_success 'Porcelain reset should remove remnants too' '
 	read_tree_u_must_succeed --reset -u HEAD &&
 	git ls-files -s >expect &&
-- 
1.8.5-rc0-281-g8951339

Re: Re* [BUG?] "error: cache entry has null sha1"

[Jeff King]

On Fri, Nov 01, 2013 at 03:44:52PM -0700, Junio C Hamano wrote:

> Here is a proposed endgame for the topic in a patch form, then.
> 
> I've added a test for low-level "read-tree --reset -u A B", and
> tried the "am --abort" I saw the problem with manually, but other
> than that, I haven't (re)thought about the issue hard enough to be
> comfortable with this change yet.

Thanks for moving this forward.

I read over the old discussion and the patches, and I think the patch is
a good thing. There was some question from me earlier on whether there
were other cases we weren't considering.  The discussion convinced me
that there almost certainly aren't. And even if there are, this is still
the right direction. Writing out a bogus CE_CONFLICTED entry is
_certainly_ wrong, so even if we do not get the details right (e.g.,
rejecting a merge we should be accepting), this patch still forms a base
for further fixups.

> Jeff King (1):
>   unpack-trees: fix "read-tree -u --reset A B" with conflicted index

My missing signoff:

  Signed-off-by: Jeff King <peff@peff.net>

-Peff