fix: check if it is a Real terminal, not just if it is a terminal

This will stop printing stuff to dumb terminals that they don't support.

I've overall audited usage of isatty and replaced the ones with intent
to mean "is a Real terminal" with checking for that. I've also caught a
case of carelessly assuming "is a tty" means "should be colour" in
nix-env.

Change-Id: I6d83725d9a2d932ac94ff2294f92c0a1100d23c9

7 files changed, 50 insertions, 15 deletions

diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc
index a407c647f..018e34509 100644
--- a/src/libmain/shared.cc
+++ b/src/libmain/shared.cc
@@ -5,9 +5,9 @@
 #include "signals.hh"
 #include "loggers.hh"
 #include "current-process.hh"
+#include "terminal.hh"
 
 #include <algorithm>
-#include <cctype>
 #include <exception>
 #include <iostream>
 
@@ -347,7 +347,7 @@ int handleExceptions(const std::string & programName, std::function<void()> fun)
 
 RunPager::RunPager()
 {
-    if (!isatty(STDOUT_FILENO)) return;
+    if (!isOutputARealTerminal(StandardOutputStream::Stdout)) return;
     char * pager = getenv("NIX_PAGER");
     if (!pager) pager = getenv("PAGER");
     if (pager && ((std::string) pager == "" || (std::string) pager == "cat")) return;
diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc
index cbeb7aa36..7d9482814 100644
--- a/src/libutil/logging.cc
+++ b/src/libutil/logging.cc
@@ -37,7 +37,15 @@ void Logger::warn(const std::string & msg)
 
 void Logger::writeToStdout(std::string_view s)
 {
-    writeFull(STDOUT_FILENO, filterANSIEscapes(s, !shouldANSI(), std::numeric_limits<unsigned int>::max(), false));
+    writeFull(
+        STDOUT_FILENO,
+        filterANSIEscapes(
+            s,
+            !shouldANSI(StandardOutputStream::Stdout),
+            std::numeric_limits<unsigned int>::max(),
+            false
+        )
+    );
     writeFull(STDOUT_FILENO, "\n");
 }
 
diff --git a/src/libutil/terminal.cc b/src/libutil/terminal.cc
index 2ba1cb81b..25e97e599 100644
--- a/src/libutil/terminal.cc
+++ b/src/libutil/terminal.cc
@@ -7,7 +7,12 @@
 
 namespace nix {
 
-bool shouldANSI()
+bool isOutputARealTerminal(StandardOutputStream fileno)
+{
+    return isatty(int(fileno)) && getEnv("TERM").value_or("dumb") != "dumb";
+}
+
+bool shouldANSI(StandardOutputStream fileno)
 {
     // Implements the behaviour described by https://bixense.com/clicolors/
     // As well as https://force-color.org/ for compatibility, since it fits in the same shape.
@@ -16,14 +21,14 @@ bool shouldANSI()
     // unset    x        set            Yes
     // unset    x        unset          If attached to a terminal
     //                                  [we choose the "modern" approach of colour-by-default]
-    auto compute = []() -> bool {
+    auto compute = [](StandardOutputStream fileno) -> bool {
         bool mustNotColour = getEnv("NO_COLOR").has_value() || getEnv("NOCOLOR").has_value();
         bool shouldForce = getEnv("CLICOLOR_FORCE").has_value() || getEnv("FORCE_COLOR").has_value();
-        bool isTerminal = isatty(STDERR_FILENO) && getEnv("TERM").value_or("dumb") != "dumb";
+        bool isTerminal = isOutputARealTerminal(fileno);
         return !mustNotColour && (shouldForce || isTerminal);
     };
-    static bool cached = compute();
-    return cached;
+    static bool cached[2] = {compute(StandardOutputStream::Stdout), compute(StandardOutputStream::Stderr)};
+    return cached[int(fileno) - 1];
 }
 
 // FIXME(jade): replace with TerminalCodeEater. wowie this is evil code.
diff --git a/src/libutil/terminal.hh b/src/libutil/terminal.hh
index 2c422ecff..28c96c780 100644
--- a/src/libutil/terminal.hh
+++ b/src/libutil/terminal.hh
@@ -6,6 +6,26 @@
 
 namespace nix {
 
+enum class StandardOutputStream {
+    Stdout = 1,
+    Stderr = 2,
+};
+
+/**
+ * Determine whether the output is a real terminal (i.e. not dumb, not a pipe).
+ *
+ * This is probably not what you want, you may want shouldANSI() or something
+ * more specific. Think about how the output should work with a pager or
+ * entirely non-interactive scripting use.
+ *
+ * The user may be redirecting the Lix output to a pager, but have stderr
+ * connected to a terminal. Think about where you are outputting the text when
+ * deciding whether to use STDERR_FILENO or STDOUT_FILENO.
+ *
+ * \param fileno file descriptor number to check if it is a tty
+ */
+bool isOutputARealTerminal(StandardOutputStream fileno);
+
 /**
  * Determine whether ANSI escape sequences are appropriate for the
  * present output.
@@ -18,8 +38,10 @@ namespace nix {
  * - CLICOLOR_FORCE or FORCE_COLOR set   -> enable colour
  * - The output is a tty; TERM != "dumb" -> enable colour
  * - Otherwise                           -> disable colour
+ *
+ * \param fileno which file descriptor number to consider. Use the one you are outputting to
  */
-bool shouldANSI();
+bool shouldANSI(StandardOutputStream fileno = StandardOutputStream::Stderr);
 
 /**
  * Truncate a string to 'width' printable characters. If 'filterAll'
diff --git a/src/nix-env/nix-env.cc b/src/nix-env/nix-env.cc
index 6dda34a32..13fadb1d8 100644
--- a/src/nix-env/nix-env.cc
+++ b/src/nix-env/nix-env.cc
@@ -1,6 +1,7 @@
 #include "attr-path.hh"
 #include "common-eval-args.hh"
 #include "derivations.hh"
+#include "terminal.hh"
 #include "eval.hh"
 #include "get-drvs.hh"
 #include "globals.hh"
@@ -17,7 +18,6 @@
 #include "legacy.hh"
 #include "eval-settings.hh" // for defexpr
 
-#include <cerrno>
 #include <ctime>
 #include <algorithm>
 #include <iostream>
@@ -1092,7 +1092,6 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
         return;
     }
 
-    bool tty = isatty(STDOUT_FILENO);
     RunPager pager;
 
     Table table;
@@ -1171,7 +1170,7 @@ static void opQuery(Globals & globals, Strings opFlags, Strings opArgs)
                     }
                 } else {
                     auto column = (std::string) "" + ch + " " + version;
-                    if (diff == cvGreater && tty)
+                    if (diff == cvGreater && shouldANSI(StandardOutputStream::Stdout))
                         column = ANSI_RED + column + ANSI_NORMAL;
                     columns.push_back(column);
                 }
diff --git a/src/nix/main.cc b/src/nix/main.cc
index 1c1e9df7e..9cbe303ac 100644
--- a/src/nix/main.cc
+++ b/src/nix/main.cc
@@ -362,6 +362,7 @@ void mainWrapped(int argc, char * * argv)
     setLogFormat(LogFormat::bar);
     Finally f([] { logger->pause(); });
     settings.verboseBuild = false;
+    // FIXME: stop messing about with log verbosity depending on if it is interactive use
     if (isatty(STDERR_FILENO)) {
         verbosity = lvlNotice;
     } else {
diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc
index 0b04a04e6..b99cd5dd0 100644
--- a/src/nix/prefetch.cc
+++ b/src/nix/prefetch.cc
@@ -4,11 +4,11 @@
 #include "shared.hh"
 #include "store-api.hh"
 #include "filetransfer.hh"
-#include "finally.hh"
 #include "tarfile.hh"
 #include "attr-path.hh"
-#include "eval-inline.hh"
+#include "eval-inline.hh" // IWYU pragma: keep
 #include "legacy.hh"
+#include "terminal.hh"
 
 #include <nlohmann/json.hpp>
 
@@ -180,7 +180,7 @@ static int main_nix_prefetch_url(int argc, char * * argv)
         if (args.size() > 2)
             throw UsageError("too many arguments");
 
-        if (isatty(STDERR_FILENO))
+        if (isOutputARealTerminal(StandardOutputStream::Stderr))
             setLogFormat(LogFormat::bar);
 
         auto store = openStore();