On Mon, May 20, 2024 at 12:14:04PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
