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

From: Junio C Hamano <[email-masked]>
Date: Tue, 21 May 2024 13:57:19 -0700

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?