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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 20, 2015, 23:05 UTC
Message-ID: <xmqq37x56sz6.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445381030-23912-3-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 28 quoted lines

> If the `get_next_task` did not explicitly called 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.
>
> As an invariant you can now assume any child not in use is
> cleaned up and ready for its next reuse.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  run-command.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index b9363da..a5ef874 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -13,6 +13,12 @@ void child_process_init(struct child_process *child)
>  	argv_array_init(&child->env_array);
>  }
>  
> +void child_process_deinit(struct child_process *child)
> +{
> +	argv_array_clear(&child->args);
> +	argv_array_clear(&child->env_array);
> +}
> +

Is this necessary (and is it necessary to make it global)? I thought that finish_command() already clears them....

    ... goes and looks ...

Ahh, of course, pp_*() functions do use start_command() but do not use finish_command(), which sort of breaks symmetry, but that cannot be helped. Because we want to wait for any of the multiple tasks running, we cannot call finish_command() that explicitly says "I want to wait for this one to finish".

And that is why you already have two calls to array-clear inside collect_finished(), just after calling task_finished().

And of course we already have these array-clear calls in finish_command().

So I agree that deinit helper should exist, but

 * it should be file-scope static;

 * it should be called by finish_command(); and

 * if you are calling it from collect_finished(), then existing
   calls to array-clear should go.

Other than that, this looks good.

Thanks.