Rubén Justo <rjusto@xxxxxxxxx> writes:
> 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.
