{"thread":{"id":"47445","subject":"[PATCH 0/2] More transport API improvements","startedAt":"2017-12-14T21:45:04Z","lastAt":"2017-12-14T22:13:54Z","messageCount":5,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"334847","messageId":"cover.1513287544.git.jonathantanmy@google.com","threadId":"47445","inReplyTo":null,"subject":"[PATCH 0/2] More transport API improvements","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-12-14T21:44:43Z","receivedAt":"2017-12-14T21:45:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Note that this is built on jt/transport-no-more-rsync.\n\nI have found the transport mechanism relatively complicated, so here is\nsome more effort in the hope of making it more readily understood.\n\nPatch 1 is probably good to go in as-is.\n\nPatch 2 is a modification of the transport API by making certain\nvariables in the transport interface struct more private, and might need\nmore discussion. I also discuss the possible future work that this\nmodification makes possible.\n\nJonathan Tan (2):\n  clone, fetch: remove redundant transport check\n  transport: make transport vtable more private\n\n builtin/clone.c      |  3 ---\n builtin/fetch.c      |  3 ---\n transport-helper.c   | 23 +++++++++++-------\n transport-internal.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++\n transport.c          | 69 ++++++++++++++++++++++++++++++++--------------------\n transport.h          | 54 ++--------------------------------------\n 6 files changed, 120 insertions(+), 93 deletions(-)\n create mode 100644 transport-internal.h\n\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334848","messageId":"26f276956001a120cc9105b0071762c2fd4a45c5.1513287544.git.jonathantanmy@google.com","threadId":"47445","inReplyTo":"cover.1513287544.git.jonathantanmy@google.com","subject":"[PATCH 2/2] transport: make transport vtable more private","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-12-14T21:44:45Z","receivedAt":"2017-12-14T21:45:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Move the definition of the transport-specific functions provided by\ntransports, whether declared in transport.c or transport-helper.c, into\nan internal header. This means that transport-using code (as opposed to\ntransport-declaring code) can no longer access these functions (without\nimporting the internal header themselves), making it clear that they\nshould use the transport_*() functions instead, and also allowing the\ninterface between the transport mechanism and an individual transport to\nindependently evolve.\n\nThis is superficially a reversal of commit 824d5776c3f2 (\"Refactor\nstruct transport_ops inlined into struct transport\", 2007-09-19).\nHowever, the scope of the involved variables was neither affected nor\ndiscussed in that commit, and I think that the advantages in making\nthose functions more private outweigh the advantages described in that\ncommit's commit message. A minor additional point is that the code has\ngotten more complicated since then, in that the function-pointer\nvariables are potentially mutated twice (once initially and once if\ntransport_take_over() is invoked), increasing the value of corralling\nthem into their own struct.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThe main evolution I see is to move all traces of the \"takeover\"\nmechanism to transport.c, instead of having individual transports (the\nbuiltin smart transport in transport.c and the remote helper\ninterface in transport-helper.c) do the connect-then-fetch etc. thing\nthemselves. This means that transport_vtable->connect must be allowed to\nreturn \"fallback\", and we have the following vtables:\n - the one for bundles (unchanged)\n - one for builtin smart transports in which fetch and push_refs are\n   NULL\n - one for remote helpers (unchanged)\n - one for the result of takeover (unchanged) (to be used when\n   get_refs_list/fetch/pull is used with a connect-supporting transport)\n - one for the result of transport_connect (connect, fetch, and\n   push_refs are NULL) (to be used when user code invokes\n   transport_connect in order to control the stream directly)\n\ntransport_get_remote_refs() and friends will always attempt to connect\n(and perform the takeover if it succeeds) before proceeding.\n\nIn this way, functionality becomes clearer:\n - Transports that do not support connect leave connect as NULL and\n   implement get_refs_list/fetch/pull.\n - Transports that support connect implement connect. If connect is\n   do-or-die (e.g. builtin smart transports), leave get_refs_list,\n   fetch, and pull as NULL. Otherwise (e.g. remote helpers) populate\n   them, and they will be used as a fallback if connect fails.\n - When transport_connect() is invoked, the vtable is switched so that\n   connect/get_refs_list/fetch/pull no longer work. The user code has\n   full control.\n - When transport_get_remote_refs() (or friends) is invoked, if connect\n   is successful, the vtable is switched so that connect no longer\n   works. get_refs_list/fetch/pull use the established connection, and\n   it is clear that the user can no longer call transport_connect().\n\nThis seems feasible to me, but I haven't tried to implement it yet.\n---\n transport-helper.c   | 23 +++++++++++-------\n transport-internal.h | 61 ++++++++++++++++++++++++++++++++++++++++++++++\n transport.c          | 69 ++++++++++++++++++++++++++++++++--------------------\n transport.h          | 54 ++--------------------------------------\n 4 files changed, 120 insertions(+), 87 deletions(-)\n create mode 100644 transport-internal.h\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex c948d5215..1a4b43ff1 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -11,6 +11,7 @@\n #include \"sigchain.h\"\n #include \"argv-array.h\"\n #include \"refs.h\"\n+#include \"transport-internal.h\"\n \n static int debug;\n \n@@ -650,7 +651,7 @@ static int fetch(struct transport *transport,\n \n \tif (process_connect(transport, 0)) {\n \t\tdo_take_over(transport);\n-\t\treturn transport->fetch(transport, nr_heads, to_fetch);\n+\t\treturn transport->vtable->fetch(transport, nr_heads, to_fetch);\n \t}\n \n \tcount = 0;\n@@ -987,7 +988,7 @@ static int push_refs(struct transport *transport,\n \n \tif (process_connect(transport, 1)) {\n \t\tdo_take_over(transport);\n-\t\treturn transport->push_refs(transport, remote_refs, flags);\n+\t\treturn transport->vtable->push_refs(transport, remote_refs, flags);\n \t}\n \n \tif (!remote_refs) {\n@@ -1035,7 +1036,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \n \tif (process_connect(transport, for_push)) {\n \t\tdo_take_over(transport);\n-\t\treturn transport->get_refs_list(transport, for_push);\n+\t\treturn transport->vtable->get_refs_list(transport, for_push);\n \t}\n \n \tif (data->push && for_push)\n@@ -1084,6 +1085,15 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n \treturn ret;\n }\n \n+static struct transport_vtable vtable = {\n+\tset_helper_option,\n+\tget_refs_list,\n+\tfetch,\n+\tpush_refs,\n+\tconnect_helper,\n+\trelease_helper\n+};\n+\n int transport_helper_init(struct transport *transport, const char *name)\n {\n \tstruct helper_data *data = xcalloc(1, sizeof(*data));\n@@ -1095,12 +1105,7 @@ int transport_helper_init(struct transport *transport, const char *name)\n \t\tdebug = 1;\n \n \ttransport->data = data;\n-\ttransport->set_option = set_helper_option;\n-\ttransport->get_refs_list = get_refs_list;\n-\ttransport->fetch = fetch;\n-\ttransport->push_refs = push_refs;\n-\ttransport->disconnect = release_helper;\n-\ttransport->connect = connect_helper;\n+\ttransport->vtable = &vtable;\n \ttransport->smart_options = &(data->transport_options);\n \treturn 0;\n }\ndiff --git a/transport-internal.h b/transport-internal.h\nnew file mode 100644\nindex 000000000..3c1a29d72\n--- /dev/null\n+++ b/transport-internal.h\n@@ -0,0 +1,61 @@\n+#ifndef TRANSPORT_INTERNAL_H\n+#define TRANSPORT_INTERNAL_H\n+\n+struct ref;\n+struct transport;\n+\n+struct transport_vtable {\n+\t/**\n+\t * Returns 0 if successful, positive if the option is not\n+\t * recognized or is inapplicable, and negative if the option\n+\t * is applicable but the value is invalid.\n+\t **/\n+\tint (*set_option)(struct transport *connection, const char *name,\n+\t\t\t  const char *value);\n+\t/**\n+\t * Returns a list of the remote side's refs. In order to allow\n+\t * the transport to try to share connections, for_push is a\n+\t * hint as to whether the ultimate operation is a push or a fetch.\n+\t *\n+\t * If the transport is able to determine the remote hash for\n+\t * the ref without a huge amount of effort, it should store it\n+\t * in the ref's old_sha1 field; otherwise it should be all 0.\n+\t **/\n+\tstruct ref *(*get_refs_list)(struct transport *transport, int for_push);\n+\n+\t/**\n+\t * Fetch the objects for the given refs. Note that this gets\n+\t * an array, and should ignore the list structure.\n+\t *\n+\t * If the transport did not get hashes for refs in\n+\t * get_refs_list(), it should set the old_sha1 fields in the\n+\t * provided refs now.\n+\t **/\n+\tint (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);\n+\n+\t/**\n+\t * Push the objects and refs. Send the necessary objects, and\n+\t * then, for any refs where peer_ref is set and\n+\t * peer_ref->new_oid is different from old_oid, tell the\n+\t * remote side to update each ref in the list from old_oid to\n+\t * peer_ref->new_oid.\n+\t *\n+\t * Where possible, set the status for each ref appropriately.\n+\t *\n+\t * The transport must modify new_sha1 in the ref to the new\n+\t * value if the remote accepted the change. Note that this\n+\t * could be a different value from peer_ref->new_oid if the\n+\t * process involved generating new commits.\n+\t **/\n+\tint (*push_refs)(struct transport *transport, struct ref *refs, int flags);\n+\tint (*connect)(struct transport *connection, const char *name,\n+\t\t       const char *executable, int fd[2]);\n+\n+\t/** get_refs_list(), fetch(), and push_refs() can keep\n+\t * resources (such as a connection) reserved for further\n+\t * use. disconnect() releases these resources.\n+\t **/\n+\tint (*disconnect)(struct transport *connection);\n+};\n+\n+#endif\ndiff --git a/transport.c b/transport.c\nindex 8c4fc5477..31b6d4d0e 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -17,6 +17,7 @@\n #include \"string-list.h\"\n #include \"sha1-array.h\"\n #include \"sigchain.h\"\n+#include \"transport-internal.h\"\n \n static void set_upstreams(struct transport *transport, struct ref *refs,\n \tint pretend)\n@@ -607,6 +608,15 @@ static int disconnect_git(struct transport *transport)\n \treturn 0;\n }\n \n+static struct transport_vtable taken_over_vtable = {\n+\tNULL,\n+\tget_refs_via_connect,\n+\tfetch_refs_via_pack,\n+\tgit_transport_push,\n+\tNULL,\n+\tdisconnect_git\n+};\n+\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child)\n {\n@@ -624,11 +634,7 @@ void transport_take_over(struct transport *transport,\n \tdata->got_remote_heads = 0;\n \ttransport->data = data;\n \n-\ttransport->set_option = NULL;\n-\ttransport->get_refs_list = get_refs_via_connect;\n-\ttransport->fetch = fetch_refs_via_pack;\n-\ttransport->push_refs = git_transport_push;\n-\ttransport->disconnect = disconnect_git;\n+\ttransport->vtable = &taken_over_vtable;\n \ttransport->smart_options = &(data->options);\n \n \ttransport->cannot_reuse = 1;\n@@ -751,6 +757,24 @@ void transport_check_allowed(const char *type)\n \t\tdie(\"transport '%s' not allowed\", type);\n }\n \n+static struct transport_vtable bundle_vtable = {\n+\tNULL,\n+\tget_refs_from_bundle,\n+\tfetch_refs_from_bundle,\n+\tNULL,\n+\tNULL,\n+\tclose_bundle\n+};\n+\n+static struct transport_vtable builtin_smart_vtable = {\n+\tNULL,\n+\tget_refs_via_connect,\n+\tfetch_refs_via_pack,\n+\tgit_transport_push,\n+\tconnect_git,\n+\tdisconnect_git\n+};\n+\n struct transport *transport_get(struct remote *remote, const char *url)\n {\n \tconst char *helper;\n@@ -787,9 +811,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tstruct bundle_transport_data *data = xcalloc(1, sizeof(*data));\n \t\ttransport_check_allowed(\"file\");\n \t\tret->data = data;\n-\t\tret->get_refs_list = get_refs_from_bundle;\n-\t\tret->fetch = fetch_refs_from_bundle;\n-\t\tret->disconnect = close_bundle;\n+\t\tret->vtable = &bundle_vtable;\n \t\tret->smart_options = NULL;\n \t} else if (!is_url(url)\n \t\t|| starts_with(url, \"file://\")\n@@ -804,12 +826,7 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\t */\n \t\tstruct git_transport_data *data = xcalloc(1, sizeof(*data));\n \t\tret->data = data;\n-\t\tret->set_option = NULL;\n-\t\tret->get_refs_list = get_refs_via_connect;\n-\t\tret->fetch = fetch_refs_via_pack;\n-\t\tret->push_refs = git_transport_push;\n-\t\tret->connect = connect_git;\n-\t\tret->disconnect = disconnect_git;\n+\t\tret->vtable = &builtin_smart_vtable;\n \t\tret->smart_options = &(data->options);\n \n \t\tdata->conn = NULL;\n@@ -843,9 +860,9 @@ int transport_set_option(struct transport *transport,\n \t\tgit_reports = set_git_option(transport->smart_options,\n \t\t\t\t\t     name, value);\n \n-\tif (transport->set_option)\n-\t\tprotocol_reports = transport->set_option(transport, name,\n-\t\t\t\t\t\t\tvalue);\n+\tif (transport->vtable->set_option)\n+\t\tprotocol_reports = transport->vtable->set_option(transport,\n+\t\t\t\t\t\t\t\t name, value);\n \n \t/* If either report is 0, report 0 (success). */\n \tif (!git_reports || !protocol_reports)\n@@ -968,7 +985,7 @@ int transport_push(struct transport *transport,\n \t*reject_reasons = 0;\n \ttransport_verify_remote_names(refspec_nr, refspec);\n \n-\tif (transport->push_refs) {\n+\tif (transport->vtable->push_refs) {\n \t\tstruct ref *remote_refs;\n \t\tstruct ref *local_refs = get_local_heads();\n \t\tint match_flags = MATCH_REFS_NONE;\n@@ -981,7 +998,7 @@ int transport_push(struct transport *transport,\n \t\tif (check_push_refs(local_refs, refspec_nr, refspec) < 0)\n \t\t\treturn -1;\n \n-\t\tremote_refs = transport->get_refs_list(transport, 1);\n+\t\tremote_refs = transport->vtable->get_refs_list(transport, 1);\n \n \t\tif (flags & TRANSPORT_PUSH_ALL)\n \t\t\tmatch_flags |= MATCH_REFS_ALL;\n@@ -1056,7 +1073,7 @@ int transport_push(struct transport *transport,\n \t\t}\n \n \t\tif (!(flags & TRANSPORT_RECURSE_SUBMODULES_ONLY))\n-\t\t\tpush_ret = transport->push_refs(transport, remote_refs, flags);\n+\t\t\tpush_ret = transport->vtable->push_refs(transport, remote_refs, flags);\n \t\telse\n \t\t\tpush_ret = 0;\n \t\terr = push_had_errors(remote_refs);\n@@ -1090,7 +1107,7 @@ int transport_push(struct transport *transport,\n const struct ref *transport_get_remote_refs(struct transport *transport)\n {\n \tif (!transport->got_remote_refs) {\n-\t\ttransport->remote_refs = transport->get_refs_list(transport, 0);\n+\t\ttransport->remote_refs = transport->vtable->get_refs_list(transport, 0);\n \t\ttransport->got_remote_refs = 1;\n \t}\n \n@@ -1127,7 +1144,7 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)\n \t\t\theads[nr_heads++] = rm;\n \t}\n \n-\trc = transport->fetch(transport, nr_heads, heads);\n+\trc = transport->vtable->fetch(transport, nr_heads, heads);\n \n \tfree(heads);\n \treturn rc;\n@@ -1144,8 +1161,8 @@ void transport_unlock_pack(struct transport *transport)\n int transport_connect(struct transport *transport, const char *name,\n \t\t      const char *exec, int fd[2])\n {\n-\tif (transport->connect)\n-\t\treturn transport->connect(transport, name, exec, fd);\n+\tif (transport->vtable->connect)\n+\t\treturn transport->vtable->connect(transport, name, exec, fd);\n \telse\n \t\tdie(\"Operation not supported by protocol\");\n }\n@@ -1153,8 +1170,8 @@ int transport_connect(struct transport *transport, const char *name,\n int transport_disconnect(struct transport *transport)\n {\n \tint ret = 0;\n-\tif (transport->disconnect)\n-\t\tret = transport->disconnect(transport);\n+\tif (transport->vtable->disconnect)\n+\t\tret = transport->vtable->disconnect(transport);\n \tfree(transport);\n \treturn ret;\n }\ndiff --git a/transport.h b/transport.h\nindex ab4fe7f27..731c78b67 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -30,6 +30,8 @@ enum transport_family {\n };\n \n struct transport {\n+\tconst struct transport_vtable *vtable;\n+\n \tstruct remote *remote;\n \tconst char *url;\n \tvoid *data;\n@@ -59,58 +61,6 @@ struct transport {\n \t */\n \tconst struct string_list *push_options;\n \n-\t/**\n-\t * Returns 0 if successful, positive if the option is not\n-\t * recognized or is inapplicable, and negative if the option\n-\t * is applicable but the value is invalid.\n-\t **/\n-\tint (*set_option)(struct transport *connection, const char *name,\n-\t\t\t  const char *value);\n-\n-\t/**\n-\t * Returns a list of the remote side's refs. In order to allow\n-\t * the transport to try to share connections, for_push is a\n-\t * hint as to whether the ultimate operation is a push or a fetch.\n-\t *\n-\t * If the transport is able to determine the remote hash for\n-\t * the ref without a huge amount of effort, it should store it\n-\t * in the ref's old_sha1 field; otherwise it should be all 0.\n-\t **/\n-\tstruct ref *(*get_refs_list)(struct transport *transport, int for_push);\n-\n-\t/**\n-\t * Fetch the objects for the given refs. Note that this gets\n-\t * an array, and should ignore the list structure.\n-\t *\n-\t * If the transport did not get hashes for refs in\n-\t * get_refs_list(), it should set the old_sha1 fields in the\n-\t * provided refs now.\n-\t **/\n-\tint (*fetch)(struct transport *transport, int refs_nr, struct ref **refs);\n-\n-\t/**\n-\t * Push the objects and refs. Send the necessary objects, and\n-\t * then, for any refs where peer_ref is set and\n-\t * peer_ref->new_oid is different from old_oid, tell the\n-\t * remote side to update each ref in the list from old_oid to\n-\t * peer_ref->new_oid.\n-\t *\n-\t * Where possible, set the status for each ref appropriately.\n-\t *\n-\t * The transport must modify new_sha1 in the ref to the new\n-\t * value if the remote accepted the change. Note that this\n-\t * could be a different value from peer_ref->new_oid if the\n-\t * process involved generating new commits.\n-\t **/\n-\tint (*push_refs)(struct transport *transport, struct ref *refs, int flags);\n-\tint (*connect)(struct transport *connection, const char *name,\n-\t\t       const char *executable, int fd[2]);\n-\n-\t/** get_refs_list(), fetch(), and push_refs() can keep\n-\t * resources (such as a connection) reserved for further\n-\t * use. disconnect() releases these resources.\n-\t **/\n-\tint (*disconnect)(struct transport *connection);\n \tchar *pack_lockfile;\n \tsigned verbose : 3;\n \t/**\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334849","messageId":"d1fd27a5a102129aa16ecf6e6c19014f7b60b543.1513287544.git.jonathantanmy@google.com","threadId":"47445","inReplyTo":"cover.1513287544.git.jonathantanmy@google.com","subject":"[PATCH 1/2] clone, fetch: remove redundant transport check","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-12-14T21:44:44Z","receivedAt":"2017-12-14T21:45:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Prior to commit a2d725b7bdf7 (\"Use an external program to implement\nfetching with curl\", 2009-08-05), if Git was compiled with NO_CURL, the\nget_refs_list and fetch methods in struct transport might not be\npopulated, hence the checks in clone and fetch. After that commit, all\ntransports populate get_refs_list and fetch, making the checks in clone\nand fetch redundant. Remove those checks.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/clone.c | 3 ---\n builtin/fetch.c | 3 ---\n 2 files changed, 6 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dbddd98f8..979af0383 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1083,9 +1083,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\twarning(_(\"--local is ignored\"));\n \ttransport->cloning = 1;\n \n-\tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n-\t\tdie(_(\"Don't know how to clone %s\"), transport->url);\n-\n \ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n \n \tif (option_depth)\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 225c73492..09eb1fc17 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1094,9 +1094,6 @@ static int do_fetch(struct transport *transport,\n \t\t\ttags = TAGS_UNSET;\n \t}\n \n-\tif (!transport->get_refs_list || !transport->fetch)\n-\t\tdie(_(\"Don't know how to fetch from %s\"), transport->url);\n-\n \t/* if not appending, truncate FETCH_HEAD */\n \tif (!append && !dry_run) {\n \t\tretcode = truncate_fetch_head();\n-- \n2.15.1.504.g5279b80103-goog\n\n"},{"id":"334851","messageId":"xmqqmv2lrmai.fsf@gitster.mtv.corp.google.com","threadId":"47445","inReplyTo":"d1fd27a5a102129aa16ecf6e6c19014f7b60b543.1513287544.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] clone, fetch: remove redundant transport check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-14T22:12:53Z","receivedAt":"2017-12-14T22:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Prior to commit a2d725b7bdf7 (\"Use an external program to implement\n> fetching with curl\", 2009-08-05), if Git was compiled with NO_CURL, the\n> get_refs_list and fetch methods in struct transport might not be\n> populated, hence the checks in clone and fetch. After that commit, all\n> transports populate get_refs_list and fetch, making the checks in clone\n> and fetch redundant. Remove those checks.\n\nI do not agree with the above justification.  We could view these\nchecks as serving to future-proof the callers for (initially buggy)\ntransports that we have not seen, so the current set of transports\nnot triggering is not a good enough reason to remove them.\n\nBut I do agree with the removal of these checks for another reason.\nA call into transport_get_remote_refs() or transport_fetch_refs()\nwould segfault if these necessary fields are not filled in a buggy\ntransport under development anyway.\n\nAnd more importantly, if it is desirable to have a check that is\nmore friendly than merely segfaulting, such a check must be added to\nthe implementation of transport API that knows the implementation\ndetails like \"fetching would require get_refs_list and fetch\nfields\"; the consumers of the API is a wrong place to do so.\n\nThanks.\n\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  builtin/clone.c | 3 ---\n>  builtin/fetch.c | 3 ---\n>  2 files changed, 6 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index dbddd98f8..979af0383 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1083,9 +1083,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\twarning(_(\"--local is ignored\"));\n>  \ttransport->cloning = 1;\n>  \n> -\tif (!transport->get_refs_list || (!is_local && !transport->fetch))\n> -\t\tdie(_(\"Don't know how to clone %s\"), transport->url);\n> -\n>  \ttransport_set_option(transport, TRANS_OPT_KEEP, \"yes\");\n>  \n>  \tif (option_depth)\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 225c73492..09eb1fc17 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1094,9 +1094,6 @@ static int do_fetch(struct transport *transport,\n>  \t\t\ttags = TAGS_UNSET;\n>  \t}\n>  \n> -\tif (!transport->get_refs_list || !transport->fetch)\n> -\t\tdie(_(\"Don't know how to fetch from %s\"), transport->url);\n> -\n>  \t/* if not appending, truncate FETCH_HEAD */\n>  \tif (!append && !dry_run) {\n>  \t\tretcode = truncate_fetch_head();\n"},{"id":"334852","messageId":"xmqqind9rm95.fsf@gitster.mtv.corp.google.com","threadId":"47445","inReplyTo":"26f276956001a120cc9105b0071762c2fd4a45c5.1513287544.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] transport: make transport vtable more private","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-14T22:13:42Z","receivedAt":"2017-12-14T22:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Move the definition of the transport-specific functions provided by\n> transports, whether declared in transport.c or transport-helper.c, into\n> an internal header. This means that transport-using code (as opposed to\n> transport-declaring code) can no longer access these functions (without\n> importing the internal header themselves), making it clear that they\n> should use the transport_*() functions instead, and also allowing the\n> interface between the transport mechanism and an individual transport to\n> independently evolve.\n\nYay!\n"}]}