Re: [PATCH 1/2] clone, fetch: remove redundant transport check

[Junio C Hamano]

Jonathan Tan  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.

> Signed-off-by: Jonathan Tan 
> ---
>  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();

[PATCH 1/2] clone, fetch: remove redundant transport check

[Jonathan Tan]

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.

Signed-off-by: Jonathan Tan 
---
 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();
-- 
2.15.1.504.g5279b80103-goog