Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect
- From
- Brandon Williams <bmwill@google.com>
- Date
- Feb 22, 2018, 18:53 UTC
- Message-ID
- <20180222185353.GH185096@google.com>
- In-Reply-To
- <20180221160116.1619fcc4ffa7e9c13a5ab9ba@google.com>
On 02/21, Jonathan Tan wrote:
Show 15 quoted lines
> On Tue, 6 Feb 2018 17:13:05 -0800 > Brandon Williams <bmwill@google.com> wrote: > > > Introduce the transport-helper capability 'stateless-connect'. This > > capability indicates that the transport-helper can be requested to run > > the 'stateless-connect' command which should attempt to make a > > stateless connection with a remote end. Once established, the > > connection can be used by the git client to communicate with > > the remote end natively in a stateless-rpc manner as supported by > > protocol v2. This means that the client must send everything the server > > needs in a single request as the client must not assume any > > state-storing on the part of the server or transport. > > Maybe it's worth mentioning that support in the actual remote helpers > will be added in a subsequent patch.
I can mention that.
Show 21 quoted lines
>
> > If a stateless connection cannot be established then the remote-helper
> > will respond in the same manner as the 'connect' command indicating that
> > the client should fallback to using the dumb remote-helper commands.
>
> This makes sense, but there doesn't seem to be any code in this patch
> that implements this.
>
> > @@ -612,6 +615,11 @@ static int process_connect_service(struct transport *transport,
> > if (data->connect) {
> > strbuf_addf(&cmdbuf, "connect %s\n", name);
> > ret = run_connect(transport, &cmdbuf);
> > + } else if (data->stateless_connect) {
> > + strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
> > + ret = run_connect(transport, &cmdbuf);
> > + if (ret)
> > + transport->stateless_rpc = 1;
>
> Why is process_connect_service() falling back to stateless_connect if
> connect doesn't work? I don't think this fallback would work, as a
> client that needs "connect" might need its full capabilities.Right now there isn't really a notion of "needing" connect since if connect fails then you need to fallback to doing the dumb thing. Also note that there isn't all fallback from connect to stateless-connect here. If the remote helper advertises connect, only connect will be tried even if stateless-connect is advertised. So this only really works in the case where stateless-connect is advertised and connect isn't, as is with our http remote-helper.
-- Brandon Williams