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

From: Junio C Hamano <gitster@pobox.com>
Date: May 20, 2024, 19:14 UTC
Message-ID: <xmqqo790fg8z.fsf@gitster.g>
In-Reply-To: <80f15223-246e-4cfb-a139-e47af829c938@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 9 quoted lines

> 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 might call close_pager_fds multiple times.  With this in mind,
> unconditionally closing stderr will become undesirable.

In a new world with such a change, what does it mean to call close_pager_fds()? It used to mean "we are really done with the pager and we no longer need them, ever".

And we still call the helper for that purpose after this change, from wait_for_pager_atexit() and wait_for_pager_signal().

So no matter what "a subsequent commit" does, it feels conceptually wrong to call it more than once in the first place. In other words, what is wrong is that this function closes stderr, but "a subsequent commit" calls this function multiple times, no?

>  static struct child_process pager_process;
>  static const char *pager_program;
> +static int old_fd2 = -1;

What does the magic number "-1" mean? We often use it to signal "uninitialized", but then what are concrete "initialized" values mean? "We dup2()'ed something else to stderr/fd #2 but before doing so we saved the original fd #2 away to this variable, so that we can restore fd #2 by another dup2() of the value of this variable when we declare that we are done with the standard error stream"?

But that does not look like what is happening here.

Show 19 quoted lines

>  /* 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 (old_fd2 != -1)
> +		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)) {
> +		old_fd2 = 1;

Equally unclear magic number "1" is used here.

This value is different from pager_process.in, and my earlier "we are saving away" does not apply, either.

>  		dup2(pager_process.in, 2);
> +	}
>  	close(pager_process.in);

Puzzled...