Fix #14051 (color codes should not be stored in output file)

cli/cmdlineparser.cpp

@@ -56,6 +56,14 @@
 #include <unordered_set>
 #include <utility>
 
+#ifdef _WIN32
+#include <fcntl.h>
+#include <io.h>
+#else
+#include <fcntl.h>
+#include <unistd.h>
+#endif
+
 #ifdef HAVE_RULES
 // xml is used for rules
 #include "xml.h"
@@ -1553,9 +1561,38 @@ CmdLineParser::Result CmdLineParser::parseFromArgs(int argc, const char* const a
         if (mSettings.templateLocation.empty())
             mSettings.templateLocation = "{bold}{file}:{line}:{column}: {dim}note:{reset} {info}\\n{code}";
     }
-    // replace static parts of the templates
-    substituteTemplateFormatStatic(mSettings.templateFormat);
-    substituteTemplateLocationStatic(mSettings.templateLocation);
+
+    {
+        bool enableColors = false;
+
+        if (!getForcedColorSetting(enableColors) && mSettings.outputFormat == Settings::OutputFormat::text) {
+#ifdef _WIN32
+            if (mSettings.outputFile.empty())
+                enableColors = _isatty(_fileno(stderr));
+            else {
+                int fd = _open(mSettings.outputFile.c_str(), _O_RDONLY);
+                if (fd != -1) {
+                    enableColors = (_isatty(fd) != 0);
+                    _close(fd);
+                }
+            }
+#else
+            if (mSettings.outputFile.empty())
+                enableColors = isatty(fileno(stderr));
+            else {
+                int fd = open(mSettings.outputFile.c_str(), O_RDONLY | O_NOCTTY);
+                if (fd != -1) {
+                    enableColors = (isatty(fd) != 0);
+                    close(fd);
+                }
+            }
+#endif
+        }
+
+        // replace static parts of the templates
+        substituteTemplateFormatStatic(mSettings.templateFormat, enableColors);
+        substituteTemplateLocationStatic(mSettings.templateLocation, enableColors);
+    }
 
     if (mSettings.force || maxconfigs)
         mSettings.checkAllConfigurations = true;

cli/cppcheckexecutor.cpp

@@ -70,7 +70,10 @@
 #endif
 
 #ifdef _WIN32
+#include <io.h>
 #include <windows.h>
+#else
+#include <unistd.h>
 #endif
 
 #if !defined(WIN32) && !defined(__MINGW32__)
@@ -612,10 +615,20 @@ void StdLogger::reportErr(const std::string &errmsg)
 
 void StdLogger::reportOut(const std::string &outmsg, Color c)
 {
-    if (c == Color::Reset)
-        std::cout << ansiToOEM(outmsg, true) << std::endl;
-    else
+    bool enableColors;
+
+    if (!getForcedColorSetting(enableColors)) {
+#ifdef _WIN32
+        enableColors = (_isatty(_fileno(stdout)) != 0);
+#else
+        enableColors = (isatty(fileno(stdout)) != 0);
+#endif
+    }
+
+    if (enableColors && c != Color::Reset)
         std::cout << c << ansiToOEM(outmsg, true) << Color::Reset << std::endl;
+    else
+        std::cout << ansiToOEM(outmsg, true) << std::endl;
 }
 
 // TODO: remove filename parameter?

lib/color.cpp

@@ -22,52 +22,27 @@
 #include <sstream>
 #include <iostream>
 
-#ifndef _WIN32
-#include <unistd.h>
-#endif
-
-bool gDisableColors = false;
-
-#ifndef _WIN32
-static bool isStreamATty(const std::ostream & os)
-{
-    static const bool stdout_tty = isatty(STDOUT_FILENO);
-    static const bool stderr_tty = isatty(STDERR_FILENO);
-    if (&os == &std::cout)
-        return stdout_tty;
-    if (&os == &std::cerr)
-        return stderr_tty;
-    return (stdout_tty && stderr_tty);
-}
-#endif
-
-static bool isColorEnabled(const std::ostream & os)
+bool getForcedColorSetting(bool &colorSetting)
 {
     // See https://bixense.com/clicolors/
     static const bool color_forced_off = (nullptr != std::getenv("NO_COLOR"));
     if (color_forced_off)
     {
-        return false;
+        colorSetting = false;
+        return true;
     }
     static const bool color_forced_on = (nullptr != std::getenv("CLICOLOR_FORCE"));
     if (color_forced_on)
     {
+        colorSetting = true;
         return true;
     }
-#ifdef _WIN32
-    (void)os;
     return false;
-#else
-    return isStreamATty(os);
-#endif
 }
 
 std::ostream& operator<<(std::ostream & os, Color c)
 {
-    if (!gDisableColors && isColorEnabled(os))
-        return os << "\033[" << static_cast<std::size_t>(c) << "m";
-
-    return os;
+    return os << "\033[" << static_cast<std::size_t>(c) << "m";
 }
 
 std::string toString(Color c)

lib/color.h

@@ -35,10 +35,11 @@ enum class Color : std::uint8_t {
     FgMagenta  = 35,
     FgDefault  = 39
 };
+
+CPPCHECKLIB bool getForcedColorSetting(bool &colorSetting);
+
 CPPCHECKLIB std::ostream& operator<<(std::ostream& os, Color c);
 
 CPPCHECKLIB std::string toString(Color c);
 
-extern CPPCHECKLIB bool gDisableColors; // for testing
-
 #endif

lib/errorlogger.cpp

