On 30/09/2022 12:27, Ævar Arnfjörð Bjarmason wrote:
This series changes the run-command API so that we pass options via a
struct instead of via the argument list, the end result is that an API
user looks like e.g.:
+ const struct run_process_parallel_opts opts = {
+ .tr2_category = "submodule",
+ .tr2_label = "parallel/update",
+
+ .jobs = update_data->max_jobs,
+
+ .get_next_task = update_clone_get_next_task,
+ .start_failure = update_clone_start_failure,
+ .task_finished = update_clone_task_finished,
+ .data = &suc,
+ };
[...]
- run_processes_parallel_tr2(suc.update_data->max_jobs, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished, &suc, "submodule",
- "parallel/update");
+ run_processes_parallel(&opts);
Overall this looks good, I've left a few comments as I feel some of the
patches are either redundant or could be helpfully squashed into an
adjacent patch to reduce the amount of churn within this series.
Best Wishes
Phillip
These patches are derived from earlier patches I sent for passing the
the "ungroup" parameter to this API[1], that's currently done with a
global variable, because we needed a minimal change for a regression
fix.
I'm submitting this now in the rc phase because there's another
concurrent series that could make use of this[2]. The alternative
would be for it to add an extra parameter to the two functions (one
after this series).
The upcoming migration to the new hook API and config-based hooks[3]
will also benefit significantly from this. I have a version of that
topic rebased on top of this, having this first gets rid of a lot of
churn, adding an optional callback just requires adding things to the
struct introduced here, not changing every single caller.
(Passing) CI at:
https://github.com/avar/git/tree/avar/hook-run-process-parallel-tty-regression-2-argument-passing
1. https://lore.kernel.org/git/cover-v3-0.2-00000000000-20220527T090618Z-avarab@xxxxxxxxx/
2. https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@xxxxxxxxxx/
3. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@xxxxxxxxx/
Brief commentary on individual patches below:
Ævar Arnfjörð Bjarmason (15):
hook tests: fix redirection logic error in 96e7225b310
submodule tests: reset "trace.out" between "grep" invocations
These bugfixes could be split out I suppose, but since we're changing
this area 1/15 seemed prudent, 2/15 is required for a later test
addition
run-command tests: test stdout of run_command_parallel()
Tighten up tests, was ejected from the "ungroup" topic.
run-command test helper: use "else if" pattern
run-command tests: use "return", not "exit"
Just setup for later changes.
run-command API: have "run_processes_parallel{,_tr2}()" return void
Turns out we've never used the boilerplate return value for anything
useful ever.
run-command API: make "jobs" parameter an "unsigned int"
run-command API: don't fall back on online_cpus()
This allows us to make the "opts" struct "const", which helps a lot in
passing it around later on.
run-command.c: add an initializer for "struct parallel_processes"
run-command API: add nascent "struct run_process_parallel_opts"
run-command API: make run_process_parallel{,_tr2}() thin wrappers
run-command API: have run_process_parallel() take an "opts" struct
run-command API: move *_tr2() users to "run_processes_parallel()"
This is arguably one logical change (and at some point I had it as
such), but as the diff would be really large I've tried to split this
into easily digestable steps.
run-command.c: don't copy *_fn to "struct parallel_processes"
run-command.c: don't copy "ungroup" to "struct parallel_processes"
The only reason for copying various parameters into "struct
parallel_processes" was because we passed them as positionals, now
that we have an "opts" struct we can just pass that along instead.
This leaves the "struct parallel_processes" in run-command.c purely
for managing our own internal state.
By avoiding this copying we also cut down a lot on the boilerplate
needed to add a new parameter.
builtin/fetch.c | 25 +++---
builtin/submodule--helper.c | 16 +++-
hook.c | 23 +++---
run-command.c | 152 ++++++++++++++----------------------
run-command.h | 71 ++++++++++++-----
submodule-config.c | 2 +
submodule.c | 18 +++--
t/helper/test-run-command.c | 77 +++++++++++-------
t/t0061-run-command.sh | 25 +++---
t/t1800-hook.sh | 2 +-
t/t5526-fetch-submodules.sh | 16 +++-
11 files changed, 247 insertions(+), 180 deletions(-)
