On Tue, May 21, 2024 at 01:57:19PM -0700, Junio C Hamano wrote: > Rubén Justo <rjusto@xxxxxxxxx> 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.
