Re: [PATCH v2 2/8] run-command API: use "opts" struct for run_processes_parallel{,_tr2}()

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, May 18, 2022 at 10:05:18PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> Add a new "struct run_process_parallel_opts" to replace the growing
> run_processes_parallel() and run_processes_parallel_tr2() argument
> lists. This refactoring makes it easier to add new options and
> parameters easier.
> 
> The *_tr2() variant of the function was added in ee4512ed481 (trace2:
> create new combined trace facility, 2019-02-22), and has subsequently
> been used by every caller except t/helper/test-run-command.c.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
[...]
> diff --git a/run-command.h b/run-command.h
> index 5bd0c933e80..b0268ed3db1 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -458,6 +458,32 @@ typedef int (*task_finished_fn)(int result,
>  				void *pp_task_cb);
>  
>  /**
> + * Options to pass to run_processes_parallel(), { 0 }-initialized
> + * means no options. Fields:
> + *
> + * tr2_category & tr2_label: sets the trace2 category and label for
> + * logging. These must either be unset, or both of them must be set.

I see this comment...

> + *
> + * jobs: see 'n' in run_processes_parallel() below.
> + *
> + * *_fn & data: see run_processes_parallel() below.
> + */
> +struct run_process_parallel_opts
> +{
> +	const char *tr2_category;
> +	const char *tr2_label;
> +
> +	int jobs;
> +
> +	get_next_task_fn get_next_task;
> +	start_failure_fn start_failure;
> +	task_finished_fn task_finished;
> +	void *data;
> +};

[moved snippet]
> -int run_processes_parallel(int n,
> -			   get_next_task_fn get_next_task,
> -			   start_failure_fn start_failure,
> -			   task_finished_fn task_finished,
> -			   void *pp_cb)
> +int run_processes_parallel(struct run_process_parallel_opts *opts)
>  {
>  	int i, code;
>  	int output_timeout = 100;
>  	int spawn_cap = 4;
>  	struct parallel_processes pp;
> +	const char *tr2_category = opts->tr2_category;
> +	const char *tr2_label = opts->tr2_label;
> +	const int do_trace2 = tr2_category && tr2_label;

...but it's not actually very well enforced here. That is, it seems I
can set one or the other but not both, and nothing bad will happen,
except that I am wasting my time setting one. If you want to enforce
them both to be set, then why not use a BUG()? But otherwise the comment
could be reworded, I think.

> +	const int n = opts->jobs;
>  
> -	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
> +	if (do_trace2)
> +		trace2_region_enter_printf(tr2_category, tr2_label, NULL,
> +					   "max:%d", ((n < 1) ? online_cpus()
> +						      : n));
> +
> +	pp_init(&pp, opts);
>  	while (1) {
>  		for (i = 0;
>  		    i < spawn_cap && !pp.shutdown &&
[/moved snippet]


Otherwise, although the number of lines of code is often higher, I find
the named initializers in the struct much easier to read at the
callsites, so I like this change.

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>