On Thu, 22 Feb 2018 10:53:53 -0800
Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> > > @@ -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.
After some in-office discussion, I think I understand how this works.
Assuming a HTTP server that supports protocol v2 (at least for
ls-refs/fetch):
1. Fetch, which supports protocol v2, will (indirectly) call
process_connect_service. If it learns that it supports v2, it must
know that what's returned may not be a fully bidirectional channel,
but may only be a stateless-connect channel (and it does know).
2. Archive/upload-archive, which does not support protocol v2, will
(indirectly) call process_connect_service. stateless_connect checks
info/refs and observes that the server supports protocol v2, so it
returns a stateless-connect channel. The user, being unaware of
protocol versions, tries to use it, and it doesn't work. (This is a
slight regression in that previously, it would fail more quickly -
archive/upload-archive has always not supported HTTP because HTTP
doesn't support connect.)
I still think that it's too confusing for process_connect_service() to
attempt to fallback to stateless-connect, at least because the user must
remember that process_connect_service() returns such a channel if
protocol v2 is used (and existing code must be updated to know this).
It's probably better to have a new API that can return either a connect
channel or a stateless-connect channel, and the user will always use it
as if it was a stateless-connect channel. The old API then can be
separately deprecated and removed, if desired.
