On 08/09/2023 18:38, Junio C Hamano wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
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(...);
Hmph, does it mean this patch would become unnecessary, once you fix
the above sequence to follow the pattern "to spawn and then ignore"?
Yes, sorry for the confusion. There are a couple of things that I think
we should address though. Firstly we should change the comment in
run-command which says execve() resets ignored signals to SIG_DFL to say
something like
Preserve the set of ignored signals so that running git via a
wrapper like nohup works as the user expects
The other thing is that we have some instances where we ignore SIGPIPE
before calling start_command() which means we're ignoring it in the
child process as well. For example in gpg-interface.c we have
sigchain_push(SIGPIPE, SIG_IGN);
ret = pipe_command(&gpg, sigc->payload, sigc->payload_len, &gpg_stdout, 0,
&gpg_stderr, 0);
sigchain_pop(SIGPIPE);
To fix that one we'd need to change pipe_command() to ignore SIGPIPE
after calling start_command() or add a flag to struct child_process to
do to that.
Another example is in upload-pack.c
/*
* If the next rev-list --stdin encounters an unknown commit,
* it terminates, which will cause SIGPIPE in the write loop
* below.
*/
sigchain_push(SIGPIPE, SIG_IGN);
if (start_command(cmd))
goto error;
rev-list does not check for errors when writing to stdout unless
GIT_FLUSH is set in the environment so if parent process exits early
rev-list will keep going until it thinks it has printed everything.
I think adding a flag to struct child_process to ignore SIGPIPE in the
parent is probably the best way to avoid problems like this.
Best Wishes
Phillip
