diff mbox series

[2/5] run-command: allow stdin for run_processes_parallel

Message ID patch-2.5-81eef2f60a0-20230123T170551Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series hook API: support stdin, convert post-rewrite | expand

Commit Message

Ævar Arnfjörð Bjarmason Jan. 23, 2023, 5:15 p.m. UTC 
From: Emily Shaffer <emilyshaffer@google.com>

While it makes sense not to inherit stdin from the parent process to
avoid deadlocking, it's not necessary to completely ban stdin to
children. An informed user should be able to configure stdin safely. By
setting `some_child.process.no_stdin=1` before calling `get_next_task()`
we provide a reasonable default behavior but enable users to set up
stdin streaming for themselves during the callback.

`some_child.process.stdout_to_stderr`, however, remains unmodifiable by
`get_next_task()` - the rest of the run_processes_parallel() API depends
on child output in stderr.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Jan. 23, 2023, 10:52 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	/*
> +	 * By default, do not inherit stdin from the parent process - otherwise,
> +	 * all children would share stdin! Users may overwrite this to provide
> +	 * something to the child's stdin by having their 'get_next_task'
> +	 * callback assign 0 to .no_stdin and an appropriate integer to .in.
> +	 */
> +	pp->children[i].process.no_stdin = 1;
> +
>  	code = opts->get_next_task(&pp->children[i].process,
>  				   opts->ungroup ? NULL : &pp->children[i].err,
>  				   opts->data,
> @@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,
>  		pp->children[i].process.err = -1;
>  		pp->children[i].process.stdout_to_stderr = 1;
>  	}
> -	pp->children[i].process.no_stdin = 1;

For this single process, by default .no_stdin is set before it is
passed to start_command(), so the default behaviour does not change.

This needs a new safety to ensure that processes that have .no_stdin
turned off do not share the same value in their .in member, doesn't
it?  Hopefully that will be added in a later step in the series?

Provided that such a safety will appear in the end result, this
conversion does make sense to me.  Let's read on.

>  	if (start_command(&pp->children[i].process)) {
>  		if (opts->start_failure)
diff mbox series

Patch

diff --git a/run-command.c b/run-command.c
index b439c7974ca..6bd16acb060 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1586,6 +1586,14 @@  static int pp_start_one(struct parallel_processes *pp,
 	if (i == opts->processes)
 		BUG("bookkeeping is hard");
 
+	/*
+	 * By default, do not inherit stdin from the parent process - otherwise,
+	 * all children would share stdin! Users may overwrite this to provide
+	 * something to the child's stdin by having their 'get_next_task'
+	 * callback assign 0 to .no_stdin and an appropriate integer to .in.
+	 */
+	pp->children[i].process.no_stdin = 1;
+
 	code = opts->get_next_task(&pp->children[i].process,
 				   opts->ungroup ? NULL : &pp->children[i].err,
 				   opts->data,
@@ -1601,7 +1609,6 @@  static int pp_start_one(struct parallel_processes *pp,
 		pp->children[i].process.err = -1;
 		pp->children[i].process.stdout_to_stderr = 1;
 	}
-	pp->children[i].process.no_stdin = 1;
 
 	if (start_command(&pp->children[i].process)) {
 		if (opts->start_failure)