| Message ID | e98dc7b1-3c93-41d2-a2ef-7f9f69789886@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use the pager in 'add -p' | expand |
Rubén Justo <rjusto@gmail.com> writes: > We send errors to the pager since 61b80509e3 (sending errors to stdout > under $PAGER, 2008-02-16). > > In a8335024c2 (pager: do not dup2 stderr if it is already redirected, > 2008-12-15) an exception was introduced to avoid redirecting stderr if > it is not connected to a terminal. > > In such exceptional cases, the close(STDERR_FILENO) we're doing in > close_pager_fds, is unnecessary. I was wondering how we can test this. > diff --git a/pager.c b/pager.c > index b8822a9381..b786601074 100644 > --- a/pager.c > +++ b/pager.c > @@ -14,6 +14,7 @@ int pager_use_color = 1; > > static struct child_process pager_process; > static const char *pager_program; > +static int close_fd2; > > /* Is the value coming back from term_columns() just a guess? */ > static int term_columns_guessed; > @@ -23,7 +24,8 @@ static void close_pager_fds(void) > { > /* signal EOF to pager */ > close(1); > - close(2); > + if (close_fd2) > + close(2); > } > > static void wait_for_pager_atexit(void) > @@ -141,8 +143,10 @@ void setup_pager(void) > > /* original process continues, but writes to the pipe */ > dup2(pager_process.in, 1); > - if (isatty(2)) > + if (isatty(2)) { > + close_fd2 = 1; > dup2(pager_process.in, 2); > + } > close(pager_process.in); At this step, we are assuming that we would start the pager only once during the whole process, so relying on the 0-initialization of close_fd2 in the BSS and setting it to 1 as needed when we dup2() is sufficient, but presumably we would want to explicitly set close_fd2 to 0 when we are not calling dup2() here for completeness, with an eye to the future where we run the pager multiple time. Other than that, this looks reasonable to me. Perhaps we should be checking the return value of our close() system calls? We would be getting scolded for closing an invalid file descriptor, if we are closing something we shouldn't be closing, right? Thanks.
diff --git a/pager.c b/pager.c index b8822a9381..b786601074 100644 --- a/pager.c +++ b/pager.c @@ -14,6 +14,7 @@ int pager_use_color = 1; static struct child_process pager_process; static const char *pager_program; +static int close_fd2; /* Is the value coming back from term_columns() just a guess? */ static int term_columns_guessed; @@ -23,7 +24,8 @@ static void close_pager_fds(void) { /* signal EOF to pager */ close(1); - close(2); + if (close_fd2) + close(2); } static void wait_for_pager_atexit(void) @@ -141,8 +143,10 @@ void setup_pager(void) /* original process continues, but writes to the pipe */ dup2(pager_process.in, 1); - if (isatty(2)) + if (isatty(2)) { + close_fd2 = 1; dup2(pager_process.in, 2); + } close(pager_process.in); /* this makes sure that the parent terminates after the pager */
We send errors to the pager since 61b80509e3 (sending errors to stdout under $PAGER, 2008-02-16). In a8335024c2 (pager: do not dup2 stderr if it is already redirected, 2008-12-15) an exception was introduced to avoid redirecting stderr if it is not connected to a terminal. In such exceptional cases, the close(STDERR_FILENO) we're doing in close_pager_fds, is unnecessary. Furthermore, in a subsequent commit we're going to introduce changes that will involve using close_pager_fds multiple times. With this in mind, controlling when we want to close stderr, become sensible. Let's close(STDERR_FILENO) only when necessary, and pave the way for the upcoming changes. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- pager.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)