| Message ID | 20230919064156.13892-1-worldhello.net@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] transport-helper: no connection restriction in connect_helper | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > > For protocol-v2, "stateless-connection" can be used to establish a > stateless connection between the client and the server, but trying to > establish http connection by calling "transport->vtable->connect" will > fail. This restriction was first introduced in commit b236752a87 > (Support remote archive from all smart transports, 2009-12-09) by > adding a limitation in the "connect_helper()" function. The above description may not be technically wrong per-se, but I found it confusing. The ".connect method must be defined" you are removing was added back when there was no "stateless" variant of the connection initiation. Many codepaths added by that patch did "if .connect is there, call it, but otherwise die()" and I think the code you were removing was added as a safety valve, not a limitation or restriction. Later, process_connect_service() learned to handle the .stateless_connect bit as a fallback for transports without .connect method defined, and the commit added that feature, edc9caf7 (transport-helper: introduce stateless-connect, 2018-03-15), forgot that the caller did not allow this fallback. When b236752a (Support remote archive from all smart transports, 2009-12-09) added "remote archive" support for "smart transports", it was for transport that supports the .connect method. connect_helper() function protected itself from getting called for a transport without the method before calling process_connect_service(), which did not work wuth such a transport. Later, edc9caf7 (transport-helper: introduce stateless-connect, 2018-03-15) added a way for a transport without the .connect method to establish a "stateless" connection in protocol-v2, process_connect_service() was taught to handle the "stateless" connection, making the old safety valve in its caller that insisted that .connect method must be defined too strict, and forgot to loosen it. or something along that line would have been easire to follow, at least to me. > Remove the restriction in the "connect_helper()" function and use the > logic in the "process_connect_service()" function to check the protocol > version and service name. By this way, we can make a connection and do > something useful. E.g., in a later commit, implements remote archive > for a repository over HTTP protocol. OK. b236752a87 was to allow "remote archive from all smart transports", but unfortunately HTTP was not among "smart transports". This series is to update smart HTTP transport (aka "stateless") to also support it? Interesting. > Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> > --- > transport-helper.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/transport-helper.c b/transport-helper.c > index 49811ef176..2e127d24a5 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name, > > /* Get_helper so connect is inited. */ > get_helper(transport); > - if (!data->connect) > - die(_("operation not supported by protocol")); > > if (!process_connect_service(transport, name, exec)) > die(_("can't connect to subservice %s"), name);
On Wed, Sep 20, 2023 at 1:19 AM Junio C Hamano <gitster@pobox.com> wrote: > > Jiang Xin <worldhello.net@gmail.com> writes: > > > From: Jiang Xin <zhiyou.jx@alibaba-inc.com> > > > > For protocol-v2, "stateless-connection" can be used to establish a > > stateless connection between the client and the server, but trying to > > establish http connection by calling "transport->vtable->connect" will > > fail. This restriction was first introduced in commit b236752a87 > > (Support remote archive from all smart transports, 2009-12-09) by > > adding a limitation in the "connect_helper()" function. > > The above description may not be technically wrong per-se, but I > found it confusing. The ".connect method must be defined" you are > removing was added back when there was no "stateless" variant of the > connection initiation. Many codepaths added by that patch did "if > .connect is there, call it, but otherwise die()" and I think the > code you were removing was added as a safety valve, not a limitation > or restriction. Later, process_connect_service() learned to handle > the .stateless_connect bit as a fallback for transports without > .connect method defined, and the commit added that feature, edc9caf7 > (transport-helper: introduce stateless-connect, 2018-03-15), forgot > that the caller did not allow this fallback. > > When b236752a (Support remote archive from all smart > transports, 2009-12-09) added "remote archive" support for > "smart transports", it was for transport that supports the > .connect method. connect_helper() function protected itself > from getting called for a transport without the method > before calling process_connect_service(), which did not work > wuth such a transport. > > Later, edc9caf7 (transport-helper: introduce > stateless-connect, 2018-03-15) added a way for a transport > without the .connect method to establish a "stateless" > connection in protocol-v2, process_connect_service() was > taught to handle the "stateless" connection, making the old > safety valve in its caller that insisted that .connect > method must be defined too strict, and forgot to loosen it. > > or something along that line would have been easire to follow, at > least to me. > These explanations are very clear and helpful, thank you. -- Jiang Xin
diff --git a/transport-helper.c b/transport-helper.c index 49811ef176..2e127d24a5 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -662,8 +662,6 @@ static int connect_helper(struct transport *transport, const char *name, /* Get_helper so connect is inited. */ get_helper(transport); - if (!data->connect) - die(_("operation not supported by protocol")); if (!process_connect_service(transport, name, exec)) die(_("can't connect to subservice %s"), name);