git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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
Previous: Trygve AabergeNext: Jeff King
Message 7 of 19 in “Regression: Ctrl-c from the pager in an alias exits it”
  1. Trygve AabergeJan 5, 2017
  2. Jeff KingJan 6, 2017
  3. Jeff KingJan 6, 2017
  4. Jeff KingJan 6, 2017
  5. Jeff KingJan 6, 2017
  6. Trygve AabergeJan 6, 2017
  7. Johannes SixtJan 6, 2017
  8. Jeff KingJan 6, 2017
  9. Johannes SixtJan 6, 2017
  10. Jeff KingJan 6, 2017
  11. 0/3 fix ^C killing pager when running aliasJeff King, Jan 7, 2017
  12. 1/3 execv_dashed_external: use child_process structJeff King, Jan 7, 2017
  13. 2/3 execv_dashed_external: stop exiting with negative codeJeff King, Jan 7, 2017
  14. 3/3 execv_dashed_external: wait for child on signal deathJeff King, Jan 7, 2017
  15. Johannes SixtJan 7, 2017
  16. Jeff KingJan 7, 2017
  17. Duy NguyenJan 7, 2017
  18. Jacob KellerJan 7, 2017
  19. Jacob KellerJan 7, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.