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

From: Linus Arver <linusa@google.com>
Date: Jan 18, 2024, 22:26 UTC
Message-ID: <owly1qaei8hw.fsf@fine.c.googlers.com>
In-Reply-To: <CANYiYbFOa-E8Pivhgn_nmy982fn7VPtb803bewnC_UV7qY3xcw@mail.gmail.com>

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

Show 34 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.
>> >
>> > 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?
>>
>
> 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().

Show 5 quoted lines

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