Re: [PATCH v4 3/6] pager: introduce wait_for_pager

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 4, 2024, 10:00 UTC
Message-ID: <3f085795-79bd-4a56-9df8-659e32179925@gmail.com>
In-Reply-To: <76c725b4-1bc4-4916-81d8-98cad8fc4ca0@gmail.com>

Hi Rubén

On 03/06/2024 21:38, Rubén Justo wrote:

Show 13 quoted lines

> Since f67b45f862 (Introduce trivial new pager.c helper infrastructure,
> 2006-02-28) we have the machinery to send our output to a pager.
> 
> That machinery, once set up, does not allow us to regain the original
> stdio streams.
> 
> In the interactive commands (i.e.: add -p) we want to use the pager for
> some output, while maintaining the interaction with the user.
> 
> Modify the pager machinery so that we can use setup_pager and, once
> we've finished sending the desired output for the pager, wait for the
> pager termination using a new function wait_for_pager.   Make this
> function reset the pager machinery before returning.

This makes sense, I've left a few comments below

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---

Show 5 quoted lines

>   static void wait_for_pager_atexit(void)
>   {
> +	if (old_fd1 == -1)
> +		return;
> +

This is good - we'll return early if we've already cleaned up the pager.

Show 10 quoted lines

>   	fflush(stdout);
>   	fflush(stderr);
>   	close_pager_fds();
>   	finish_command(&pager_process);
>   }
>   
> +void wait_for_pager(void)
> +{
> +	if (old_fd1 == -1)
> +		return;

Isn't it a bug to call this with old_fd1 == -1 or have I missed something?

Show 7 quoted lines

> +	wait_for_pager_atexit();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		old_fd2 = -1;

We're leaking old_fd1 and old_fd2 here. wait_for_pager_atexit() flushes stdout and stderr so this switching of fds should play nicely with code that uses stdio.

Show 25 quoted lines

> @@ -113,6 +134,7 @@ void prepare_pager_args(struct child_process *pager_process, const char *pager)
>   
>   void setup_pager(void)
>   {
> +	static int once = 0;
>   	const char *pager = git_pager(isatty(1));
>   
>   	if (!pager)
> @@ -142,16 +164,18 @@ void setup_pager(void)
>   		return;
>   
>   	/* original process continues, but writes to the pipe */
> +	old_fd1 = dup(1);
>   	dup2(pager_process.in, 1);
>   	if (isatty(2)) {
> -		close_fd2 = 1;
> +		old_fd2 = dup(2);
>   		dup2(pager_process.in, 2);
>   	}
>   	close(pager_process.in);
>   
> -	/* this makes sure that the parent terminates after the pager */
> -	sigchain_push_common(wait_for_pager_signal);
> -	atexit(wait_for_pager_atexit);
> +	if (!once++) {

We only need to increment "once" when we enter this block, not every time the code is run.

> +		sigchain_push_common(wait_for_pager_signal);

I think we should be calling this each time we setup the pager and pop it in wait_for_pager(). Imagine a caller sets up a signal handler before calling setup_pager() and wants to pop it after the pager has finished

	sigchain_push(...)
	setup_pager(...)
	do_something()
	wait_for_pager()
	sigchain_pop(...)

With the changes here it will pop the signal handler added by setup_pager() rather than the one it is expecting.

> +		atexit(wait_for_pager_atexit);

It is a bit of a shame we have to leave this function active when the pager has finished. We could add a wrapper around atexit() that allows us to pop functions we no-longer want to call but I don't think it is worth the effort here. wait_for_pager_atexit() is careful to return early if it is not needed.

Best Wishes

Phillip