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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2015, 18:49 UTC
Message-ID: <xmqq61218jfd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445279086-31066-3-git-send-email-sbeller@google.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"

Show 7 quoted lines

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

Show 16 quoted lines

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