[v2,1/3] transport-helper: no connection restriction in connect_helper

Commit Message

Jiang Xin Sept. 23, 2023, 3:21 p.m. UTC

From: Jiang Xin <[e-mail]>

When commit 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. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which did not work with such a transport.

Later, commit 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, which
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.

Remove the restriction in the "connect_helper()" function and give the
function "process_connect_service()" the opportunity to establish a
connection using ".connect" or ".stateless_connect" for protocol v2. So
we can connect with a stateless-rpc and do something useful. E.g., in a
later commit, implements remote archive for a repository over HTTP
protocol.

Helped-by: Junio C Hamano <[e-mail]>
Signed-off-by: Jiang Xin <[e-mail]>
---
 transport-helper.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Junio C Hamano Sept. 25, 2023, 9:11 p.m. UTC | #1

Jiang Xin <[e-mail]> writes:

> Remove the restriction in the "connect_helper()" function and give the
> function "process_connect_service()" the opportunity to establish a
> connection using ".connect" or ".stateless_connect" for protocol v2. So
> we can connect with a stateless-rpc and do something useful. E.g., in a
> later commit, implements remote archive for a repository over HTTP
> protocol.

OK.  Given that process_connect_service() does this:

	if (data->connect) {
		strbuf_addf(&cmdbuf, "connect %s\n", name);
		ret = run_connect(transport, &cmdbuf);
	} else if (data->stateless_connect &&
		   (get_protocol_version_config() == protocol_v2) &&
		   (!strcmp("git-upload-pack", name) ||
		    !strcmp("git-upload-archive", name))) {
		strbuf_addf(&cmdbuf, "stateless-connect %s\n", name);
		ret = run_connect(transport, &cmdbuf);
		if (ret)
			transport->stateless_rpc = 1;
	}

in the spirit of the original "safety valve", it becomes tempting to
suggest we make sure at least .connect or .stateless_connect exists
in the transport to be safe, but then we will need to keep the logic
of safety valve in connect_helper() and the actual dispatching in
process_connect_service() in sync, which is a maintenance burden.

It however makes me wonder if we should add

	else
		die(_("operation not supported by protocol"));

at the end of the "if/else if" cascade in process_connect_service(),
so that callers that end up following this callpath with a transport
that defines neither would be caught.

Other than that, the patch is as good as the previous round, and the
explanation is vastly easier to understand.

Thanks.

> Helped-by: Junio C Hamano <[e-mail]>
> Signed-off-by: Jiang Xin <[e-mail]>
> ---
>  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);

Patch

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