| Message ID | 8d5ff2fc224e2ce7981bcae492de02a622889208.1549411880.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Resend of GIT_TEST_PROTOCOL_VERSION patches | expand |
On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote: > In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is > returned, but this is not true in proxy_request(). Do this in > proxy_request() too. Can we do this as a general rule? If we look at the code in post_rpc(), there are two cases: when large_request is set and when it is not. When it's not, we have the whole request in a buffer, and we can happily resend it. But when it's not, we cannot restart it, because we'll have thrown away some of the data. So we send an initial probe_rpc() as a sanity check. If that works and we later get a 401 on the real request, we still fail anyway. In the case of proxy_request(), we don't know ahead of time whether the request is large or not; we just proxy the data through. And we don't do the probe thing at all. So wouldn't we dropping some data for the follow-up request? -Peff
> On Tue, Feb 05, 2019 at 04:21:22PM -0800, Jonathan Tan wrote: > > > In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is > > returned, but this is not true in proxy_request(). Do this in > > proxy_request() too. > > Can we do this as a general rule? If we look at the code in post_rpc(), > there are two cases: when large_request is set and when it is not. > > When it's not, we have the whole request in a buffer, and we can happily > resend it. > > But when it's not, we cannot restart it, because we'll have thrown away > some of the data. So we send an initial probe_rpc() as a sanity check. > If that works and we later get a 401 on the real request, we still fail > anyway. > > In the case of proxy_request(), we don't know ahead of time whether the > request is large or not; we just proxy the data through. And we don't do > the probe thing at all. So wouldn't we dropping some data for the > follow-up request? Thanks - I'll look into this. Maybe the best way is to somehow make the v2 code path also use post_rpc() - I'll see if that's possible. In the meantime, do you have any other opinions on the other patches, besides introducing a prereq [1]? I don't have any strong opinions for or against this, so I didn't reply, but I slightly prefer not having the prereq so that test readers and authors don't need to juggle so many variables in their heads. If everything looks good, I'll suggest that we drop this patch from this patch set for me to work on it independently. (Having said that, this patch set is based on js/protocol-advertise-multi, which is still under review, so it is not so urgent.) [1] https://public-inbox.org/git/20190206213458.GC12737@sigill.intra.peff.net/
On Mon, Feb 11, 2019 at 11:20:54AM -0800, Jonathan Tan wrote: > > In the case of proxy_request(), we don't know ahead of time whether the > > request is large or not; we just proxy the data through. And we don't do > > the probe thing at all. So wouldn't we dropping some data for the > > follow-up request? > > Thanks - I'll look into this. Maybe the best way is to somehow make the > v2 code path also use post_rpc() - I'll see if that's possible. Yeah, that makes sense. > In the meantime, do you have any other opinions on the other patches, > besides introducing a prereq [1]? I don't have any strong opinions for > or against this, so I didn't reply, but I slightly prefer not having the > prereq so that test readers and authors don't need to juggle so many > variables in their heads. I think Ævar convinced me that the way you've done it is the right way, so ignore my earlier comments. :) I just took another pass and left a few very minor nits, but it all looks good overall. > If everything looks good, I'll suggest that we drop this patch from this > patch set for me to work on it independently. (Having said that, this > patch set is based on js/protocol-advertise-multi, which is still under > review, so it is not so urgent.) Yeah, that's fine by me (with or without my nits addressed). -Peff
diff --git a/remote-curl.c b/remote-curl.c index 293bcdb95b..437a8e76d8 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1295,7 +1295,9 @@ static size_t proxy_out(char *buffer, size_t eltsize, static int proxy_request(struct proxy_state *p) { struct active_request_slot *slot; + int err; +retry: slot = get_active_slot(); curl_easy_setopt(slot->curl, CURLOPT_ENCODING, ""); @@ -1312,7 +1314,12 @@ static int proxy_request(struct proxy_state *p) curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, proxy_out); curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, p); - if (run_slot(slot, NULL) != HTTP_OK) + err = run_slot(slot, NULL); + if (err == HTTP_REAUTH) { + credential_fill(&http_auth); + goto retry; + } + if (err != HTTP_OK) return -1; return 0;
In post_rpc(), remote-curl calls credential_fill() if HTTP_REAUTH is returned, but this is not true in proxy_request(). Do this in proxy_request() too. When t5551 is run using GIT_TEST_PROTOCOL_VERSION=2, one of the tests that used to fail now pass. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- remote-curl.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)