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

[Junio C Hamano <gitster@pobox.com>]

Jiang Xin <worldhello.net@gmail.com> 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 <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);