ansi-color: $SYSTEMD_COLORS=true should mean generic "auto", not 24bit

Follow-up for af718e05350884c0b6a9fa7248e5d2f2564c93f0

The commit changed the documentation, but the actual impl
was apparently not updated. And the documented behavior
feels a bit off. I think generally "auto"/true should
override $NO_COLOR.

Plus, the test for auto-24bit is at odds with the logic
we merged. I guess it was overlooked after applying
https://github.com/systemd/systemd/pull/40303#discussion_r2720450393

diff --git a/man/common-variables.xml b/man/common-variables.xml
index 93f18d453e55e6a61e7b59d62e38b60eab3cb3e5..0f807b2cde1d2eca24051f50c17d546e01f2d356 100644 (file)
--- a/man/common-variables.xml
+++ b/man/common-variables.xml
@@ -225,15 +225,16 @@
       <term><varname>$SYSTEMD_COLORS</varname></term>
 
       <listitem>
-        <para>Takes a boolean argument, or a special value.</para>
+        <para>Takes a boolean argument, or a special value. By default (unset), <command>systemd</command>
+        and related utilities will use colors in their output if possible. If <varname>$COLORTERM</varname>
+        is set to <literal>truecolor</literal> or <literal>24bit</literal>, 24-bit colors will be enabled,
+        256 colors otherwise, unless <varname>$NO_COLOR</varname> or <varname>$TERM</varname> indicates
+        colors are disabled.</para>
 
         <variablelist>
           <varlistentry>
             <term><option>true</option></term>
-            <listitem><para>The default. <command>systemd</command> and related utilities will use colors in
-            their output if possible. Same as <literal>auto-24bit</literal> if <varname>$COLORTERM</varname>
-            is set to <literal>truecolor</literal> or <literal>24bit</literal>; same as
-            <literal>auto-256</literal> otherwise.</para></listitem>
+            <listitem><para>Same as unset, except that <varname>$NO_COLOR</varname> is ignored.</para></listitem>
           </varlistentry>
 
           <varlistentry>

diff --git a/src/basic/ansi-color.c b/src/basic/ansi-color.c
index da91cf6d7e2a43f46c1c71a6786c017bf08cdbcc..e5c78c93458f670c958ab44476bffbb466a6fd74 100644 (file)
--- a/src/basic/ansi-color.c
+++ b/src/basic/ansi-color.c
@@ -59,7 +59,7 @@ static ColorMode get_color_mode_impl(void) {
                 return m;
 
         /* Next, check for the presence of $NO_COLOR; value is ignored. */
-        if (getenv("NO_COLOR"))
+        if (m != COLOR_TRUE && getenv("NO_COLOR"))
                 return COLOR_OFF;
 
         /* If the above didn't work, we turn colors off unless we are on a TTY. And if we are on a TTY we
@@ -108,9 +108,10 @@ static const char* const color_mode_table[_COLOR_MODE_MAX] = {
         [COLOR_AUTO_16]    = "auto-16",
         [COLOR_AUTO_256]   = "auto-256",
         [COLOR_AUTO_24BIT] = "auto-24bit",
+        [COLOR_TRUE]       = "true",
 };
 
-DEFINE_STRING_TABLE_LOOKUP_WITH_BOOLEAN(color_mode, ColorMode, COLOR_24BIT);
+DEFINE_STRING_TABLE_LOOKUP_WITH_BOOLEAN(color_mode, ColorMode, COLOR_TRUE);
 
 /*
  * Check that the string is formatted like an ANSI color code, i.e. that it consists of one or more

diff --git a/src/basic/ansi-color.h b/src/basic/ansi-color.h
index 04b265dd16c0caac8b9f3d83b7a449ad7fba8e29..e686e1167b14eb1a01db4b68f51589cce4755596 100644 (file)
--- a/src/basic/ansi-color.h
+++ b/src/basic/ansi-color.h
@@ -17,6 +17,9 @@ typedef enum ColorMode {
         COLOR_AUTO_256,
         COLOR_AUTO_24BIT,
 
+        /* Same as default (unset), except that $NO_COLOR is ignored/overridden */
+        COLOR_TRUE,
+
         _COLOR_MODE_MAX,
         _COLOR_MODE_INVALID = -EINVAL,
 } ColorMode;

diff --git a/src/test/test-format-table.c b/src/test/test-format-table.c
index a2e142791ff80851c0d6e1637a49d532f0b21bed..7b501d11e42179c29682bef6e90ade9fea3f19a3 100644 (file)
--- a/src/test/test-format-table.c
+++ b/src/test/test-format-table.c
@@ -849,14 +849,18 @@ TEST(table_ansi) {
                                  TABLE_STRING_WITH_ANSI, ANSI_GREY "thisisgrey"));
 
         unsigned saved_columns = columns();
