From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 08 Apr 2026 17:59:37 GMT
Subject: Re: [PATCH] run_processes_parallel(): fix order of sigpipe handling
Message-ID: <xmqqcy09z62e.fsf@gitster.g>
In-Reply-To: <20260408172055.GA2293804@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> We can fix it by reordering the code a bit. We should run pp_init()
> first, and then push our SIG_IGN onto the stack afterwards, so that it
> is truly ignored while feeding the sub-processes.
>
> Note that we also reorder the popping at the end of the function, too.
> This is not technically necessary, as we are doing two pops either way,
> but now the pops will correctly match their pushes.

Sounds quite sensible.

> This also fixes a related case that we can't test yet. If we did have
> more than one process to run, then one child causing SIGPIPE would cause
> us to kill() all of the children (which might still actually be
> running). But the hook API is the only user of the new feed_pipe
> feature, and it does not yet support parallel hook execution. So for now
> we'll always execute the processes sequentially. Once parallel hook
> execution exists, we'll be able to add a test which covers this.

Great.

> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>
> Signed-off-by: Jeff King <peff@peff.net>

Thanks, all of you, for addressing the issue so quickly.

Applied.

> ---
>  run-command.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 32c290ee6a..574d5c40f0 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1895,14 +1895,19 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  					   "max:%"PRIuMAX,
>  					   (uintmax_t)opts->processes);
>  
> +	pp_init(&pp, opts, &pp_sig);
> +
>  	/*
>  	 * 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.
> +	 *
> +	 * This _must_ come after pp_init(), because it installs its own
> +	 * SIGPIPE handler (to cleanup children), and we want to supersede
> +	 * that.
>  	 */
>  	sigchain_push(SIGPIPE, SIG_IGN);
>  
> -	pp_init(&pp, opts, &pp_sig);
>  	while (1) {
>  		for (i = 0;
>  		    i < spawn_cap && !pp.shutdown &&
> @@ -1928,10 +1933,10 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  		}
>  	}
>  
> -	pp_cleanup(&pp, opts);
> -
>  	sigchain_pop(SIGPIPE);
>  
> +	pp_cleanup(&pp, opts);
> +
>  	if (do_trace2)
>  		trace2_region_leave(tr2_category, tr2_label, NULL);
>  }