[PATCH] diff: let --output=<file> default to --no-color

[PATCH] diff: let --output=<file> default to --no-color

[Johannes Schindelin]

It is highly unlikely that any user would want to see ANSI color
sequences in a file. So let's stop doing that by default.

This is a backwards-incompatible change.

The reason this was not caught earlier is most likely that either
--output=<file> is not used, or only when stdout is redirected
anyway.

Technically, we do not default to --no-color here. Instead, we try to
override the GIT_COLOR_AUTO default because it would let want_color()
test whether stdout (instead of the specified file) is connected to a
terminal. Practically, we require the user to require color "always"
to force writing ANSI color sequences to the output file.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1

	Just something I noted while working on a bit more consistency
	with the diffopt.file patches.

	This is a backwards-incompatible change, though. So I extracted it
	from the patch series.

 diff.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/diff.c b/diff.c
index fa78fc1..b66b9be 100644
--- a/diff.c
+++ b/diff.c
@@ -3977,6 +3977,8 @@ int diff_opt_parse(struct diff_options *options,
 		if (!options->file)
 			die_errno("Could not open '%s'", path);
 		options->close_file = 1;
+		if (options->use_color != GIT_COLOR_ALWAYS)
+			options->use_color = GIT_COLOR_NEVER;
 		return argcount;
 	} else
 		return 0;
-- 
2.9.0.118.g0e1a633

base-commit: ab7797dbe95fff38d9265869ea367020046db118

Re: [PATCH] diff: let --output=<file> default to --no-color

[Junio C Hamano]

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

> It is highly unlikely that any user would want to see ANSI color
> sequences in a file. So let's stop doing that by default.
>
> This is a backwards-incompatible change.
>
> The reason this was not caught earlier is most likely that either
> --output=<file> is not used, or only when stdout is redirected
> anyway.
>
> Technically, we do not default to --no-color here. Instead, we try to
> override the GIT_COLOR_AUTO default because it would let want_color()
> test whether stdout (instead of the specified file) is connected to a
> terminal. Practically, we require the user to require color "always"
> to force writing ANSI color sequences to the output file.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1
>
> 	Just something I noted while working on a bit more consistency
> 	with the diffopt.file patches.
>
> 	This is a backwards-incompatible change, though. So I extracted it
> 	from the patch series.

I think this is a bugfix.  Even though we are writing to a file that
is separate from the standard output (because we explicitly opened
that file outselves), --color=auto would still let want_color() make
the decision based on isatty(1) with hardcoded 1.  That does not
simply make sense.

If we imagine that your format-patch series with 06/10 and 07/10
swapped, then the "do not freopen(), instead point opened file with
diffopt.file" step would be seen as introducing the same bug as the
bug this patch fixes, so in that sense it is an equivalent to 06/10.
We do not need to view this patch as a compatibility-breaking
change, I would think.

Perhaps I should tweak 06/10 to assign GIT_COLOR_NEVER not 0 while
queuing it.

Thanks.

>  diff.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index fa78fc1..b66b9be 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3977,6 +3977,8 @@ int diff_opt_parse(struct diff_options *options,
>  		if (!options->file)
>  			die_errno("Could not open '%s'", path);
>  		options->close_file = 1;
> +		if (options->use_color != GIT_COLOR_ALWAYS)
> +			options->use_color = GIT_COLOR_NEVER;
>  		return argcount;
>  	} else
>  		return 0;

Re: [PATCH] diff: let --output=<file> default to --no-color

[Johannes Schindelin]

Hi Junio,

On Fri, 24 Jun 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > It is highly unlikely that any user would want to see ANSI color
> > sequences in a file. So let's stop doing that by default.
> >
> > This is a backwards-incompatible change.
> >
> > The reason this was not caught earlier is most likely that either
> > --output=<file> is not used, or only when stdout is redirected
> > anyway.
> >
> > Technically, we do not default to --no-color here. Instead, we try to
> > override the GIT_COLOR_AUTO default because it would let want_color()
> > test whether stdout (instead of the specified file) is connected to a
> > terminal. Practically, we require the user to require color "always"
> > to force writing ANSI color sequences to the output file.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1
> >
> > 	Just something I noted while working on a bit more consistency
> > 	with the diffopt.file patches.
> >
> > 	This is a backwards-incompatible change, though. So I extracted it
> > 	from the patch series.
> 
> I think this is a bugfix.

Okay.

> Perhaps I should tweak 06/10 to assign GIT_COLOR_NEVER not 0 while
> queuing it.

Good point.

Thanks,
Dscho