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

[Junio C Hamano <gitster@pobox.com>]

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