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

Stefan Beller [off-list ref] 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 <redacted>
---

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?

On Tue, Oct 20, 2015 at 11:49 AM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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"

quoted

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

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?

It does, yes. In the reroll I move the initialization to both pp_init (to init
for the first use) and to the place where in_use is set to false.

quoted

 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,