Re: [PATCH v2 00/22] run-command API: pass functions & opts via struct

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Oct 12, 2022, 13:39 UTC
Message-ID: <9b4df247-c67c-8e5c-9c2e-1972db6a02e6@gmail.com>
In-Reply-To: <cover-v2-00.22-00000000000-20221012T084850Z-avarab@gmail.com>

Hi Ævar

On 12/10/2022 10:01, Ævar Arnfjörð Bjarmason wrote:

Show 21 quoted lines

> 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