Re: [PATCH v4 3/6] pager: introduce wait_for_pager

["Rubén Justo" <rjusto@gmail.com>]

On Tue, Jun 04, 2024 at 11:00:37AM +0100, Phillip Wood wrote:
> 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?

It is.  I'll remove it to avoid confusion.

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

Good eyes.  Will fix.  Thanks.

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

OK. :-)

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

I hadn't thought about this.  Thank you for pointing it out.

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

Thanks!