| Message ID | 76c725b4-1bc4-4916-81d8-98cad8fc4ca0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
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
Rubén Justo <rjusto@gmail.com> writes: > static struct child_process pager_process; > static const char *pager_program; > -static int close_fd2; > +static int old_fd1 = -1, old_fd2 = -1; ;-) Presumably when old_fd2 does not have a valid value (i.e. -1) it means we did not do dup2() to save it away, and we refrain from closing #2 in that case? It is curious that #1 did not have similar problem 2/6 addressed, as we never redirected it with dup() to save it away. But now we do for some reason that the proposed log message did not explain. We should say something like We need to take back the standard output and the standard error stream after we are done with an invocation of the pager. For that, save away the original file descriptors 1 and 2 when spawning the pager, and restore them once the pager exits. The presence of saved fd#2 can be used to replace the "close_fd2" flag introduced in the previous patch. perhaps. > /* Is the value coming back from term_columns() just a guess? */ > static int term_columns_guessed; > @@ -24,20 +24,41 @@ static void close_pager_fds(void) > { > /* signal EOF to pager */ > close(1); > - if (close_fd2) > + if (old_fd2 != -1) > close(2); > } OK. > static void wait_for_pager_atexit(void) > { > + if (old_fd1 == -1) > + return; > + > fflush(stdout); > fflush(stderr); > close_pager_fds(); > finish_command(&pager_process); > } > > +void wait_for_pager(void) > +{ > + if (old_fd1 == -1) > + return; > + > + 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; > + } > +} Presumably these use old_fd1's validity as a signal to see if have pager running that need to be cleaned up? It feels a bit unnatural why we do not ask about such a process the structure that is set up to control it, namely, the pager_process structure, but this is OK for now. It is just a naming issue, but it smells strange that the normal code path (wait_for_pager()) calls a function that is an atexit handler, which is more specific and only to be called atexit. I would have made static void finish_pager(void) { fflush(stdout); fflush(stderr); close_pager_fds(); finish_command(&pager_process); } and then called it from the atexit handler and wait_for_pager(). > @@ -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); Can dup(2) fail and return -1? > > - /* this makes sure that the parent terminates after the pager */ > - sigchain_push_common(wait_for_pager_signal); > - atexit(wait_for_pager_atexit); > + if (!once++) { > + sigchain_push_common(wait_for_pager_signal); > + atexit(wait_for_pager_atexit); > + } > } Can we give a name better than "once" to this thing? > 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); Other than that, overall it looks good. Thanks, will queue.
Phillip Wood <phillip.wood123@gmail.com> writes: >> +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? Good point. >> + 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() Yeah, that needs fixing. >> + if (!once++) { > > We only need to increment "once" when we enter this block, not every > time the code is run. Running this 4 billion times and we'll be in a trouble ;-). >> + 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(). Good point.
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!
diff --git a/pager.c b/pager.c index b786601074..925f860335 100644 --- a/pager.c +++ b/pager.c @@ -14,7 +14,7 @@ int pager_use_color = 1; static struct child_process pager_process; static const 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,20 +24,41 @@ 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) { + if (old_fd1 == -1) + return; + fflush(stdout); fflush(stderr); close_pager_fds(); finish_command(&pager_process); } +void wait_for_pager(void) +{ + if (old_fd1 == -1) + return; + + 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; + } +} + 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 +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++) { + sigchain_push_common(wait_for_pager_signal); + 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);
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. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- pager.c | 36 ++++++++++++++++++++++++++++++------ pager.h | 1 + 2 files changed, 31 insertions(+), 6 deletions(-)