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

[Brandon Williams]

On 02/27, Jonathan Nieder wrote:
> Brandon Williams 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.
> >
> > 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.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  transport-helper.c | 8 
> >  transport.c| 1 +
> >  transport.h| 6 ++
> >  3 files changed, 15 insertions(+)
> 
> Please add documentation for this command to
> Documentation/gitremote-helpers.txt.
> 
> That helps reviewers, since it means reviewers can get a sense of what
> the interface is meant to be.  It helps remote helper implementers as
> well: it tells them what they can rely on and what can't rely on in
> this interface.  For the same reason it helpers remote helper callers
> as well.
> 
> Thanks,
> Jonathan

Thanks for reminding me.  I had intended to at some point but had
forgotten to do so.  I'm going to mark this it as experimental and for
internal use only so that we can still tweak the interface if we want
before it becomes stable.

-- 
Brandon Williams

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

[Jonathan Nieder]

Brandon Williams 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.
>
> 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.
>
> Signed-off-by: Brandon Williams 
> ---
>  transport-helper.c | 8 
>  transport.c| 1 +
>  transport.h| 6 ++
>  3 files changed, 15 insertions(+)

Please add documentation for this command to
Documentation/gitremote-helpers.txt.

That helps reviewers, since it means reviewers can get a sense of what
the interface is meant to be.  It helps remote helper implementers as
well: it tells them what they can rely on and what can't rely on in
this interface.  For the same reason it helpers remote helper callers
as well.

Thanks,
Jonathan

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

[Jonathan Tan]

On Thu, 22 Feb 2018 10:53:53 -0800
Brandon Williams  wrote:

> > > @@ -612,6 +615,11 @@ static int process_connect_service(struct transport 
> > > *transport,
> > >   if (data->connect) {
> > >   strbuf_addf(, "connect %s\n", name);
> > >   ret = run_connect(transport, );
> > > + } else if (data->stateless_connect) {
> > > + strbuf_addf(, "stateless-connect %s\n", name);
> > > + ret = run_connect(transport, );
> > > + 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.

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

[Brandon Williams]

On 02/21, Jonathan Tan wrote:
> On Tue,  6 Feb 2018 17:13:05 -0800
> Brandon Williams  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.

> 
> > 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(, "connect %s\n", name);
> > ret = run_connect(transport, );
> > +   } else if (data->stateless_connect) {
> > +   strbuf_addf(, "stateless-connect %s\n", name);
> > +   ret = run_connect(transport, );
> > +   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

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

[Jonathan Tan]

On Tue,  6 Feb 2018 17:13:05 -0800
Brandon Williams  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.

> 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(, "connect %s\n", name);
>   ret = run_connect(transport, );
> + } else if (data->stateless_connect) {
> + strbuf_addf(, "stateless-connect %s\n", name);
> + ret = run_connect(transport, );
> + 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.

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

[Brandon Williams]

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.

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.

Signed-off-by: Brandon Williams 
---
 transport-helper.c | 8 
 transport.c| 1 +
 transport.h| 6 ++
 3 files changed, 15 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index c032a2a87..82eb57c4a 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -26,6 +26,7 @@ struct helper_data {
option : 1,
push : 1,
connect : 1,
+   stateless_connect : 1,
signed_tags : 1,
check_connectivity : 1,
no_disconnect_req : 1,
@@ -188,6 +189,8 @@ static struct child_process *get_helper(struct transport 
*transport)
refspecs[refspec_nr++] = xstrdup(arg);
} else if (!strcmp(capname, "connect")) {
data->connect = 1;
+   } else if (!strcmp(capname, "stateless-connect")) {
+   data->stateless_connect = 1;
} else if (!strcmp(capname, "signed-tags")) {
data->signed_tags = 1;
} else if (skip_prefix(capname, "export-marks ", )) {
@@ -612,6 +615,11 @@ static int process_connect_service(struct transport 
*transport,
if (data->connect) {
strbuf_addf(, "connect %s\n", name);
ret = run_connect(transport, );
+   } else if (data->stateless_connect) {
+   strbuf_addf(, "stateless-connect %s\n", name);
+   ret = run_connect(transport, );
+   if (ret)
+   transport->stateless_rpc = 1;
}
 
strbuf_release();
diff --git a/transport.c b/transport.c
index c275f46ed..9125174f7 100644
--- a/transport.c
+++ b/transport.c
@@ -250,6 +250,7 @@ static int fetch_refs_via_pack(struct transport *transport,
data->options.check_self_contained_and_connected;
args.cloning = transport->cloning;
args.update_shallow = data->options.update_shallow;
+   args.stateless_rpc = transport->stateless_rpc;
 
if (!data->got_remote_heads)
refs_tmp = get_refs_via_connect(transport, 0, NULL);
diff --git a/transport.h b/transport.h
index 4b656f315..9eac809ee 100644
--- a/transport.h
+++ b/transport.h
@@ -55,6 +55,12 @@ struct transport {
 */
unsigned cloning : 1;
 
+   /*
+* Indicates that the transport is connected via a half-duplex
+* connection and should operate in stateless-rpc mode.
+*/
+   unsigned stateless_rpc : 1;
+
/*
 * These strings will be passed to the {pre, post}-receive hook,
 * on the remote side, if both sides support the push options 
capability.
-- 
2.16.0.rc1.238.g530d649a79-goog