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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux