Re: [PATCH v4 2/6] pager: do not close fd 2 unnecessarily

[Junio C Hamano <gitster@pobox.com>]

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.