git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Jeff King <peff@peff.net>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH] rebase -i: ignore signals when forking subprocesses
Date: Thu, 07 Sep 2023 14:16:07 -0700	[thread overview]
Message-ID: <xmqq5y4lk7l4.fsf@gitster.g> (raw)
In-Reply-To: <pull.1581.git.1694080982621.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Thu, 07 Sep 2023 10:03:02 +0000")

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

The above made me wonder if we can guarantee that the intention of
the end-user is always to kill only the external program and not
"git".  But with or without this change, "git" will stop making
progress after such a signal (in other words, it is not like killing
"exec sleep 20" will make "git rebase -i -x 'sleep 20'" to move to
the next step without complaining), so "ignore signals" is not all
that harmful as the phrasing makes it sound like.  With the patch,
we just handle signals that will kill the external programs, and
their consequences, a bit more gracefully.

But that makes me wonder what happens if the external program has
good reasons to ignore the signal (that is, the program does not die
when signaled, without misbehaving).  If "git" dies in such a case,
would it help the overall end-user experience, or would it even
hurt?  If the latter, then "git" that ignores these interactive
interrupts would be improvement in both cases (i.e. external
programs that dies with signals, and those that ignores them).  If
the former, however, "git" that ignores the signals would be a
regression.

Other than that, the change is well reasoned, I would think.

Thanks.


      parent reply	other threads:[~2023-09-07 21:16 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-07 10:03 [PATCH] rebase -i: ignore signals when forking subprocesses Phillip Wood via GitGitGadget
2023-09-07 12:57 ` Johannes Schindelin
2023-09-08 10:02   ` Phillip Wood
2023-09-10  8:24     ` Johannes Schindelin
2023-09-07 21:06 ` Jeff King
2023-09-08  9:59   ` Phillip Wood
2023-09-08 13:11     ` Phillip Wood
2023-09-10 10:05     ` Oswald Buddenhagen
2023-09-11 10:00       ` Phillip Wood
2023-09-11 10:14         ` Phillip Wood
2023-09-11 10:32           ` Oswald Buddenhagen
2023-09-13 15:33             ` Phillip Wood
2023-09-13 16:40               ` Oswald Buddenhagen
2023-09-14  9:56     ` Jeff King
2023-09-14 13:50       ` Phillip Wood
2023-09-07 21:16 ` Junio C Hamano [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq5y4lk7l4.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).