Re: [PATCH v3 28/35] transport-helper: introduce stateless-connect

From: Jonathan Tan <jonathantanmy@google.com>
Date: Feb 22, 2018, 21:55 UTC
Message-ID: <20180222135551.b9131397d4dc2b6bed376d0d@google.com>
In-Reply-To: <20180222185353.GH185096@google.com>

On Thu, 22 Feb 2018 10:53:53 -0800 Brandon Williams <bmwill@google.com> wrote:

Show 21 quoted lines

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