[PATCH 0/9] following http redirects

[PATCH 0/9] following http redirects

[Jeff King]

Git's http handling takes care of redirects by setting
CURLOPT_FOLLOWLOCATION, and just assuming curl will handle the details.
For the most part, this works, but there are a few downsides:

  1. We do extra round-trips for subsequent requests, as we
     always feed curl the original URL, and it then has to go hit the
     redirect.

  2. Some redirects are to upgrade http->https. In this case, we end up
     sending the credential over unencrypted http. We do advertise the
     proper unencrypted URL to the credential helpers (and to the user
     when we prompt them), so to some degree this is the user's fault.
     Still, there's no reason not to be friendly and avoid exposing the
     password in plaintext if we know that the server is just going to
     redirect us to an encrypted connection anyway.

  3. Cross-server redirects do not work at all with authentication. If
     we tell curl to go to host X, and it redirects to host Y, it will
     drop the auth credentials to avoid revealing them to Y. This is a
     good thing, because we prompted the user for host X, and curl is
     being safe. However, it means that we have no way to feed curl the
     right password (and the user does not even get a useful error
     message; simply a 401 from Y, since we didn't give it any
     credential).

None of these is fixable at the curl layer; they depend on information
we have outside of curl (e.g., the notion that if info/refs is
redirected, so should the rest of the requests be).

This series gets the redirect information from curl (after the fact),
and uses it update the base URL for the repository for the remainder of
the program run.

Kyle, I didn't call into the urlmatch code at all; we might want another
patch on top to re-check the http.$url.* config after following such a
redirect.

The patches are:

  [1/9]: http_get_file: style fixes
  [2/9]: http_request: factor out curlinfo_strbuf
  [3/9]: http: refactor options to http_get_*
  [4/9]: http: hoist credential request out of handle_curl_result
  [5/9]: http: provide effective url to callers
  [6/9]: http: update base URLs when we see redirects
  [7/9]: remote-curl: make refs_url a strbuf
  [8/9]: remote-curl: store url as a strbuf
  [9/9]: remote-curl: rewrite base url from info/refs redirects

Here is sample output from running a clone against a private repo on
github, which redirects from http to https. Generated with:

  GIT_CURL_VERBOSE=1 git clone http://github.com/github/github 2>&1 |
  egrep '^(< HTTP|< Location|> GET|> POST|^Authorization|\* Connected to)' |
  sed -e 's/\(Authorization\): .*/\1: **redacted**/' 

Here is the output with stock git (I've added numbering for each
connection):

