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

From: Jeff King <peff@peff.net>
Date: Feb 11, 2019, 20:38 UTC
Message-ID: <20190211203827.GA9010@sigill.intra.peff.net>
In-Reply-To: <20190211192054.80935-1-jonathantanmy@google.com>

On Mon, Feb 11, 2019 at 11:20:54AM -0800, Jonathan Tan wrote:

Show 7 quoted lines

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

Show 5 quoted lines

> 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