Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> if (max_children != 1 && list->nr != 1) {
> struct parallel_fetch_state state = { argv.v, list, 0, 0 };
> + struct run_process_parallel_opts run_opts = {
> + .tr2_category = "fetch",
> + .tr2_label = "parallel/fetch",
> + };
>
> strvec_push(&argv, "--end-of-options");
> - result = run_processes_parallel_tr2(max_children,
> - &fetch_next_remote,
> - &fetch_failed_to_start,
> - &fetch_finished,
> - &state,
> - "fetch", "parallel/fetch");
> + result = run_processes_parallel(max_children,
> + &fetch_next_remote,
> + &fetch_failed_to_start,
> + &fetch_finished, &state,
> + &run_opts);
If the idea is that with unset .tr2_* members we can silently bypass
the overhead to invoke trace2 machinery without changing much in the
caller side (or even better, instead of doing this as run_opts but
as tr2_opts, and allow the caller to pass NULL to decline tracing at
runtime), that would be wonderful.
If we are going to throw random other members into the struct that
are unrelated to tr2, then it makes it unclear why we have the three
*_fn and its callback state still passed as separate parameters,
rather than making them members of the struct. After all, it is
clear that this new struct is designed to be used only with the
run_process_parallel() API, so it is doubly dubious why these three
*_fn and callback state are not members.
So, I dunno.
Either
(1) making it to very clear that this is only about trace2 and name
the type as such, or
(2) making it about run_process_parallel (and keep the name), and
move the *_fn parameters to it (which will allow us to add more
optional callbacks if needed),
would make it better (simply by clarifying why we have this extra
structure and what it is meant to be used for), but the interface as
posted is halfway between the two, and does not look well suited for
either purpose, making the reader feel somewhat frustrating.
