Re: [PATCH 3/4] pager: introduce wait_for_pager
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 12, 2024, 13:17 UTC
- 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:
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 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
Show 112 quoted lines
> 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);