rebase -i: ignore signals when forking subprocesses #1581
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Phillip,
On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> From: Phillip Wood <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> |
|
On the Git mailing list, Jeff King wrote (reply to this): On Thu, Sep 07, 2023 at 10:03:02AM +0000, Phillip Wood via GitGitGadget wrote:
> 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 think that would be quite surprising in most cases, as the user may
not be aware when or if a sub-program is in use.
Imagine that you're running a script which runs git-commit in a loop,
which in turn runs "gc --auto", which in turn runs a pre-auto-gc hook.
You want to stop it, so you hit ^C, which sends SIGINT to all of the
processes.
I think most people would expect the whole process chain to stop
immediately. But in your proposal, we'd kill the hook only. That kind-of
propagates up to "gc --auto" (which says "OK, the hook says don't run").
And then that doesn't really propagate to git-commit, which ignores the
exit code of gc and continues on, running the post-commit hook and so
on. And then outer loop of the script continues, invoking the next
commit, and so on. To actually quit you have to hit ^C several times
with the right timing (the exact number of which is unknown to you, as
it depends on the depth of the process chain).
I think this really comes down to: does the user perceive the child
process as the current "main" process running in the foreground? For
most run-command invocations, I would say no. For something like running
an editor, the answer is more clearly yes.
For something like sequencer "exec" commands, I think it really depends
on what the command is doing. If it is "git commit --amend", then that
is going to open an editor and take over the terminal. If it is "make",
then probably not. But it may be OK to do here, just because we know
that a signal exit from the child will be immediately read and
propagated by the sequencer machinery (assuming the child dies; if it
blocks SIGINT, too, then now you cannot interrupt it at all!).
In the classic Unix world, I think the solution here is setsid(),
process groups, and controlling terminals. One example in your commit
message is the sequencer kicking off git-commit, which kicks off an
editor. The user hits ^C then, and the sequencer is killed. But I think
your patch is papering over the deeper bug. In 913ef36093
(launch_editor: ignore terminal signals while editor has control,
2012-11-30), we did this same "ignore INT" trick. But I think the right
solution is actually to start the editor in its own process group, and
let it be the foreground of the terminal. And then a ^C while the editor
is running would not only not hit git-commit, but it would not hit any
sequencer or other intermediate processes above it.
I've never done it before, but from my reading we basically want to do
(in the forked process before we exec):
setsid();
open("/dev/tty");
But of course none of that probably has any meaning on Windows. I'm not
sure if there are analogous concepts there we could access with
alternate code, or if it would need to be a totally different strategy.
-Peff |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Phillip Wood via GitGitGadget" <[email protected]> writes:
> From: Phillip Wood <[email protected]>
>
> 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.
|
|
This branch is now known as |
|
This patch series was integrated into seen via git@64579a9. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Peff
Thanks for your thoughts, I hoped I get a interesting response if I cc'd you and I wasn't disappointed!
On 07/09/2023 22:06, Jeff King wrote:
> On Thu, Sep 07, 2023 at 10:03:02AM +0000, Phillip Wood via GitGitGadget wrote:
> >> 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 think that would be quite surprising in most cases, as the user may
> not be aware when or if a sub-program is in use.
> > Imagine that you're running a script which runs git-commit in a loop,
> which in turn runs "gc --auto", which in turn runs a pre-auto-gc hook.
> You want to stop it, so you hit ^C, which sends SIGINT to all of the
> processes.
> > I think most people would expect the whole process chain to stop
> immediately. But in your proposal, we'd kill the hook only. That kind-of
> propagates up to "gc --auto" (which says "OK, the hook says don't run").
> And then that doesn't really propagate to git-commit, which ignores the
> exit code of gc and continues on, running the post-commit hook and so
> on. And then outer loop of the script continues, invoking the next
> commit, and so on. To actually quit you have to hit ^C several times
> with the right timing (the exact number of which is unknown to you, as
> it depends on the depth of the process chain).
Ah, I hadn't thought about "gc --auto" I was assuming that the calling code would see the child had been killed and exit but that's not always the case.
> I think this really comes down to: does the user perceive the child
> process as the current "main" process running in the foreground? For
> most run-command invocations, I would say no. For something like running
> an editor, the answer is more clearly yes.
> > For something like sequencer "exec" commands, I think it really depends
> on what the command is doing. If it is "git commit --amend", then that
> is going to open an editor and take over the terminal. If it is "make",
> then probably not. But it may be OK to do here, just because we know
> that a signal exit from the child will be immediately read and
> propagated by the sequencer machinery (assuming the child dies; if it
> blocks SIGINT, too, then now you cannot interrupt it at all!).
The child not dying is tricky, if it is in the same process group as git then even if git dies the I think the shell will wait for the child to exit before showing the prompt again so it is not clear to me that the user is disadvantaged by git ignoring SIGINT in that case. Part of the motivation for this patch is that I'd like to move the sequencer to a model where it only saves its state to disk when it is stopping for the user to fix conflicts. To do that safely it cannot die if the user interprets a child with SIGINT.
> In the classic Unix world, I think the solution here is setsid(),
> process groups, and controlling terminals. One example in your commit
> message is the sequencer kicking off git-commit, which kicks off an
> editor. The user hits ^C then, and the sequencer is killed. But I think
> your patch is papering over the deeper bug. In 913ef36093
> (launch_editor: ignore terminal signals while editor has control,
> 2012-11-30), we did this same "ignore INT" trick.
Yes, that was the inspiration for this patch
> But I think the right
> solution is actually to start the editor in its own process group, and
> let it be the foreground of the terminal. And then a ^C while the editor
> is running would not only not hit git-commit, but it would not hit any
> sequencer or other intermediate processes above it.
> > I've never done it before, but from my reading we basically want to do
> (in the forked process before we exec):
> > setsid();
> open("/dev/tty");
Do we want a whole new session? As I understand it to launch a foreground job shells put the child in its own process group and then call tcsetpgrp() to change the foreground process group of the controlling terminal to that of the child. I agree that would be a better way of doing things on unix.
> But of course none of that probably has any meaning on Windows. I'm not
> sure if there are analogous concepts there we could access with
> alternate code, or if it would need to be a totally different strategy.
Lets see if Johannes has any comments about that.
Best Wishes
Phillip
|
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): Hi Johannes
On 07/09/2023 13:57, Johannes Schindelin wrote:
> Hi Phillip,
> > On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> >> From: Phillip Wood <[email protected]>
>>
>> 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
Thanks
>>
>> Signed-off-by: Phillip Wood <[email protected]>
>> ---
>> 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.
Peff pointed out it doesn't play well with "gc --auto" either. Do you have any thoughts (particularly about the implications for Windows) on his suggestion to put the child in it's own session, or putting the child in its own process group and making that the foreground process group of the controlling terminal?
> However, we could easily make it an option that `run_command()` handles,
> much like `no_stdin`.
That's an interesting idea.
Best Wishes
Phillip
|
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 08/09/2023 10:59, Phillip Wood wrote:
>> But I think the right
>> solution is actually to start the editor in its own process group, and
>> let it be the foreground of the terminal. And then a ^C while the editor
>> is running would not only not hit git-commit, but it would not hit any
>> sequencer or other intermediate processes above it.
>>
>> I've never done it before, but from my reading we basically want to do
>> (in the forked process before we exec):
>>
>> setsid();
>> open("/dev/tty");
> > Do we want a whole new session? As I understand it to launch a > foreground job shells put the child in its own process group and then > call tcsetpgrp() to change the foreground process group of the > controlling terminal to that of the child. I agree that would be a > better way of doing things on unix.
It is better for handling SIGINT and SIGQUIT when we don't want git to be killed but in complicates the handling of SIGTSTP and friends. We'd need to pass WUNTRACED to waitpid() and then do "raise(WSTOPSIG(wstatus))" to propagate the signal up to the shell. When resuming we'd need to call tcsetpgrp() again if git is resumed in the foreground before sending SIGCONT to the child.
>> But of course none of that probably has any meaning on Windows. I'm not
>> sure if there are analogous concepts there we could access with
>> alternate code, or if it would need to be a totally different strategy.
> > Lets see if Johannes has any comments about that.
I had a quick google and it looks like cygwin somehow manages to implement tcsetpgrp() but the windows terminal does not have any concept of a foreground process group
Best Wishes
Phillip
|
|
This patch series was integrated into seen via git@fdddcf7. |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): Hi Phillip,
On Fri, 8 Sep 2023, Phillip Wood wrote:
> On 07/09/2023 13:57, Johannes Schindelin wrote:
> >
> > On Thu, 7 Sep 2023, Phillip Wood via GitGitGadget wrote:
> >
> > > 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.
>
> Peff pointed out it doesn't play well with "gc --auto" either. Do you have any
> thoughts (particularly about the implications for Windows) on his suggestion
> to put the child in it's own session, or putting the child in its own process
> group and making that the foreground process group of the controlling
> terminal?
The concept of "sessions" does not really translate well into the Windows
world. Neither does the concept of a "process group".
Ciao,
Johannes |
|
On the Git mailing list, Oswald Buddenhagen wrote (reply to this): On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
>Ah, I hadn't thought about "gc --auto". I was assuming that the calling >code would see the child had been killed and exit but that's not always >the case.
>
that's a quite reasonable assumption.
ignoring gc's exit status is ok-ish, but ignoring its termination signal is absolutely not.
>On 07/09/2023 22:06, Jeff King wrote:
>> I think this really comes down to: does the user perceive the child
>> process as the current "main" process running in the foreground?
>> that is indeed a key point here.
note that the shell doesn't enable job control in scripts, either.
>The child not dying is tricky, if it is in the same process group as >git then even if git dies the I think the shell will wait for the child >to exit before showing the prompt again so it is not clear to me that >the user is disadvantaged by git ignoring SIGINT in that case.
>
there is no such thing as waiting for grandchildren. the grandchild is reparented to init when the child exits.
there is a situation were one can be deadlocked by a non-exiting grandchild: when doing a blocking read of the child's output past its exit, when the grandchild has inherited stdout. but that's a implementation bug in the parent. and not relevant here.
On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>On 08/09/2023 10:59, Phillip Wood wrote:
>>> I've never done it before, but from my reading we basically want to >>> do
>>> (in the forked process before we exec):
>>>
>>> �� setsid();
>>> �� open("/dev/tty");
>> >> Do we want a whole new session? As I understand it to launch a >> foreground job shells put the child in its own process group and then >> call tcsetpgrp() to change the foreground process group of the >> controlling terminal to that of the child.
>
this would indeed be the right way if we wanted to isolate the children more, but ...
>It is better for handling SIGINT and SIGQUIT when we don't want git to >be killed but in complicates the handling of SIGTSTP and friends. [...]
>
... this shows that we really don't want that; we don't want to replicate interactive shell behavior. that is even before the divergence on windows.
so i think your patch is approaching things the right way.
though blocking signals doesn't appear right - to ensure git's own clean exit while it has no children, it must catch sigint anyway, and temporarily ignoring it around spawning children sounds racy.
regards |
|
User |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 11/09/2023 11:00, Phillip Wood wrote:
> There is an inevitable race between wait() returning and calling > signal() to restore the handlers for SIGINT and SIGQUIT,
In principle if we installed a signal handler to set a flag if a signal is received while calling wait() and then once wait() returns successfully see if the child was killed we can tell if the signal was received while the child was alive. In practice if the child is catching SIGINT or SIGQUIT we cannot rely on it re-raising the signal so that wont work.
Best Wishes
Phillip
|
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 10/09/2023 11:05, Oswald Buddenhagen wrote:
>> The child not dying is tricky, if it is in the same process group as >> git then even if git dies the I think the shell will wait for the >> child to exit before showing the prompt again so it is not clear to me >> that the user is disadvantaged by git ignoring SIGINT in that case.
>>
> there is no such thing as waiting for grandchildren. the grandchild is > reparented to init when the child exits.
> > there is a situation were one can be deadlocked by a non-exiting > grandchild: when doing a blocking read of the child's output past its > exit, when the grandchild has inherited stdout. but that's a > implementation bug in the parent. and not relevant here.
Yes I got carried away and thought that the shell waited for all the processes in the foreground process group, but it can only wait on those processes that it created.
> On Fri, Sep 08, 2023 at 02:11:51PM +0100, Phillip Wood wrote:
>> On 08/09/2023 10:59, Phillip Wood wrote:
>>>> I've never done it before, but from my reading we basically want to do
>>>> (in the forked process before we exec):
>>>>
>>>> setsid();
>>>> open("/dev/tty");
>>>
>>> Do we want a whole new session? As I understand it to launch a >>> foreground job shells put the child in its own process group and then >>> call tcsetpgrp() to change the foreground process group of the >>> controlling terminal to that of the child.
>>
> this would indeed be the right way if we wanted to isolate the children > more, but ...
> >> It is better for handling SIGINT and SIGQUIT when we don't want git to >> be killed but in complicates the handling of SIGTSTP and friends. [...]
>>
> ... this shows that we really don't want that; we don't want to > replicate interactive shell behavior. that is even before the divergence > on windows.
Yeah, I'm not enthusiastic about emulating the shell's job control in git.
> so i think your patch is approaching things the right way.
> though blocking signals doesn't appear right - to ensure git's own clean > exit while it has no children, it must catch sigint anyway, and > temporarily ignoring it around spawning children sounds racy.
There is an inevitable race between wait() returning and calling signal() to restore the handlers for SIGINT and SIGQUIT, it is such a small window I'm not sure it is a problem in practice. There is also a race when creating the child but if we block signals before calling fork, then ignore SIGINT and SIGQUIT in the parent before unblocking them we're OK because the child will be killed as soon as it unblocks signal by any signal received while the signals were blocks and we'll detect that in the parent and exit. Currently editor.c just ignores the signals after fork has returned in the parent which means it is theoretically possible to kill git with SIGINT while the child is running.
Best Wishes
Phillip
|
|
This patch series was integrated into seen via git@1dd5a2f. |
|
On the Git mailing list, Oswald Buddenhagen wrote (reply to this): On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>On 11/09/2023 11:00, Phillip Wood wrote:
>> There is an inevitable race between wait() returning and calling >> signal() to restore the handlers for SIGINT and SIGQUIT,
>
>In principle if we installed a signal handler to set a flag if a signal >is received while calling wait() and then once wait() returns >successfully see if the child was killed we can tell if the signal was >received while the child was alive.
>
yes, this is what i was already writing:
my point is that you shouldn't be doing that in the first place.
install the handlers when the sequencer is entered and leave them there.
the handlers need to set (volatile) flag variables, which are checked by
the sequencer on a regular basis.
a few notes on that:
- install without SA_RESTART, so syscalls can actually return with EINTR
and give us the opportunity to check the flag.
- an alternative to setting flags is setjmp()/longjmp(), but you really
don't want to go there.
- install with SA_RESETHAND, so the second ctrl-c will kill git
regardless, providing an escape hatch.
>In practice if the child is catching SIGINT or SIGQUIT we cannot rely >on it re-raising the signal so that wont work.
>
yes, but that's a minor issue, i think.
by far most hooks and other things that might be invoked within sequencer context don't mess with signals in the first place.
the things that do should be presumed to do the right thing, which means at least a non-zero exit status in case of a premature termination, which will yield pretty much the same effect on our side anyway.
so the only actually problematic situation would be us completely ignoring the exit code (like the git-gc call, but that's clearly a bug in git, and we control both sides, so it's easily fixable).
regards |
|
There was a status update in the "Cooking" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Will merge to 'next'? source: <[email protected]> |
|
There was a status update in the "Cooking" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Will merge to 'next'? source: <[email protected]> |
|
This patch series was integrated into seen via git@e34459f. |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 11/09/2023 11:32, Oswald Buddenhagen wrote:
> On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>> On 11/09/2023 11:00, Phillip Wood wrote:
>>> There is an inevitable race between wait() returning and calling >>> signal() to restore the handlers for SIGINT and SIGQUIT,
>>
>> In principle if we installed a signal handler to set a flag if a >> signal is received while calling wait() and then once wait() returns >> successfully see if the child was killed we can tell if the signal was >> received while the child was alive.
>>
> yes, this is what i was already writing:
I'm afraid that was not clear to me from your message.
> my point is that you shouldn't be doing that in the first place.
> install the handlers when the sequencer is entered and leave them there.
> the handlers need to set (volatile) flag variables, which are checked by
> the sequencer on a regular basis.
I did consider doing that before I submitted this patch but it is a much more invasive and substantial change. The patch here makes it safe for the user to interrupt a subprocess started by the sequencer. If I understand correctly your suggestion implies that the user could interrupt the sequencer at any point and we'd need to exit and ensure that they could safely continue the rebase afterwards. That is not the case at the moment and I'm concerned making that promise could turn into a maintenance burden in the future.
Best Wishes
Phillip |
|
On the Git mailing list, Oswald Buddenhagen wrote (reply to this): On Wed, Sep 13, 2023 at 04:33:06PM +0100, Phillip Wood wrote:
>On 11/09/2023 11:32, Oswald Buddenhagen wrote:
>> On Mon, Sep 11, 2023 at 11:14:31AM +0100, Phillip Wood wrote:
>>> On 11/09/2023 11:00, Phillip Wood wrote:
>>>> There is an inevitable race between wait() returning and calling >>>> signal() to restore the handlers for SIGINT and SIGQUIT,
>>>
>>> In principle if we installed a signal handler to set a flag if a >>> signal is received while calling wait() and then once wait() returns >>> successfully see if the child was killed we can tell if the signal was >>> received while the child was alive.
>>>
>> yes, this is what i was already writing:
>
>I'm afraid that was not clear to me from your message.
>
i meant, this is what i already wrote before i read your reply-to-self.
i just pasted it into the new reply i sent instead without adjusting for the new context. the sentence was meant to explain the slight "impedance mismatch".
>> install the handlers when the sequencer is entered and leave them >> there. the handlers need to set (volatile) flag variables, which are >> checked by the sequencer on a regular basis.
>
>I did consider doing that before I submitted this patch
>but it is a much more invasive and substantial change.
>
yes
>The patch here makes it safe for the user to interrupt a subprocess >started by the sequencer.
>
for the exec case, i don't see how this actually improves anything. whether git gets killed along with the child, or catches the child's abnormal exit and immediately exits, makes no difference. arguably, it's even counter-productive, because from the outside it's random whether git will just exit on sigint or report that its child exited on sigint and exit with some other status.
actual value would come from doing something before exiting, but the commit message is pretty much saying that this is not the case.
the commit/edit case is more complicated, but arguably the problem is the (hypothetical?) editor that just ignores sigint rather than reprogramming the terminal appropriately for full-screen use. git-commit ignoring sigint seems like a somewhat misguided workaround, and piling on top of that won't really improve things.
>If I understand correctly your suggestion implies that the user could >interrupt the sequencer at any point and we'd need to exit and ensure >that they could safely continue the rebase afterwards.
>
yes
>That is not the case at the moment
>and I'm concerned making that promise could turn into a maintenance >burden in the future.
>
of course it would. the question is whether it would be worth it. with delayed state commits, some extra trasactionality might well be required.
regards |
|
This patch series was integrated into seen via git@089d16c. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
> Do we want a whole new session? As I understand it to launch a foreground
> job shells put the child in its own process group and then call tcsetpgrp()
> to change the foreground process group of the controlling terminal to that
> of the child. I agree that would be a better way of doing things on unix.
One thing I am not clear on is the convention on who does the process
group and controlling terminal setup. Should Git do it to say "I am
handing off control of the terminal to the editor that I am spawning"?
Or should the editor say "I am an editor which has a user interface that
takes over the terminal; I will control it while I am running".
The latter makes much more sense to me, as Git cannot know how the
editor plans to behave. But as I understand it, this kind of job control
stuff is implemented by the calling shell, which does the tcsetpgrp()
call.
So I dunno. It sounds to me like the "right" thing here is making Git
more shell-like in handing control to a program (like the editor) that
we expect to be in the foreground of the terminal. As opposed to the
"ignore SIGINT temporarily" thing which feels more like band-aid. But
I'm wary of getting into a rabbit hole of portability headaches and
weird corners of Unix terminal-handling conventions.
-Peff |
|
On the Git mailing list, Phillip Wood wrote (reply to this): On 14/09/2023 10:56, Jeff King wrote:
> On Fri, Sep 08, 2023 at 10:59:06AM +0100, Phillip Wood wrote:
> >> Do we want a whole new session? As I understand it to launch a foreground
>> job shells put the child in its own process group and then call tcsetpgrp()
>> to change the foreground process group of the controlling terminal to that
>> of the child. I agree that would be a better way of doing things on unix.
> > One thing I am not clear on is the convention on who does the process
> group and controlling terminal setup. Should Git do it to say "I am
> handing off control of the terminal to the editor that I am spawning"?
> Or should the editor say "I am an editor which has a user interface that
> takes over the terminal; I will control it while I am running".
As I understand it the editor has a controlling terminal (assuming there is a controlling terminal associated with the editor's session id), not the other way round. If the editor is launched in the background then it will receive SIGTTIN when it tries to read from it's controlling terminal which stops the process unless the process installs a signal handler.
> The latter makes much more sense to me, as Git cannot know how the
> editor plans to behave. But as I understand it, this kind of job control
> stuff is implemented by the calling shell, which does the tcsetpgrp()
> call.
Yes, my understanding is that the shell puts all the processes in a pipeline in the same process group and calls tcsetpgrp() if it wants that job to be run in the foreground.
> So I dunno. It sounds to me like the "right" thing here is making Git
> more shell-like in handing control to a program (like the editor) that
> we expect to be in the foreground of the terminal. As opposed to the
> "ignore SIGINT temporarily" thing which feels more like band-aid. But
> I'm wary of getting into a rabbit hole of portability headaches and
> weird corners of Unix terminal-handling conventions.
I'm wary of that too, it has the potential to end up adding a lot of fiddly code checking if git is in the foreground or background and propagating SIGTSTP and friends up to the shell. In any case we'd need the "ignore SIGINT" thing for windows anyway. Ignoring SIGINT and SIGQUIT in the parent is what system(3) does. As long as git exits promptly when the interrupted child is killed I think that is reasonable. Would you be happier if we re-raised the signal once we have cleaned up any state that needs to be written before exiting?
Best Wishes
Phillip
|
|
This patch series was integrated into seen via git@5df5123. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@9a23bc2. |
|
This patch series was integrated into seen via git@a8fb882. |
|
This patch series was integrated into seen via git@4582b61. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@7483e93. |
|
This patch series was integrated into seen via git@a515234. |
|
This patch series was integrated into seen via git@e430124. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@f448de2. |
|
This patch series was integrated into seen via git@da3b3d5. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@1a4bdc0. |
|
This patch series was integrated into seen via git@294d244. |
|
This patch series was integrated into seen via git@5f6d503. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@4a8849d. |
|
This patch series was integrated into seen via git@779abdb. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@3367ef8. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@4363fb1. |
|
This patch series was integrated into seen via git@9c3c503. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@ef020ed. |
|
This patch series was integrated into seen via git@ceb225b. |
|
There was a status update in the "Stalled" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@acfb45e. |
|
There was a status update in the "Discarded" section about the branch If the commit log editor or other external programs (spawned via "exec" insn in the todo list) receive internactive signal during "git rebase -i", it caused not just the spawned program but the "Git" process that spawned them, which is often not what the end user intended. "git" learned to ignore SIGINT and SIGQUIT while waiting for these subprocesses. Expecting a reroll. cf. <[email protected]> source: <[email protected]> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Cc: Johannes Schindelin [email protected]
Cc: Jeff King [email protected]
cc: Phillip Wood [email protected]
cc: Oswald Buddenhagen [email protected]