Jiang Xin <worldhello.net@xxxxxxxxx> 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.
>> >
>> > Helped-by: Junio C Hamano <gitster@xxxxxxxxx>
>> > Signed-off-by: Jiang Xin <zhiyou.jx@xxxxxxxxxxxxxxx>
>> > ---
>> > 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?
>>
>
> It's not necessary to check data->connect here, because it will
> terminate if fail to connect by calling the function
> "process_connect_service()".
In the process_connect_service() we have
if (data->connect) {
...
} else if (data->stateless_connect && ...) {
...
}
strbuf_release(&cmdbuf);
return ret;
and so if both data->connect and data->stateless_connect are false, that
function could silently do nothing. IOW that function expects the
connection type to be guaranteed to be set, so it makes sense to check
for the correctness of this in the connect_helper().
>> 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?)
>
> In the function "process_connect_service()", we can see that "connect"
> has a higher priority than "stateless-connect".
What I mean is, does it make sense for connect_helper() to recognize
invalid or possibly buggy states? IOW, is having both data->connect and
data->stateless_connect enabled a bug? If we only ever set one or the
other (we treat them as mutually exclusive) elsewhere in the codebase,
and if we are doing the sort of "correctness" check in the
connect_helper(), then it makes sense to detect that both are set and
print an error or warning (as a programmer bug).
