Re: [PATCH 2/4] fetch: try mirrors if selected

["Shawn O. Pearce" <spearce@spearce.org>]

Sam Vilain <sam@vilain.net> wrote:
> diff --git a/builtin-fetch.c b/builtin-fetch.c
> index 209f502..b3b8766 100644
> @@ -109,6 +109,109 @@ static void find_non_local_tags(struct transport *transport,
>  			struct ref **head,
>  			struct ref ***tail);
>  
> +char* get_url_hostname(const char *url)

Minor nit, but we mark any function not used outside of the module
as static.  Especially in a builtin-*.c since they all link into
the same namespace.  If this is meant to be reused, it belongs in
connect.c most likely, that's where we already have code like this
to get the SSH hostname out of a URL for SSH connections.

I don't have time right now to read the rest of this series, but
the general approach of fetching to a temporary mirror space before
checking if you really are current is a good one.

I'm not sure that storing the list of mirrors inside of the remote
makes much sense, I would think the user would want to store only
a handful of "fast" URLs.

And even then I wonder why this can't just be the url[1]..url[n-1]
entries in the configuration file.  push pushes to all of the
URLs at once, "seeding the mirrors".  Why can't fetch use the same
configuration?

-- 
Shawn.