Re: [PATCH 2/8] run-command: Call get_next_task with a clean child process.
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:06:56
Stefan Beller [off-list ref] writes:
quoted hunk
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 <redacted> --- 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.