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

From: Rubén Justo <[email-masked]>
Date: Tue, 21 May 2024 23:35:17 +0200

On Tue, May 21, 2024 at 01:57:19PM -0700, Junio C Hamano wrote:
> Rubén Justo <[email-masked]> writes:
> 
> >> >  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.
> >>  ....
> >> 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.
> 
> We need to be prepared to see a series chomped at an early stage and
> it should still make sense.  If the series does not make sense when
> you stop before applying patch 3, it is a strong sign that this step
> and the next step can be separated and structured better.
> 
> Or perhaps if they are made into a single patch it makes more sense
> and becomes easier to explain?
> 

Adding logic to adjust when we close(stderr) in close_pager_fds() makes
sense on its own, I think.

And, the values for the flag "do-we-want-to-close-stderr-at-exit", too,
to me.

I am happy with the series;  the 'P' command introduced in v2 is a good
improvement.  Combining 2/5 and 3/5, I think it is not a good idea.

Therefore, I'm not sure how to alleviate the puzzling.