[PATCH 0/4] color.ui docs & add color.ui=isatty

[PATCH 0/4] color.ui docs & add color.ui=isatty

[Ævar Arnfjörð Bjarmason]

I started writing this for the reasons explained in 4/4, but the
justification there is a bit of a stretch. Where we'd realistically
like to diverge color.ui=auto and color.pager=false, but since I wrote
it already maybe some people will come out of the woodworks telling me
that this is what they've always wanted for whatever reason.

Maybe if we don't like 4/4 we should take 3/4 because we might want
another such option in the future, but that's probably overly hedging
our bets, still. I really don't like this pattern that we have a
multi-value config option that dies instead of warns on unknown future
values, so I'm leaning towards saying that should be accepted to git.

But while I was working towards 4/4 I did some nice fixes in [12]/4. I
think those should go in regardless, so they're non-RFC.

Ævar Arnfjörð Bjarmason (4):
  config doc: move color.ui documentation to one place
  config doc: clarify "to a terminal" in color.ui
  color.ui config: don't die on unknown values
  color.ui config: add "isatty" setting

 Documentation/config.txt | 100 +++++++++++++++++++++------------------
 color.c                  |  25 ++++++++--
 color.h                  |   1 +
 t/t7006-pager.sh         |  31 ++++++++++++
 4 files changed, 107 insertions(+), 50 deletions(-)

-- 
2.17.0.290.gded63e768a

[PATCH 1/4] config doc: move color.ui documentation to one place

[Ævar Arnfjörð Bjarmason]

Ever since b982592d66 ("git-status: document colorization config
options", 2006-09-11) we've slowly been accumulating more and more
color.* options, where the documentation for each new one has
seemingly been copy/pasted with minor adjustments from the last.

This has resulted in documentation where we're describing what sort of
values color.ui or its overriding variables can take a grand total of
9 times.

This makes for hard and tedious reading, and is going to be a royal
pain if we're ever going to add more color.ui values.

Instead let's briefly describe what each variable is for, and then
copy/paste a new boilerplate saying that this variable takes the exact
same values as color.ui, and if it's unset it'll fallback to whatever
color.ui is set to.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 81 ++++++++++++++++++----------------------
 1 file changed, 36 insertions(+), 45 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 7d8383433c..44735dd88e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1103,21 +1103,19 @@ clean.requireForce::
 	-i or -n.   Defaults to true.
 
 color.advice::
-	A boolean to enable/disable color in hints (e.g. when a push
-	failed, see `advice.*` for a list).  May be set to `always`,
-	`false` (or `never`) or `auto` (or `true`), in which case colors
-	are used only when the error output goes to a terminal. If
-	unset, then the value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in hints (e.g. when a push failed,
+	see `advice.*` for a list). See `color.ui` for possible values
+	and the default. If unset, the value of `color.ui` is used as
+	a fallback.
 
 color.advice.hint::
 	Use customized color for hints.
 
 color.branch::
-	A boolean to enable/disable color in the output of
-	linkgit:git-branch[1]. May be set to `always`,
-	`false` (or `never`) or `auto` (or `true`), in which case colors are used
-	only when the output is to a terminal. If unset, then the
-	value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in the output of
+	linkgit:git-branch[1]. See `color.ui` for possible values and
+	the default. If unset, the value of `color.ui` is used as a
+	fallback.
 
 color.branch.<slot>::
 	Use customized color for branch coloration. `<slot>` is one of
@@ -1127,13 +1125,11 @@ color.branch.<slot>::
 	refs).
 
 color.diff::
-	Whether to use ANSI escape sequences to add color to patches.
-	If this is set to `always`, linkgit:git-diff[1],
-	linkgit:git-log[1], and linkgit:git-show[1] will use color
-	for all patches.  If it is set to `true` or `auto`, those
-	commands will only use color when output is to the terminal.
-	If unset, then the value of `color.ui` is used (`auto` by
-	default).
+	Enables or disables colors when patches are emitted
+	(e.g. linkgit:git-diff[1], linkgit:git-log[1], and
+	linkgit:git-show[1]). See `color.ui` for possible values and
+	the default. If unset, the value of `color.ui` is used as a
+	fallback.
 +
 This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
@@ -1165,10 +1161,10 @@ color.decorate.<slot>::
 	branches, remote-tracking branches, tags, stash and HEAD, respectively.
 
 color.grep::
-	When set to `always`, always highlight matches.  When `false` (or
-	`never`), never.  When set to `true` or `auto`, use color only
-	when the output is written to the terminal.  If unset, then the
-	value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in the output of
+	linkgit:git-grep[1]. See `color.ui` for possible values and
+	the default. If unset, the value of `color.ui` is used as a
+	fallback.
 
 color.grep.<slot>::
 	Use customized color for grep colorization.  `<slot>` specifies which
@@ -1197,12 +1193,11 @@ color.grep.<slot>::
 --
 
 color.interactive::
