Hi Phillip, On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > > If the user presses Ctrl-C to interrupt a program run by a rebase "exec" > command then SIGINT will also be sent to the git process running the > rebase resulting in it being killed. Fortunately the consequences of > this are not severe as all the state necessary to continue the rebase is > saved to disc but it would be better to avoid killing git and instead > report that the command failed. A similar situation occurs when the > sequencer runs "git commit" or "git merge". If the user generates SIGINT > while editing the commit message then the git processes creating the > commit will ignore it but the git process running the rebase will be > killed. > > Fix this by ignoring SIGINT and SIGQUIT when forking "exec" commands, > "git commit" and "git merge". This matches what git already does when > running the user's editor and matches the behavior of the standard > library's system() function. ACK > > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx> > --- > rebase -i: ignore signals when forking subprocesses > > Having written this I started thinking about what happens when we fork > hooks, merge strategies and merge drivers. I now wonder if it would be > better to change run_command() instead - are there any cases where we > actually want git to be killed when the user interrupts a child process? I am not sure that we can rely on arbitrary hooks to do the right thing upon Ctrl+C, which is to wrap up and leave. So I _guess_ that we will have to leave it an opt-in. However, we could easily make it an option that `run_command()` handles, much like `no_stdin`. Ciao, Johannes > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1581%2Fphillipwood%2Fsequencer-subprocesses-ignore-sigint-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1581/phillipwood/sequencer-subprocesses-ignore-sigint-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1581 > > sequencer.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index a66dcf8ab26..26d70f68454 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -1059,6 +1059,7 @@ static int run_git_commit(const char *defmsg, > unsigned int flags) > { > struct child_process cmd = CHILD_PROCESS_INIT; > + int res; > > if ((flags & CLEANUP_MSG) && (flags & VERBATIM_MSG)) > BUG("CLEANUP_MSG and VERBATIM_MSG are mutually exclusive"); > @@ -1116,10 +1117,16 @@ static int run_git_commit(const char *defmsg, > if (!(flags & EDIT_MSG)) > strvec_push(&cmd.args, "--allow-empty-message"); > > + sigchain_push(SIGINT, SIG_IGN); > + sigchain_push(SIGQUIT, SIG_IGN); > if (is_rebase_i(opts) && !(flags & EDIT_MSG)) > - return run_command_silent_on_success(&cmd); > + res = run_command_silent_on_success(&cmd); > else > - return run_command(&cmd); > + res = run_command(&cmd); > + sigchain_pop(SIGINT); > + sigchain_pop(SIGQUIT); > + > + return res; > } > > static int rest_is_empty(const struct strbuf *sb, int start) > @@ -3628,10 +3635,14 @@ static int do_exec(struct repository *r, const char *command_line) > struct child_process cmd = CHILD_PROCESS_INIT; > int dirty, status; > > + sigchain_push(SIGINT, SIG_IGN); > + sigchain_push(SIGQUIT, SIG_IGN); > fprintf(stderr, _("Executing: %s\n"), command_line); > cmd.use_shell = 1; > strvec_push(&cmd.args, command_line); > status = run_command(&cmd); > + sigchain_pop(SIGINT); > + sigchain_pop(SIGQUIT); > > /* force re-reading of the cache */ > discard_index(r->index); > @@ -4111,7 +4122,11 @@ static int do_merge(struct repository *r, > NULL, 0); > rollback_lock_file(&lock); > > + sigchain_push(SIGINT, SIG_IGN); > + sigchain_push(SIGQUIT, SIG_IGN); > ret = run_command(&cmd); > + sigchain_pop(SIGINT); > + sigchain_pop(SIGQUIT); > > /* force re-reading of the cache */ > if (!ret) { > > base-commit: 1fc548b2d6a3596f3e1c1f8b1930d8dbd1e30bf3 > -- > gitgitgadget >
