Hi Ævar
On 12/10/2022 10:01, Æ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",
+
+ .processes = 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);
I thought the first version was longer than it needed to be and now it
has grown by 50%. I think there are a number of changes here that are
not related to converting run_processes_parallel() to take a struct of
options. My feeling is that the test cleanups are worthwhile but the
changes in patches 8-10 are needed and patches 12-14 would be better
squashed together.
I'm afraid that I'm think this fits a pattern of submitting series that
incorporate unrelated changes and so end up longer than they need to be.
I think this leads to poorer reviews and a greater change of regressions
because reviewers are overwhelmed by the sheer volume of changes they're
expected to review. I find it particularly fustrating to review code in
one patch only to find it is deleted in the next patch.
I think the overall goal here is worthwhile, but I'd really like to see
a much shorter more focused series. In general two shorter series are
much more likely to receive good quality reviews than one longer series
making the same changes.
Best Wishes
Phillip
