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

From: Shawn O. Pearce <spearce@spearce.org>
Date: Nov 26, 2009, 01:20 UTC
Message-ID: <20091126012022.GP11919@spearce.org>
In-Reply-To: <1259143617-26580-3-git-send-email-sam@vilain.net>

Sam Vilain <sam@vilain.net> wrote:

Show 7 quoted lines

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