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

> Rubén Justo <rjusto@gmail.com> 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?
>