Re: [PATCH] start_command: reset disposition of all signals in child

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 8, 2023, 16:24 UTC
Message-ID: <xmqqmsxwtyy3.fsf@gitster.g>
In-Reply-To: <ba69ab35-3204-4360-a36d-3253680b2479@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 17 quoted lines

> On 08/09/2023 16:42, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> [3] This is really a work-around for not moving the child into its own
>>>      process group and changing the foreground process group of the
>>>      controlling terminal.
>> I am puzzled, as I somehow thought that "does the user conceive a
>> subprocess as external and different-from-git entity, or is it
>> merely an implementation detail?  many use of subprocesses in our
>> codebase, it is the latter." from Peff was a good argument against
>> such isolation between spawning "git" and spawned subprocesses.
>
> It is and in those cases we do not ignore SIGINT and SIGQUIT in the
> parent when we fork the subprocess. What I was trying to say is that
> in the few cases where we do ignore SIGINT and SIGQUIT in the parent
> when we fork a subprocess we're working round the child being in the
> same process group at the parent.

Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at random, the typical pattern seem to be (this example was taken from builtin/receive-pack.c):

	code = start_command(&proc);
	if (code) {
		...
		return code;
	}
	sigchain_push(SIGPIPE, SIG_IGN);
	while (1) {
		...
	}
	close(proc.in);
	sigchain_pop(SIGPIPE);
	return finish_command(&proc);

The way we spawn an editor in editor.c looks the same:

		p.use_shell = 1;
		p.trace2_child_class = "editor";
		if (start_command(&p) < 0) {
			strbuf_release(&realpath);
			return error("unable to start editor '%s'", editor);
		}

		sigchain_push(SIGINT, SIG_IGN);
		sigchain_push(SIGQUIT, SIG_IGN);
		ret = finish_command(&p);

IOW, we do not ignore then spawn. We spawn and ignore only in the parent, so there shouldn't be any reason to worry about our child inheriting the "we the parent git process do not want to be killed by \C-c" settings, should there?

I have a vague recollection that the "propagate what was already ignored to be ignored down to the child, too" was not about signals we ignored, but inherited from the end-user who started git with certain signals ignored, but it is so old a piece of code that the details of the rationale escapes me.