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

["Rubén Justo" <rjusto@gmail.com>]

On Mon, May 20, 2024 at 12:14:04PM -0700, Junio C Hamano wrote:
> 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.
> 
> 
> > 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()?

There is no change in what calling close_pager_fds() means.

> 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().

Yes, no change here either.  In the next commit, a new client of the
helper is introduced, the new API: wait_for_pager().

> 
> 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?

This series is trying to allow triggering the pager multiple times.
Reaching to close_pager_fds() multiple times is a consequence of it.

> 
> >  static struct child_process pager_process;
> >  static const char *pager_program;
> > +static int old_fd2 = -1;
> 
> What does the magic number "-1" mean?

Invalid fd.

> 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.
> 
> >  /* 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.

It applies, in 3/5.

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

Thanks for reading the series.