{"thread":{"id":"48608","subject":"[PATCH 0/4] color.ui docs & add color.ui=isatty","startedAt":"2018-05-30T21:07:03Z","lastAt":"2018-06-01T05:54:01Z","messageCount":18,"participants":["Ævar Arnfjörð Bjarmason","Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"348842","messageId":"20180530210641.19771-1-avarab@gmail.com","threadId":"48608","inReplyTo":null,"subject":"[PATCH 0/4] color.ui docs & add color.ui=isatty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T21:06:37Z","receivedAt":"2018-05-30T21:07:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"I started writing this for the reasons explained in 4/4, but the\njustification there is a bit of a stretch. Where we'd realistically\nlike to diverge color.ui=auto and color.pager=false, but since I wrote\nit already maybe some people will come out of the woodworks telling me\nthat this is what they've always wanted for whatever reason.\n\nMaybe if we don't like 4/4 we should take 3/4 because we might want\nanother such option in the future, but that's probably overly hedging\nour bets, still. I really don't like this pattern that we have a\nmulti-value config option that dies instead of warns on unknown future\nvalues, so I'm leaning towards saying that should be accepted to git.\n\nBut while I was working towards 4/4 I did some nice fixes in [12]/4. I\nthink those should go in regardless, so they're non-RFC.\n\nÆvar Arnfjörð Bjarmason (4):\n  config doc: move color.ui documentation to one place\n  config doc: clarify \"to a terminal\" in color.ui\n  color.ui config: don't die on unknown values\n  color.ui config: add \"isatty\" setting\n\n Documentation/config.txt | 100 +++++++++++++++++++++------------------\n color.c                  |  25 ++++++++--\n color.h                  |   1 +\n t/t7006-pager.sh         |  31 ++++++++++++\n 4 files changed, 107 insertions(+), 50 deletions(-)\n\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348843","messageId":"20180530210641.19771-3-avarab@gmail.com","threadId":"48608","inReplyTo":"20180530210641.19771-1-avarab@gmail.com","subject":"[PATCH 2/4] config doc: clarify \"to a terminal\" in color.ui","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T21:06:39Z","receivedAt":"2018-05-30T21:07:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the documentation added when color.ui=auto was made the default\nin 4c7f1819b3 (\"make color.ui default to 'auto'\", 2013-06-10) to\ndescribe \"auto\" as kicking in when writing to the terminal or a pager,\nnot just to the terminal.\n\nI had someone ask me why it was that git was writing colors with\ncolor.ui=auto in situations where isatty(3) would return 0. The\nexisting documentation about that would have been true before\n85fb65ed6e (\"\"git -p cmd\" to page anywhere\", 2006-06-06), but since\nthen \"auto\" has always used a heuristic that isn't quite what isatty()\nwould return, rather it checks if we're connected to a TTY or if we're\nabout to emit to a pager, trusting that the pager can handle color\noutput.\n\nInstead we have the color.pager variable introduced in\naa086eb813 (\"pager: config variable pager.color\", 2006-07-30) to\nexplicitly disable color output to the pager. Let's change the\ndocumentation to reflect this.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 44735dd88e..4767363519 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1288,7 +1288,9 @@ color.ui::\n \tor the `--color` option. Set it to `always` if you want all\n \toutput not intended for machine consumption to use color, to\n \t`true` or `auto` (this is the default since Git 1.8.4) if you\n-\twant such output to use color when written to the terminal.\n+\twant such output to use color when written to the terminal (as\n+\tdetermined by a call to `isatty(3)`) or to a pager (unless\n+\t`color.pager` is set to false).\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348844","messageId":"20180530210641.19771-4-avarab@gmail.com","threadId":"48608","inReplyTo":"20180530210641.19771-1-avarab@gmail.com","subject":"[RFC PATCH 3/4] color.ui config: don't die on unknown values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T21:06:40Z","receivedAt":"2018-05-30T21:07:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Before this change git will die on any unknown color.ui values:\n\n    $ git -c color.ui=doesnotexist show\n    fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit\n\nThis makes the failure mode of introducing any new values in the\nfuture really bad, as explained in the documentation being added\nhere. Instead let's warn and fall back to \"auto\".\n\nThe reason for the !warned++ pattern is when stepping through this in\nthe debugger I found that git_config_colorbool() is called more than\nonce on e.g. a \"show\" if color.ui=foo is set in the config, but\ncolor.ui=bar in the command-line, and would then warn about\nboth.\n\nMaybe we should warn about both in that case, but I don't know if\nthere's other cases where not doing this would cause a warning flood,\nand in any case the user is unlikely to have such a bad value in\nmultiple places, so this should be good enough.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config.txt |  5 +++++\n color.c                  | 13 +++++++++++++\n t/t7006-pager.sh         | 16 ++++++++++++++++\n 3 files changed, 34 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4767363519..b882a88214 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1291,6 +1291,11 @@ color.ui::\n \twant such output to use color when written to the terminal (as\n \tdetermined by a call to `isatty(3)`) or to a pager (unless\n \t`color.pager` is set to false).\n++\n+Setting this to some value unknown to git will warn and fall back to\n+`auto`. This is so that new values can be recognized in the future\n+without the git configuration file being incompatible between versions\n+to the point of most porcelain commands dying on the older version.\n \n column.ui::\n \tSpecify whether supported commands should output in columns.\ndiff --git a/color.c b/color.c\nindex b1c24c69de..e52c6cdd29 100644\n--- a/color.c\n+++ b/color.c\n@@ -311,6 +311,19 @@ int git_config_colorbool(const char *var, const char *value)\n \tif (!var)\n \t\treturn -1;\n \n+\t/*\n+\t * If future git versions introduce new color.ui settings we\n+\t * don't want to die right below when git_config_bool() fails\n+\t * to parse them as bool.\n+\t */\n+\tif (git_parse_maybe_bool(value) < 0) {\n+\t\tstatic int warned = 0;\n+\t\tif (!warned++)\n+\t\t\twarning(_(\"unknown value '%s' for '%s', falling back to 'auto'\"),\n+\t\t\t\tvalue, var);\n+\t\treturn GIT_COLOR_AUTO;\n+\t}\n+\n \t/* Missing or explicit false to turn off colorization */\n \tif (!git_config_bool(var, value))\n \t\treturn 0;\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex 7541ba5edb..b16f2ac28b 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -309,6 +309,14 @@ test_expect_success 'no color when stdout is a regular file' '\n \t! colorful colorless.log\n '\n \n+test_expect_success 'unknown color.ui values default to \"auto\" (regular file)' '\n+\trm -f colorless.log &&\n+\ttest_config color.ui doesnotexist &&\n+\tgit log >colorless.log 2>err &&\n+\ttest_i18ngrep \"falling back\" err &&\n+\t! colorful colorless.log\n+'\n+\n test_expect_success TTY 'color when writing to a pager' '\n \trm -f paginated.out &&\n \ttest_config color.ui auto &&\n@@ -316,6 +324,14 @@ test_expect_success TTY 'color when writing to a pager' '\n \tcolorful paginated.out\n '\n \n+test_expect_success TTY 'unknown color.ui values default to \"auto\" (pager)' '\n+\trm -f paginated.out &&\n+\ttest_config color.ui doesnotexist &&\n+\ttest_terminal git log 2>err &&\n+\ttest_i18ngrep \"falling back\" err &&\n+\tcolorful paginated.out\n+'\n+\n test_expect_success TTY 'colors are suppressed by color.pager' '\n \trm -f paginated.out &&\n \ttest_config color.ui auto &&\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348845","messageId":"20180530210641.19771-2-avarab@gmail.com","threadId":"48608","inReplyTo":"20180530210641.19771-1-avarab@gmail.com","subject":"[PATCH 1/4] config doc: move color.ui documentation to one place","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T21:06:38Z","receivedAt":"2018-05-30T21:07:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Ever since b982592d66 (\"git-status: document colorization config\noptions\", 2006-09-11) we've slowly been accumulating more and more\ncolor.* options, where the documentation for each new one has\nseemingly been copy/pasted with minor adjustments from the last.\n\nThis has resulted in documentation where we're describing what sort of\nvalues color.ui or its overriding variables can take a grand total of\n9 times.\n\nThis makes for hard and tedious reading, and is going to be a royal\npain if we're ever going to add more color.ui values.\n\nInstead let's briefly describe what each variable is for, and then\ncopy/paste a new boilerplate saying that this variable takes the exact\nsame values as color.ui, and if it's unset it'll fallback to whatever\ncolor.ui is set to.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config.txt | 81 ++++++++++++++++++----------------------\n 1 file changed, 36 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 7d8383433c..44735dd88e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1103,21 +1103,19 @@ clean.requireForce::\n \t-i or -n.   Defaults to true.\n \n color.advice::\n-\tA boolean to enable/disable color in hints (e.g. when a push\n-\tfailed, see `advice.*` for a list).  May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors\n-\tare used only when the error output goes to a terminal. If\n-\tunset, then the value of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in hints (e.g. when a push failed,\n+\tsee `advice.*` for a list). See `color.ui` for possible values\n+\tand the default. If unset, the value of `color.ui` is used as\n+\ta fallback.\n \n color.advice.hint::\n \tUse customized color for hints.\n \n color.branch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-branch[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in the output of\n+\tlinkgit:git-branch[1]. See `color.ui` for possible values and\n+\tthe default. If unset, the value of `color.ui` is used as a\n+\tfallback.\n \n color.branch.<slot>::\n \tUse customized color for branch coloration. `<slot>` is one of\n@@ -1127,13 +1125,11 @@ color.branch.<slot>::\n \trefs).\n \n color.diff::\n-\tWhether to use ANSI escape sequences to add color to patches.\n-\tIf this is set to `always`, linkgit:git-diff[1],\n-\tlinkgit:git-log[1], and linkgit:git-show[1] will use color\n-\tfor all patches.  If it is set to `true` or `auto`, those\n-\tcommands will only use color when output is to the terminal.\n-\tIf unset, then the value of `color.ui` is used (`auto` by\n-\tdefault).\n+\tEnables or disables colors when patches are emitted\n+\t(e.g. linkgit:git-diff[1], linkgit:git-log[1], and\n+\tlinkgit:git-show[1]). See `color.ui` for possible values and\n+\tthe default. If unset, the value of `color.ui` is used as a\n+\tfallback.\n +\n This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the\n@@ -1165,10 +1161,10 @@ color.decorate.<slot>::\n \tbranches, remote-tracking branches, tags, stash and HEAD, respectively.\n \n color.grep::\n-\tWhen set to `always`, always highlight matches.  When `false` (or\n-\t`never`), never.  When set to `true` or `auto`, use color only\n-\twhen the output is written to the terminal.  If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in the output of\n+\tlinkgit:git-grep[1]. See `color.ui` for possible values and\n+\tthe default. If unset, the value of `color.ui` is used as a\n+\tfallback.\n \n color.grep.<slot>::\n \tUse customized color for grep colorization.  `<slot>` specifies which\n@@ -1197,12 +1193,11 @@ color.grep.<slot>::\n --\n \n color.interactive::\n-\tWhen set to `always`, always use colors for interactive prompts\n-\tand displays (such as those used by \"git-add --interactive\" and\n-\t\"git-clean --interactive\"). When false (or `never`), never.\n-\tWhen set to `true` or `auto`, use colors only when the output is\n-\tto the terminal. If unset, then the value of `color.ui` is\n-\tused (`auto` by default).\n+\tEnables or disables colors in interactive prompts and displays\n+\t(such as those used by \"git-add --interactive\" and \"git-clean\n+\t--interactive\"). See `color.ui` for possible values and the\n+\tdefault. If unset, the value of `color.ui` is used as a\n+\tfallback.\n \n color.interactive.<slot>::\n \tUse customized color for 'git add --interactive' and 'git clean\n@@ -1215,27 +1210,24 @@ color.pager::\n \tuse (default is true).\n \n color.push::\n-\tA boolean to enable/disable color in push errors. May be set to\n-\t`always`, `false` (or `never`) or `auto` (or `true`), in which\n-\tcase colors are used only when the error output goes to a terminal.\n-\tIf unset, then the value of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in push errors. See `color.ui` for\n+\tpossible values and the default. If unset, the value of\n+\t`color.ui` is used as a fallback.\n \n color.push.error::\n \tUse customized color for push errors.\n \n color.showBranch::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-show-branch[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in the output of\n+\tlinkgit:git-show-branch[1]. See `color.ui` for possible values\n+\tand the default. If unset, the value of `color.ui` is used as\n+\ta fallback.\n \n color.status::\n-\tA boolean to enable/disable color in the output of\n-\tlinkgit:git-status[1]. May be set to `always`,\n-\t`false` (or `never`) or `auto` (or `true`), in which case colors are used\n-\tonly when the output is to a terminal. If unset, then the\n-\tvalue of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors in the output of\n+\tlinkgit:git-status[1]. See `color.ui` for possible values\n+\tand the default. If unset, the value of `color.ui` is used as\n+\ta fallback.\n \n color.status.<slot>::\n \tUse customized color for status colorization. `<slot>` is\n@@ -1279,10 +1271,9 @@ blame.coloring::\n \tor 'none' which is the default.\n \n color.transport::\n-\tA boolean to enable/disable color when pushes are rejected. May be\n-\tset to `always`, `false` (or `never`) or `auto` (or `true`), in which\n-\tcase colors are used only when the error output goes to a terminal.\n-\tIf unset, then the value of `color.ui` is used (`auto` by default).\n+\tEnables or disables colors when pushes are rejected. See\n+\t`color.ui` for possible values and the default. If unset, the\n+\tvalue of `color.ui` is used as a fallback.\n \n color.transport.rejected::\n \tUse customized color when a push was rejected.\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348846","messageId":"20180530210641.19771-5-avarab@gmail.com","threadId":"48608","inReplyTo":"20180530210641.19771-1-avarab@gmail.com","subject":"[RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-30T21:06:41Z","receivedAt":"2018-05-30T21:07:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A co-worker of mine who was using UNIX systems when dinosaurs roamed\nthe earth was lamenting that kids these days were using tools like\n\"git\" that thought they knew better than isatty(3) when deciding\nwhether or not something was a terminal, and the state of the\ndocumentation fixed earlier in this series certainly didn't help.\n\nSo this setting is a small gift to all the UNIX graybeards out\nthere. Now they can set color.ui=isatty and only emit fancy colors in\nsituations when the gods of old intended, not whatever heuristic we've\ndecided to set \"auto\" to.\n\nAs noted here this is *currently* the same as setting color.ui=auto &\ncolor.pager=false, but I think it's good to explicitly have this\nsetting for any future changes. The reason, as now noted in the\ndocumentation is that the \"auto\" setting may become even smarter in\nthe future and learn even deeper heuristics for when to turn itself on\neven if isatty(3) were returning true.\n\nAt that point the fans of plain isatty(3) will become even more upset\nat what we're doing, so let's give them a simple future-proof opt-out.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n Documentation/config.txt | 10 ++++++++++\n color.c                  | 12 ++++++++----\n color.h                  |  1 +\n t/t7006-pager.sh         | 15 +++++++++++++++\n 4 files changed, 34 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex b882a88214..183569786f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1292,6 +1292,16 @@ color.ui::\n \tdetermined by a call to `isatty(3)`) or to a pager (unless\n \t`color.pager` is set to false).\n +\n+If you don't like the magic of `auto` and prefer for git to just ask\n+`isatty(3)` whether it's connected to a terminal set this to\n+`isatty`. This will cause it to always obey that function, except\n+(like `auto`) if `TERM=dumb` is set in the environment. Currently this\n+is equivalent to setting `auto` and `color.pager=false`, but in the\n+future `auto` may be smart enough to handle other cases, i.e. when\n+`isatty(3)` returns `1` but something else other than `TERM=dumb`\n+suggests the terminal can't handle colors or not. If you'd like to\n+avoid all that magic use `isatty`.\n++\n Setting this to some value unknown to git will warn and fall back to\n `auto`. This is so that new values can be recognized in the future\n without the git configuration file being incompatible between versions\ndiff --git a/color.c b/color.c\nindex e52c6cdd29..05f95649bc 100644\n--- a/color.c\n+++ b/color.c\n@@ -306,6 +306,8 @@ int git_config_colorbool(const char *var, const char *value)\n \t\t\treturn 1;\n \t\tif (!strcasecmp(value, \"auto\"))\n \t\t\treturn GIT_COLOR_AUTO;\n+\t\tif (!strcasecmp(value, \"isatty\"))\n+\t\t\treturn GIT_COLOR_ISATTY;\n \t}\n \n \tif (!var)\n@@ -332,13 +334,14 @@ int git_config_colorbool(const char *var, const char *value)\n \treturn GIT_COLOR_AUTO;\n }\n \n-static int check_auto_color(int fd)\n+static int check_auto_color(int fd, int isatty_only)\n {\n \tstatic int color_stderr_is_tty = -1;\n \tint *is_tty_p = fd == 1 ? &color_stdout_is_tty : &color_stderr_is_tty;\n \tif (*is_tty_p < 0)\n \t\t*is_tty_p = isatty(fd);\n-\tif (*is_tty_p || (fd == 1 && pager_in_use() && pager_use_color)) {\n+\tif (*is_tty_p || (!isatty_only && fd == 1 && pager_in_use() &&\n+\t\t\t  pager_use_color)) {\n \t\tif (!is_terminal_dumb())\n \t\t\treturn 1;\n \t}\n@@ -359,9 +362,10 @@ int want_color_fd(int fd, int var)\n \tif (var < 0)\n \t\tvar = git_use_color_default;\n \n-\tif (var == GIT_COLOR_AUTO) {\n+\tif (var == GIT_COLOR_AUTO || var == GIT_COLOR_ISATTY) {\n+\t\tint isatty_only = var == GIT_COLOR_ISATTY;\n \t\tif (want_auto[fd] < 0)\n-\t\t\twant_auto[fd] = check_auto_color(fd);\n+\t\t\twant_auto[fd] = check_auto_color(fd, isatty_only);\n \t\treturn want_auto[fd];\n \t}\n \treturn var;\ndiff --git a/color.h b/color.h\nindex 5b744e1bc6..01d8cc01a5 100644\n--- a/color.h\n+++ b/color.h\n@@ -57,6 +57,7 @@ struct strbuf;\n #define GIT_COLOR_NEVER  0\n #define GIT_COLOR_ALWAYS 1\n #define GIT_COLOR_AUTO   2\n+#define GIT_COLOR_ISATTY 3\n \n /* A default list of colors to use for commit graphs and show-branch output */\n extern const char *column_colors_ansi[];\ndiff --git a/t/t7006-pager.sh b/t/t7006-pager.sh\nindex b16f2ac28b..ea4f2f47d0 100755\n--- a/t/t7006-pager.sh\n+++ b/t/t7006-pager.sh\n@@ -309,6 +309,13 @@ test_expect_success 'no color when stdout is a regular file' '\n \t! colorful colorless.log\n '\n \n+test_expect_success 'no color when stdout is a regular file (isatty)' '\n+\trm -f colorless.log &&\n+\ttest_config color.ui isatty &&\n+\tgit log >colorless.log &&\n+\t! colorful colorless.log\n+'\n+\n test_expect_success 'unknown color.ui values default to \"auto\" (regular file)' '\n \trm -f colorless.log &&\n \ttest_config color.ui doesnotexist &&\n@@ -340,6 +347,14 @@ test_expect_success TTY 'colors are suppressed by color.pager' '\n \t! colorful paginated.out\n '\n \n+test_expect_success TTY 'colors are suppressed by color.ui=isatty when writing to a pager' '\n+\trm -f paginated.out &&\n+\ttest_config color.ui isatty &&\n+\ttest_config color.pager true &&\n+\ttest_terminal git log &&\n+\t! colorful paginated.out\n+'\n+\n test_expect_success 'color when writing to a file intended for a pager' '\n \trm -f colorful.log &&\n \ttest_config color.ui auto &&\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348851","messageId":"CAGZ79kZVq1szZ=nPQ87nv1Hrg+yCrQbbkzfuNg1itfEGrBR03w@mail.gmail.com","threadId":"48608","inReplyTo":"20180530210641.19771-4-avarab@gmail.com","subject":"Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-30T22:32:54Z","receivedAt":"2018-05-30T22:32:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 30, 2018 at 2:06 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> Before this change git will die on any unknown color.ui values:\n>\n>     $ git -c color.ui=doesnotexist show\n>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit\n>\n> This makes the failure mode of introducing any new values in the\n> future really bad, as explained in the documentation being added\n> here. Instead let's warn and fall back to \"auto\".\n\nWe had a similar regression in the protocol.version setting in 2.16 IIRC.\nIt makes sense to fix it this way.\n\n> The reason for the !warned++ pattern is when stepping through this in\n> the debugger I found that git_config_colorbool() is called more than\n> once on e.g. a \"show\" if color.ui=foo is set in the config, but\n> color.ui=bar in the command-line, and would then warn about\n> both.\n>\n> Maybe we should warn about both in that case, but I don't know if\n> there's other cases where not doing this would cause a warning flood,\n> and in any case the user is unlikely to have such a bad value in\n> multiple places, so this should be good enough.\n\nagreed.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  Documentation/config.txt |  5 +++++\n>  color.c                  | 13 +++++++++++++\n>  t/t7006-pager.sh         | 16 ++++++++++++++++\n>  3 files changed, 34 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 4767363519..b882a88214 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1291,6 +1291,11 @@ color.ui::\n>         want such output to use color when written to the terminal (as\n>         determined by a call to `isatty(3)`) or to a pager (unless\n>         `color.pager` is set to false).\n> ++\n> +Setting this to some value unknown to git will warn and fall back to\n> +`auto`. This is so that new values can be recognized in the future\n> +without the git configuration file being incompatible between versions\n> +to the point of most porcelain commands dying on the older version.\n>\n>  column.ui::\n>         Specify whether supported commands should output in columns.\n> diff --git a/color.c b/color.c\n> index b1c24c69de..e52c6cdd29 100644\n> --- a/color.c\n> +++ b/color.c\n> @@ -311,6 +311,19 @@ int git_config_colorbool(const char *var, const char *value)\n>         if (!var)\n>                 return -1;\n>\n> +       /*\n> +        * If future git versions introduce new color.ui settings we\n> +        * don't want to die right below when git_config_bool() fails\n> +        * to parse them as bool.\n> +        */\n\nI am not sure we need to document this here. Ignoring unknown\n(or warning about unknown) values is the standard for config keys\nat least, for values we probably want a similar thing.\n\n> +       if (git_parse_maybe_bool(value) < 0) {\n> +               static int warned = 0;\n\nAs someone who moved a lot of global state into the repository object\nthis is a fine example where we do not want to have the counter not\nin the repository? (although strictly speaking you'd want to have the\nwarning approximately once per repository that is configured wrongly\nor do you want to have this warning once at all?)\n\nI think we should take patches 1-3 at least.\n\nThanks,\nStefan\n"},{"id":"348853","messageId":"xmqqmuwgda5k.fsf@gitster-ct.c.googlers.com","threadId":"48608","inReplyTo":"20180530210641.19771-5-avarab@gmail.com","subject":"Re: [RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-30T22:57:27Z","receivedAt":"2018-05-30T22:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> A co-worker of mine who was using UNIX systems when dinosaurs roamed\n> the earth was lamenting that kids these days were using tools like\n> \"git\" that thought they knew better than isatty(3) when deciding\n> whether or not something was a terminal, and the state of the\n> documentation fixed earlier in this series certainly didn't help.\n>\n> So this setting is a small gift to all the UNIX graybeards out\n> there. Now they can set color.ui=isatty and only emit fancy colors in\n> situations when the gods of old intended, not whatever heuristic we've\n> decided to set \"auto\" to.\n\nRe-read the above again, and notice that you are *only* hinting that\nyou consider difference between \"auto\" and \"isatty\" is important,\nand that your \"isatty\" is better, without telling what the\ndifference is, let alone why you think \"isatty\" is better.\n\n>\n> As noted here this is *currently* the same as setting color.ui=auto &\n> color.pager=false, but I think it's good to explicitly have this\n> setting for any future changes. The reason, as now noted in the\n> documentation is that the \"auto\" setting may become even smarter in\n> the future and learn even deeper heuristics for when to turn itself on\n> even if isatty(3) were returning true.\n\nDo you mean s/true/false/ in the last part?\n\n> At that point the fans of plain isatty(3) will become even more upset\n> at what we're doing, so let's give them a simple future-proof opt-out.\n\nYou still haven't explained why \"auto\" that does more than \"isatty\"\nis and will be irritating.\n\nThat's not a good way to sell a patch.\n\nAlso even \"isatty\" still needs to do more than isatty(1) call.  The\nprocess that is trying to do color.ui=isatty may be talking to an\noutgoing pipe due to the use of \"git -p cmd\", by that time, it is\ntoo late to call isatty(1) and obtain the info the caller wishes to.\n"},{"id":"348854","messageId":"xmqqin74d9rh.fsf@gitster-ct.c.googlers.com","threadId":"48608","inReplyTo":"20180530210641.19771-4-avarab@gmail.com","subject":"Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-30T23:05:54Z","receivedAt":"2018-05-30T23:06:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Before this change git will die on any unknown color.ui values:\n>\n>     $ git -c color.ui=doesnotexist show\n>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit\n\nI do not think \"unit\" is correct, so there may be some room for\nimprovement.  For _this_ particular case, I agree that it is not the\nend of the world if we did not color the output (because we do not\nknow what the 'doesnotyetexist' token from the future is trying to\ntell us), but as a general principle, we should diagnose and die, if\na misconfiguration is easy to correct, than blindly go ahead and do\nrandom things that the end-user did not expect by giving something\nwe do not (but they thought they do) understand.\n\nIf we really want to introduce \"here is a setting you may not\nunderstand, in which case you may safely ignore\", the right way to\ndo so is to follow the model the index extension took, where from\nthe syntax of the unknown thing an old/existing code can tell if it\nis optional.  Forcing all codepaths to forever ignore what they do\nnot understand and what they happen to think is a safe fallback is\nsimply being irresponsible---the existing code does not understand\nthe new setting so they do not even know if their \"current\nbehaviour\" as a fallback is a safe and sensible one from the point\nof view of the person who asked for the feature from the future.\n"},{"id":"348865","messageId":"20180531052538.GB17068@sigill.intra.peff.net","threadId":"48608","inReplyTo":"20180530210641.19771-2-avarab@gmail.com","subject":"Re: [PATCH 1/4] config doc: move color.ui documentation to one place","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T05:25:38Z","receivedAt":"2018-05-31T05:25:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 30, 2018 at 09:06:38PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Ever since b982592d66 (\"git-status: document colorization config\n> options\", 2006-09-11) we've slowly been accumulating more and more\n> color.* options, where the documentation for each new one has\n> seemingly been copy/pasted with minor adjustments from the last.\n> \n> This has resulted in documentation where we're describing what sort of\n> values color.ui or its overriding variables can take a grand total of\n> 9 times.\n\nYeah, I agree the current state is poor.\n\n> This makes for hard and tedious reading, and is going to be a royal\n> pain if we're ever going to add more color.ui values.\n> \n> Instead let's briefly describe what each variable is for, and then\n> copy/paste a new boilerplate saying that this variable takes the exact\n> same values as color.ui, and if it's unset it'll fallback to whatever\n> color.ui is set to.\n\nDefinitely an improvement. Though I wonder if we should go further and\nshow the user the relationship in the documentation explicitly. Like:\n\n  color.<system>::\n\tA boolean to enable/disable color in a particular part of Git,\n\toverriding `color.ui` (see `color.ui` for possible values). The\n\tcurrent set of systems is:\n\n\tadvice::\n\t\tHints shown with the \"hint:\" prefix, controlled by\n\t\t`advice.*` config.\n\n\tbranch::\n\t\tOutput from the linkgit:git-branch[1] command.\n\n\t...etc...\n\nWe could possibly do the same with color.<system>.<slot>. Or maybe even\nmake a single hierarchical list of systems, and then the color slots\nunder each. I think if our mental model in adding these options is\nto have this kind of hierarchy, then it makes sense to communicate it\nexplicitly to the user and get them using the same mental model.\n\n-Peff\n"},{"id":"348866","messageId":"20180531052711.GC17068@sigill.intra.peff.net","threadId":"48608","inReplyTo":"20180530210641.19771-3-avarab@gmail.com","subject":"Re: [PATCH 2/4] config doc: clarify \"to a terminal\" in color.ui","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T05:27:11Z","receivedAt":"2018-05-31T05:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 30, 2018 at 09:06:39PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> Change the documentation added when color.ui=auto was made the default\n> in 4c7f1819b3 (\"make color.ui default to 'auto'\", 2013-06-10) to\n> describe \"auto\" as kicking in when writing to the terminal or a pager,\n> not just to the terminal.\n\nMakes sense. Usually the pager only kicks in when we're going to a\nterminal in the first place, so the two would be the same thing. But\nobviously you can override that with \"-p\".\n\n-Peff\n"},{"id":"348867","messageId":"20180531053810.GD17068@sigill.intra.peff.net","threadId":"48608","inReplyTo":"20180530210641.19771-5-avarab@gmail.com","subject":"Re: [RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-31T05:38:10Z","receivedAt":"2018-05-31T05:38:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 30, 2018 at 09:06:41PM +0000, Ævar Arnfjörð Bjarmason wrote:\n\n> A co-worker of mine who was using UNIX systems when dinosaurs roamed\n> the earth was lamenting that kids these days were using tools like\n> \"git\" that thought they knew better than isatty(3) when deciding\n> whether or not something was a terminal, and the state of the\n> documentation fixed earlier in this series certainly didn't help.\n> \n> So this setting is a small gift to all the UNIX graybeards out\n> there. Now they can set color.ui=isatty and only emit fancy colors in\n> situations when the gods of old intended, not whatever heuristic we've\n> decided to set \"auto\" to.\n\nI'm having trouble thinking of a case where anybody would actually\ncare about the distinction between these two.\n\nIf you're not using \"-p\", then colors will kick in if isatty() is true.\nThe whole \"also check the pager\" thing is only because we may check the\ncolor isatty() after starting the pager. If we didn't, then nobody would\never see color. If your pager doesn't understand color, then fine. Tell\nGit not to send it color with color.pager=false.\n\nIf you are using \"-p\", we might send color to your pager even though you\nweren't originally going to a terminal. But either your pager can handle\ncolors, in which case this is fine, or it cannot, in which case you\nwould already have needed to set color.pager as above to un-break all of\nthe non-p cases.\n\nIs there some case where a pager can only handle color if _it's_ output\nis going to a tty, and otherwise not? And you'd want color predicated on\nwhether the original output was a tty or not, even if you said \"-p\"?\nThat's the only case I can think of where the existing config is not\nsufficient, but I'm having a hard time envisioning a practical use.\n\n> As noted here this is *currently* the same as setting color.ui=auto &\n> color.pager=false, but I think it's good to explicitly have this\n> setting for any future changes. The reason, as now noted in the\n> documentation is that the \"auto\" setting may become even smarter in\n> the future and learn even deeper heuristics for when to turn itself on\n> even if isatty(3) were returning true.\n\nAre we actually thinking about teaching it deeper heuristics? I think\nthe fact that it _is_ based on isatty() is meant to be understood by the\nuser, and we wouldn't want to change that. True, there is the pager\nexception, but the point of that is to maintain the \"are we going to a\ntty\" model, even in the face of sticking a pager in the middle.\n\n-Peff\n"},{"id":"348876","messageId":"874liofgv6.fsf@evledraar.gmail.com","threadId":"48608","inReplyTo":"20180531053810.GD17068@sigill.intra.peff.net","subject":"Re: [RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:01:49Z","receivedAt":"2018-05-31T07:05:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 31 2018, Jeff King wrote:\n\n> On Wed, May 30, 2018 at 09:06:41PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> A co-worker of mine who was using UNIX systems when dinosaurs roamed\n>> the earth was lamenting that kids these days were using tools like\n>> \"git\" that thought they knew better than isatty(3) when deciding\n>> whether or not something was a terminal, and the state of the\n>> documentation fixed earlier in this series certainly didn't help.\n>>\n>> So this setting is a small gift to all the UNIX graybeards out\n>> there. Now they can set color.ui=isatty and only emit fancy colors in\n>> situations when the gods of old intended, not whatever heuristic we've\n>> decided to set \"auto\" to.\n>\n> I'm having trouble thinking of a case where anybody would actually\n> care about the distinction between these two.\n>\n> If you're not using \"-p\", then colors will kick in if isatty() is true.\n> The whole \"also check the pager\" thing is only because we may check the\n> color isatty() after starting the pager. If we didn't, then nobody would\n> ever see color. If your pager doesn't understand color, then fine. Tell\n> Git not to send it color with color.pager=false.\n>\n> If you are using \"-p\", we might send color to your pager even though you\n> weren't originally going to a terminal. But either your pager can handle\n> colors, in which case this is fine, or it cannot, in which case you\n> would already have needed to set color.pager as above to un-break all of\n> the non-p cases.\n>\n> Is there some case where a pager can only handle color if _it's_ output\n> is going to a tty, and otherwise not?\n\nMaybe I'm missing something but how is that something we can deal with?\nWe just:\n\n 1. use isatty() to see if we're attached to a terminal\n 2a. If yes and no pager is invoked (e.g. git status) and \"auto\" we use colors\n 2b. If yes and we're invoking a pager (e.g. git log) and \"auto\" we use colors\n     3b. At this point we're writing to the pager so isatty() is false,\n         but we set our own GIT_PAGER_IN_USE and turn on colors if \"auto\"\n\nI suppose we can imagine a pager that sometimes emits to a terminal and\nsometimes e.g. opens a browser with the output, and we could ourselves\nsomehow detect this...\n\n> And you'd want color predicated on\n> whether the original output was a tty or not, even if you said \"-p\"?\n> That's the only case I can think of where the existing config is not\n> sufficient, but I'm having a hard time envisioning a practical use.\n>\n>> As noted here this is *currently* the same as setting color.ui=auto &\n>> color.pager=false, but I think it's good to explicitly have this\n>> setting for any future changes. The reason, as now noted in the\n>> documentation is that the \"auto\" setting may become even smarter in\n>> the future and learn even deeper heuristics for when to turn itself on\n>> even if isatty(3) were returning true.\n>\n> Are we actually thinking about teaching it deeper heuristics? I think\n> the fact that it _is_ based on isatty() is meant to be understood by the\n> user, and we wouldn't want to change that. True, there is the pager\n> exception, but the point of that is to maintain the \"are we going to a\n> tty\" model, even in the face of sticking a pager in the middle.\n\nAs noted in the cover letter I started writing this whole thing before\nunderstanding some of the subtleties, and now I think this \"isatty\"\nthing is probably pretty useless, and was wondering if others wanted it.\n\nReasons to take it are:\n\n 1) To make user intent clearer. I.e. we could just also make it a\n    synonym for color.ui=auto & color.pager=false and those used to\n    isatty semantics skimming the docs would more easily find the right\n    thing.\n\n 2) If there are any cases where isatty() is true, but we can detect via\n    other means (e.g. inspecting other env variables) that non-pager\n    output can't handle colors some of the time. Of course if we find\n    such cases isatty() would suck more, but that's presumably what\n    isatty() purists want :)\n\n"},{"id":"348877","messageId":"87k1rk9uc4.fsf@evledraar.gmail.com","threadId":"48608","inReplyTo":"xmqqmuwgda5k.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:07:23Z","receivedAt":"2018-05-31T07:08:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 30 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> A co-worker of mine who was using UNIX systems when dinosaurs roamed\n>> the earth was lamenting that kids these days were using tools like\n>> \"git\" that thought they knew better than isatty(3) when deciding\n>> whether or not something was a terminal, and the state of the\n>> documentation fixed earlier in this series certainly didn't help.\n>>\n>> So this setting is a small gift to all the UNIX graybeards out\n>> there. Now they can set color.ui=isatty and only emit fancy colors in\n>> situations when the gods of old intended, not whatever heuristic we've\n>> decided to set \"auto\" to.\n>\n> Re-read the above again, and notice that you are *only* hinting that\n> you consider difference between \"auto\" and \"isatty\" is important,\n> and that your \"isatty\" is better, without telling what the\n> difference is, let alone why you think \"isatty\" is better.\n>\n>>\n>> As noted here this is *currently* the same as setting color.ui=auto &\n>> color.pager=false, but I think it's good to explicitly have this\n>> setting for any future changes. The reason, as now noted in the\n>> documentation is that the \"auto\" setting may become even smarter in\n>> the future and learn even deeper heuristics for when to turn itself on\n>> even if isatty(3) were returning true.\n>\n> Do you mean s/true/false/ in the last part?\n\nNo \"true\" as noted in\nhttps://public-inbox.org/git/874liofgv6.fsf@evledraar.gmail.com/\n\n>> At that point the fans of plain isatty(3) will become even more upset\n>> at what we're doing, so let's give them a simple future-proof opt-out.\n>\n> You still haven't explained why \"auto\" that does more than \"isatty\"\n> is and will be irritating.\n>\n> That's not a good way to sell a patch.\n\nI'm not really trying to sell this thing, as noted in the CL. This is\nmore of a \"I wrote this, does anyone find this useful?\".\n\n> Also even \"isatty\" still needs to do more than isatty(1) call.  The\n> process that is trying to do color.ui=isatty may be talking to an\n> outgoing pipe due to the use of \"git -p cmd\", by that time, it is\n> too late to call isatty(1) and obtain the info the caller wishes to.\n"},{"id":"348878","messageId":"87in749u7t.fsf@evledraar.gmail.com","threadId":"48608","inReplyTo":"20180531052538.GB17068@sigill.intra.peff.net","subject":"Re: [PATCH 1/4] config doc: move color.ui documentation to one place","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:09:58Z","receivedAt":"2018-05-31T07:10:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 31 2018, Jeff King wrote:\n\n> On Wed, May 30, 2018 at 09:06:38PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Ever since b982592d66 (\"git-status: document colorization config\n>> options\", 2006-09-11) we've slowly been accumulating more and more\n>> color.* options, where the documentation for each new one has\n>> seemingly been copy/pasted with minor adjustments from the last.\n>>\n>> This has resulted in documentation where we're describing what sort of\n>> values color.ui or its overriding variables can take a grand total of\n>> 9 times.\n>\n> Yeah, I agree the current state is poor.\n>\n>> This makes for hard and tedious reading, and is going to be a royal\n>> pain if we're ever going to add more color.ui values.\n>>\n>> Instead let's briefly describe what each variable is for, and then\n>> copy/paste a new boilerplate saying that this variable takes the exact\n>> same values as color.ui, and if it's unset it'll fallback to whatever\n>> color.ui is set to.\n>\n> Definitely an improvement. Though I wonder if we should go further and\n> show the user the relationship in the documentation explicitly. Like:\n>\n>   color.<system>::\n> \tA boolean to enable/disable color in a particular part of Git,\n> \toverriding `color.ui` (see `color.ui` for possible values). The\n> \tcurrent set of systems is:\n>\n> \tadvice::\n> \t\tHints shown with the \"hint:\" prefix, controlled by\n> \t\t`advice.*` config.\n>\n> \tbranch::\n> \t\tOutput from the linkgit:git-branch[1] command.\n>\n> \t...etc...\n>\n> We could possibly do the same with color.<system>.<slot>. Or maybe even\n> make a single hierarchical list of systems, and then the color slots\n> under each. I think if our mental model in adding these options is\n> to have this kind of hierarchy, then it makes sense to communicate it\n> explicitly to the user and get them using the same mental model.\n\nI wouldn't be opposed to some twist on that, but I really dislike the\nvariables that are documented in such a way that you can't find them in\nthe documentation by searching for their fully qualified name.\n"},{"id":"348879","messageId":"87h8mo9tuy.fsf@evledraar.gmail.com","threadId":"48608","inReplyTo":"xmqqin74d9rh.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:17:41Z","receivedAt":"2018-05-31T07:17:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 30 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> Before this change git will die on any unknown color.ui values:\n>>\n>>     $ git -c color.ui=doesnotexist show\n>>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit\n>\n> I do not think \"unit\" is correct, so there may be some room for\n> improvement.  For _this_ particular case, I agree that it is not the\n> end of the world if we did not color the output (because we do not\n> know what the 'doesnotyetexist' token from the future is trying to\n> tell us), but as a general principle, we should diagnose and die, if\n> a misconfiguration is easy to correct.\n\nMany users (including myself) use the same ~/.gitconfig on many\ndifferent machines with different git versions. Maybe at some point I'm\nwilling to set the new setting to a value I know is supported on most of\nthem, but it sucks at that point if I logging into 1-3% of old machines\nends up killing git on any invocation.\n\n> than blindly go ahead and do random things that the end-user did not\n> expect by giving something we do not (but they thought they do)\n> understand.\n\nI think this is highly dependent on what variables we give this\ntreatment. There may be some where we genuinely have no idea what they\nmean, but in this case and for http.sslVersion (which warns, doesn't die\non unknown values) it's reasonable to assume that degrading to a known\nvalue is better than outright dying.\n\n> If we really want to introduce \"here is a setting you may not\n> understand, in which case you may safely ignore\", the right way to\n> do so is to follow the model the index extension took, where from\n> the syntax of the unknown thing an old/existing code can tell if it\n> is optional.  Forcing all codepaths to forever ignore what they do\n> not understand and what they happen to think is a safe fallback is\n> simply being irresponsible---the existing code does not understand\n> the new setting so they do not even know if their \"current\n> behaviour\" as a fallback is a safe and sensible one from the point\n> of view of the person who asked for the feature from the future.\n\nThis seems needlessly complex. color.ui is one of the most prominent\nconfig variales, so you're proposing we split it up into some dual-key\narrangement and force all users to migrate? I think just following what\nwe're doing with http.sslVersion makes more sense.\n"},{"id":"348973","messageId":"20180601053044.GA8292@sigill.intra.peff.net","threadId":"48608","inReplyTo":"874liofgv6.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 4/4] color.ui config: add \"isatty\" setting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-01T05:30:44Z","receivedAt":"2018-06-01T05:30:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 31, 2018 at 09:01:49AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Is there some case where a pager can only handle color if _it's_ output\n> > is going to a tty, and otherwise not?\n> \n> Maybe I'm missing something but how is that something we can deal with?\n> We just:\n> \n>  1. use isatty() to see if we're attached to a terminal\n>  2a. If yes and no pager is invoked (e.g. git status) and \"auto\" we use colors\n>  2b. If yes and we're invoking a pager (e.g. git log) and \"auto\" we use colors\n>      3b. At this point we're writing to the pager so isatty() is false,\n>          but we set our own GIT_PAGER_IN_USE and turn on colors if \"auto\"\n> \n> I suppose we can imagine a pager that sometimes emits to a terminal and\n> sometimes e.g. opens a browser with the output, and we could ourselves\n> somehow detect this...\n\nI was imagining something where we remembered the original isatty()\nvalue (from before the pager) and then reacted to that. But no, I don't\nactually have a use case there. I was trying to think through possible\nreasons to want this \"isatty\" version of color.ui.\n\n> As noted in the cover letter I started writing this whole thing before\n> understanding some of the subtleties, and now I think this \"isatty\"\n> thing is probably pretty useless, and was wondering if others wanted it.\n\nOK, I agree that it doesn't seem all that useful.\n\n> Reasons to take it are:\n> \n>  1) To make user intent clearer. I.e. we could just also make it a\n>     synonym for color.ui=auto & color.pager=false and those used to\n>     isatty semantics skimming the docs would more easily find the right\n>     thing.\n\nI'd much prefer just having a documentation patch that uses the word\n\"isatty\", if that's something we think a user might search for (which\nseems plausible to me).\n\n>  2) If there are any cases where isatty() is true, but we can detect via\n>     other means (e.g. inspecting other env variables) that non-pager\n>     output can't handle colors some of the time. Of course if we find\n>     such cases isatty() would suck more, but that's presumably what\n>     isatty() purists want :)\n\nYeah, I think we can punt on that until such an \"other means\" comes\nalong.\n\n-Peff\n"},{"id":"348974","messageId":"20180601053147.GB8292@sigill.intra.peff.net","threadId":"48608","inReplyTo":"87in749u7t.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/4] config doc: move color.ui documentation to one place","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-01T05:31:47Z","receivedAt":"2018-06-01T05:31:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 31, 2018 at 09:09:58AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> >> Instead let's briefly describe what each variable is for, and then\n> >> copy/paste a new boilerplate saying that this variable takes the exact\n> >> same values as color.ui, and if it's unset it'll fallback to whatever\n> >> color.ui is set to.\n> >\n> > Definitely an improvement. Though I wonder if we should go further and\n> > show the user the relationship in the documentation explicitly. Like:\n> >\n> >   color.<system>::\n> > \tA boolean to enable/disable color in a particular part of Git,\n> > \toverriding `color.ui` (see `color.ui` for possible values). The\n> > \tcurrent set of systems is:\n> >\n> > \tadvice::\n> > \t\tHints shown with the \"hint:\" prefix, controlled by\n> > \t\t`advice.*` config.\n> >\n> > \tbranch::\n> > \t\tOutput from the linkgit:git-branch[1] command.\n> >\n> > \t...etc...\n> >\n> > We could possibly do the same with color.<system>.<slot>. Or maybe even\n> > make a single hierarchical list of systems, and then the color slots\n> > under each. I think if our mental model in adding these options is\n> > to have this kind of hierarchy, then it makes sense to communicate it\n> > explicitly to the user and get them using the same mental model.\n> \n> I wouldn't be opposed to some twist on that, but I really dislike the\n> variables that are documented in such a way that you can't find them in\n> the documentation by searching for their fully qualified name.\n\nYeah, good point. We could probably write \"color.advice\", etc, in the\nsecond level list. It's redundant, but more explicit. And we still\nbenefit from the grouping.\n\n-Peff\n"},{"id":"348975","messageId":"20180601055355.GC8292@sigill.intra.peff.net","threadId":"48608","inReplyTo":"87h8mo9tuy.fsf@evledraar.gmail.com","subject":"Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-06-01T05:53:55Z","receivedAt":"2018-06-01T05:54:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 31, 2018 at 09:17:41AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n> >\n> >> Before this change git will die on any unknown color.ui values:\n> >>\n> >>     $ git -c color.ui=doesnotexist show\n> >>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit\n> >\n> > I do not think \"unit\" is correct, so there may be some room for\n> > improvement.  For _this_ particular case, I agree that it is not the\n> > end of the world if we did not color the output (because we do not\n> > know what the 'doesnotyetexist' token from the future is trying to\n> > tell us), but as a general principle, we should diagnose and die, if\n> > a misconfiguration is easy to correct.\n> \n> Many users (including myself) use the same ~/.gitconfig on many\n> different machines with different git versions. Maybe at some point I'm\n> willing to set the new setting to a value I know is supported on most of\n> them, but it sucks at that point if I logging into 1-3% of old machines\n> ends up killing git on any invocation.\n\nOne way I've dealt with this in the past is by breaking my config into\nmultiple files, and using an \"[include]\" for the relevant ones in each\nenvironment. That's not quite turn-key, because you need some way to\ndecide which to include and which not to, and there's no good way to\nhave Git invoke that.\n\nSome options I've pondered:\n\n  - we could add [includeIf \"version:2.18.0\"] for a minimum-version\n    check\n\n  - we could add [includeIf \"env:FOO\"] to check \"$FOO\" as a boolean.\n    That punts the work to your shell environment, but it's flexible\n    enough to let you decide however you like (checking git version,\n    machine name, etc)\n\n  - similarly, we could add [includeIf \"sh:test -f /etc/foo\"], but\n    running actual shell is nasty for a lot of reasons. Relying on the\n    environment punts on that.\n\nYou can actually do the environment thing today, but it's a bit hacky.\nE.g., with this in your .profile or similar:\n\n  git version |\n  perl -ne '\n    my $ok = /git version (\\d+\\.\\d+)/ && $1 >= 2.15;\n    exit !$ok;\n  ' &&\n  export GIT_CONFIG_PARAMETERS=\"'include.path=$HOME/.gitconfig-2.15'\"\n\nI know that's more work than just having Git ignore config it doesn't\nunderstand, but it's also a lot more flexible (instead of just ignoring\nand using the defaults, you can say \"for this version do X, for that one\ndo Y\").\n\n-Peff\n"}]}