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

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

Linus Arver <linusa@google.com> writes:

> Jiang Xin <worldhello.net@gmail.com> writes:
>
>> From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
>>
>> 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(),
>
> OK.
>
>> which did not work with such a transport.
>
> How about 'which only worked with the ".connect" method.' ?
>
>>
>> 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
>
> s/which/where
>
>> process_connect_service() was taught to handle the "stateless"
>> connection,
>
> I think using 'the ".stateless_connect" method' is more consistent with
> the rest of this text.
>
>> making the old safety valve in its caller that insisted
>> that ".connect" method must be defined too strict, and forgot to loosen
>> it.
>
> I think just "...making the old protection too strict. But edc9caf7
> forgot to adjust this protection accordingly." is simpler to read.
>
>> 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 <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"));
>
> Should we still terminate early here if both data->connect and
> data->stateless_connect are not truthy? It would save a few CPU cycles,
> but even better, remain true to the the original intent of the code.
> Maybe there was a really good reason to terminate early here that we're
> not aware of?
>
> But also, what about the case where both are enabled? Should we print an
> error message? (Maybe this concern is outside the scope of this series?)
>
>>  	if (!process_connect_service(transport, name, exec))
>>  		die(_("can't connect to subservice %s"), name);
>> -- 
>> 2.41.0.232.g2f6f0bca4f.agit.8.0.4.dev

Thanks for a review to get the topic that hasn't seen much reviews
unstuck.  Very much appreciated.