Linus Arver [off-list ref] writes:

Jiang Xin [off-list ref] writes:

quoted

From: Jiang Xin <redacted>

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.

quoted

which did not work with such a transport.

How about 'which only worked with the ".connect" method.' ?

quoted

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

quoted

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.

quoted

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.

quoted

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 [off-list ref]
Signed-off-by: Jiang Xin <redacted>
---
 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?)

quoted

 	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.