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

On 08/09/2023 17:24, Junio C Hamano wrote:

Phillip Wood [off-list ref] writes:

quoted

On 08/09/2023 16:42, Junio C Hamano wrote:

quoted

"Phillip Wood via GitGitGadget" [off-list ref] writes:

quoted

[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