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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 25, 2023, 21:11 UTC
Message-ID: <xmqqjzserma3.fsf@gitster.g>
In-Reply-To: <20230923152201.14741-2-worldhello.net@gmail.com>

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

Show 6 quoted lines

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

Show 19 quoted lines

> Helped-by: Junio C Hamano <gitster@pobox.com>
> 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);