| Message ID | patch-v2-10.22-eb9d672b0d8-20221012T084850Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | run-command API: pass functions & opts via struct | expand |
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@gmail.com > 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@gmail.com> > --- > 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 > ) > ' >
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 ) '
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). 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@gmail.com> --- 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(-)