From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 12 Jul 2024 13:17:08 GMT
Subject: Re: [PATCH 3/4] pager: introduce wait_for_pager
Message-ID: <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com>
In-Reply-To: <4b5d0e7c-9492-4495-9bc1-40ebea850fde@gmail.com>

Hi Rubén

On 12/07/2024 02:00, Rubén Justo wrote:
> 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 looks good and addresses my previous comments about leaking file 
descriptors and restoring signal dispositions. A couple of thoughts 
occurred to me as I was reading it again:

  - We ignore any errors when duplicating fds,
    "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our
    code base, though if we cannot redirect the output to the pager or
    restore stdout when the pager exits that's a problem for "git add -p"

  - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them
    being passed to the pager.

Best Wishes

Phillip

> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>   pager.c | 43 +++++++++++++++++++++++++++++++++++++------
>   pager.h |  1 +
>   2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/pager.c b/pager.c
> index 251adfc2ad..bea4345f6f 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -14,7 +14,7 @@ int pager_use_color = 1;
>   
>   static struct child_process pager_process;
>   static char *pager_program;
> -static int close_fd2;
> +static int old_fd1 = -1, old_fd2 = -1;
>   
>   /* Is the value coming back from term_columns() just a guess? */
>   static int term_columns_guessed;
> @@ -24,11 +24,11 @@ static void close_pager_fds(void)
>   {
>   	/* signal EOF to pager */
>   	close(1);
> -	if (close_fd2)
> +	if (old_fd2 != -1)
>   		close(2);
>   }
>   
> -static void wait_for_pager_atexit(void)
> +static void finish_pager(void)
>   {
>   	fflush(stdout);
>   	fflush(stderr);
> @@ -36,8 +36,34 @@ static void wait_for_pager_atexit(void)
>   	finish_command(&pager_process);
>   }
>   
> +static void wait_for_pager_atexit(void)
> +{
> +	if (old_fd1 == -1)
> +		return;
> +
> +	finish_pager();
> +}
> +
> +void wait_for_pager(void)
> +{
> +	finish_pager();
> +	sigchain_pop_common();
> +	unsetenv("GIT_PAGER_IN_USE");
> +	dup2(old_fd1, 1);
> +	close(old_fd1);
> +	old_fd1 = -1;
> +	if (old_fd2 != -1) {
> +		dup2(old_fd2, 2);
> +		close(old_fd2);
> +		old_fd2 = -1;
> +	}
> +}
> +
>   static void wait_for_pager_signal(int signo)
>   {
> +	if (old_fd1 == -1)
> +		return;
> +
>   	close_pager_fds();
>   	finish_command_in_signal(&pager_process);
>   	sigchain_pop(signo);
> @@ -113,6 +139,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 +169,20 @@ void setup_pager(void)
>   		die("unable to execute pager '%s'", pager);
>   
>   	/* 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) {
> +		once++;
> +		atexit(wait_for_pager_atexit);
> +	}
>   }
>   
>   int pager_in_use(void)
> diff --git a/pager.h b/pager.h
> index b77433026d..103ecac476 100644
> --- a/pager.h
> +++ b/pager.h
> @@ -5,6 +5,7 @@ struct child_process;
>   
>   const char *git_pager(int stdout_is_tty);
>   void setup_pager(void);
> +void wait_for_pager(void);
>   int pager_in_use(void);
>   int term_columns(void);
>   void term_clear_line(void);