Re: [PATCH] fetch: add "--parallel", which fetches all remotes in parallel

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 18, 2019, 17:45 UTC
Message-ID: <xmqq5znzz17j.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190717015903.4384-1-palmer@sifive.com>

Palmer Dabbelt <palmer@sifive.com> writes:

> * I'm not sure if it's safe to access the .git database from multiple
>   processes at the same time.

It is supposed to, and also you are supposed to keep it that way ;-)

> * As I was writing the documentation I found "fetch --jobs".  It seems
>   like I should use that instead of adding a new argrument, but I wasn't
>   sure.

Why not? What makes you feel it is a bad idea to follow that pattern?

Ah, --jobs that is taken already is right now too tied to fetches that happen in submodules, which arguably was a design mistake.

    -j::
    --jobs=<n>::
            Number of parallel children to be used for fetching
            submodules.  Each will fetch from different submodules,
            such that fetching many submodules will be faster. By
            default submodules will be fetched one at a time.

The simplest endgame would be to replace "submodule" with "repository" in the above description, perhaps like

	Number of parallel jobs to be used for fetching from
	multiple repositories (both fetching with "--multiple" from
	multiple repositories, and also fetching updated contents
	for submodules).  By default, fetching from multiple
	repositories and submodules is done one at a time.

and nobody would have complained if the system were like so from the beginning. Existing users, however, may want extra flexibility, and would complain loudly if we did the above, in which case, we may have to

 - introduce --fetch-jobs=<n> for what you are adding;

 - introduce --submodule-fetch-jobs=<n> as a synonym for existing
   --jobs=<n> and deprecate the current use of --jobs=<n>;

 - eventually repurpose --jobs=<n> as a short-hand to give both
   --fetch-jobs and --submoduje-fetch-jobs at the same time.

> +static int parallel = 0;

Don't explicitly "= 0;" initialize file-scope static. Instead let BSS take care of it.

Show 19 quoted lines

>  static int git_fetch_config(const char *k, const char *v, void *cb)
>  {
> @@ -178,6 +179,8 @@ static struct option builtin_fetch_options[] = {
>  			TRANSPORT_FAMILY_IPV6),
>  	OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>  			N_("report that we have only objects reachable from this object")),
> +	OPT_BOOL(0, "parallel", &parallel,
> +		 N_("fetch in parallel from each remote")),
>  	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
>  	OPT_BOOL(0, "auto-gc", &enable_auto_gc,
>  		 N_("run 'gc --auto' after fetching")),
> @@ -1456,12 +1459,15 @@ static void add_options_to_argv(struct argv_array *argv)
>  
>  }
>  
> -static int fetch_multiple(struct string_list *list)
> +static int fetch_multiple(struct string_list *list, int i)
>  {
> -	int i, result = 0;

'i' is perfectly a good name for a local variable that is used for loop control purposes, but makes a horrible name for a parameter.

Existing 'list' is not any better either---we know it is a list by its type already, the name should say what the list is about, what it represents. But having a horribly named parameter already is not a good reason to make the code even worse.

And as you said, recursion makes the code structure harder to follow here. Keeping an array of --jobs=<n> cmd structures, looping to fill them by starting, doing wait() to reap any of the started ones that first exits to refill the slot just opened, etc. would be easier to see if done in a loop, I think.