From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 25 Nov 2025 23:15:26 GMT
Subject: Re: [PATCH v3 01/10] run-command: add stdin callback for parallelization
Message-ID: <xmqqsee1puup.fsf@gitster.g>
In-Reply-To: <20251124172043.1650014-2-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> +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.

> +	/*
> +	 * 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).

> @@ -1809,6 +1875,8 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  
>  	pp_cleanup(&pp, opts);
>  
> +	sigchain_pop(SIGPIPE);

OK.

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