Re: [PATCH 1/6] run-command API: replace run_processes_parallel_tr2() with opts struct

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 23, 2022, 04:24 UTC
Message-ID: <xmqq7d7g4ec1.fsf@gitster.g>
In-Reply-To: <patch-1.6-8bf71ce63dd-20220421T122108Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 19 quoted lines

>  	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.