[3/4] pager: introduce wait_for_pager

Commit Message

Rubén Justo July 12, 2024, 1 a.m. UTC

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.

Signed-off-by: Rubén Justo <[e-mail]>
---
 pager.c | 43 +++++++++++++++++++++++++++++++++++++------
 pager.h |  1 +
 2 files changed, 38 insertions(+), 6 deletions(-)

Comments

Phillip Wood July 12, 2024, 1:17 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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);

Patch

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);