Re: Regression: Ctrl-c from the pager in an alias exits it
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Jan 6, 2017, 14:39 UTC
- Message-ID
- <3d433abf-71a2-4702-f62b-e254520dc32c@kdbg.org>
- In-Reply-To
- <20170106073224.5hsrib77tx5tgx7d@sigill.intra.peff.net>
Am 06.01.2017 um 08:32 schrieb Jeff King:
Show 50 quoted lines
> On Fri, Jan 06, 2017 at 02:26:02AM -0500, Jeff King wrote:
>
>> You'll notice that it actually calls wait() on the pager. That's due to
>> a3da882120 (pager: do wait_for_pager on signal death, 2009-01-22), which
>> IIRC was addressing a very similar problem. We want to stop feeding the
>> pager when we get a signal, but we don't want the main process to
>> actually exit, or the pager loses the controlling terminal.
>>
>> In our new scenario we have an extra process, though. The git-log child
>> will wait on the pager, but the parent process can't. It doesn't know
>> about it. I think that it in turn needs to wait on the child when it
>> dies, and then the whole chain will stand still until the pager exits.
>
> And here's a patch to do that. It seems to work.
>
> I'll sleep on it and then write up a commit message tomorrow if it still
> makes sense.
>
> diff --git a/run-command.c b/run-command.c
> index ca905a9e80..db47c429b7 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
>
> static void cleanup_children(int sig, int in_signal)
> {
> + struct child_to_clean *children_to_wait_for = NULL;
> +
> while (children_to_clean) {
> struct child_to_clean *p = children_to_clean;
> children_to_clean = p->next;
> @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
> }
>
> kill(p->pid, sig);
> + p->next = children_to_wait_for;
> + children_to_wait_for = p;
> + }
> +
> + while (children_to_wait_for) {
> + struct child_to_clean *p = children_to_wait_for;
> + children_to_wait_for = p->next;
> +
> + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> + ; /* spin waiting for process exit or error */
> +
> if (!in_signal)
> free(p);
> }
>This looks like the minimal change necessary. I wonder, though, whether the new local variable is really required. Wouldn't it be sufficient to walk the children_to_clean chain twice?
-- Hannes