From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 07 Oct 2022 09:51:02 GMT
Subject: Re: [PATCH 08/15] run-command API: don't fall back on online_cpus()
Message-ID: <8f95fbdb-b211-56af-8693-0e5a84afebac@gmail.com>
In-Reply-To: <patch-08.15-4223980f296-20220930T111343Z-avarab@gmail.com>

On 30/09/2022 12:28, Ævar Arnfjörð Bjarmason wrote:
> When a "jobs = 0" is passed let's BUG() out rather than fall back on
> online_cpus().

Why? It seems like a useful default behavior.

> The default behavior was added when this API was
> implemented in c553c72eed6 (run-command: add an asynchronous parallel
> child processor, 2015-12-15).
> 
> Most of our code in-tree that scales up to "online_cpus()" by default
> calls that function by itself. By having these callers of the
> "run_processes_parallel()" API do the same we can in subsequent
> commits pass all arguments down as a "const struct".

This is not convincing, you can have a function with a const struct 
interface that sets the number of jobs by doing

f(const struct *pp) {
	struct pp mutable;

	if (!pp->n) {
		mutable = *pp;
		mutable.n = online_cpus();
		pp = &mutable;
	}
	...
}

I'm not convinced by the reasoning given for removing the default behavior

Best Wishes

Phillip

> The preceding commit has an overview of the API callers that passed
> "jobs = 0". There were only two of them (actually three, but they
> resolved to these two config parsing codepaths).
> 
> The "fetch.parallel" caller already had a test for the
> "fetch.parallel=0" case added in 0353c688189 (fetch: do not run a
> redundant fetch from submodule, 2022-05-16), but there was no such
> test for "submodule.fetchJobs". Let's add one here.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/fetch.c             | 2 ++
>   run-command.c               | 6 +++---
>   submodule-config.c          | 2 ++
>   t/t5526-fetch-submodules.sh | 5 +++++
>   4 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 78043fb67ef..82f1da14ec1 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -122,6 +122,8 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>   		fetch_parallel_config = git_config_int(k, v);
>   		if (fetch_parallel_config < 0)
>   			die(_("fetch.parallel cannot be negative"));
> +		if (!fetch_parallel_config)
> +			fetch_parallel_config = online_cpus();
>   		return 0;
>   	}
>   
> diff --git a/run-command.c b/run-command.c
> index 80d282dbdb6..1a604af14fb 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1568,8 +1568,8 @@ static void pp_init(struct parallel_processes *pp,
>   {
>   	unsigned int i;
>   
> -	if (jobs < 1)
> -		jobs = online_cpus();
> +	if (!jobs)
> +		BUG("you must provide a non-zero number of jobs!");
>   
>   	pp->max_processes = jobs;
>   
> @@ -1843,7 +1843,7 @@ void run_processes_parallel_tr2(unsigned int jobs, get_next_task_fn get_next_tas
>   				const char *tr2_category, const char *tr2_label)
>   {
>   	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
> -				   ((jobs < 1) ? online_cpus() : jobs));
> +				   jobs);
>   
>   	run_processes_parallel(jobs, get_next_task, start_failure,
>   			       task_finished, pp_cb);
> diff --git a/submodule-config.c b/submodule-config.c
> index cd7ee236a12..4dc61b3a78a 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -303,6 +303,8 @@ int parse_submodule_fetchjobs(const char *var, const char *value)
>   	int fetchjobs = git_config_int(var, value);
>   	if (fetchjobs < 0)
>   		die(_("negative values not allowed for submodule.fetchJobs"));
> +	if (!fetchjobs)
> +		fetchjobs = online_cpus();
>   	return fetchjobs;
>   }
>   
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index e36f9fdf242..98a287ffb90 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -724,6 +724,11 @@ test_expect_success 'fetching submodules respects parallel settings' '
>   		GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
>   		grep "9 tasks" trace.out &&
>   		>trace.out &&
> +
> +		GIT_TRACE=$(pwd)/trace.out git -c submodule.fetchJobs=0 fetch &&
> +		grep "preparing to run up to [0-9]* tasks" trace.out &&
> +		! grep "up to 0 tasks" trace.out &&
> +		>trace.out
>   	)
>   '
>