Re: [PATCHv8 6/9] fetching submodules: respect `submodule.fetchJobs` config option

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2016, 03:29 UTC
Message-ID: <xmqqa8nfeswj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1454623776-3347-7-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 10 quoted lines

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 586840d..5aa1c2d 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
>  static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosity;
>  static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
>  static int tags = TAGS_DEFAULT, unshallow, update_shallow;
> -static int max_children = 1;
> +static int max_children = -1;

This makes sense as you would later be doing "If left unspecified, i.e. -1, then fall back to blah" ...

g> diff --git a/submodule-config.c b/submodule-config.c

Show 8 quoted lines

> index 2841c5e..339b59d 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -32,6 +32,7 @@ enum lookup_type {
>  
>  static struct submodule_cache cache;
>  static int is_cache_init;
> +static unsigned long parallel_jobs = -1;

... but I do not think this does. For one thing, you would not be doing "If parallel_jobs < -1, then that is unspecified yet" for the unsigned long variable, and for another, I do not think you would be behaving differently for the first time (i.e. "unspecified yet" case).

I think you want to initialize this to 1, if your "not configured at all" default is supposed to be 1.

Show 8 quoted lines

> @@ -233,6 +234,13 @@ static int parse_generic_submodule_config(const char *key,
>  					  const char *value,
>  					  struct parse_config_parameter *me)
>  {
> +	if (!strcmp(key, "fetchjobs")) {
> +		if (!git_parse_ulong(value, &parallel_jobs)) {
> +			warning(_("Error parsing submodule.fetchJobs; Defaulting to 1."));
> +			parallel_jobs = 1;

Hmph, this is not a die() because...?  Not a rhetorical question.

> +unsigned long config_parallel_submodules(void)
> +{
> +	return parallel_jobs;
> +}

It is not a crime to make this return "int", as the code that eventually uses it will assign it to a variable that will be passed to run_processes_parallel() that takes an "int".

In fact, returning "int" would be preferrable. You are causing truncation somewhere in the callchain anyway. If the truncation bothers you, this function or immediately after calling git_parse_ulong() would be a good place to do a range check. The variable parallel_jobs has to stay "unsigned long" in any case as you are calling git_parse_ulong().

Show 16 quoted lines

> diff --git a/submodule.c b/submodule.c
> index b83939c..eb7d54b 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -751,6 +751,11 @@ int fetch_populated_submodules(const struct argv_array *options,
>  	argv_array_push(&spf.args, "--recurse-submodules-default");
>  	/* default value, "--submodule-prefix" and its value are added later */
>  
> +	if (max_parallel_jobs < 0)
> +		max_parallel_jobs = config_parallel_submodules();
> +	if (max_parallel_jobs < 0)
> +		max_parallel_jobs = 1;
> +
>  	calculate_changed_submodule_paths();
>  	run_processes_parallel(max_parallel_jobs,
>  			       get_next_submodule,