Hi Ævar
On 12/10/2022 10:01, Ævar Arnfjörð Bjarmason wrote:
When a "jobs = 0" is passed let's BUG() out rather than fall back on
online_cpus(). The default behavior was added when this API was
implemented in c553c72eed6 (run-command: add an asynchronous parallel
child processor, 2015-12-15).
This commit message still does not explain why this change is necessary.
The cover letter says you hope it is obvious but it isn't. As I pointed
out in my previous review[1] I think it is possible for
run_processes_parallel() to take a const struct without this change.
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/8f95fbdb-b211-56af-8693-0e5a84afebac@xxxxxxxxx
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".
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@xxxxxxxxx>
---
builtin/fetch.c | 2 ++
run-command.c | 7 +++----
submodule-config.c | 2 ++
t/t5526-fetch-submodules.sh | 5 +++++
4 files changed, 12 insertions(+), 4 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 5a63008b6e1..21c00a48823 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1562,8 +1562,8 @@ static void pp_init(struct parallel_processes *pp,
task_finished_fn task_finished,
void *data, int ungroup)
{
- if (n < 1)
- n = online_cpus();
+ if (!n)
+ BUG("you must provide a non-zero number of processes!");
pp->max_processes = n;
@@ -1831,8 +1831,7 @@ void run_processes_parallel_tr2(size_t n, get_next_task_fn get_next_task,
task_finished_fn task_finished, void *pp_cb,
const char *tr2_category, const char *tr2_label)
{
- trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
- ((n < 1) ? online_cpus() : n));
+ trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d", n);
run_processes_parallel(n, 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
)
'
