Re: [PATCH v3 01/10] run-command: add stdin callback for parallelization
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 25, 2025, 23:15 UTC
- Message-ID
- <xmqqsee1puup.fsf@gitster.g>
- In-Reply-To
- <20251124172043.1650014-2-adrian.ratiu@collabora.com>
Adrian Ratiu <adrian.ratiu@collabora.com> writes:
Show 10 quoted lines
> +static void pp_buffer_stdin(struct parallel_processes *pp,
> + const struct run_process_parallel_opts *opts)
> +{
> + /* Buffer stdin for each pipe. */
> + for (size_t i = 0; i < opts->processes; i++) {
> + struct child_process *proc = &pp->children[i].process;
> + int ret;
> +
> + if (pp->children[i].state != GIT_CP_WORKING || proc->in <= 0)
> + continue;This combination of two conditions is a recurring theme in this series (e.g., something close to the reverse of this is called child-ready-for-cleanup later).
I wonder if it makes the code easier to follow if we add a set of helpers that take a pointer to &pp->children[i] (and we probably should give the struct that describes each child managed in the "struct parallel_processes" some name) and asks "is this thing done?" etc.
Show 22 quoted lines
> + /*
> + * Child tasks might receive input via stdin, terminating early (or not), so
> + * ignore the default SIGPIPE which gets handled by each feed_pipe_fn which
> + * actually writes the data to children stdin fds.
> + */
> + sigchain_push(SIGPIPE, SIG_IGN);
> +
> pp_init(&pp, opts, &pp_sig);
> while (1) {
> for (i = 0;
> @@ -1792,13 +1864,7 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
> }
> if (!pp.nr_processes)
> break;
> - if (opts->ungroup) {
> - for (size_t i = 0; i < opts->processes; i++)
> - pp.children[i].state = GIT_CP_WAIT_CLEANUP;
> - } else {
> - pp_buffer_stderr(&pp, opts, output_timeout);
> - pp_output(&pp);
> - }
> + pp_handle_child_IO(&pp, opts, output_timeout);OK, this helper roughly does the same thing as the removed if/else (and a bit more).
Show 5 quoted lines
> @@ -1809,6 +1875,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts) > > pp_cleanup(&pp, opts); > > + sigchain_pop(SIGPIPE);
OK.
Show 15 quoted lines
> diff --git a/run-command.h b/run-command.h > index 0df25e445f..e536ed7544 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -420,6 +420,22 @@ typedef int (*start_failure_fn)(struct strbuf *out, > void *pp_cb, > void *pp_task_cb); > > +/** > + * This callback is repeatedly called on every child process who requests > + * start_command() to create a pipe by setting child_process.in < 0. > + * > + * pp_cb is the callback cookie as passed into run_processes_parallel, and > + * pp_task_cb is the callback cookie as passed into get_next_task_fn. > + * The contents of 'send' will be read into the pipe and passed to the pipe.
There is no 'send' seen around here. Does this refer to something a later step adds?