@@ -594,20 +594,35 @@ static void replace(std::string& source, const std::unordered_map<std::string, s
     }
 }
 
-static void replaceColors(std::string& source) {
-    // TODO: colors are not applied when either stdout or stderr is not a TTY because we resolve them before the stream usage
-    static const std::unordered_map<std::string, std::string> substitutionMap =
-    {
-        {"{reset}",   ::toString(Color::Reset)},
-        {"{bold}",    ::toString(Color::Bold)},
-        {"{dim}",     ::toString(Color::Dim)},
-        {"{red}",     ::toString(Color::FgRed)},
-        {"{green}",   ::toString(Color::FgGreen)},
-        {"{blue}",    ::toString(Color::FgBlue)},
-        {"{magenta}", ::toString(Color::FgMagenta)},
-        {"{default}", ::toString(Color::FgDefault)},
-    };
-    replace(source, substitutionMap);
+static void replaceColors(std::string& source, bool enableColors) {
+    if (enableColors) {
+        static const std::unordered_map<std::string, std::string> substitutionMap =
+        {
+            {"{reset}",   ::toString(Color::Reset)},
+            {"{bold}",    ::toString(Color::Bold)},
+            {"{dim}",     ::toString(Color::Dim)},
+            {"{red}",     ::toString(Color::FgRed)},
+            {"{green}",   ::toString(Color::FgGreen)},
+            {"{blue}",    ::toString(Color::FgBlue)},
+            {"{magenta}", ::toString(Color::FgMagenta)},
+            {"{default}", ::toString(Color::FgDefault)},
+        };
+        replace(source, substitutionMap);
+    }
+    else {
+        static const std::unordered_map<std::string, std::string> substitutionMap =
+        {
+            {"{reset}",   ""},
+            {"{bold}",    ""},
+            {"{dim}",     ""},
+            {"{red}",     ""},
+            {"{green}",   ""},
+            {"{blue}",    ""},
+            {"{magenta}", ""},
+            {"{default}", ""},
+        };
+        replace(source, substitutionMap);
+    }
 }
 
 std::string ErrorMessage::toString(bool verbose, const std::string &templateFormat, const std::string &templateLocation) const
@@ -913,16 +928,16 @@ std::string replaceStr(std::string s, const std::string &from, const std::string
     return s;
 }
 
-void substituteTemplateFormatStatic(std::string& templateFormat)
+void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors)
 {
     replaceSpecialChars(templateFormat);
-    replaceColors(templateFormat);
+    replaceColors(templateFormat, enableColors);
 }
 
-void substituteTemplateLocationStatic(std::string& templateLocation)
+void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors)
 {
     replaceSpecialChars(templateLocation);
-    replaceColors(templateLocation);
+    replaceColors(templateLocation, enableColors);
 }
 
 std::string getClassification(const std::string &guideline, ReportType reportType) {

lib/errorlogger.h

@@ -288,10 +288,10 @@ class CPPCHECKLIB ErrorLogger {
 std::string replaceStr(std::string s, const std::string &from, const std::string &to);
 
 /** replaces the static parts of the location template **/
-CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat);
+CPPCHECKLIB void substituteTemplateFormatStatic(std::string& templateFormat, bool enableColors = false);
 
 /** replaces the static parts of the location template **/
-CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation);
+CPPCHECKLIB void substituteTemplateLocationStatic(std::string& templateLocation, bool enableColors = false);
 
 /** Get a classification string from the given guideline and reporttype */
 CPPCHECKLIB std::string getClassification(const std::string &guideline, ReportType reportType);

test/cli/other_test.py

@@ -2426,6 +2426,41 @@ def test_xml_output(tmp_path):  # #13391 / #13485
 '''.format(version_str, test_file_exp, test_file_exp, test_file_exp))
 
 
+def test_outputfile(tmp_path): # #14051
+    test_file = tmp_path / 'test.cpp'
+    out_file = tmp_path / 'out.txt'
+    with open(test_file, 'wt') as f:
+        f.write(
+"""
+int main()
+{
+    int x = 1 / 0;
+}
+""")
+
+    args = [
+        '-q',
+        '--output-file={}'.format(out_file),
+        str(test_file)
+    ]
+
+    out_exp = [
+        '{}:4:15: error: Division by zero. [zerodiv]'.format(test_file),
+        '    int x = 1 / 0;',
+        '              ^',
+    ]
+
+    exitcode, stdout, stderr = cppcheck(args)
+    assert exitcode == 0, stdout
+    assert stdout == ''
+    assert stderr == ''
+
+    with open(out_file, 'rt') as f:
+        out_text = f.read()
+
+    assert out_text.splitlines() == out_exp
+
+
 def test_internal_error_loc_int(tmp_path):
     test_file = tmp_path / 'test.c'
     with open(test_file, 'wt') as f:

test/main.cpp

@@ -31,7 +31,6 @@ int main(int argc, char *argv[])
 #endif
 
     Preprocessor::macroChar = '$';     // While macroChar is char(1) per default outside test suite, we require it to be a human-readable character here.
-    gDisableColors = true;
 
     options args(argc, argv);
 

test/testcolor.cpp

@@ -30,8 +30,7 @@ class TestColor : public TestFixture {
     }
 
     void toString() const {
-        // TODO: color conversion is dependent on stdout/stderr being a TTY
-        ASSERT_EQUALS("", ::toString(Color::FgRed));
+        ASSERT_EQUALS("\033[31m", ::toString(Color::FgRed));
     }
 };