Re: [PATCH 1/2] clone, fetch: remove redundant transport check
From: Junio C Hamano <hidden>
Date: 2017-12-14 22:13:03
Jonathan Tan [off-list ref] writes:
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.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.
quoted hunk
Signed-off-by: Jonathan Tan <redacted> --- builtin/clone.c | 3 --- builtin/fetch.c | 3 --- 2 files changed, 6 deletions(-)diff --git a/builtin/clone.c b/builtin/clone.c index dbddd98f8..979af0383 100644 --- a/builtin/clone.c +++ b/builtin/clone.c@@ -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)diff --git a/builtin/fetch.c b/builtin/fetch.c index 225c73492..09eb1fc17 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c@@ -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();