Fix bug where colours were incorrectly applied

exa assumed that the COLUMNS environment variable being present always meant that the output was to a terminal, so it should use colours. But because this variable can be overridden, colours were being incorrectly set! The ‘fix’ is to stop trying to be clever while only calculating the terminal width once, and instead just stick it in a lazy_static so it’s usable everywhere.
ogham · Jun 25, 2017 · 84b01f2 · 84b01f2
1 parent b8bb148
commit 84b01f2
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 62 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -16,6 +16,7 @@ ansi_term = "0.8.0"
 datetime = "0.4.3"
 getopts = "0.2.14"
 glob = "0.2"
+lazy_static = "0.2"
 libc = "0.2.9"
 locale = "0.2.1"
 natord = "1.0.7"

diff --git a/src/exa.rs b/src/exa.rs
@@ -18,6 +18,10 @@ extern crate zoneinfo_compiled;
 
 #[cfg(feature="git")] extern crate git2;
 
+#[macro_use]
+extern crate lazy_static;
+
+
 use std::ffi::OsStr;
 use std::io::{stderr, Write, Result as IOResult};
 use std::path::{Component, Path};

diff --git a/src/options/view.rs b/src/options/view.rs
@@ -7,7 +7,6 @@ use output::{Grid, Details, GridDetails};
 use output::column::{Columns, TimeTypes, SizeFormat};
 use output::file_name::Classify;
 use options::{FileFilter, DirAction, Misfire};
-use term::dimensions;
 use fs::feature::xattr;
 
 
@@ -45,10 +44,6 @@ impl Mode {
     pub fn deduce(matches: &getopts::Matches, filter: FileFilter, dir_action: DirAction) -> Result<(Mode, Colours), Misfire> {
         use options::misfire::Misfire::*;
 
-        let colour_scale = || {
-            matches.opt_present("color-scale") || matches.opt_present("colour-scale")
-        };
-
         let long = || {
             if matches.opt_present("across") && !matches.opt_present("grid") {
                 Err(Useless("across", true, "long"))
@@ -57,19 +52,7 @@ impl Mode {
                 Err(Useless("oneline", true, "long"))
             }
             else {
-                let term_colours = TerminalColours::deduce(matches)?;
-                let colours = match term_colours {
-                    TerminalColours::Always    => Colours::colourful(colour_scale()),
-                    TerminalColours::Never     => Colours::plain(),
-                    TerminalColours::Automatic => {
-                        if dimensions().is_some() {
-                            Colours::colourful(colour_scale())
-                        }
-                        else {
-                            Colours::plain()
-                        }
-                    },
-                };
+                let colours = Colours::deduce(matches)?;
 
                 let details = Details {
                     columns: Some(Columns::deduce(matches)?),
@@ -105,15 +88,8 @@ impl Mode {
         };
 
         let other_options_scan = || {
-            let term_colours = TerminalColours::deduce(matches)?;
-            let term_width   = TerminalWidth::deduce()?;
-
-            if let Some(width) = term_width.width() {
-                let colours = match term_colours {
-                    TerminalColours::Always     |
-                    TerminalColours::Automatic  => Colours::colourful(colour_scale()),
-                    TerminalColours::Never      => Colours::plain(),
-                };
+            if let Some(width) = TerminalWidth::deduce()?.width() {
+                let colours = Colours::deduce(matches)?;
 
                 if matches.opt_present("oneline") {
                     if matches.opt_present("across") {
@@ -148,10 +124,7 @@ impl Mode {
                 // as the program’s stdout being connected to a file, then
                 // fallback to the lines view.
 
-                let colours = match term_colours {
-                    TerminalColours::Always    => Colours::colourful(colour_scale()),
-                    TerminalColours::Never | TerminalColours::Automatic => Colours::plain(),
-                };
+                let colours = Colours::deduce(matches)?;
 
                 if matches.opt_present("tree") {
                     let details = Details {
@@ -222,7 +195,7 @@ impl TerminalWidth {
                 Err(e)     => Err(Misfire::FailedParse(e)),
             }
         }
-        else if let Some((width, _)) = dimensions() {
+        else if let Some(width) = *TERM_WIDTH {
             Ok(TerminalWidth::Terminal(width))
         }
         else {
@@ -373,10 +346,37 @@ impl TerminalColours {
 }
 
 
+impl Colours {
+    fn deduce(matches: &getopts::Matches) -> Result<Colours, Misfire> {
+        use self::TerminalColours::*;
+
+        let tc = TerminalColours::deduce(matches)?;
+        if tc == Always || (tc == Automatic && TERM_WIDTH.is_some()) {
+            let scale = matches.opt_present("color-scale") || matches.opt_present("colour-scale");
+            Ok(Colours::colourful(scale))
+        }
+        else {
+            Ok(Colours::plain())
+        }
+    }
+}
+
+
 
 impl Classify {
     fn deduce(matches: &getopts::Matches) -> Classify {
         if matches.opt_present("classify") { Classify::AddFileIndicators }
                                       else { Classify::JustFilenames }
     }
 }
+
+
+// Gets, then caches, the width of the terminal that exa is running in.
+// This gets used multiple times above, with no real guarantee of order,
+// so it’s easier to just cache it the first time it runs.
+lazy_static! {
+    static ref TERM_WIDTH: Option<usize> = {
+        use term::dimensions;
+        dimensions().map(|t| t.0)
+    };
+}
diff --git a/xtests/run.sh b/xtests/run.sh
@@ -29,8 +29,6 @@ $exa $testcases/files -lhB | diff -q - $results/files_lhb2  || exit 1
 $exa $testcases/attributes/dirs/empty-with-attribute -lh | diff -q - $results/empty  || exit 1
 
 $exa --color-scale         $testcases/files -l | diff -q - $results/files_l_scale  || exit 1
-$exa_binary                $testcases/files -l | diff -q - $results/files_l_bw     || exit 1
-$exa_binary --colour=never $testcases/files -l | diff -q - $results/files_l_bw     || exit 1
 
 
 # Grid view tests
@@ -110,6 +108,14 @@ COLUMNS=80 $exa $testcases/links    2>&1 | diff -q - $results/links        || ex
 $exa $testcases/links/* -1 | diff -q - $results/links_1_files || exit 1
 
 
+# Colours and terminals
+# Just because COLUMNS is present, doesn’t mean output is to a terminal
+COLUMNS=80 $exa_binary                    $testcases/files -l | diff -q - $results/files_l_bw  || exit 1
+COLUMNS=80 $exa_binary --colour=always    $testcases/files -l | diff -q - $results/files_l     || exit 1
+COLUMNS=80 $exa_binary --colour=never     $testcases/files -l | diff -q - $results/files_l_bw  || exit 1
+COLUMNS=80 $exa_binary --colour=automatic $testcases/files -l | diff -q - $results/files_l_bw  || exit 1
+
+
 # Git
 $exa $testcases/git/additions -l --git 2>&1 | diff -q - $results/git_additions  || exit 1
 $exa $testcases/git/edits     -l --git 2>&1 | diff -q - $results/git_edits      || exit 1