-	When set to `always`, always use colors for interactive prompts
-	and displays (such as those used by "git-add --interactive" and
-	"git-clean --interactive"). When false (or `never`), never.
-	When set to `true` or `auto`, use colors only when the output is
-	to the terminal. If unset, then the value of `color.ui` is
-	used (`auto` by default).
+	Enables or disables colors in interactive prompts and displays
+	(such as those used by "git-add --interactive" and "git-clean
+	--interactive"). See `color.ui` for possible values and the
+	default. If unset, the value of `color.ui` is used as a
+	fallback.
 
 color.interactive.<slot>::
 	Use customized color for 'git add --interactive' and 'git clean
@@ -1215,27 +1210,24 @@ color.pager::
 	use (default is true).
 
 color.push::
-	A boolean to enable/disable color in push errors. May be set to
-	`always`, `false` (or `never`) or `auto` (or `true`), in which
-	case colors are used only when the error output goes to a terminal.
-	If unset, then the value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in push errors. See `color.ui` for
+	possible values and the default. If unset, the value of
+	`color.ui` is used as a fallback.
 
 color.push.error::
 	Use customized color for push errors.
 
 color.showBranch::
-	A boolean to enable/disable color in the output of
-	linkgit:git-show-branch[1]. May be set to `always`,
-	`false` (or `never`) or `auto` (or `true`), in which case colors are used
-	only when the output is to a terminal. If unset, then the
-	value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in the output of
+	linkgit:git-show-branch[1]. See `color.ui` for possible values
+	and the default. If unset, the value of `color.ui` is used as
+	a fallback.
 
 color.status::
-	A boolean to enable/disable color in the output of
-	linkgit:git-status[1]. May be set to `always`,
-	`false` (or `never`) or `auto` (or `true`), in which case colors are used
-	only when the output is to a terminal. If unset, then the
-	value of `color.ui` is used (`auto` by default).
+	Enables or disables colors in the output of
+	linkgit:git-status[1]. See `color.ui` for possible values
+	and the default. If unset, the value of `color.ui` is used as
+	a fallback.
 
 color.status.<slot>::
 	Use customized color for status colorization. `<slot>` is
@@ -1279,10 +1271,9 @@ blame.coloring::
 	or 'none' which is the default.
 
 color.transport::
-	A boolean to enable/disable color when pushes are rejected. May be
-	set to `always`, `false` (or `never`) or `auto` (or `true`), in which
-	case colors are used only when the error output goes to a terminal.
-	If unset, then the value of `color.ui` is used (`auto` by default).
+	Enables or disables colors when pushes are rejected. See
+	`color.ui` for possible values and the default. If unset, the
+	value of `color.ui` is used as a fallback.
 
 color.transport.rejected::
 	Use customized color when a push was rejected.
-- 
2.17.0.290.gded63e768a

Re: [PATCH 1/4] config doc: move color.ui documentation to one place

[Jeff King]

On Wed, May 30, 2018 at 09:06:38PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Ever since b982592d66 ("git-status: document colorization config
> options", 2006-09-11) we've slowly been accumulating more and more
> color.* options, where the documentation for each new one has
> seemingly been copy/pasted with minor adjustments from the last.
> 
> This has resulted in documentation where we're describing what sort of
> values color.ui or its overriding variables can take a grand total of
> 9 times.

Yeah, I agree the current state is poor.

> This makes for hard and tedious reading, and is going to be a royal
> pain if we're ever going to add more color.ui values.
> 
> Instead let's briefly describe what each variable is for, and then
> copy/paste a new boilerplate saying that this variable takes the exact
> same values as color.ui, and if it's unset it'll fallback to whatever
> color.ui is set to.

Definitely an improvement. Though I wonder if we should go further and
show the user the relationship in the documentation explicitly. Like:

  color.<system>::
	A boolean to enable/disable color in a particular part of Git,
	overriding `color.ui` (see `color.ui` for possible values). The
	current set of systems is:

	advice::
		Hints shown with the "hint:" prefix, controlled by
		`advice.*` config.

	branch::
		Output from the linkgit:git-branch[1] command.

	...etc...

We could possibly do the same with color.<system>.<slot>. Or maybe even
make a single hierarchical list of systems, and then the color slots
under each. I think if our mental model in adding these options is
to have this kind of hierarchy, then it makes sense to communicate it
explicitly to the user and get them using the same mental model.

-Peff

Re: [PATCH 1/4] config doc: move color.ui documentation to one place

[Ævar Arnfjörð Bjarmason]

On Thu, May 31 2018, Jeff King wrote:

> On Wed, May 30, 2018 at 09:06:38PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> Ever since b982592d66 ("git-status: document colorization config
>> options", 2006-09-11) we've slowly been accumulating more and more
>> color.* options, where the documentation for each new one has
>> seemingly been copy/pasted with minor adjustments from the last.
>>
>> This has resulted in documentation where we're describing what sort of
>> values color.ui or its overriding variables can take a grand total of
>> 9 times.
>
> Yeah, I agree the current state is poor.
>
>> This makes for hard and tedious reading, and is going to be a royal
>> pain if we're ever going to add more color.ui values.
>>
>> Instead let's briefly describe what each variable is for, and then
>> copy/paste a new boilerplate saying that this variable takes the exact
>> same values as color.ui, and if it's unset it'll fallback to whatever
>> color.ui is set to.
>
> Definitely an improvement. Though I wonder if we should go further and
> show the user the relationship in the documentation explicitly. Like:
>
>   color.<system>::
> 	A boolean to enable/disable color in a particular part of Git,
> 	overriding `color.ui` (see `color.ui` for possible values). The
> 	current set of systems is:
>
> 	advice::
> 		Hints shown with the "hint:" prefix, controlled by
> 		`advice.*` config.
>
> 	branch::
> 		Output from the linkgit:git-branch[1] command.
>
> 	...etc...
>
> We could possibly do the same with color.<system>.<slot>. Or maybe even
> make a single hierarchical list of systems, and then the color slots
> under each. I think if our mental model in adding these options is
> to have this kind of hierarchy, then it makes sense to communicate it
> explicitly to the user and get them using the same mental model.

I wouldn't be opposed to some twist on that, but I really dislike the
variables that are documented in such a way that you can't find them in
the documentation by searching for their fully qualified name.

Re: [PATCH 1/4] config doc: move color.ui documentation to one place

[Jeff King]

On Thu, May 31, 2018 at 09:09:58AM +0200, Ævar Arnfjörð Bjarmason wrote:

> >> Instead let's briefly describe what each variable is for, and then
> >> copy/paste a new boilerplate saying that this variable takes the exact
> >> same values as color.ui, and if it's unset it'll fallback to whatever
> >> color.ui is set to.
> >
> > Definitely an improvement. Though I wonder if we should go further and
> > show the user the relationship in the documentation explicitly. Like:
> >
> >   color.<system>::
> > 	A boolean to enable/disable color in a particular part of Git,
> > 	overriding `color.ui` (see `color.ui` for possible values). The
> > 	current set of systems is:
> >
> > 	advice::
> > 		Hints shown with the "hint:" prefix, controlled by
> > 		`advice.*` config.
> >
> > 	branch::
> > 		Output from the linkgit:git-branch[1] command.
> >
> > 	...etc...
> >
> > We could possibly do the same with color.<system>.<slot>. Or maybe even
> > make a single hierarchical list of systems, and then the color slots
> > under each. I think if our mental model in adding these options is
> > to have this kind of hierarchy, then it makes sense to communicate it
> > explicitly to the user and get them using the same mental model.
> 
> I wouldn't be opposed to some twist on that, but I really dislike the
> variables that are documented in such a way that you can't find them in
> the documentation by searching for their fully qualified name.

Yeah, good point. We could probably write "color.advice", etc, in the
second level list. It's redundant, but more explicit. And we still
benefit from the grouping.

-Peff

[PATCH 2/4] config doc: clarify "to a terminal" in color.ui

[Ævar Arnfjörð Bjarmason]

Change the documentation added when color.ui=auto was made the default
in 4c7f1819b3 ("make color.ui default to 'auto'", 2013-06-10) to
describe "auto" as kicking in when writing to the terminal or a pager,
not just to the terminal.

I had someone ask me why it was that git was writing colors with
color.ui=auto in situations where isatty(3) would return 0. The
existing documentation about that would have been true before
85fb65ed6e (""git -p cmd" to page anywhere", 2006-06-06), but since
then "auto" has always used a heuristic that isn't quite what isatty()
would return, rather it checks if we're connected to a TTY or if we're
about to emit to a pager, trusting that the pager can handle color
output.

Instead we have the color.pager variable introduced in
aa086eb813 ("pager: config variable pager.color", 2006-07-30) to
explicitly disable color output to the pager. Let's change the
documentation to reflect this.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 44735dd88e..4767363519 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1288,7 +1288,9 @@ color.ui::
 	or the `--color` option. Set it to `always` if you want all
 	output not intended for machine consumption to use color, to
 	`true` or `auto` (this is the default since Git 1.8.4) if you
-	want such output to use color when written to the terminal.
+	want such output to use color when written to the terminal (as
+	determined by a call to `isatty(3)`) or to a pager (unless
+	`color.pager` is set to false).
 
 column.ui::
 	Specify whether supported commands should output in columns.
-- 
2.17.0.290.gded63e768a

Re: [PATCH 2/4] config doc: clarify "to a terminal" in color.ui

[Jeff King]

On Wed, May 30, 2018 at 09:06:39PM +0000, Ævar Arnfjörð Bjarmason wrote:

> Change the documentation added when color.ui=auto was made the default
> in 4c7f1819b3 ("make color.ui default to 'auto'", 2013-06-10) to
> describe "auto" as kicking in when writing to the terminal or a pager,
> not just to the terminal.

Makes sense. Usually the pager only kicks in when we're going to a
terminal in the first place, so the two would be the same thing. But
obviously you can override that with "-p".

-Peff

[RFC PATCH 3/4] color.ui config: don't die on unknown values

[Ævar Arnfjörð Bjarmason]

Before this change git will die on any unknown color.ui values:

    $ git -c color.ui=doesnotexist show
    fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit

This makes the failure mode of introducing any new values in the
future really bad, as explained in the documentation being added
here. Instead let's warn and fall back to "auto".

The reason for the !warned++ pattern is when stepping through this in
the debugger I found that git_config_colorbool() is called more than
once on e.g. a "show" if color.ui=foo is set in the config, but
color.ui=bar in the command-line, and would then warn about
both.

Maybe we should warn about both in that case, but I don't know if
there's other cases where not doing this would cause a warning flood,
and in any case the user is unlikely to have such a bad value in
multiple places, so this should be good enough.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt |  5 +++++
 color.c                  | 13 +++++++++++++
 t/t7006-pager.sh         | 16 ++++++++++++++++
 3 files changed, 34 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 4767363519..b882a88214 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1291,6 +1291,11 @@ color.ui::
 	want such output to use color when written to the terminal (as
 	determined by a call to `isatty(3)`) or to a pager (unless
 	`color.pager` is set to false).
++
+Setting this to some value unknown to git will warn and fall back to
+`auto`. This is so that new values can be recognized in the future
+without the git configuration file being incompatible between versions
+to the point of most porcelain commands dying on the older version.
 
 column.ui::
 	Specify whether supported commands should output in columns.
diff --git a/color.c b/color.c
index b1c24c69de..e52c6cdd29 100644
--- a/color.c
+++ b/color.c
@@ -311,6 +311,19 @@ int git_config_colorbool(const char *var, const char *value)
 	if (!var)
 		return -1;
 
+	/*
+	 * If future git versions introduce new color.ui settings we
+	 * don't want to die right below when git_config_bool() fails
+	 * to parse them as bool.
+	 */
+	if (git_parse_maybe_bool(value) < 0) {
+		static int warned = 0;
+		if (!warned++)
+			warning(_("unknown value '%s' for '%s', falling back to 'auto'"),
+				value, var);
+		return GIT_COLOR_AUTO;
+	}
+
 	/* Missing or explicit false to turn off colorization */
 	if (!git_config_bool(var, value))
 		return 0;
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 7541ba5edb..b16f2ac28b 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -309,6 +309,14 @@ test_expect_success 'no color when stdout is a regular file' '
 	! colorful colorless.log
 '
 
+test_expect_success 'unknown color.ui values default to "auto" (regular file)' '
+	rm -f colorless.log &&
+	test_config color.ui doesnotexist &&
+	git log >colorless.log 2>err &&
+	test_i18ngrep "falling back" err &&
+	! colorful colorless.log
+'
+
 test_expect_success TTY 'color when writing to a pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
@@ -316,6 +324,14 @@ test_expect_success TTY 'color when writing to a pager' '
 	colorful paginated.out
 '
 
+test_expect_success TTY 'unknown color.ui values default to "auto" (pager)' '
+	rm -f paginated.out &&
+	test_config color.ui doesnotexist &&
+	test_terminal git log 2>err &&
+	test_i18ngrep "falling back" err &&
+	colorful paginated.out
+'
+
 test_expect_success TTY 'colors are suppressed by color.pager' '
 	rm -f paginated.out &&
 	test_config color.ui auto &&
-- 
2.17.0.290.gded63e768a

Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values

[Stefan Beller]

On Wed, May 30, 2018 at 2:06 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Before this change git will die on any unknown color.ui values:
>
>     $ git -c color.ui=doesnotexist show
>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit
>
> This makes the failure mode of introducing any new values in the
> future really bad, as explained in the documentation being added
> here. Instead let's warn and fall back to "auto".

We had a similar regression in the protocol.version setting in 2.16 IIRC.
It makes sense to fix it this way.

> The reason for the !warned++ pattern is when stepping through this in
> the debugger I found that git_config_colorbool() is called more than
> once on e.g. a "show" if color.ui=foo is set in the config, but
> color.ui=bar in the command-line, and would then warn about
> both.
>
> Maybe we should warn about both in that case, but I don't know if
> there's other cases where not doing this would cause a warning flood,
> and in any case the user is unlikely to have such a bad value in
> multiple places, so this should be good enough.

agreed.

>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  Documentation/config.txt |  5 +++++
>  color.c                  | 13 +++++++++++++
>  t/t7006-pager.sh         | 16 ++++++++++++++++
>  3 files changed, 34 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4767363519..b882a88214 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1291,6 +1291,11 @@ color.ui::
>         want such output to use color when written to the terminal (as
>         determined by a call to `isatty(3)`) or to a pager (unless
>         `color.pager` is set to false).
> ++
> +Setting this to some value unknown to git will warn and fall back to
> +`auto`. This is so that new values can be recognized in the future
> +without the git configuration file being incompatible between versions
> +to the point of most porcelain commands dying on the older version.
>
>  column.ui::
>         Specify whether supported commands should output in columns.
> diff --git a/color.c b/color.c
> index b1c24c69de..e52c6cdd29 100644
> --- a/color.c
> +++ b/color.c
> @@ -311,6 +311,19 @@ int git_config_colorbool(const char *var, const char *value)
>         if (!var)
>                 return -1;
>
> +       /*
> +        * If future git versions introduce new color.ui settings we
> +        * don't want to die right below when git_config_bool() fails
> +        * to parse them as bool.
> +        */

I am not sure we need to document this here. Ignoring unknown
(or warning about unknown) values is the standard for config keys
at least, for values we probably want a similar thing.

> +       if (git_parse_maybe_bool(value) < 0) {
> +               static int warned = 0;

As someone who moved a lot of global state into the repository object
this is a fine example where we do not want to have the counter not
in the repository? (although strictly speaking you'd want to have the
warning approximately once per repository that is configured wrongly
or do you want to have this warning once at all?)

I think we should take patches 1-3 at least.

Thanks,
Stefan

Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Before this change git will die on any unknown color.ui values:
>
>     $ git -c color.ui=doesnotexist show
>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit

I do not think "unit" is correct, so there may be some room for
improvement.  For _this_ particular case, I agree that it is not the
end of the world if we did not color the output (because we do not
know what the 'doesnotyetexist' token from the future is trying to
tell us), but as a general principle, we should diagnose and die, if
a misconfiguration is easy to correct, than blindly go ahead and do
random things that the end-user did not expect by giving something
we do not (but they thought they do) understand.

If we really want to introduce "here is a setting you may not
understand, in which case you may safely ignore", the right way to
do so is to follow the model the index extension took, where from
the syntax of the unknown thing an old/existing code can tell if it
is optional.  Forcing all codepaths to forever ignore what they do
not understand and what they happen to think is a safe fallback is
simply being irresponsible---the existing code does not understand
the new setting so they do not even know if their "current
behaviour" as a fallback is a safe and sensible one from the point
of view of the person who asked for the feature from the future.

Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values

[Ævar Arnfjörð Bjarmason]

On Wed, May 30 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Before this change git will die on any unknown color.ui values:
>>
>>     $ git -c color.ui=doesnotexist show
>>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit
>
> I do not think "unit" is correct, so there may be some room for
> improvement.  For _this_ particular case, I agree that it is not the
> end of the world if we did not color the output (because we do not
> know what the 'doesnotyetexist' token from the future is trying to
> tell us), but as a general principle, we should diagnose and die, if
> a misconfiguration is easy to correct.

Many users (including myself) use the same ~/.gitconfig on many
different machines with different git versions. Maybe at some point I'm
willing to set the new setting to a value I know is supported on most of
them, but it sucks at that point if I logging into 1-3% of old machines
ends up killing git on any invocation.

> than blindly go ahead and do random things that the end-user did not
> expect by giving something we do not (but they thought they do)
> understand.

I think this is highly dependent on what variables we give this
treatment. There may be some where we genuinely have no idea what they
mean, but in this case and for http.sslVersion (which warns, doesn't die
on unknown values) it's reasonable to assume that degrading to a known
value is better than outright dying.

> If we really want to introduce "here is a setting you may not
> understand, in which case you may safely ignore", the right way to
> do so is to follow the model the index extension took, where from
> the syntax of the unknown thing an old/existing code can tell if it
> is optional.  Forcing all codepaths to forever ignore what they do
> not understand and what they happen to think is a safe fallback is
> simply being irresponsible---the existing code does not understand
> the new setting so they do not even know if their "current
> behaviour" as a fallback is a safe and sensible one from the point
> of view of the person who asked for the feature from the future.

This seems needlessly complex. color.ui is one of the most prominent
config variales, so you're proposing we split it up into some dual-key
arrangement and force all users to migrate? I think just following what
we're doing with http.sslVersion makes more sense.

Re: [RFC PATCH 3/4] color.ui config: don't die on unknown values

[Jeff King]

On Thu, May 31, 2018 at 09:17:41AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> >
> >> Before this change git will die on any unknown color.ui values:
> >>
> >>     $ git -c color.ui=doesnotexist show
> >>     fatal: bad numeric config value 'doesnotexist' for 'color.ui': invalid unit
> >
> > I do not think "unit" is correct, so there may be some room for
> > improvement.  For _this_ particular case, I agree that it is not the
> > end of the world if we did not color the output (because we do not
> > know what the 'doesnotyetexist' token from the future is trying to
> > tell us), but as a general principle, we should diagnose and die, if
> > a misconfiguration is easy to correct.
> 
> Many users (including myself) use the same ~/.gitconfig on many
> different machines with different git versions. Maybe at some point I'm
> willing to set the new setting to a value I know is supported on most of
> them, but it sucks at that point if I logging into 1-3% of old machines
> ends up killing git on any invocation.

One way I've dealt with this in the past is by breaking my config into
multiple files, and using an "[include]" for the relevant ones in each
environment. That's not quite turn-key, because you need some way to
decide which to include and which not to, and there's no good way to
have Git invoke that.

Some options I've pondered:

  - we could add [includeIf "version:2.18.0"] for a minimum-version
    check

  - we could add [includeIf "env:FOO"] to check "$FOO" as a boolean.
    That punts the work to your shell environment, but it's flexible
    enough to let you decide however you like (checking git version,
    machine name, etc)

  - similarly, we could add [includeIf "sh:test -f /etc/foo"], but
    running actual shell is nasty for a lot of reasons. Relying on the
    environment punts on that.

You can actually do the environment thing today, but it's a bit hacky.
E.g., with this in your .profile or similar:

  git version |
  perl -ne '
    my $ok = /git version (\d+\.\d+)/ && $1 >= 2.15;
    exit !$ok;
  ' &&
  export GIT_CONFIG_PARAMETERS="'include.path=$HOME/.gitconfig-2.15'"

I know that's more work than just having Git ignore config it doesn't
understand, but it's also a lot more flexible (instead of just ignoring
and using the defaults, you can say "for this version do X, for that one
do Y").

-Peff

[RFC PATCH 4/4] color.ui config: add "isatty" setting

[Ævar Arnfjörð Bjarmason]

A co-worker of mine who was using UNIX systems when dinosaurs roamed
the earth was lamenting that kids these days were using tools like
"git" that thought they knew better than isatty(3) when deciding
whether or not something was a terminal, and the state of the
documentation fixed earlier in this series certainly didn't help.

So this setting is a small gift to all the UNIX graybeards out
there. Now they can set color.ui=isatty and only emit fancy colors in
situations when the gods of old intended, not whatever heuristic we've
decided to set "auto" to.

As noted here this is *currently* the same as setting color.ui=auto &
color.pager=false, but I think it's good to explicitly have this
setting for any future changes. The reason, as now noted in the
documentation is that the "auto" setting may become even smarter in
the future and learn even deeper heuristics for when to turn itself on
even if isatty(3) were returning true.

At that point the fans of plain isatty(3) will become even more upset
at what we're doing, so let's give them a simple future-proof opt-out.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt | 10 ++++++++++
 color.c                  | 12 ++++++++----
 color.h                  |  1 +
 t/t7006-pager.sh         | 15 +++++++++++++++
 4 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b882a88214..183569786f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1292,6 +1292,16 @@ color.ui::
 	determined by a call to `isatty(3)`) or to a pager (unless
 	`color.pager` is set to false).
 +
+If you don't like the magic of `auto` and prefer for git to just ask
+`isatty(3)` whether it's connected to a terminal set this to
+`isatty`. This will cause it to always obey that function, except
+(like `auto`) if `TERM=dumb` is set in the environment. Currently this
+is equivalent to setting `auto` and `color.pager=false`, but in the
+future `auto` may be smart enough to handle other cases, i.e. when
+`isatty(3)` returns `1` but something else other than `TERM=dumb`
+suggests the terminal can't handle colors or not. If you'd like to
+avoid all that magic use `isatty`.
++
 Setting this to some value unknown to git will warn and fall back to
 `auto`. This is so that new values can be recognized in the future
 without the git configuration file being incompatible between versions
diff --git a/color.c b/color.c
index e52c6cdd29..05f95649bc 100644
--- a/color.c
+++ b/color.c
@@ -306,6 +306,8 @@ int git_config_colorbool(const char *var, const char *value)
 			return 1;
 		if (!strcasecmp(value, "auto"))
 			return GIT_COLOR_AUTO;
+		if (!strcasecmp(value, "isatty"))
+			return GIT_COLOR_ISATTY;
 	}
 
 	if (!var)
@@ -332,13 +334,14 @@ int git_config_colorbool(const char *var, const char *value)
 	return GIT_COLOR_AUTO;
 }
 
-static int check_auto_color(int fd)
+static int check_auto_color(int fd, int isatty_only)
 {
 	static int color_stderr_is_tty = -1;
 	int *is_tty_p = fd == 1 ? &color_stdout_is_tty : &color_stderr_is_tty;
 	if (*is_tty_p < 0)
 		*is_tty_p = isatty(fd);
-	if (*is_tty_p || (fd == 1 && pager_in_use() && pager_use_color)) {
+	if (*is_tty_p || (!isatty_only && fd == 1 && pager_in_use() &&
+			  pager_use_color)) {
 		if (!is_terminal_dumb())
 			return 1;
 	}
@@ -359,9 +362,10 @@ int want_color_fd(int fd, int var)
 	if (var < 0)
 		var = git_use_color_default;
 
-	if (var == GIT_COLOR_AUTO) {
+	if (var == GIT_COLOR_AUTO || var == GIT_COLOR_ISATTY) {
+		int isatty_only = var == GIT_COLOR_ISATTY;
 		if (want_auto[fd] < 0)
-			want_auto[fd] = check_auto_color(fd);
+			want_auto[fd] = check_auto_color(fd, isatty_only);
 		return want_auto[fd];
 	}
 	return var;
diff --git a/color.h b/color.h
index 5b744e1bc6..01d8cc01a5 100644
--- a/color.h
+++ b/color.h
@@ -57,6 +57,7 @@ struct strbuf;
 #define GIT_COLOR_NEVER  0
 #define GIT_COLOR_ALWAYS 1
 #define GIT_COLOR_AUTO   2
+#define GIT_COLOR_ISATTY 3
 
 /* A default list of colors to use for commit graphs and show-branch output */
 extern const char *column_colors_ansi[];
diff --git a/t/t7006-pager.sh b/t/t7006-pager.sh
index b16f2ac28b..ea4f2f47d0 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -309,6 +309,13 @@ test_expect_success 'no color when stdout is a regular file' '
 	! colorful colorless.log
 '
 
+test_expect_success 'no color when stdout is a regular file (isatty)' '
+	rm -f colorless.log &&
+	test_config color.ui isatty &&
+	git log >colorless.log &&
+	! colorful colorless.log
+'
+
 test_expect_success 'unknown color.ui values default to "auto" (regular file)' '
 	rm -f colorless.log &&
 	test_config color.ui doesnotexist &&
@@ -340,6 +347,14 @@ test_expect_success TTY 'colors are suppressed by color.pager' '
 	! colorful paginated.out
 '
 
+test_expect_success TTY 'colors are suppressed by color.ui=isatty when writing to a pager' '
+	rm -f paginated.out &&
+	test_config color.ui isatty &&
+	test_config color.pager true &&
+	test_terminal git log &&
+	! colorful paginated.out
+'
+
 test_expect_success 'color when writing to a file intended for a pager' '
 	rm -f colorful.log &&
 	test_config color.ui auto &&
-- 
2.17.0.290.gded63e768a

Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> A co-worker of mine who was using UNIX systems when dinosaurs roamed
> the earth was lamenting that kids these days were using tools like
> "git" that thought they knew better than isatty(3) when deciding
> whether or not something was a terminal, and the state of the
> documentation fixed earlier in this series certainly didn't help.
>
> So this setting is a small gift to all the UNIX graybeards out
> there. Now they can set color.ui=isatty and only emit fancy colors in
> situations when the gods of old intended, not whatever heuristic we've
> decided to set "auto" to.

Re-read the above again, and notice that you are *only* hinting that
you consider difference between "auto" and "isatty" is important,
and that your "isatty" is better, without telling what the
difference is, let alone why you think "isatty" is better.

>
> As noted here this is *currently* the same as setting color.ui=auto &
> color.pager=false, but I think it's good to explicitly have this
> setting for any future changes. The reason, as now noted in the
> documentation is that the "auto" setting may become even smarter in
> the future and learn even deeper heuristics for when to turn itself on
> even if isatty(3) were returning true.

Do you mean s/true/false/ in the last part?

> At that point the fans of plain isatty(3) will become even more upset
> at what we're doing, so let's give them a simple future-proof opt-out.

You still haven't explained why "auto" that does more than "isatty"
is and will be irritating.

That's not a good way to sell a patch.

Also even "isatty" still needs to do more than isatty(1) call.  The
process that is trying to do color.ui=isatty may be talking to an
outgoing pipe due to the use of "git -p cmd", by that time, it is
too late to call isatty(1) and obtain the info the caller wishes to.

Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting

[Ævar Arnfjörð Bjarmason]

On Wed, May 30 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> A co-worker of mine who was using UNIX systems when dinosaurs roamed
>> the earth was lamenting that kids these days were using tools like
>> "git" that thought they knew better than isatty(3) when deciding
>> whether or not something was a terminal, and the state of the
>> documentation fixed earlier in this series certainly didn't help.
>>
>> So this setting is a small gift to all the UNIX graybeards out
>> there. Now they can set color.ui=isatty and only emit fancy colors in
>> situations when the gods of old intended, not whatever heuristic we've
>> decided to set "auto" to.
>
> Re-read the above again, and notice that you are *only* hinting that
> you consider difference between "auto" and "isatty" is important,
> and that your "isatty" is better, without telling what the
> difference is, let alone why you think "isatty" is better.
>
>>
>> As noted here this is *currently* the same as setting color.ui=auto &
>> color.pager=false, but I think it's good to explicitly have this
>> setting for any future changes. The reason, as now noted in the
>> documentation is that the "auto" setting may become even smarter in
>> the future and learn even deeper heuristics for when to turn itself on
>> even if isatty(3) were returning true.
>
> Do you mean s/true/false/ in the last part?

No "true" as noted in
https://public-inbox.org/git/874liofgv6.fsf@evledraar.gmail.com/

>> At that point the fans of plain isatty(3) will become even more upset
>> at what we're doing, so let's give them a simple future-proof opt-out.
>
> You still haven't explained why "auto" that does more than "isatty"
> is and will be irritating.
>
> That's not a good way to sell a patch.

I'm not really trying to sell this thing, as noted in the CL. This is
more of a "I wrote this, does anyone find this useful?".

> Also even "isatty" still needs to do more than isatty(1) call.  The
> process that is trying to do color.ui=isatty may be talking to an
> outgoing pipe due to the use of "git -p cmd", by that time, it is
> too late to call isatty(1) and obtain the info the caller wishes to.

Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting

[Jeff King]

On Wed, May 30, 2018 at 09:06:41PM +0000, Ævar Arnfjörð Bjarmason wrote:

> A co-worker of mine who was using UNIX systems when dinosaurs roamed
> the earth was lamenting that kids these days were using tools like
> "git" that thought they knew better than isatty(3) when deciding
> whether or not something was a terminal, and the state of the
> documentation fixed earlier in this series certainly didn't help.
> 
> So this setting is a small gift to all the UNIX graybeards out
> there. Now they can set color.ui=isatty and only emit fancy colors in
> situations when the gods of old intended, not whatever heuristic we've
> decided to set "auto" to.

I'm having trouble thinking of a case where anybody would actually
care about the distinction between these two.

If you're not using "-p", then colors will kick in if isatty() is true.
The whole "also check the pager" thing is only because we may check the
color isatty() after starting the pager. If we didn't, then nobody would
ever see color. If your pager doesn't understand color, then fine. Tell
Git not to send it color with color.pager=false.

If you are using "-p", we might send color to your pager even though you
weren't originally going to a terminal. But either your pager can handle
colors, in which case this is fine, or it cannot, in which case you
would already have needed to set color.pager as above to un-break all of
the non-p cases.

Is there some case where a pager can only handle color if _it's_ output
is going to a tty, and otherwise not? And you'd want color predicated on
whether the original output was a tty or not, even if you said "-p"?
That's the only case I can think of where the existing config is not
sufficient, but I'm having a hard time envisioning a practical use.

> As noted here this is *currently* the same as setting color.ui=auto &
> color.pager=false, but I think it's good to explicitly have this
> setting for any future changes. The reason, as now noted in the
> documentation is that the "auto" setting may become even smarter in
> the future and learn even deeper heuristics for when to turn itself on
> even if isatty(3) were returning true.

Are we actually thinking about teaching it deeper heuristics? I think
the fact that it _is_ based on isatty() is meant to be understood by the
user, and we wouldn't want to change that. True, there is the pager
exception, but the point of that is to maintain the "are we going to a
tty" model, even in the face of sticking a pager in the middle.

-Peff

Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting

[Ævar Arnfjörð Bjarmason]

On Thu, May 31 2018, Jeff King wrote:

> On Wed, May 30, 2018 at 09:06:41PM +0000, Ævar Arnfjörð Bjarmason wrote:
>
>> A co-worker of mine who was using UNIX systems when dinosaurs roamed
>> the earth was lamenting that kids these days were using tools like
>> "git" that thought they knew better than isatty(3) when deciding
>> whether or not something was a terminal, and the state of the
>> documentation fixed earlier in this series certainly didn't help.
>>
>> So this setting is a small gift to all the UNIX graybeards out
>> there. Now they can set color.ui=isatty and only emit fancy colors in
>> situations when the gods of old intended, not whatever heuristic we've
>> decided to set "auto" to.
>
> I'm having trouble thinking of a case where anybody would actually
> care about the distinction between these two.
>
> If you're not using "-p", then colors will kick in if isatty() is true.
> The whole "also check the pager" thing is only because we may check the
> color isatty() after starting the pager. If we didn't, then nobody would
> ever see color. If your pager doesn't understand color, then fine. Tell
> Git not to send it color with color.pager=false.
>
> If you are using "-p", we might send color to your pager even though you
> weren't originally going to a terminal. But either your pager can handle
> colors, in which case this is fine, or it cannot, in which case you
> would already have needed to set color.pager as above to un-break all of
> the non-p cases.
>
> Is there some case where a pager can only handle color if _it's_ output
> is going to a tty, and otherwise not?

Maybe I'm missing something but how is that something we can deal with?
We just:

 1. use isatty() to see if we're attached to a terminal
 2a. If yes and no pager is invoked (e.g. git status) and "auto" we use colors
 2b. If yes and we're invoking a pager (e.g. git log) and "auto" we use colors
     3b. At this point we're writing to the pager so isatty() is false,
         but we set our own GIT_PAGER_IN_USE and turn on colors if "auto"

I suppose we can imagine a pager that sometimes emits to a terminal and
sometimes e.g. opens a browser with the output, and we could ourselves
somehow detect this...

> And you'd want color predicated on
> whether the original output was a tty or not, even if you said "-p"?
> That's the only case I can think of where the existing config is not
> sufficient, but I'm having a hard time envisioning a practical use.
>
>> As noted here this is *currently* the same as setting color.ui=auto &
>> color.pager=false, but I think it's good to explicitly have this
>> setting for any future changes. The reason, as now noted in the
>> documentation is that the "auto" setting may become even smarter in
>> the future and learn even deeper heuristics for when to turn itself on
>> even if isatty(3) were returning true.
>
> Are we actually thinking about teaching it deeper heuristics? I think
> the fact that it _is_ based on isatty() is meant to be understood by the
> user, and we wouldn't want to change that. True, there is the pager
> exception, but the point of that is to maintain the "are we going to a
> tty" model, even in the face of sticking a pager in the middle.

As noted in the cover letter I started writing this whole thing before
understanding some of the subtleties, and now I think this "isatty"
thing is probably pretty useless, and was wondering if others wanted it.

Reasons to take it are:

 1) To make user intent clearer. I.e. we could just also make it a
    synonym for color.ui=auto & color.pager=false and those used to
    isatty semantics skimming the docs would more easily find the right
    thing.

 2) If there are any cases where isatty() is true, but we can detect via
    other means (e.g. inspecting other env variables) that non-pager
    output can't handle colors some of the time. Of course if we find
    such cases isatty() would suck more, but that's presumably what
    isatty() purists want :)

Re: [RFC PATCH 4/4] color.ui config: add "isatty" setting

[Jeff King]

On Thu, May 31, 2018 at 09:01:49AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Is there some case where a pager can only handle color if _it's_ output
> > is going to a tty, and otherwise not?
> 
> Maybe I'm missing something but how is that something we can deal with?
> We just:
> 
>  1. use isatty() to see if we're attached to a terminal
>  2a. If yes and no pager is invoked (e.g. git status) and "auto" we use colors
>  2b. If yes and we're invoking a pager (e.g. git log) and "auto" we use colors
>      3b. At this point we're writing to the pager so isatty() is false,
>          but we set our own GIT_PAGER_IN_USE and turn on colors if "auto"
> 
> I suppose we can imagine a pager that sometimes emits to a terminal and
> sometimes e.g. opens a browser with the output, and we could ourselves
> somehow detect this...

I was imagining something where we remembered the original isatty()
value (from before the pager) and then reacted to that. But no, I don't
actually have a use case there. I was trying to think through possible
reasons to want this "isatty" version of color.ui.

> As noted in the cover letter I started writing this whole thing before
> understanding some of the subtleties, and now I think this "isatty"
> thing is probably pretty useless, and was wondering if others wanted it.

OK, I agree that it doesn't seem all that useful.

> Reasons to take it are:
> 
>  1) To make user intent clearer. I.e. we could just also make it a
>     synonym for color.ui=auto & color.pager=false and those used to
>     isatty semantics skimming the docs would more easily find the right
>     thing.

I'd much prefer just having a documentation patch that uses the word
"isatty", if that's something we think a user might search for (which
seems plausible to me).

>  2) If there are any cases where isatty() is true, but we can detect via
>     other means (e.g. inspecting other env variables) that non-pager
>     output can't handle colors some of the time. Of course if we find
>     such cases isatty() would suck more, but that's presumably what
>     isatty() purists want :)

Yeah, I think we can punt on that until such an "other means" comes
along.

-Peff