Re: [PATCH] add: check color.ui for interactive add

From: Jeff King <peff@peff.net>
Date: Jun 6, 2023, 02:13 UTC
Message-ID: <20230606021349.GC89456@coredump.intra.peff.net>
In-Reply-To: <pull.1541.git.1685994164018.gitgitgadget@gmail.com>

On Mon, Jun 05, 2023 at 07:42:43PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 5 quoted lines

> The fix is simple, to use git_color_default_config() as the fallback for
> git_add_config(). A more robust change would instead encapsulate the
> git_use_color_default global in methods that would check the config
> setting if it has not been initialized yet. Some ideas are being
> discussed on this front [1], but nothing has been finalized.

I think it should be OK to load the color config here, but note...

>     This is also a reoccurrence of the "config not loaded" bug from [3].

...that this case is a little different than the core.replaceRefs one. One of the reasons we don't just load all config by default is that it was an intentional scheme that not all programs would respect all config, and color in particular was one of the things that wasn't meant to be supported everywhere.

In other words, the idea was that porcelain like "git diff" would use git_diff_ui_config(), which would load all the bells and whistles (like color). But plumbing like git-diff-tree uses git_diff_basic_config(), which skips those. And that way we can freely introduce new config options without worrying that they will unexpectedly change the behavior of plumbing commands (because each command has to manually opt into the new config).

Now I won't claim that this approach hasn't created all sorts of headaches over the years, and we might not be better off with something more explicit (e.g., we parse all the config, but plumbing sets a flag to say "I am plumbing; do not respect color.ui"). But it is roughly the approach we've taken, so I'm mostly warning you that there may be dragons here. :)

I say "roughly" because I actually think the rules have been bent a bit over the years. In particular, I think that git_use_color_default is initialized to COLOR_AUTO, so something like:

  git diff-tree -p HEAD

ends up colorizing, even though it's plumbing. Which is maybe not so bad, but it's doubly silly that:

  git -c color.diff=false diff-tree -p HEAD

still colorizes, even though "git diff" in an equivalent situation would not! That seems like a bug, but it's one that I suspect has been there since we flipped color on by default many years ago, and nobody has really complained.

So all of this is a big digression from your patch. I think for "git add" it is probably OK to enable color by default. But I mostly want to point out that trying to roll this into a more elaborate fix may run afoul of all kinds of rabbit holes.

-Peff