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

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