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

From: Jiang Xin <worldhello.net@gmail.com>
Date: Jan 19, 2024, 10:56 UTC
Message-ID: <CANYiYbGy-APMD7Cw=m-=8dkMcXCp9c+x_6OCoBhWBfvUUWm2ow@mail.gmail.com>
In-Reply-To: <owly1qaei8hw.fsf@fine.c.googlers.com>

On Fri, Jan 19, 2024 at 6:26 AM Linus Arver <linusa@google.com> wrote:

Show 53 quoted lines

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

If both data->connect and data->stateless_connect are false, process_connect_service() will return 0 instead of making a connection and returning 1. The return value will be checked in the function connect_helper() as follows:

        if (!process_connect_service(transport, name, exec))
                die(_("can't connect to subservice %s"), name);

So I think it's not necessary to make double check in connect_helper().

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

The best position to address the bug that both data->connect and data->stateless_connect are enabled is in the function get_helper() as below:

        } else if (!strcmp(capname, "connect")) {
                data->connect = 1;
        } else if (!strcmp(capname, "stateless-connect")) {
                data->stateless_connect = 1;
        }
        ... ...
        if (data->connect && data->stateless_connect)
                die("cannot have both connect and stateless_connect enabled");

I consider this change to be off-topic and it will not be introduced in this series.

-- Jiang Xin