{"thread":{"id":"42688","subject":"[PATCH] diff: let --output= default to --no-color","startedAt":"2016-06-22T14:41:41Z","lastAt":"2016-06-26T06:57:40Z","messageCount":3,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289843","messageId":"100cea0edf4f0986c000eb945a5e5955b8b59787.1466604435.git.johannes.schindelin@gmx.de","threadId":"42688","inReplyTo":null,"subject":"[PATCH] diff: let --output= default to --no-color","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-22T14:41:07Z","receivedAt":"2016-06-22T14:41:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"It is highly unlikely that any user would want to see ANSI color\nsequences in a file. So let's stop doing that by default.\n\nThis is a backwards-incompatible change.\n\nThe reason this was not caught earlier is most likely that either\n--output= is not used, or only when stdout is redirected\nanyway.\n\nTechnically, we do not default to --no-color here. Instead, we try to\noverride the GIT_COLOR_AUTO default because it would let want_color()\ntest whether stdout (instead of the specified file) is connected to a\nterminal. Practically, we require the user to require color \"always\"\nto force writing ANSI color sequences to the output file.\n\nSigned-off-by: Johannes Schindelin \n---\nPublished-As: https://github.com/dscho/git/releases/tag/diff-o-v1\n\n\tJust something I noted while working on a bit more consistency\n\twith the diffopt.file patches.\n\n\tThis is a backwards-incompatible change, though. So I extracted it\n\tfrom the patch series.\n\n diff.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex fa78fc1..b66b9be 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -3977,6 +3977,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (!options->file)\n \t\t\tdie_errno(\"Could not open '%s'\", path);\n \t\toptions->close_file = 1;\n+\t\tif (options->use_color != GIT_COLOR_ALWAYS)\n+\t\t\toptions->use_color = GIT_COLOR_NEVER;\n \t\treturn argcount;\n \t} else\n \t\treturn 0;\n-- \n2.9.0.118.g0e1a633\n\nbase-commit: ab7797dbe95fff38d9265869ea367020046db118\n"},{"id":"290071","messageId":"xmqqshw2dyks.fsf@gitster.mtv.corp.google.com","threadId":"42688","inReplyTo":"100cea0edf4f0986c000eb945a5e5955b8b59787.1466604435.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] diff: let --output= default to --no-color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-24T22:14:11Z","receivedAt":"2016-06-24T22:14:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> It is highly unlikely that any user would want to see ANSI color\n> sequences in a file. So let's stop doing that by default.\n>\n> This is a backwards-incompatible change.\n>\n> The reason this was not caught earlier is most likely that either\n> --output= is not used, or only when stdout is redirected\n> anyway.\n>\n> Technically, we do not default to --no-color here. Instead, we try to\n> override the GIT_COLOR_AUTO default because it would let want_color()\n> test whether stdout (instead of the specified file) is connected to a\n> terminal. Practically, we require the user to require color \"always\"\n> to force writing ANSI color sequences to the output file.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1\n>\n> \tJust something I noted while working on a bit more consistency\n> \twith the diffopt.file patches.\n>\n> \tThis is a backwards-incompatible change, though. So I extracted it\n> \tfrom the patch series.\n\nI think this is a bugfix. Even though we are writing to a file that\nis separate from the standard output (because we explicitly opened\nthat file outselves), --color=auto would still let want_color() make\nthe decision based on isatty(1) with hardcoded 1. That does not\nsimply make sense.\n\nIf we imagine that your format-patch series with 06/10 and 07/10\nswapped, then the \"do not freopen(), instead point opened file with\ndiffopt.file\" step would be seen as introducing the same bug as the\nbug this patch fixes, so in that sense it is an equivalent to 06/10.\nWe do not need to view this patch as a compatibility-breaking\nchange, I would think.\n\nPerhaps I should tweak 06/10 to assign GIT_COLOR_NEVER not 0 while\nqueuing it.\n\nThanks.\n\n> diff.c | 2 ++\n> 1 file changed, 2 insertions(+)\n>\n> diff --git a/diff.c b/diff.c\n> index fa78fc1..b66b9be 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -3977,6 +3977,8 @@ int diff_opt_parse(struct diff_options *options,\n> \t\tif (!options->file)\n> \t\t\tdie_errno(\"Could not open '%s'\", path);\n> \t\toptions->close_file = 1;\n> +\t\tif (options->use_color != GIT_COLOR_ALWAYS)\n> +\t\t\toptions->use_color = GIT_COLOR_NEVER;\n> \t\treturn argcount;\n> \t} else\n> \t\treturn 0;\n"},{"id":"290182","messageId":"alpine.DEB.2.20.1606260856480.12947@virtualbox","threadId":"42688","inReplyTo":"xmqqshw2dyks.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] diff: let --output= default to --no-color","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-06-26T06:57:32Z","receivedAt":"2016-06-26T06:57:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 24 Jun 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > It is highly unlikely that any user would want to see ANSI color\n> > sequences in a file. So let's stop doing that by default.\n> >\n> > This is a backwards-incompatible change.\n> >\n> > The reason this was not caught earlier is most likely that either\n> > --output= is not used, or only when stdout is redirected\n> > anyway.\n> >\n> > Technically, we do not default to --no-color here. Instead, we try to\n> > override the GIT_COLOR_AUTO default because it would let want_color()\n> > test whether stdout (instead of the specified file) is connected to a\n> > terminal. Practically, we require the user to require color \"always\"\n> > to force writing ANSI color sequences to the output file.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > Published-As: https://github.com/dscho/git/releases/tag/diff-o-v1\n> >\n> > \tJust something I noted while working on a bit more consistency\n> > \twith the diffopt.file patches.\n> >\n> > \tThis is a backwards-incompatible change, though. So I extracted it\n> > \tfrom the patch series.\n> \n> I think this is a bugfix.\n\nOkay.\n\n> Perhaps I should tweak 06/10 to assign GIT_COLOR_NEVER not 0 while\n> queuing it.\n\nGood point.\n\nThanks,\nDscho\n"}]}