From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 04 Jun 2024 10:00:37 GMT
Subject: Re: [PATCH v4 3/6] pager: introduce wait_for_pager
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:
> 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>
> ---

>   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.

>   	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?

> +	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.

> @@ -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