From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 08 Sep 2023 16:43:33 GMT
Subject: Re: [PATCH] start_command: reset disposition of all signals in child
Message-ID: <376d3ea0-a3eb-4b25-8bf2-ca40c4699e26@gmail.com>
In-Reply-To: <xmqqmsxwtyy3.fsf@gitster.g>

On 08/09/2023 17:24, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> 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?

Oh I should have looked more carefully at the existing uses. It looks 
like it is only my sequencer patch that does

	sigchain_push(SIGINT, SIG_IGN);
	sigchain_push(SIGQUIT, SIG_IGN);
	res = run_command(...);

In that case the existing behavior is a problem but maybe I should 
change those call sites to use start_command() and finish_command() 
instead if we decide that other patch is a good idea.

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

The comment

	/* ignored signals get reset to SIG_DFL on execve */

in start_command() makes it look like the code assumes ignored signals 
will be reset to SIG_DFL by execve() which is not what happens. Maybe 
that comment is just wrong and there is a good reason for the current 
behavior.

Best Wishes

Phillip