From: Phillip Wood Date: Fri, 07 Oct 2022 09:59:45 GMT Subject: Re: [PATCH 00/15] run-command API: pass functions & opts via struct Message-ID: <52ca644a-0719-0ee6-56bd-a4b2686ae204@gmail.com> In-Reply-To: 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@gmail.com/ > 2. https://lore.kernel.org/git/20220922232947.631309-1-calvinwan@google.com/ > 3. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/ > > 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(-) >