Re: [PATCH 1/2] transport-helper: no connection restriction in connect_helper

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 19, 2023, 17:18 UTC
Message-ID: <xmqqy1h2f5dv.fsf@gitster.g>
In-Reply-To: <20230919064156.13892-1-worldhello.net@gmail.com>

Jiang Xin <worldhello.net@gmail.com> writes:

Show 8 quoted lines

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

Show 5 quoted lines

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

Show 18 quoted lines

> 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);