-        bool saved_color = colors_enabled();
-        _cleanup_free_ char *saved_term = NULL;
-        const char *e = getenv("TERM");
+        _cleanup_free_ char *saved_term = NULL, *saved_color = NULL;
+        const char *e;
+
+        e = getenv("TERM");
         if (e)
                 ASSERT_NOT_NULL((saved_term = strdup(e)));
+        e = getenv("SYSTEMD_COLORS");
+        if (e)
+                ASSERT_NOT_NULL((saved_color = strdup(e)));
 
         ASSERT_OK_ERRNO(setenv("COLUMNS", "200", /* overwrite= */ true));
-        ASSERT_OK_ERRNO(setenv("SYSTEMD_COLORS", "1", /* overwrite= */ true));
+        ASSERT_OK_ERRNO(setenv("SYSTEMD_COLORS", "24bit", /* overwrite= */ true));
         ASSERT_OK_ERRNO(setenv("TERM", FALLBACK_TERM, /* overwrite= */ true));
         reset_terminal_feature_caches();
 
@@ -904,7 +908,7 @@ TEST(table_ansi) {
         ASSERT_OK(sd_json_variant_dump(j, SD_JSON_FORMAT_COLOR_AUTO|SD_JSON_FORMAT_PRETTY_AUTO, /* f= */ NULL, /* prefix= */ NULL));
 
         ASSERT_OK(setenvf("COLUMNS", /* overwrite= */ true, "%u", saved_columns));
-        ASSERT_OK(setenvf("SYSTEMD_COLORS", /* overwrite= */ true, "%i", saved_color));
+        ASSERT_OK(set_unset_env("SYSTEMD_COLORS", saved_color, /* overwrite= */ true));
         ASSERT_OK(set_unset_env("TERM", saved_term, /* overwrite= */ true));
 }
 

diff --git a/src/test/test-qrcode-util.c b/src/test/test-qrcode-util.c
index 3545c656fb2e167468135c11e3d65663528fdcb7..dc2573b4f8433abe1f612e1c27370abc7ba93bda 100644 (file)
--- a/src/test/test-qrcode-util.c
+++ b/src/test/test-qrcode-util.c
@@ -11,7 +11,7 @@ static int run(int argc, char **argv) {
 
         test_setup_logging(LOG_DEBUG);
 
-        assert_se(setenv("SYSTEMD_COLORS", "1", 1) == 0); /* Force the qrcode to be printed */
+        assert_se(setenv("SYSTEMD_COLORS", "24bit", 1) == 0); /* Force the qrcode to be printed */
 
         r = print_qrcode(stdout, "This should say \"TEST\"", "TEST");
         if (r == -EOPNOTSUPP)

diff --git a/src/test/test-terminal-util.c b/src/test/test-terminal-util.c
index d2bab189499a60881d40b307d97b093b67715c73..4e2b751ad262776509e046647ed3a5c8a3157448 100644 (file)
--- a/src/test/test-terminal-util.c
+++ b/src/test/test-terminal-util.c
@@ -332,18 +332,19 @@ TEST(get_color_mode) {
         test_get_color_mode_with_env("SYSTEMD_COLORS", "no",    COLOR_OFF);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "16",    COLOR_16);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "256",   COLOR_256);
-        test_get_color_mode_with_env("SYSTEMD_COLORS", "1",     COLOR_24BIT);
-        test_get_color_mode_with_env("SYSTEMD_COLORS", "yes",   COLOR_24BIT);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "24bit", COLOR_24BIT);
 
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-16",    terminal_is_dumb() ? COLOR_OFF : COLOR_16);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-256",   terminal_is_dumb() ? COLOR_OFF : COLOR_256);
-        ASSERT_OK_ERRNO(setenv("COLORTERM", "truecolor", true));
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-24bit", terminal_is_dumb() ? COLOR_OFF : COLOR_24BIT);
+        ASSERT_OK_ERRNO(setenv("COLORTERM", "truecolor", true));
+        test_get_color_mode_with_env("SYSTEMD_COLORS", "1",          terminal_is_dumb() ? COLOR_OFF : COLOR_24BIT);
+        test_get_color_mode_with_env("SYSTEMD_COLORS", "yes",        terminal_is_dumb() ? COLOR_OFF : COLOR_24BIT);
         ASSERT_OK_ERRNO(unsetenv("COLORTERM"));
-        test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-24bit", terminal_is_dumb() ? COLOR_OFF : COLOR_256);
+        test_get_color_mode_with_env("SYSTEMD_COLORS", "true",       terminal_is_dumb() ? COLOR_OFF : COLOR_256);
 
         ASSERT_OK_ERRNO(setenv("NO_COLOR", "1", true));
+        test_get_color_mode_with_env("SYSTEMD_COLORS", "true",       terminal_is_dumb() ? COLOR_OFF : COLOR_256);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-16",    COLOR_OFF);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-256",   COLOR_OFF);
         test_get_color_mode_with_env("SYSTEMD_COLORS", "auto-24bit", COLOR_OFF);