More transport API improvements

Note that this is built on jt/transport-no-more-rsync.

I have found the transport mechanism relatively complicated, so here is some more effort in the hope of making it more readily understood.

Patch 1 is probably good to go in as-is.

Patch 2 is a modification of the transport API by making certain variables in the transport interface struct more private, and might need more discussion. I also discuss the possible future work that this modification makes possible.

Jonathan Tan (2):
clone, fetch: remove redundant transport check
transport: make transport vtable more private

 builtin/clone.c      |  3 ---
 builtin/fetch.c      |  3 ---
 transport-helper.c   | 23 +++++++++++-------
 transport-internal.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 transport.c          | 69 ++++++++++++++++++++++++++++++++--------------------
 transport.h          | 54 ++--------------------------------------
 6 files changed, 120 insertions(+), 93 deletions(-)
 create mode 100644 transport-internal.h

2.15.1.504.g5279b80103-goog

Prior to commit a2d725b7bdf7 ("Use an external program to implement fetching with curl", 2009-08-05), if Git was compiled with NO_CURL, the get_refs_list and fetch methods in struct transport might not be populated, hence the checks in clone and fetch. After that commit, all transports populate get_refs_list and fetch, making the checks in clone and fetch redundant. Remove those checks.

---
 builtin/clone.c | 3 ---
 builtin/fetch.c | 3 ---
 2 files changed, 6 deletions(-)

index dbddd98f8..979af0383 100644
@@ -1083,9 +1083,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		warning(_("--local is ignored"));
 	transport->cloning = 1;
 
-	if (!transport->get_refs_list || (!is_local && !transport->fetch))
-		die(_("Don't know how to clone %s"), transport->url);
-
 	transport_set_option(transport, TRANS_OPT_KEEP, "yes");
 
 	if (option_depth)

index 225c73492..09eb1fc17 100644
@@ -1094,9 +1094,6 @@ static int do_fetch(struct transport *transport,
 			tags = TAGS_UNSET;
 	}
 
-	if (!transport->get_refs_list || !transport->fetch)
-		die(_("Don't know how to fetch from %s"), transport->url);
-
 	/* if not appending, truncate FETCH_HEAD */
 	if (!append && !dry_run) {
 		retcode = truncate_fetch_head();

2.15.1.504.g5279b80103-goog

I do not agree with the above justification. We could view these checks as serving to future-proof the callers for (initially buggy) transports that we have not seen, so the current set of transports not triggering is not a good enough reason to remove them.

But I do agree with the removal of these checks for another reason. A call into transport_get_remote_refs() or transport_fetch_refs() would segfault if these necessary fields are not filled in a buggy transport under development anyway.

And more importantly, if it is desirable to have a check that is more friendly than merely segfaulting, such a check must be added to the implementation of transport API that knows the implementation details like "fetching would require get_refs_list and fetch fields"; the consumers of the API is a wrong place to do so.

Thanks.

Move the definition of the transport-specific functions provided by transports, whether declared in transport.c or transport-helper.c, into an internal header. This means that transport-using code (as opposed to transport-declaring code) can no longer access these functions (without importing the internal header themselves), making it clear that they should use the transport_*() functions instead, and also allowing the interface between the transport mechanism and an individual transport to independently evolve.

This is superficially a reversal of commit 824d5776c3f2 ("Refactor struct transport_ops inlined into struct transport", 2007-09-19). However, the scope of the involved variables was neither affected nor discussed in that commit, and I think that the advantages in making those functions more private outweigh the advantages described in that commit's commit message. A minor additional point is that the code has gotten more complicated since then, in that the function-pointer variables are potentially mutated twice (once initially and once if transport_take_over() is invoked), increasing the value of corralling them into their own struct.

---
The main evolution I see is to move all traces of the "takeover" mechanism to transport.c, instead of having individual transports (the builtin smart transport in transport.c and the remote helper interface in transport-helper.c) do the connect-then-fetch etc. thing themselves. This means that transport_vtable->connect must be allowed to return "fallback", and we have the following vtables:
- the one for bundles (unchanged)
- one for builtin smart transports in which fetch and push_refs are NULL
- one for remote helpers (unchanged)
- one for the result of takeover (unchanged) (to be used when get_refs_list/fetch/pull is used with a connect-supporting transport)
- one for the result of transport_connect (connect, fetch, and push_refs are NULL) (to be used when user code invokes transport_connect in order to control the stream directly)

transport_get_remote_refs() and friends will always attempt to connect (and perform the takeover if it succeeds) before proceeding.

In this way, functionality becomes clearer:
- Transports that do not support connect leave connect as NULL and implement get_refs_list/fetch/pull.
- Transports that support connect implement connect. If connect is do-or-die (e.g. builtin smart transports), leave get_refs_list, fetch, and pull as NULL. Otherwise (e.g. remote helpers) populate them, and they will be used as a fallback if connect fails.
- When transport_connect() is invoked, the vtable is switched so that connect/get_refs_list/fetch/pull no longer work. The user code has full control.
- When transport_get_remote_refs() (or friends) is invoked, if connect is successful, the vtable is switched so that connect no longer works. get_refs_list/fetch/pull use the established connection, and it is clear that the user can no longer call transport_connect().

This seems feasible to me, but I haven't tried to implement it yet.

---
 transport-helper.c   | 23 +++++++++++-------
 transport-internal.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 transport.c          | 69 ++++++++++++++++++++++++++++++++--------------------
 transport.h          | 54 ++--------------------------------------
 4 files changed, 120 insertions(+), 87 deletions(-)
 create mode 100644 transport-internal.h

index c948d5215..1a4b43ff1 100644
@@ -11,6 +11,7 @@
 #include "sigchain.h"
 #include "argv-array.h"
 #include "refs.h"
+#include "transport-internal.h"
 
 static int debug;
 

@@ -650,7 +651,7 @@ static int fetch(struct transport *transport,
 
 	if (process_connect(transport, 0)) {
 		do_take_over(transport);
-		return transport->fetch(transport, nr_heads, to_fetch);
+		return transport->vtable->fetch(transport, nr_heads, to_fetch);
 	}
 
 	count = 0;

@@ -987,7 +988,7 @@ static int push_refs(struct transport *transport,
 
 	if (process_connect(transport, 1)) {
 		do_take_over(transport);
-		return transport->push_refs(transport, remote_refs, flags);
+		return transport->vtable->push_refs(transport, remote_refs, flags);
 	}
 
 	if (!remote_refs) {

@@ -1035,7 +1036,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 
 	if (process_connect(transport, for_push)) {
 		do_take_over(transport);
-		return transport->get_refs_list(transport, for_push);
+		return transport->vtable->get_refs_list(transport, for_push);
 	}
 
 	if (data->push && for_push)

@@ -1084,6 +1085,15 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)
 	return ret;
 }
 
+static struct transport_vtable vtable = {
+	set_helper_option,
+	get_refs_list,
+	fetch,
+	push_refs,
+	connect_helper,
+	release_helper
+};
+
 int transport_helper_init(struct transport *transport, const char *name)
 {
 	struct helper_data *data = xcalloc(1, sizeof(*data));

@@ -1095,12 +1105,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 		debug = 1;
 
 	transport->data = data;
-	transport->set_option = set_helper_option;
-	transport->get_refs_list = get_refs_list;
-	transport->fetch = fetch;
-	transport->push_refs = push_refs;
-	transport->disconnect = release_helper;
-	transport->connect = connect_helper;
+	transport->vtable = &vtable;
 	transport->smart_options = &(data->transport_options);
 	return 0;
 }

2.15.1.504.g5279b80103-goog

Yay!