From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 06 Jan 2017 14:39:59 GMT
Subject: Re: Regression: Ctrl-c from the pager in an alias exits it
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:
> 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