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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

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

> @@ -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().

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