Re: [PATCH 2/5] pager: do not close fd 2 unnecessarily
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 22, 2024, 17:40 UTC
- Message-ID
- <xmqqa5kh68zh.fsf@gitster.g>
- In-Reply-To
- <501a610c-550f-45da-a311-d4c941ae4870@gmail.com>
Rubén Justo <rjusto@gmail.com> writes:
Show 20 quoted lines
> Perhaps this makes more sense?: > > 1: 8fe915a820 ! 1: 70cc34efc4 pager: do not close fd 2 unnecessarily > @@ pager.c: int pager_use_color = 1; > > static struct child_process pager_process; > static const char *pager_program; > -+static int old_fd2 = -1; > ++static int old_fd2; > > /* Is the value coming back from term_columns() just a guess? */ > static int term_columns_guessed; > @@ pager.c: static void close_pager_fds(void) > /* signal EOF to pager */ > close(1); > - close(2); > -+ if (old_fd2 != -1) > ++ if (old_fd2) > + close(2); > }
Not really. The name "old_fd2" strongly implies "where did fd#2 come from?" and it did not come from fd#0, did it? Until [3/5] this variable used to mean something different from "this was the saved fd#2 we can use to restore it later", which is the name "old_fd2" clearly wants to stand for.
If you really want to have them as two separate patches, I would expect the proposed log message for the [3/5] step to say something like
... we added variable X to signal if we should close fd#2 in
function F in the previous step. As store away the original
fd#2 with dup(2) to be restored later after we close() it, the
question the previous step asked, "should we be the one closing
fd#2?" becomes equivalent to "have we stored away the original
fd#2 (in which case we close() fd#2 when we are done with the
pager and restore the original one)?" Rename X to old_fd2 and
have it serve both purposes.or somesuch.