1 * Connected to github.com (192.30.252.129) port 80 (#0)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack

2 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required

3 * Connected to github.com (192.30.252.129) port 80 (#2)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack

4 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required

5 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 200 OK

6 * Connected to github.com (192.30.252.129) port 80 (#3)
    > POST /github/github/git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/git-upload-pack

7 * Connected to github.com (192.30.252.129) port 443 (#4)
    [... POST actually happens, and I hit ^C before it finishes ...]

You can see that connections 3 and 6 are extraneous round-trips, and
that we reveal the credential over cleartext in step 6. Curiously, even
though we have fed the credential to curl after step 2, curl does not
share it until the server sends another 401 in step 4. I'm not sure why
that is the case.

Here's the output with these patches applied:

1 * Connected to github.com (192.30.252.129) port 80 (#0)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 301 Moved Permanently
    < Location: https://github.com/github/github/info/refs?service=git-upload-pack

2 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required

3 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    < HTTP/1.1 401 Authorization Required

4 * Connected to github.com (192.30.252.129) port 443 (#1)
    > GET /github/github/info/refs?service=git-upload-pack HTTP/1.1
    Authorization: **redacted**
    < HTTP/1.1 200 OK

5 * Connected to github.com (192.30.252.129) port 443 (#2)
    [... POST actually happens ...]

We do 2 fewer round-trips. That isn't all that exciting, but keep in
mind that it saves one per request after the initial contact. So if we
had more ref negotiation, we'd see more savings. Similarly, dumb http
would see many more savings (since it makes individual requests for each
loose object and pack).

We also go straight to https in steps 3 and 4, meaning the credential
never gets passed in plaintext to the old URL.

I also tested cross-server redirects using a hacky "nc -l" setup as the
server. I confirmed that it does not work at all with stock git (you are
prompted for the password for "localhost", which curl then drops after
following the redirect), and that it does work after the patches.

The one downside I can think of is that we are making the assumption
that if info/refs redirects, the rest of the repo will, too. So you
could not have "info/refs" redirect to another server to get the ref
advertisement, but then expect to get the actual pack from the original
location. I'm OK with making that assumption; I can't imagine the
insanity of the setup that would be required to break it.

-Peff

[PATCH 1/9] http_get_file: style fixes

[Jeff King]

Besides being ugly, the extra parentheses are idiomatic for
suppressing compiler warnings when we are assigning within a
conditional. We aren't doing that here, and they just
confuse the reader.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/http.c b/http.c
index f3e1439..a985c40 100644
--- a/http.c
+++ b/http.c
@@ -958,7 +958,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 
 	strbuf_addf(&tmpfile, "%s.temp", filename);
 	result = fopen(tmpfile.buf, "a");
-	if (! result) {
+	if (!result) {
 		error("Unable to open local file %s", tmpfile.buf);
 		ret = HTTP_ERROR;
 		goto cleanup;
@@ -967,7 +967,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 	ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
 	fclose(result);
 
-	if ((ret == HTTP_OK) && move_temp_to_file(tmpfile.buf, filename))
+	if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))
 		ret = HTTP_ERROR;
 cleanup:
 	strbuf_release(&tmpfile);
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 2/9] http_request: factor out curlinfo_strbuf

[Jeff King]

When we retrieve the content-type of an http response, curl
gives us a pointer to internal storage, which we then copy
into a strbuf. Let's factor out the get-and-copy routine,
which can be used for getting other curl info.

Signed-off-by: Jeff King <peff@peff.net>
---
 http.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/http.c b/http.c
index a985c40..d325669 100644
--- a/http.c
+++ b/http.c
@@ -837,6 +837,18 @@ int handle_curl_result(struct slot_results *results)
 	}
 }
 
+static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
+{
+	char *ptr;
+	CURLcode ret;
+
+	strbuf_reset(buf);
+	ret = curl_easy_getinfo(curl, info, &ptr);
+	if (!ret && ptr)
+		strbuf_addstr(buf, ptr);
+	return ret;
+}
+
 /* http_request() targets */
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
@@ -895,13 +907,8 @@ static int http_request(const char *url, struct strbuf *type,
 		ret = HTTP_START_FAILED;
 	}
 
-	if (type) {
-		char *t;
-		strbuf_reset(type);
-		curl_easy_getinfo(slot->curl, CURLINFO_CONTENT_TYPE, &t);
-		if (t)
-			strbuf_addstr(type, t);
-	}
+	if (type)
+		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 3/9] http: refactor options to http_get_*

[Jeff King]

Over time, the http_get_strbuf function has grown several
optional parameters. We now have a bitfield with multiple
boolean options, as well as an optional strbuf for returning
the content-type of the response. And a future patch in this
series is going to add another strbuf option.

Treating these as separate arguments has a few downsides:

  1. Most call sites need to add extra NULLs and 0s for the
     options they aren't interested in.

  2. The http_get_* functions are actually wrappers around
     2 layers of low-level implementation functions. We have
     to pass these options through individually.

  3. The http_get_strbuf wrapper learned these options, but
     nobody bothered to do so for http_get_file, even though
     it is backed by the same function that does understand
     the options.

Let's consolidate the options into a single struct. For the
common case of the default options, we'll allow callers to
simply pass a NULL for the options struct.

The resulting code is often a few lines longer, but it ends
up being easier to read (and to change as we add new
options, since we do not need to update each call site).

Signed-off-by: Jeff King <peff@peff.net>
---
 http-push.c   |  4 ++--
 http.c        | 44 +++++++++++++++++++++++++-------------------
 http.h        | 15 ++++++++++-----
 remote-curl.c |  9 +++++++--
 4 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/http-push.c b/http-push.c
index 69200ba..34cb70f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1542,7 +1542,7 @@ static int remote_exists(const char *path)
 
 	sprintf(url, "%s%s", repo->url, path);
 
-	switch (http_get_strbuf(url, NULL, NULL, 0)) {
+	switch (http_get_strbuf(url, NULL, NULL)) {
 	case HTTP_OK:
 		ret = 1;
 		break;
@@ -1566,7 +1566,7 @@ static void fetch_symref(const char *path, char **symref, unsigned char *sha1)
 	url = xmalloc(strlen(repo->url) + strlen(path) + 1);
 	sprintf(url, "%s%s", repo->url, path);
 
-	if (http_get_strbuf(url, NULL, &buffer, 0) != HTTP_OK)
+	if (http_get_strbuf(url, &buffer, NULL) != HTTP_OK)
 		die("Couldn't get %s for remote symref\n%s", url,
 		    curl_errorstr);
 	free(url);
diff --git a/http.c b/http.c
index d325669..41ee7aa 100644
--- a/http.c
+++ b/http.c
@@ -853,8 +853,9 @@ static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 #define HTTP_REQUEST_STRBUF	0
 #define HTTP_REQUEST_FILE	1
 
-static int http_request(const char *url, struct strbuf *type,
-			void *result, int target, int options)
+static int http_request(const char *url,
+			void *result, int target,
+			const struct http_get_options *options)
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
@@ -887,9 +888,9 @@ static int http_request(const char *url, struct strbuf *type,
 	}
 
 	strbuf_addstr(&buf, "Pragma:");
-	if (options & HTTP_NO_CACHE)
+	if (options && options->no_cache)
 		strbuf_addstr(&buf, " no-cache");
-	if (options & HTTP_KEEP_ERROR)
+	if (options && options->keep_error)
 		curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
 	headers = curl_slist_append(headers, buf.buf);
@@ -907,8 +908,9 @@ static int http_request(const char *url, struct strbuf *type,
 		ret = HTTP_START_FAILED;
 	}
 
-	if (type)
-		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE, type);
+	if (options && options->content_type)
+		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
+				options->content_type);
 
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
@@ -917,11 +919,10 @@ static int http_request_reauth(const char *url,
 }
 
 static int http_request_reauth(const char *url,
-			       struct strbuf *type,
 			       void *result, int target,
-			       int options)
+			       struct http_get_options *options)
 {
-	int ret = http_request(url, type, result, target, options);
+	int ret = http_request(url, result, target, options);
 	if (ret != HTTP_REAUTH)
 		return ret;
 
@@ -931,7 +932,7 @@ static int http_request_reauth(const char *url,
 	 * making our next request. We only know how to do this for
 	 * the strbuf case, but that is enough to satisfy current callers.
 	 */
-	if (options & HTTP_KEEP_ERROR) {
+	if (options && options->keep_error) {
 		switch (target) {
 		case HTTP_REQUEST_STRBUF:
 			strbuf_reset(result);
@@ -940,15 +941,14 @@ int http_get_strbuf(const char *url,
 			die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");
 		}
 	}
-	return http_request(url, type, result, target, options);
+	return http_request(url, result, target, options);
 }
 
 int http_get_strbuf(const char *url,
-		    struct strbuf *type,
-		    struct strbuf *result, int options)
+		    struct strbuf *result,
+		    struct http_get_options *options)
 {
-	return http_request_reauth(url, type, result,
-				   HTTP_REQUEST_STRBUF, options);
+	return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
 }
 
 /*
@@ -957,7 +957,8 @@ int http_get_strbuf(const char *url,
  * If a previous interrupted download is detected (i.e. a previous temporary
  * file is still around) the download is resumed.
  */
-static int http_get_file(const char *url, const char *filename, int options)
+static int http_get_file(const char *url, const char *filename,
+			 struct http_get_options *options)
 {
 	int ret;
 	struct strbuf tmpfile = STRBUF_INIT;
@@ -971,7 +972,7 @@ static int http_get_file(const char *url, const char *filename, int options)
 		goto cleanup;
 	}
 
-	ret = http_request_reauth(url, NULL, result, HTTP_REQUEST_FILE, options);
+	ret = http_request_reauth(url, result, HTTP_REQUEST_FILE, options);
 	fclose(result);
 
 	if (ret == HTTP_OK && move_temp_to_file(tmpfile.buf, filename))
@@ -983,12 +984,15 @@ int http_fetch_ref(const char *base, struct ref *ref)
 
 int http_fetch_ref(const char *base, struct ref *ref)
 {
+	struct http_get_options options = {0};
 	char *url;
 	struct strbuf buffer = STRBUF_INIT;
 	int ret = -1;
 
+	options.no_cache = 1;
+
 	url = quote_ref_url(base, ref->name);
-	if (http_get_strbuf(url, NULL, &buffer, HTTP_NO_CACHE) == HTTP_OK) {
+	if (http_get_strbuf(url, &buffer, &options) == HTTP_OK) {
 		strbuf_rtrim(&buffer);
 		if (buffer.len == 40)
 			ret = get_sha1_hex(buffer.buf, ref->old_sha1);
@@ -1072,6 +1076,7 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 
 int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 {
+	struct http_get_options options = {0};
 	int ret = 0, i = 0;
 	char *url, *data;
 	struct strbuf buf = STRBUF_INIT;
@@ -1081,7 +1086,8 @@ int http_get_info_packs(const char *base_url, struct packed_git **packs_head)
 	strbuf_addstr(&buf, "objects/info/packs");
 	url = strbuf_detach(&buf, NULL);
 
-	ret = http_get_strbuf(url, NULL, &buf, HTTP_NO_CACHE);
+	options.no_cache = 1;
+	ret = http_get_strbuf(url, &buf, &options);
 	if (ret != HTTP_OK)
 		goto cleanup;
 
diff --git a/http.h b/http.h
index d77c1b5..40404b4 100644
--- a/http.h
+++ b/http.h
@@ -125,11 +125,16 @@ extern char *get_remote_object_url(const char *url, const char *hex,
 extern char *get_remote_object_url(const char *url, const char *hex,
 				   int only_two_digit_prefix);
 
-/* Options for http_request_*() */
-#define HTTP_NO_CACHE		1
-#define HTTP_KEEP_ERROR		2
+/* Options for http_get_*() */
+struct http_get_options {
+	unsigned no_cache:1,
+		 keep_error:1;
 
-/* Return values for http_request_*() */
+	/* If non-NULL, returns the content-type of the response. */
+	struct strbuf *content_type;
+};
+
+/* Return values for http_get_*() */
 #define HTTP_OK			0
 #define HTTP_MISSING_TARGET	1
 #define HTTP_ERROR		2
@@ -142,7 +147,7 @@ extern char *get_remote_object_url(const char *url, const char *hex,
  *
  * If the result pointer is NULL, a HTTP HEAD request is made instead of GET.
  */
-int http_get_strbuf(const char *url, struct strbuf *content_type, struct strbuf *result, int options);
+int http_get_strbuf(const char *url, struct strbuf *result, struct http_get_options *options);
 
 extern int http_fetch_ref(const char *base, struct ref *ref);
 
diff --git a/remote-curl.c b/remote-curl.c
index b5ebe01..66705bb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -206,6 +206,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct discovery *last = last_discovery;
 	char *refs_url;
 	int http_ret, maybe_smart = 0;
+	struct http_get_options options;
 
 	if (last && !strcmp(service, last->service))
 		return last;
@@ -223,8 +224,12 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	}
 	refs_url = strbuf_detach(&buffer, NULL);
 
-	http_ret = http_get_strbuf(refs_url, &type, &buffer,
-				   HTTP_NO_CACHE | HTTP_KEEP_ERROR);
+	memset(&options, 0, sizeof(options));
+	options.content_type = &type;
+	options.no_cache = 1;
+	options.keep_error = 1;
+
+	http_ret = http_get_strbuf(refs_url, &buffer, &options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 4/9] http: hoist credential request out of handle_curl_result

[Jeff King]

When we are handling a curl response code in http_request or
in the remote-curl RPC code, we use the handle_curl_result
helper to translate curl's response into an easy-to-use
code. When we see an HTTP 401, we do one of two things:

  1. If we already had a filled-in credential, we mark it as
     rejected, and then return HTTP_NOAUTH to indicate to
     the caller that we failed.

  2. If we didn't, then we ask for a new credential and tell
     the caller HTTP_REAUTH to indicate that they may want
     to try again.

Rejecting in the first case makes sense; it is the natural
result of the request we just made. However, prompting for
more credentials in the second step does not always make
sense. We do not know for sure that the caller is going to
make a second request, and nor are we sure that it will be
to the same URL. Logically, the prompt belongs not to the
request we just finished, but to the request we are (maybe)
about to make.

In practice, it is very hard to trigger any bad behavior.
Currently, if we make a second request, it will always be to
the same URL (even in the face of redirects, because curl
handles the redirects internally). And we almost always
retry on HTTP_REAUTH these days. The one exception is if we
are streaming a large RPC request to the server (e.g., a
pushed packfile), in which case we cannot restart. It's
extremely unlikely to see a 401 response at this stage,
though, as we would typically have seen it when we sent a
probe request, before streaming the data.

This patch drops the automatic prompt out of case 2, and
instead requires the caller to do it. This is a few extra
lines of code, and the bug it fixes is unlikely to come up
in practice. But it is conceptually cleaner, and paves the
way for better handling of credentials across redirects.

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

diff --git a/http.c b/http.c
index 41ee7aa..5504b2c 100644
--- a/http.c
+++ b/http.c
@@ -47,7 +47,7 @@ static int curl_save_cookies;
 static const char *curl_http_proxy;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
-static struct credential http_auth = CREDENTIAL_INIT;
+struct credential http_auth = CREDENTIAL_INIT;
 static int http_proactive_auth;
 static const char *user_agent;
 
@@ -823,7 +823,6 @@ int handle_curl_result(struct slot_results *results)
 			credential_reject(&http_auth);
 			return HTTP_NOAUTH;
 		} else {
-			credential_fill(&http_auth);
 			return HTTP_REAUTH;
 		}
 	} else {
@@ -941,6 +940,9 @@ static int http_request_reauth(const char *url,
 			die("BUG: HTTP_KEEP_ERROR is only supported with strbufs");
 		}
 	}
+
+	credential_fill(&http_auth);
+
 	return http_request(url, result, target, options);
 }
 
diff --git a/http.h b/http.h
index 40404b4..17116ab 100644
--- a/http.h
+++ b/http.h
@@ -102,6 +102,7 @@ extern size_t http_post_buffer;
 extern int active_requests;
 extern int http_is_verbose;
 extern size_t http_post_buffer;
+extern struct credential http_auth;
 
 extern char curl_errorstr[CURL_ERROR_SIZE];
 
diff --git a/remote-curl.c b/remote-curl.c
index 66705bb..d524462 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "sideband.h"
 #include "argv-array.h"
+#include "credential.h"
 
 static struct remote *remote;
 static const char *url; /* always ends with a trailing slash */
@@ -468,6 +469,8 @@ static int post_rpc(struct rpc_state *rpc)
 	if (large_request) {
 		do {
 			err = probe_rpc(rpc);
+			if (err == HTTP_REAUTH)
+				credential_fill(&http_auth);
 		} while (err == HTTP_REAUTH);
 		if (err != HTTP_OK)
 			return -1;
@@ -567,8 +570,10 @@ retry:
 	curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
 
 	err = run_slot(slot);
-	if (err == HTTP_REAUTH && !large_request)
+	if (err == HTTP_REAUTH && !large_request) {
+		credential_fill(&http_auth);
 		goto retry;
+	}
 	if (err != HTTP_OK)
 		err = -1;
 
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 5/9] http: provide effective url to callers

[Jeff King]

When we ask curl to access a URL, it may follow one or more
redirects to reach the final location. We have no idea
this has happened, as curl takes care of the details and
simply returns the final content to us.

The final URL that we ended up with can be accessed via
CURLINFO_EFFECTIVE_URL. Let's make that optionally available
to callers of http_get_*, so that they can make further
decisions based on the redirection.

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

diff --git a/http.c b/http.c
index 5504b2c..65a0048 100644
--- a/http.c
+++ b/http.c
@@ -911,6 +911,10 @@ static int http_request(const char *url,
 		curlinfo_strbuf(slot->curl, CURLINFO_CONTENT_TYPE,
 				options->content_type);
 
+	if (options && options->effective_url)
+		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
+				options->effective_url);
+
 	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 
diff --git a/http.h b/http.h
index 17116ab..974ede7 100644
--- a/http.h
+++ b/http.h
@@ -133,6 +133,12 @@ struct http_get_options {
 
 	/* If non-NULL, returns the content-type of the response. */
 	struct strbuf *content_type;
+
+	/*
+	 * If non-NULL, returns the URL we ended up at, including any
+	 * redirects we followed.
+	 */
+	struct strbuf *effective_url;
 };
 
 /* Return values for http_get_*() */
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 6/9] http: update base URLs when we see redirects

[Jeff King]

If a caller asks the http_get_* functions to go to a
particular URL and we end up elsewhere due to a redirect,
the effective_url field can tell us where we went.

It would be nice to remember this redirect and short-cut
further requests for two reasons:

  1. It's more efficient. Otherwise we spend an extra http
     round-trip to the server for each subsequent request,
     just to get redirected.

  2. If we end up with an http 401 and are going to ask for
     credentials, it is to feed them to the redirect target.
     If the redirect is an http->https upgrade, this means
     our credentials may be provided on the http leg, just
     to end up redirected to https. And if the redirect
     crosses server boundaries, then curl will drop the
     credentials entirely as it follows the redirect.

However, it, it is not enough to simply record the effective
URL we saw and use that for subsequent requests. We were
originally fed a "base" url like:

   http://example.com/foo.git

and we want to figure out what the new base is, even though
the URLs we see may be:

     original: http://example.com/foo.git/info/refs
    effective: http://example.com/bar.git/info/refs

Subsequent requests will not be for "info/refs", but for
other paths relative to the base. We must ask the caller to
pass in the original base, and we must pass the redirected
base back to the caller (so that it can generte more URLs
from it). Furthermore, we need to feed the new base to the
credential code, so that requests to credential helpers (or
to the user) match the URL we will be requesting.

This patch teaches http_request_reauth to do this munging.
Since it is the caller who cares about making more URLs, it
seems at first glance that callers could simply check
effective_url themselves and handle it. However, since we
need to update the credential struct before the second
re-auth request, we have to do it inside http_request_reauth.

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

diff --git a/http.c b/http.c
index 65a0048..8775b5c 100644
--- a/http.c
+++ b/http.c
@@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
 	return ret;
 }
 
+/*
+ * Update the "base" url to a more appropriate value, as deduced by
+ * redirects seen when requesting a URL starting with "url".
+ *
+ * The "asked" parameter is a URL that we asked curl to access, and must begin
+ * with "base".
+ *
+ * The "got" parameter is the URL that curl reported to us as where we ended
+ * up.
+ *
+ * Returns 1 if we updated the base url, 0 otherwise.
+ *
+ * Our basic strategy is to compare "base" and "asked" to find the bits
+ * specific to our request. We then strip those bits off of "got" to yield the
+ * new base. So for example, if our base is "http://example.com/foo.git",
+ * and we ask for "http://example.com/foo.git/info/refs", we might end up
+ * with "https://other.example.com/foo.git/info/refs". We would want the
+ * new URL to become "https://other.example.com/foo.git".
+ *
+ * Note that this assumes a sane redirect scheme. It's entirely possible
+ * in the example above to end up at a URL that does not even end in
+ * "info/refs".  In such a case we simply punt, as there is not much we can
+ * do (and such a scheme is unlikely to represent a real git repository,
+ * which means we are likely about to abort anyway).
+ */
+static int update_url_from_redirect(struct strbuf *base,
+				    const char *asked,
+				    const struct strbuf *got)
+{
+	const char *tail;
+	size_t tail_len;
+
+	if (!strcmp(asked, got->buf))
+		return 0;
+
+	if (strncmp(asked, base->buf, base->len))
+		die("BUG: update_url_from_redirect: %s is not a superset of %s",
+		    asked, base->buf);
+
+	tail = asked + base->len;
+	tail_len = strlen(tail);
+
+	if (got->len < tail_len ||
+	    strcmp(tail, got->buf + got->len - tail_len))
+		return 0; /* insane redirect scheme */
+
+	strbuf_reset(base);
+	strbuf_add(base, got->buf, got->len - tail_len);
+	return 1;
+}
+
 static int http_request_reauth(const char *url,
 			       void *result, int target,
 			       struct http_get_options *options)
 {
 	int ret = http_request(url, result, target, options);
+
+	if (options && options->effective_url && options->base_url) {
+		if (update_url_from_redirect(options->base_url,
+					     url, options->effective_url)) {
+			credential_from_url(&http_auth, options->base_url->buf);
+			url = options->effective_url->buf;
+		}
+	}
+
 	if (ret != HTTP_REAUTH)
 		return ret;
 
diff --git a/http.h b/http.h
index 974ede7..12ca5c8 100644
--- a/http.h
+++ b/http.h
@@ -139,6 +139,14 @@ struct http_get_options {
 	 * redirects we followed.
 	 */
 	struct strbuf *effective_url;
+
+	/*
+	 * If both base_url and effective_url are non-NULL, the base URL will
+	 * be munged to reflect any redirections going from the requested url
+	 * to effective_url. See the definition of update_url_from_redirect
+	 * for details.
+	 */
+	struct strbuf *base_url;
 };
 
 /* Return values for http_get_*() */
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 7/9] remote-curl: make refs_url a strbuf

[Jeff King]

In the discover_refs function, we use a strbuf named
"buffer" for multiple purposes. First we build the info/refs
URL in it, and then detach that to a bare pointer. Then, we
use the same strbuf to store the result of fetching the
refs.

Let's instead keep a separate refs_url strbuf. This is less
confusing, as the "buffer" strbuf is now used for only one
thing.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is not technically required.  In my original draft, having it
as a strbuf was helpful, but that didn't make it to the final version.
However, I think it's a worthwhile cleanup on its own, so I left it in.

 remote-curl.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index d524462..7fb092f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -204,8 +204,8 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct strbuf exp = STRBUF_INIT;
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
+	struct strbuf refs_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
-	char *refs_url;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options options;
 
@@ -213,24 +213,23 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&buffer, "%sinfo/refs", url);
+	strbuf_addf(&refs_url, "%sinfo/refs", url);
 	if ((!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) &&
 	     git_env_bool("GIT_SMART_HTTP", 1)) {
 		maybe_smart = 1;
 		if (!strchr(url, '?'))
-			strbuf_addch(&buffer, '?');
+			strbuf_addch(&refs_url, '?');
 		else
-			strbuf_addch(&buffer, '&');
-		strbuf_addf(&buffer, "service=%s", service);
+			strbuf_addch(&refs_url, '&');
+		strbuf_addf(&refs_url, "service=%s", service);
 	}
-	refs_url = strbuf_detach(&buffer, NULL);
 
 	memset(&options, 0, sizeof(options));
 	options.content_type = &type;
 	options.no_cache = 1;
 	options.keep_error = 1;
 
-	http_ret = http_get_strbuf(refs_url, &buffer, &options);
+	http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
 	switch (http_ret) {
 	case HTTP_OK:
 		break;
@@ -283,7 +282,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	else
 		last->refs = parse_info_refs(last);
 
-	free(refs_url);
+	strbuf_release(&refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
 	strbuf_release(&buffer);
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 8/9] remote-curl: store url as a strbuf

[Jeff King]

We use a strbuf to generate the string containing the remote
URL, but then detach it to a bare pointer. This makes it
harder to later manipulate the URL, as we have forgotten the
length (and the allocation semantics are not as clear).

Let's instead keep the strbuf around. As a bonus, this
eliminates a confusing double-use of the "buf" strbuf in
main(). Prior to this, it was used both for constructing the
url, and for reading commands from stdin.

The downside is that we have to update each call site to
refer to "url.buf" rather than just "url" when they want the
C string.

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

diff --git a/remote-curl.c b/remote-curl.c
index 7fb092f..5add2cb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -12,7 +12,8 @@ static struct remote *remote;
 #include "credential.h"
 
 static struct remote *remote;
-static const char *url; /* always ends with a trailing slash */
+/* always ends with a trailing slash */
+static struct strbuf url = STRBUF_INIT;
 
 struct options {
 	int verbosity;
@@ -131,7 +132,8 @@ static struct ref *parse_info_refs(struct discovery *heads)
 			mid = &data[i];
 		if (data[i] == '\n') {
 			if (mid - start != 40)
-				die("%sinfo/refs not valid: is this a git repository?", url);
+				die("%sinfo/refs not valid: is this a git repository?",
+				    url.buf);
 			data[i] = 0;
 			ref_name = mid + 1;
 			ref = xmalloc(sizeof(struct ref) +
@@ -150,7 +152,7 @@ static struct ref *parse_info_refs(struct discovery *heads)
 	}
 
 	ref = alloc_ref("HEAD");
-	if (!http_fetch_ref(url, ref) &&
+	if (!http_fetch_ref(url.buf, ref) &&
 	    !resolve_remote_symref(ref, refs)) {
 		ref->next = refs;
 		refs = ref;
@@ -213,11 +215,11 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		return last;
 	free_discovery(last);
 
-	strbuf_addf(&refs_url, "%sinfo/refs", url);
-	if ((!prefixcmp(url, "http://") || !prefixcmp(url, "https://")) &&
+	strbuf_addf(&refs_url, "%sinfo/refs", url.buf);
+	if ((!prefixcmp(url.buf, "http://") || !prefixcmp(url.buf, "https://")) &&
 	     git_env_bool("GIT_SMART_HTTP", 1)) {
 		maybe_smart = 1;
-		if (!strchr(url, '?'))
+		if (!strchr(url.buf, '?'))
 			strbuf_addch(&refs_url, '?');
 		else
 			strbuf_addch(&refs_url, '&');
@@ -235,13 +237,13 @@ static struct discovery* discover_refs(const char *service, int for_push)
 		break;
 	case HTTP_MISSING_TARGET:
 		show_http_message(&type, &buffer);
-		die("repository '%s' not found", url);
+		die("repository '%s' not found", url.buf);
 	case HTTP_NOAUTH:
 		show_http_message(&type, &buffer);
-		die("Authentication failed for '%s'", url);
+		die("Authentication failed for '%s'", url.buf);
 	default:
 		show_http_message(&type, &buffer);
-		die("unable to access '%s': %s", url, curl_errorstr);
+		die("unable to access '%s': %s", url.buf, curl_errorstr);
 	}
 
 	last= xcalloc(1, sizeof(*last_discovery));
@@ -607,7 +609,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)
 	rpc->out = client.out;
 	strbuf_init(&rpc->result, 0);
 
-	strbuf_addf(&buf, "%s%s", url, svc);
+	strbuf_addf(&buf, "%s%s", url.buf, svc);
 	rpc->service_url = strbuf_detach(&buf, NULL);
 
 	strbuf_addf(&buf, "Content-Type: application/x-%s-request", svc);
@@ -659,7 +661,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
 	for (i = 0; i < nr_heads; i++)
 		targets[i] = xstrdup(sha1_to_hex(to_fetch[i]->old_sha1));
 
-	walker = get_http_walker(url);
+	walker = get_http_walker(url.buf);
 	walker->get_all = 1;
 	walker->get_tree = 1;
 	walker->get_history = 1;
@@ -706,7 +708,7 @@ static int fetch_git(struct discovery *heads,
 		depth_arg = strbuf_detach(&buf, NULL);
 		argv[argc++] = depth_arg;
 	}
-	argv[argc++] = url;
+	argv[argc++] = url.buf;
 	argv[argc++] = NULL;
 
 	for (i = 0; i < nr_heads; i++) {
@@ -804,7 +806,7 @@ static int push_dav(int nr_spec, char **specs)
 		argv[argc++] = "--dry-run";
 	if (options.verbosity > 1)
 		argv[argc++] = "--verbose";
-	argv[argc++] = url;
+	argv[argc++] = url.buf;
 	for (i = 0; i < nr_spec; i++)
 		argv[argc++] = specs[i];
 	argv[argc++] = NULL;
@@ -837,7 +839,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)
 	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
 	for_each_string_list_item(cas_option, &cas_options)
 		argv_array_push(&args, cas_option->string);
-	argv_array_push(&args, url);
+	argv_array_push(&args, url.buf);
 	for (i = 0; i < nr_spec; i++)
 		argv_array_push(&args, specs[i]);
 
@@ -918,14 +920,12 @@ int main(int argc, const char **argv)
 	remote = remote_get(argv[1]);
 
 	if (argc > 2) {
-		end_url_with_slash(&buf, argv[2]);
+		end_url_with_slash(&url, argv[2]);
 	} else {
-		end_url_with_slash(&buf, remote->url[0]);
+		end_url_with_slash(&url, remote->url[0]);
 	}
 
-	url = strbuf_detach(&buf, NULL);
-
-	http_init(remote, url, 0);
+	http_init(remote, url.buf, 0);
 
 	do {
 		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
-- 
1.8.4.rc3.19.g9da5bf6

[PATCH 9/9] remote-curl: rewrite base url from info/refs redirects

[Jeff King]

For efficiency and security reasons, an earlier commit in
this series taught http_get_* to re-write the base url based
on redirections we saw while making a specific request.

This commit wires that option into the info/refs request,
meaning that a redirect from

    http://example.com/foo.git/info/refs

to

    https://example.com/bar.git/info/refs

will behave as if "https://example.com/bar.git" had been
provided to git in the first place.

The tests bear some explanation. We introduce two new
hierearchies into the httpd test config:

  1. Requests to /smart-redir-limited will work only for the
     initial info/refs request, but not any subsequent
     requests. As a result, we can confirm whether the
     client is re-rooting its requests after the initial
     contact, since otherwise it will fail (it will ask for
     "repo.git/git-upload-pack", which is not redirected).

  2. Requests to smart-redir-auth will redirect, and require
     auth after the redirection. Since we are using the
     redirected base for further requests, we also update
     the credential struct, in order not to mislead the user
     (or credential helpers) about which credential is
     needed. We can therefore check the GIT_ASKPASS prompts
     to make sure we are prompting for the new location.
     Because we have neither multiple servers nor https
     support in our test setup, we can only redirect between
     paths, meaning we need to turn on
     credential.useHttpPath to see the difference.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote-curl.c           |  4 ++++
 t/lib-httpd.sh          |  3 ++-
 t/lib-httpd/apache.conf |  2 ++
 t/t5551-http-fetch.sh   | 11 +++++++++++
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/remote-curl.c b/remote-curl.c
index 5add2cb..c9b891a 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -207,6 +207,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	struct strbuf type = STRBUF_INIT;
 	struct strbuf buffer = STRBUF_INIT;
 	struct strbuf refs_url = STRBUF_INIT;
+	struct strbuf effective_url = STRBUF_INIT;
 	struct discovery *last = last_discovery;
 	int http_ret, maybe_smart = 0;
 	struct http_get_options options;
@@ -228,6 +229,8 @@ static struct discovery* discover_refs(const char *service, int for_push)
 
 	memset(&options, 0, sizeof(options));
 	options.content_type = &type;
+	options.effective_url = &effective_url;
+	options.base_url = &url;
 	options.no_cache = 1;
 	options.keep_error = 1;
 
@@ -287,6 +290,7 @@ static struct discovery* discover_refs(const char *service, int for_push)
 	strbuf_release(&refs_url);
 	strbuf_release(&exp);
 	strbuf_release(&type);
+	strbuf_release(&effective_url);
 	strbuf_release(&buffer);
 	last_discovery = last;
 	return last;
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 54dbbfe..ad8f1ef 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -204,7 +204,8 @@ expect_askpass() {
 }
 
 expect_askpass() {
-	dest=$HTTPD_DEST
+	dest=$HTTPD_DEST${3+/$3}
+
 	{
 		case "$1" in
 		none)
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 397c480..3a03e82 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -110,6 +110,8 @@ RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
 RewriteEngine on
 RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
 RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
+RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
+RewriteRule ^/smart-redir-limited/(.*)/info/refs$ /smart/$1/info/refs [R=301]
 
 <IfDefine SSL>
 LoadModule ssl_module modules/mod_ssl.so
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 8196af1..fb16833 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -113,6 +113,10 @@ test_expect_success 'follow redirects (302)' '
 	git clone $HTTPD_URL/smart-redir-temp/repo.git --quiet repo-t
 '
 
+test_expect_success 'redirects re-root further requests' '
+	git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
+'
+
 test_expect_success 'clone from password-protected repository' '
 	echo two >expect &&
 	set_askpass user@host &&
@@ -146,6 +150,13 @@ test_expect_success 'no-op half-auth fetch does not require a password' '
 	expect_askpass none
 '
 
+test_expect_success 'redirects send auth to new location' '
+	set_askpass user@host &&
+	git -c credential.useHttpPath=true \
+	  clone $HTTPD_URL/smart-redir-auth/repo.git repo-redir-auth &&
+	expect_askpass both user@host auth/smart/repo.git
+'
+
 test_expect_success 'disable dumb http on server' '
 	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" \
 		config http.getanyfile false
-- 
1.8.4.rc3.19.g9da5bf6

Re: [PATCH 6/9] http: update base URLs when we see redirects

[brian m. carlson]

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

On Sat, Sep 28, 2013 at 04:34:05AM -0400, Jeff King wrote:
> Subsequent requests will not be for "info/refs", but for
> other paths relative to the base. We must ask the caller to
> pass in the original base, and we must pass the redirected
> base back to the caller (so that it can generte more URLs

s/generte/generate/

> from it). Furthermore, we need to feed the new base to the
> credential code, so that requests to credential helpers (or
> to the user) match the URL we will be requesting.

-- 
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 6/9] http: update base URLs when we see redirects

[Eric Sunshine]

On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
> diff --git a/http.c b/http.c
> index 65a0048..8775b5c 100644
> --- a/http.c
> +++ b/http.c
> @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
> +static int update_url_from_redirect(struct strbuf *base,
> +                                   const char *asked,
> +                                   const struct strbuf *got)
> +{
> +       const char *tail;
> +       size_t tail_len;
> +
> +       if (!strcmp(asked, got->buf))
> +               return 0;
> +
> +       if (strncmp(asked, base->buf, base->len))
> +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
> +                   asked, base->buf);

Is there something non-obvious going on here? die(...,base->buf) takes
advantage of the terminating NUL promised by strbuf, but then
strncmp(...,base->buf,base->len) is used rather than the simpler
strcmp(...,base->buf).

> +       tail = asked + base->len;
> +       tail_len = strlen(tail);
> +
> +       if (got->len < tail_len ||
> +           strcmp(tail, got->buf + got->len - tail_len))
> +               return 0; /* insane redirect scheme */
> +
> +       strbuf_reset(base);
> +       strbuf_add(base, got->buf, got->len - tail_len);
> +       return 1;
> +}

Re: [PATCH 6/9] http: update base URLs when we see redirects

[Jeff King]

On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:

> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
> > diff --git a/http.c b/http.c
> > index 65a0048..8775b5c 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
> > +static int update_url_from_redirect(struct strbuf *base,
> > +                                   const char *asked,
> > +                                   const struct strbuf *got)
> > +{
> > +       const char *tail;
> > +       size_t tail_len;
> > +
> > +       if (!strcmp(asked, got->buf))
> > +               return 0;
> > +
> > +       if (strncmp(asked, base->buf, base->len))
> > +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
> > +                   asked, base->buf);
> 
> Is there something non-obvious going on here? die(...,base->buf) takes
> advantage of the terminating NUL promised by strbuf, but then
> strncmp(...,base->buf,base->len) is used rather than the simpler
> strcmp(...,base->buf).

Yes, we are not checking for equality, but rather making sure that
"asked" begins with "base->buf". It might be more clearly written as:

  if (prefixcmp(asked, base->buf))

I was trying to take advantage of the fact that we know base->len
already, but this it not a particularly performance-critical code path.
We can afford the extra strlen that prefixcmp will do.

-Peff

Re: [PATCH 6/9] http: update base URLs when we see redirects

[Eric Sunshine]

On Sun, Sep 29, 2013 at 3:32 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:
>
>> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King <peff@peff.net> wrote:
>> > diff --git a/http.c b/http.c
>> > index 65a0048..8775b5c 100644
>> > --- a/http.c
>> > +++ b/http.c
>> > @@ -921,11 +921,71 @@ static int http_request_reauth(const char *url,
>> > +static int update_url_from_redirect(struct strbuf *base,
>> > +                                   const char *asked,
>> > +                                   const struct strbuf *got)
>> > +{
>> > +       const char *tail;
>> > +       size_t tail_len;
>> > +
>> > +       if (!strcmp(asked, got->buf))
>> > +               return 0;
>> > +
>> > +       if (strncmp(asked, base->buf, base->len))
>> > +               die("BUG: update_url_from_redirect: %s is not a superset of %s",
>> > +                   asked, base->buf);
>>
>> Is there something non-obvious going on here? die(...,base->buf) takes
>> advantage of the terminating NUL promised by strbuf, but then
>> strncmp(...,base->buf,base->len) is used rather than the simpler
>> strcmp(...,base->buf).
>
> Yes, we are not checking for equality, but rather making sure that
> "asked" begins with "base->buf". It might be more clearly written as:

Ah right, I knew that that was the intention but had a synapse
misfire. Sorry for the noise.

>
>   if (prefixcmp(asked, base->buf))
>
> I was trying to take advantage of the fact that we know base->len
> already, but this it not a particularly performance-critical code path.
> We can afford the extra strlen that prefixcmp will do.
>
> -Peff

Re: [PATCH 6/9] http: update base URLs when we see redirects

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> + * Our basic strategy is to compare "base" and "asked" to find the bits
> + * specific to our request. We then strip those bits off of "got" to yield the
> + * new base. So for example, if our base is "http://example.com/foo.git",
> + * and we ask for "http://example.com/foo.git/info/refs", we might end up
> + * with "https://other.example.com/foo.git/info/refs". We would want the
> + * new URL to become "https://other.example.com/foo.git".

Not "https://other.example.com/foo.git/info/refs"?

Re: [PATCH 6/9] http: update base URLs when we see redirects

[Jeff King]

On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > + * Our basic strategy is to compare "base" and "asked" to find the bits
> > + * specific to our request. We then strip those bits off of "got" to yield the
> > + * new base. So for example, if our base is "http://example.com/foo.git",
> > + * and we ask for "http://example.com/foo.git/info/refs", we might end up
> > + * with "https://other.example.com/foo.git/info/refs". We would want the
> > + * new URL to become "https://other.example.com/foo.git".
> 
> Not "https://other.example.com/foo.git/info/refs"?

I think my use of "the new URL" is ambiguous. I meant "the new base",
from which one could then construct the new refs URL as you suggest.

-Peff

Re: [PATCH 6/9] http: update base URLs when we see redirects

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > + * Our basic strategy is to compare "base" and "asked" to find the bits
>> > + * specific to our request. We then strip those bits off of "got" to yield the
>> > + * new base. So for example, if our base is "http://example.com/foo.git",
>> > + * and we ask for "http://example.com/foo.git/info/refs", we might end up
>> > + * with "https://other.example.com/foo.git/info/refs". We would want the
>> > + * new URL to become "https://other.example.com/foo.git".
>> 
>> Not "https://other.example.com/foo.git/info/refs"?
>
> I think my use of "the new URL" is ambiguous. I meant "the new base",
> from which one could then construct the new refs URL as you suggest.

Ahh, there is nothing we need to do to make the new URL to
"https://.../info/refs"; we already have it in "got".  And the
function resets "base" and then adds "got" excluding its tail part
to compute the new base.  So "s/new URL/new base/" would be all we
need to do, I think.

Thanks.