Re: [PATCH 8/8] remote-curl: in v2, fill credentials if needed

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 11, 2019, 19:20 UTC
Message-ID: <20190211192054.80935-1-jonathantanmy@google.com>
In-Reply-To: <20190206212928.GB12737@sigill.intra.peff.net>

Show 21 quoted lines

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