Re: [PATCH 6/9] http: update base URLs when we see redirects
Jeff King writes: > On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote: > >> Jeff King 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
On Fri, Oct 18, 2013 at 11:25:13AM -0700, Junio C Hamano wrote: > Jeff King 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
Jeff King 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";? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
On Sun, Sep 29, 2013 at 3:32 PM, Jeff King 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 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
On Sun, Sep 29, 2013 at 03:26:45PM -0400, Eric Sunshine wrote:
> On Sat, Sep 28, 2013 at 4:34 AM, Jeff King 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
On Sat, Sep 28, 2013 at 4:34 AM, Jeff King 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;
> +}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] http: update base URLs when we see redirects
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 signature.asc Description: Digital signature
[PATCH 6/9] http: update base URLs when we see redirects
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
---
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 10064
