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

From: Junio C Hamano <gitster@pobox.com>
Date: May 18, 2022, 21:45 UTC
Message-ID: <xmqqee0qtszn.fsf@gitster.g>
In-Reply-To: <patch-v2-2.8-5f0a6e9925f-20220518T195858Z-avarab@gmail.com>

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

Show 16 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",
> +
> +			.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.

Show 13 quoted lines

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

Show 6 quoted lines

> +/**
> + * 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);