On Fri, Jan 19, 2024 at 6:26 AM Linus Arver <linusa@xxxxxxxxxx> wrote:
>
> 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().
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().
>
> >> 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
