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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 20, 2016, 06:26 UTC
Message-ID: <alpine.DEB.2.20.1606200814510.22630@virtualbox>
In-Reply-To: <CAPig+cTiexRhzS3MwMEntGYxKms-XQvtoc7HOnUGJvDaBSK7JA@mail.gmail.com>

Hi Eric,

On Sun, 19 Jun 2016, Eric Sunshine wrote:

Show 16 quoted lines

> 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>