From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 09 Apr 2026 13:40:22 GMT
Subject: Re: [PATCH] run_processes_parallel(): fix order of sigpipe handling
Message-ID: <xmqqo6jsw8u1.fsf@gitster.g>
In-Reply-To: <20260408234236.GA2980392@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> Currently neither subprocess is marked with the clean-on-exit bit.
>> but if somebody is careless and flips the bit for these
>> subprocesses, start_command() will call mark_child_for_cleanup() and
>> causes sigchain_push_common() to set up cleanup_children_on_signal()
>> to be called, which would lead to a very similar bug.
>
> Hmm, good catch. This is a bit worrisome, as we've added some
> clean_on_exit calls recently, and might do so again.
>
>> I wonder if swapping the order of start_command() and sigchain_push()
>> in these two code paths have downsides, or is it making the code worse
>> just to futureproof it against a future that is unlikely to come?
>
> I don't think it's really making the code worse. It's putting the
> SIGPIPE handling closer to where we're actually doing the writes. But I
> don't like that it is a subtle thing that people writing new code have
> to worry about. And I wouldn't be surprised if there are other cases
> where we disable SIGPIPE, and then call start_command() in a much lower
> level of the call chain, which would make reordering harder.

OK.  Such a change is easy, compared to anything we try to do better
;-).

> I wonder if we can do better.
>
> The root of the issue is that sigchain_push_common() is not really
> expressing what we want. We are trying to install a handler to do
> cleanup _if_ we are about to exit. And that is always going to be true
> for SIGTERM, etc, where we might install handlers but never expect them
> to rescue us from exiting. But for SIGPIPE, our desired action is really
> dependent on whether the signal is being ignored in general.

Hmph, I actually was hoping that we do not have to go in the route
that each specific signal differently, but I guess it is unavoidable
as they come with their own semantics (somewhat similar to the way
how their default disposition has to be different).

> That's a total rewrite of our signal handling, though. There might be
> details I haven't considered.
>
> All of that is out-of-scope for -rc2, though. ;)

Of course.