Re: [PATCH 5/5] format-patch: avoid freopen()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Sun, 19 Jun 2016, Eric Sunshine wrote:

> On Sat, Jun 18, 2016 at 6:04 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > We just taught the relevant functions to respect the diffopt.file field,
> > to allow writing somewhere else than stdout. Let's make use of it.
> > [...]
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/builtin/log.c b/builtin/log.c
> > @@ -1569,6 +1570,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
> >                 setup_pager();
> >
> >         if (output_directory) {
> > +               rev.diffopt.use_color = 0;
> 
> What is this change about? It doesn't seem to be related to anything
> else in the patch.

Good point, I completely forgot to mention this is the commit message.

When format-patch calls the diff machinery, want_color() is used to
determine whether to use ANSI color sequences or not. If use_color is not
set explicitly, isatty(1) is used to determine whether or not the user
wants color. When stdout was freopen()ed, this isatty(1) call naturally
looked at the file descriptor that was reopened, and determined correctly
that no color was desired.

With the freopen() call gone, stdout may very well be the terminal. But we
still do not want color because the output is intended to go to a file (at
least if output_directory is set).

So how about this commit message (I inserted the "Note: ..." paragraph)?

-- snipsnap --
format-patch: avoid freopen()

We just taught the relevant functions to respect the diffopt.file field,
to allow writing somewhere else than stdout. Let's make use of it.

Technically, we do not need to avoid that call in a builtin: we assume
that builtins (as opposed to library functions) are stand-alone programs
that may do with their (global) state. Yet, we want to be able to reuse
that code in properly lib-ified code, e.g. when converting scripts into
builtins.

Note: we now have to set use_color = 0 explicitly when writing to files,
as the auto-detection whether to colorify the output *still* looks at
stdout (which is no longer freopen()ed, and therefore might still be
printing to the terminal).

Further, while we did not *have* to touch the cmd_show() and cmd_cherry()
code paths (because they do not want to write anywhere but stdout as of
yet), it just makes sense to be consistent, making it easier and safer to
move the code later.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>