On Thu, 22 Feb 2018 10:53:53 -0800
Brandon Williams [off-list ref] wrote:

quoted

quoted

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