color: Treat "true" and "yes" as "auto", not "always" Per the git documentation for color.ui [1], setting color.ui to "true" (or "yes") should behave identically to "auto", enabling color only when output is written to a terminal or an active pager. Previously, repo was equating "true" and "yes" with "always", which caused color escape codes to be emitted unconditionally, even when output was piped or redirected. Replace the duplicated string-matching logic in SetDefaultColoring and Coloring.__init__ with a single CONFIG_TO_COLOR_SETTING dict that maps all git color config values to their behavior. This makes the mapping easy to verify against the git docs and impossible to get out of sync between the two call sites. Added tests for SetDefaultColoring and Coloring.__init__ covering all color mode values (auto, true, yes, always, never, no, false), case insensitivity, TTY vs pipe behavior, active pager detection, and unrecognised input. [1] https://git-scm.com/docs/git-config#Documentation/git-config.txt-colorui Bug: 295841573 Change-Id: I8a04b9c7e4154de37ed7518c010233039e0afdc9 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/602981 Tested-by: Brian Gan <brgan@google.com> Reviewed-by: Gavin Mak <gavinmak@google.com> Commit-Queue: Brian Gan <brgan@google.com>
diff --git a/color.py b/color.py index 03fb655..4068634 100644 --- a/color.py +++ b/color.py
@@ -84,29 +84,39 @@ DEFAULT = None +# Placholder value that indicates we need to check if the user is in an +# interactive terminal session to determine if we turn on color or not. +_CHECK_CONSOLE = object() + +# https://git-scm.com/docs/git-config#Documentation/git-config.txt-colorui +_CONFIG_TO_COLOR_SETTING = { + "false": False, + "never": False, + "no": False, + "auto": _CHECK_CONSOLE, + "true": _CHECK_CONSOLE, + "yes": _CHECK_CONSOLE, + "always": True, +} + def SetDefaultColoring(state: Optional[str]) -> None: """Set coloring behavior to |state|. This is useful for overriding config options via the command line. """ - if state is None: - # Leave it alone -- return quick! - return global DEFAULT - state = state.lower() - if state in ("auto",): + + if isinstance(state, str): + state = state.lower() + if state in _CONFIG_TO_COLOR_SETTING: DEFAULT = state - elif state in ("always", "yes", "true"): - DEFAULT = "always" - elif state in ("never", "no", "false"): - DEFAULT = "never" class Coloring: def __init__(self, config, section_type): - self._section = "color.%s" % section_type + self._section = f"color.{section_type}" self._config = config self._out = sys.stdout @@ -115,16 +125,12 @@ on = self._config.GetString(self._section) if on is None: on = self._config.GetString("color.ui") + if isinstance(on, str): + on = on.lower() - if on == "auto": - if pager.active or os.isatty(1): - self._on = True - else: - self._on = False - elif on in ("true", "always"): - self._on = True - else: - self._on = False + self._on = _CONFIG_TO_COLOR_SETTING.get(on, _CHECK_CONSOLE) + if self._on is _CHECK_CONSOLE: + self._on = pager.active or os.isatty(1) def redirect(self, out): self._out = out
diff --git a/tests/test_color.py b/tests/test_color.py index 8b75d21..72d7499 100644 --- a/tests/test_color.py +++ b/tests/test_color.py
@@ -14,6 +14,8 @@ """Unittests for the color.py module.""" +from unittest import mock + import pytest import utils_for_test @@ -24,9 +26,14 @@ @pytest.fixture def coloring() -> color.Coloring: """Create a Coloring object for testing.""" + return _make_coloring("always") + + +def _make_coloring(default_state: str) -> color.Coloring: + """Set the default color mode and return a Coloring using test config.""" config_fixture = utils_for_test.FIXTURES_DIR / "test.gitconfig" config = git_config.GitConfig(config_fixture) - color.SetDefaultColoring("true") + color.SetDefaultColoring(default_state) return color.Coloring(config, "status") @@ -72,3 +79,76 @@ assert val == "\033[2;34;47m" val = coloring._parse("empty", "green", "white", "bold") assert val == "\033[1;32;47m" + + +class TestSetDefaultColoring: + """Tests for SetDefaultColoring.""" + + def test_none_leaves_default_unchanged(self) -> None: + color.DEFAULT = "auto" + color.SetDefaultColoring(None) + assert color.DEFAULT == "auto" + + @pytest.mark.parametrize( + "value, expected", + ( + # auto/true/yes all store their lowercase form. + ("auto", "auto"), + ("Auto", "auto"), + ("true", "true"), + ("True", "true"), + ("yes", "yes"), + ("Yes", "yes"), + # "always" stores as "always". + ("always", "always"), + ("Always", "always"), + # never/no/false store their lowercase form. + ("never", "never"), + ("no", "no"), + ("false", "false"), + ), + ) + def test_maps_to_expected(self, value: str, expected: str) -> None: + color.SetDefaultColoring(value) + assert color.DEFAULT == expected + + def test_unrecognised_leaves_default_unchanged(self) -> None: + color.DEFAULT = "auto" + color.SetDefaultColoring("garbage") + assert color.DEFAULT == "auto" + + +class TestColoringInit: + """Tests for Coloring.__init__ color mode logic.""" + + @pytest.mark.parametrize( + "state, isatty, pager_active, expected", + ( + # "always" enables color unconditionally. + ("always", False, False, True), + # "never" disables color unconditionally. + ("never", True, True, False), + # auto/true/yes enable color only on a TTY or active pager. + ("auto", True, False, True), + ("auto", False, False, False), + ("auto", False, True, True), + ("true", True, False, True), + ("true", False, False, False), + ("true", False, True, True), + ("yes", True, False, True), + ("yes", False, False, False), + ("yes", False, True, True), + ), + ) + def test_color_mode( + self, + state: str, + isatty: bool, + pager_active: bool, + expected: bool, + ) -> None: + with mock.patch("os.isatty", return_value=isatty), mock.patch( + "pager.active", pager_active + ): + c = _make_coloring(state) + assert c.is_on is expected