[PATCH] rebase -i: ignore signals when forking subprocesses

From: Phillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date: Sep 7, 2023, 10:03 UTC
Message-ID: <pull.1581.git.1694080982621.gitgitgadget@gmail.com>

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
    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?

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