Re: [PATCH v4 06/10] format-patch: explicitly switch off color when writing to files

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2016, 22:01 UTC
Message-ID: <xmqq1t3mfdpy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <c0fdb78fbb7b19e4b367c50a9c0c570193e98fa3.1466607667.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 15 quoted lines

> We rely on the auto-detection ("is stdout a terminal?") to determine
> whether to use color in the output of format-patch or not. That happens
> to work because we freopen() stdout when redirecting the output to files.
>
> However, we are about to fix that work-around, in which case the
> auto-detection has no chance to guess whether to use color or not.
>
> But then, we do not need to guess to begin with. As argued in the commit
> message of 7787570c (format-patch: ignore ui.color, 2011-09-13), we do not
> allow the ui.color setting to affect format-patch's output. The only time,
> therefore, that we allow color sequences to be written to the output files
> is when the user specified the --color command-line option explicitly.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

The right fix in the longer term (long after this series lands, that is) is probably to update the world view that the codepath from want_color_auto() to check_auto_color() has always held. In their world view, when they are asked to make --color=auto decision, the output always goes the standard output, and that is why they hardcode isatty(1) to decide. The existing freopen() was a part of that world view.

We'd need a workaround like this patch if we want to leave the want_color_auto() as-is, and as a workaround I think this is the least invasive one, so let's queue it as-is.

If the codepaths that use diffopt.file (not just this one that is about output directory hence known to be writing to a file, but all the log/diff family of commands after this series up to 5/10 has been applied) have a way to tell want_color_auto() that the output is going to fileno(diffopt.file), and have check_auto_color() use that fd instead of the hardcoded 1, the problem this step is trying to address goes away, and I think that would be the longer-term fix.

Thanks.

Show 16 quoted lines

>  builtin/log.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 27bc88d..5683a42 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1578,6 +1578,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		setup_pager();
>  
>  	if (output_directory) {
> +		if (rev.diffopt.use_color != GIT_COLOR_ALWAYS)
> +			rev.diffopt.use_color = 0;
>  		if (use_stdout)
>  			die(_("standard output, or directory, which one?"));
>  		if (mkdir(output_directory, 0777) < 0 && errno != EEXIST)