Re: [PATCH 2/5] run-command: Call get_next_task with a clean child process.

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

Stefan Beller <sbeller@google.com> writes:

> If the `get_next_task` did not explicitly called child_process_init

I locally did "If get_next_task did not explicitly call child_process_init"

> and only filled in some fields, there may have been some stale data
> in the child process. This is hard to debug and also adds a review
> burden for each new user of that API. To improve the situation, we
> pass only cleanly initialized child structs to the get_next_task.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Again this makes sense.

Another way, which I suspect may be conceptually cleaner, would be
to do this clean-up where pp->children[i].in_use is set to false, as
that is where the particular task is declared to be available for
reuse.  That location should be responsible to ensure that the task
indeed is reusable by calling child_process_init().

By the way, does child_process_init() leak old argv/env arrays, or
have they already been cleared by calling finish_command() when we
come to this codepath?

>  run-command.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 8f47c6e..b8b5747 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1010,6 +1010,8 @@ static int pp_start_one(struct parallel_processes *pp)
>  	if (i == pp->max_processes)
>  		die("BUG: bookkeeping is hard");
>  
> +	child_process_init(&pp->children[i].process);
> +
>  	if (!pp->get_next_task(&pp->children[i].data,
>  			       &pp->children[i].process,
>  			       &pp->children[i].err,