Re: [PATCH v3 3/4] pager: introduce wait_for_pager
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jul 15, 2024, 14:13 UTC
- Message-ID
- <384f0147-d611-493b-a3d4-d83c65bd1114@gmail.com>
- In-Reply-To
- <f48ac176-9938-4677-a956-350fb50dbc0f@gmail.com>
Hi Rubén
On 14/07/2024 17:04, 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.
Do you have any comments on my thoughts in <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ?
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);