color.ui docs & add color.ui=isatty

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.

Quoted 8 lines · Æ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.

Yeah, I agree the current state is poor.

Quoted 7 lines

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.

Quoted 20 lines

Quoted 4 lines

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.

Quoted 7 lines

...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.

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.

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 <[email protected]>

---
 Documentation/config.txt |  5 +++++
 color.c                  | 13 +++++++++++++
 t/t7006-pager.sh         | 16 ++++++++++++++++
 3 files changed, 34 insertions(+)

Documentation/config.txt +5 -0

index 4767363519..b882a88214 100644
@@ -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.

color.c +0 -0

index b1c24c69de..e52c6cdd29 100644

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

Quoted 4 lines · Æ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

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.

Quoted 7 lines

Quoted 4 lines · Æ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

Quoted 6 lines

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.

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.

Quoted 10 lines · Æ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.

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.

Quoted 1 line

Quoted 6 lines

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?

Quoted 2 lines

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.

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.

Quoted 10 lines · Æ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.

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.

Quoted 6 lines

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.

Quoted 2 lines

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...

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 :)