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

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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",
> +
> +			.jobs = max_children,
> +
> +			.get_next_task = &fetch_next_remote,
> +			.start_failure = &fetch_failed_to_start,
> +			.task_finished = &fetch_finished,
> +			.data = &state,
> +		};
>  
>  		strvec_push(&argv, "--end-of-options");
> +		result = run_processes_parallel(&run_opts);

;-)

Can't tell if this is going overboard, but it probably is better
than piling more parameter on top of existing ones.

> 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 would have written "unset" -> "set to NULL" if I were writing
this, but it should hopefully be obvious from the context, so it is
OK.

> + * jobs: see 'n' in run_processes_parallel() below.
> + *
> + * *_fn & data: see run_processes_parallel() below.
> + */

OK.

> +/**
> + * Options are passed via the "struct run_process_parallel_opts" above.
> +
>   * Runs up to n processes at the same time. Whenever a process can be
>   * started, the callback get_next_task_fn is called to obtain the data
>   * required to start another child process.

Beyond the post context follows this text.

    * The children started via this function run in parallel. Their output
    * (both stdout and stderr) is routed to stderr in a manner that output
    * from different tasks does not interleave.
    *
    * start_failure_fn and task_finished_fn can be NULL to omit any
    * special handling.
    */
   int run_processes_parallel(struct run_process_parallel_opts *opts);

The forward reference of 'n' we saw earlier does have matching 'n'
here, but the 'n' no longer exists, so it probably is a good idea to
rewrite the comment before this function.

    Runs up to opts->jobs processes at the time.  Whenever a process
    can be started, the callback opts->get_next_task_fn is called to
    obtain the data required to start another child process. ...

The forward reference of 'data' we saw earlier does not have any
matching description here (it is a flaw in the original and not the
problem with this patch).  The description of get_next_task_fn that
appears much earlier in this file talks about two "callback
cookies", pp_cb and pp_task_cb, but it is unclear how opts->data
(after this patch) relates to either of these two.  Presumably the
calling convention around the "callback cookie" is the same across
get_next_task_fn, start_failure_fn, and task_finished_fn?  If so,
perhaps this is a good place to describe how opts->data is fed into
them.

> +int run_processes_parallel(struct run_process_parallel_opts *opts);