Re: [PATCH v4 3/6] pager: introduce wait_for_pager
On Tue, Jun 04, 2024 at 11:00:37AM +0100, Phillip Wood wrote:
Show 41 quoted lines
> 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.
Show 10 quoted lines
>
> > + 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.
Show 28 quoted lines
> 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.Show 15 quoted lines
>
> > + 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.
Show 14 quoted lines
>
> > + 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
>
Rubén Justo <rjusto@gmail.com>