# [PATCH] diff: let --output= default to --no-color 3 messages from 2016-06-22 to 2016-06-26. Participants: Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/42688 ## Johannes Schindelin, 2016-06-22 14:41 Subject: [PATCH] diff: let --output= default to --no-color Message-ID: <100cea0edf4f0986c000eb945a5e5955b8b59787.1466604435.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/100cea0edf4f0986c000eb945a5e5955b8b59787.1466604435.git.johannes.schindelin%40gmx.de ``` 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= 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 --- 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 ``` ## Junio C Hamano, 2016-06-24 22:14 Subject: Re: [PATCH] diff: let --output= default to --no-color Message-ID: URL: https://gitlist.dev/e/xmqqshw2dyks.fsf%40gitster.mtv.corp.google.com In-Reply-To: <100cea0edf4f0986c000eb945a5e5955b8b59787.1466604435.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin 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= 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 > --- > 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; ``` ## Johannes Schindelin, 2016-06-26 06:57 Subject: Re: [PATCH] diff: let --output= default to --no-color Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1606260856480.12947%40virtualbox In-Reply-To: ``` Hi Junio, On Fri, 24 Jun 2016, Junio C Hamano wrote: > Johannes Schindelin 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= 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 > > --- > > 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 ```