| Message ID | f48ac176-9938-4677-a956-350fb50dbc0f@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Hi Rubén On 14/07/2024 17:04, 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. Do you have any comments on my thoughts in <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ? Best Wishes Phillip > 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);
On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote: > Hi Rubén > > On 14/07/2024 17:04, 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. > > Do you have any comments on my thoughts in > <8434fafe-f545-49bc-8cc1-d4e8fb634bec@gmail.com> ? Oops! I thought I had responded, but somehow I must not have. For reference, these are the points you indicated: > - We ignore any errors when duplicating fds, > "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our > code base, though if we cannot redirect the output to the pager or > restore stdout when the pager exits that's a problem for "git add -p" > > - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them > being passed to the pager. Both points are interesting and improve resilience to unexpected situations. I remember that the first point was already suggested in the previous thread. IMHO both points should be considered with a more global perspective than the scope of this series. As I said in the first message of this thread, I have left out interesting points that may deserve to be addressed in future series, with the intention of not prolonging the discussion of the current changes too much. Sorry for not responding sooner.
Hi Rubén On 15/07/2024 21:04, Rubén Justo wrote: > On Mon, Jul 15, 2024 at 03:13:09PM +0100, Phillip Wood wrote: > For reference, these are the points you indicated: > >> - We ignore any errors when duplicating fds, >> "git grep '[^a-z_]dup2\{0,1\}(' shows that's not unusual in our >> code base, though if we cannot redirect the output to the pager or >> restore stdout when the pager exits that's a problem for "git add -p" >> >> - We should perhaps be marking old_fd[12] with O_CLOEXEC to stop them >> being passed to the pager. > > Both points are interesting and improve resilience to unexpected > situations. I remember that the first point was already suggested in > the previous thread. > > IMHO both points should be considered with a more global perspective > than the scope of this series. I'm not sure what you mean by this. It is true that we were ignoring dup2() errors in this function before but this patch the O_CLOEXEC issue is new. > As I said in the first message of this thread, I have left out > interesting points that may deserve to be addressed in future series, > with the intention of not prolonging the discussion of the current > changes too much. I think there is a difference between adding new features which I agree should be left and getting the implementation details of this helper function right. Having said that ignoring the dup() errors isn't making anything worse than it was and the extra fds are for the terminal which the pager is accessing anyway. > Sorry for not responding sooner. No worries, thanks for your reply Phillip
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);
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 | 43 +++++++++++++++++++++++++++++++++++++------ pager.h | 1 + 2 files changed, 38 insertions(+), 6 deletions(-)