threads / patch / 35230

v4remote-curl: fix large pushes with GSSAPI

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

## tl;dr

17 messages between Oct 29, 2013 and Nov 3, 2013. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

brian m. carlson· Oct 29, 2013, 02:36 UTC · lore

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(-)

Show changes to remote-curl.c +12 −1

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

Jeff King· Oct 30, 2013, 08:45 UTC · re: brian m. carlson · lore

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

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.

Show 6 quoted lines

> +#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:

Show changes to 3 files +22 −16

http.c, http.h, remote-curl.c

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

brian m. carlson· Oct 30, 2013, 22:40 UTC · re: Jeff King · lore

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

On Wed, Oct 30, 2013 at 04:45:10AM -0400, Jeff King wrote:

Show 124 quoted lines

> 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

Jeff King· Oct 31, 2013, 06:34 UTC · re: brian m. carlson · lore

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

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

Jeff King· Oct 31, 2013, 06:35 UTC · re: Jeff King · lore

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

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(+)

Show changes to 2 files +7 −0

http.c, http.h

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

Jeff King· Oct 31, 2013, 06:36 UTC · re: Jeff King · lore

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

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(-)

Show changes to remote-curl.c +13 −9

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

Junio C Hamano· Oct 31, 2013, 17:11 UTC · re: Jeff King · lore

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

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...

Jeff King· Oct 31, 2013, 17:15 UTC · re: Junio C Hamano · lore

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

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

Show 9 quoted lines

>  - 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

Jeff King· Oct 31, 2013, 17:21 UTC · re: Jeff King · lore

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

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

Show 18 quoted lines

> 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

Junio C Hamano· Oct 31, 2013, 18:07 UTC · re: Jeff King · lore

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

Thanks for the pointers.

Junio C Hamano· Nov 1, 2013, 22:44 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

> 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

Junio C Hamano· Nov 1, 2013, 22:44 UTC · re: Junio C Hamano · lore

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

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(-)

Show changes to unpack-trees.c +17 −8

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

Junio C Hamano· Nov 1, 2013, 22:44 UTC · re: Junio C Hamano · lore

[PATCH 2/3] t1005: reindent

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(-)

Show changes to 2 files +89 −89

t/lib-read-tree.sh, t/t1005-read-tree-reset.sh

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

Junio C Hamano· Nov 1, 2013, 22:44 UTC · re: Junio C Hamano · lore

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

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(+)

Show changes to t/t1005-read-tree-reset.sh +15 −0

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

Jeff King· Nov 3, 2013, 08:03 UTC · re: Junio C Hamano · lore

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

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

Show 6 quoted lines

> 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

Jeff King· Oct 31, 2013, 06:36 UTC · re: Jeff King · lore

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

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

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(-)

Show changes to remote-curl.c +9 −2

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

brian m. carlson· Oct 31, 2013, 22:49 UTC · re: Jeff King · lore

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

On Thu, Oct 31, 2013 at 02:36:51AM -0400, Jeff King wrote:

Show 20 quoted lines

> 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