Merge branch 'strict-mode-properly'

This adds a check for the EXA_STRICT environment variable, and uses it to put exa in a strict mode, which enables more checks for useless/redundant arguments.

Its other goal was to move all the option parsing tests out of options/mod.rs and into the files where they’re actually relevant. THIS IS NOT YET DONE and I’ll have to do some more work on this prior to release to clear up the last few lingering issues. There are almost certainly going to be cases where redundant arguments are still complained about (or ignored) by mistake.

But I want to get this branch merged so I can take care of some other stuff for the next release.

Author: ogham

src/exa.rs

@@ -22,14 +22,15 @@ extern crate term_size;
 extern crate lazy_static;
 
 
+use std::env::var_os;
 use std::ffi::{OsStr, OsString};
 use std::io::{stderr, Write, Result as IOResult};
 use std::path::{Component, PathBuf};
 
 use ansi_term::{ANSIStrings, Style};
 
 use fs::{Dir, File};
-use options::Options;
+use options::{Options, Vars};
 pub use options::Misfire;
 use output::{escape, lines, grid, grid_details, details, View, Mode};
 
@@ -55,10 +56,20 @@ pub struct Exa<'args, 'w, W: Write + 'w> {
     pub args: Vec<&'args OsStr>,
 }
 
+/// The “real” environment variables type.
+/// Instead of just calling `var_os` from within the options module,
+/// the method of looking up environment variables has to be passed in.
+struct LiveVars;
+impl Vars for LiveVars {
+    fn get(&self, name: &'static str) -> Option<OsString> {
+        var_os(name)
+    }
+}
+
 impl<'args, 'w, W: Write + 'w> Exa<'args, 'w, W> {
     pub fn new<I>(args: I, writer: &'w mut W) -> Result<Exa<'args, 'w, W>, Misfire>
     where I: Iterator<Item=&'args OsString> {
-        Options::getopts(args).map(move |(options, args)| {
+        Options::parse(args, LiveVars).map(move |(options, args)| {
             Exa { options, writer, args }
         })
     }

src/options/dir_action.rs

@@ -8,12 +8,12 @@ impl DirAction {
 
     /// Determine which action to perform when trying to list a directory.
     pub fn deduce(matches: &MatchedFlags) -> Result<DirAction, Misfire> {
-        let recurse = matches.has(&flags::RECURSE);
-        let list    = matches.has(&flags::LIST_DIRS);
-        let tree    = matches.has(&flags::TREE);
+        let recurse = matches.has(&flags::RECURSE)?;
+        let list    = matches.has(&flags::LIST_DIRS)?;
+        let tree    = matches.has(&flags::TREE)?;
 
         // Early check for --level when it wouldn’t do anything
-        if !recurse && !tree && matches.get(&flags::LEVEL).is_some() {
+        if !recurse && !tree && matches.get(&flags::LEVEL)?.is_some() {
             return Err(Misfire::Useless2(&flags::LEVEL, &flags::RECURSE, &flags::TREE));
         }
 
@@ -37,7 +37,7 @@ impl RecurseOptions {
 
     /// Determine which files should be recursed into.
     pub fn deduce(matches: &MatchedFlags, tree: bool) -> Result<RecurseOptions, Misfire> {
-        let max_depth = if let Some(level) = matches.get(&flags::LEVEL) {
+        let max_depth = if let Some(level) = matches.get(&flags::LEVEL)? {
             match level.to_string_lossy().parse() {
                 Ok(l)   => Some(l),
                 Err(e)  => return Err(Misfire::FailedParse(e)),
@@ -55,51 +55,50 @@ impl RecurseOptions {
 #[cfg(test)]
 mod test {
     use super::*;
-    use std::ffi::OsString;
     use options::flags;
-
-    pub fn os(input: &'static str) -> OsString {
-        let mut os = OsString::new();
-        os.push(input);
-        os
-    }
+    use options::parser::Flag;
 
     macro_rules! test {
-        ($name:ident: $type:ident <- $inputs:expr => $result:expr) => {
+        ($name:ident: $type:ident <- $inputs:expr; $stricts:expr => $result:expr) => {
             #[test]
             fn $name() {
-                use options::parser::{Args, Arg};
-                use std::ffi::OsString;
-
-                static TEST_ARGS: &[&Arg] = &[ &flags::RECURSE, &flags::LIST_DIRS, &flags::TREE, &flags::LEVEL ];
-
-                let bits = $inputs.as_ref().into_iter().map(|&o| os(o)).collect::<Vec<OsString>>();
-                let results = Args(TEST_ARGS).parse(bits.iter());
-                assert_eq!($type::deduce(&results.unwrap().flags), $result);
+                use options::parser::Arg;
+                use options::test::parse_for_test;
+                use options::test::Strictnesses::*;
+
+                static TEST_ARGS: &[&Arg] = &[&flags::RECURSE, &flags::LIST_DIRS, &flags::TREE, &flags::LEVEL ];
+                for result in parse_for_test($inputs.as_ref(), TEST_ARGS, $stricts, |mf| $type::deduce(mf)) {
+                    assert_eq!(result, $result);
+                }
             }
         };
     }
 
 
     // Default behaviour
-    test!(empty:           DirAction <- []               => Ok(DirAction::List));
+    test!(empty:           DirAction <- [];               Both => Ok(DirAction::List));
 
     // Listing files as directories
-    test!(dirs_short:      DirAction <- ["-d"]           => Ok(DirAction::AsFile));
-    test!(dirs_long:       DirAction <- ["--list-dirs"]  => Ok(DirAction::AsFile));
+    test!(dirs_short:      DirAction <- ["-d"];           Both => Ok(DirAction::AsFile));
+    test!(dirs_long:       DirAction <- ["--list-dirs"];  Both => Ok(DirAction::AsFile));
 
     // Recursing
-    test!(rec_short:       DirAction <- ["-R"]                           => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: None })));
-    test!(rec_long:        DirAction <- ["--recurse"]                    => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: None })));
-    test!(rec_lim_short:   DirAction <- ["-RL4"]                         => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: Some(4) })));
-    test!(rec_lim_short_2: DirAction <- ["-RL=5"]                        => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: Some(5) })));
-    test!(rec_lim_long:    DirAction <- ["--recurse", "--level", "666"]  => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: Some(666) })));
-    test!(rec_lim_long_2:  DirAction <- ["--recurse", "--level=0118"]    => Ok(DirAction::Recurse(RecurseOptions { tree: false, max_depth: Some(118) })));
-    test!(rec_tree:        DirAction <- ["--recurse", "--tree"]          => Ok(DirAction::Recurse(RecurseOptions { tree: true,  max_depth: None })));
-    test!(rec_short_tree:  DirAction <- ["--tree", "--recurse"]          => Ok(DirAction::Recurse(RecurseOptions { tree: true,  max_depth: None })));
+    use self::DirAction::Recurse;
+    test!(rec_short:       DirAction <- ["-R"];                           Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: None })));
+    test!(rec_long:        DirAction <- ["--recurse"];                    Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: None })));
+    test!(rec_lim_short:   DirAction <- ["-RL4"];                         Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: Some(4) })));
+    test!(rec_lim_short_2: DirAction <- ["-RL=5"];                        Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: Some(5) })));
+    test!(rec_lim_long:    DirAction <- ["--recurse", "--level", "666"];  Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: Some(666) })));
+    test!(rec_lim_long_2:  DirAction <- ["--recurse", "--level=0118"];    Both => Ok(Recurse(RecurseOptions { tree: false, max_depth: Some(118) })));
+    test!(rec_tree:        DirAction <- ["--recurse", "--tree"];          Both => Ok(Recurse(RecurseOptions { tree: true,  max_depth: None })));
+    test!(rec_short_tree:  DirAction <- ["--tree", "--recurse"];          Both => Ok(Recurse(RecurseOptions { tree: true,  max_depth: None })));
 
     // Errors
-    test!(error:           DirAction <- ["--list-dirs", "--recurse"]  => Err(Misfire::Conflict(&flags::RECURSE, &flags::LIST_DIRS)));
-    test!(error_2:         DirAction <- ["--list-dirs", "--tree"]     => Err(Misfire::Conflict(&flags::TREE,    &flags::LIST_DIRS)));
-    test!(underwaterlevel: DirAction <- ["--level=4"]                 => Err(Misfire::Useless2(&flags::LEVEL, &flags::RECURSE, &flags::TREE)));
+    test!(error:           DirAction <- ["--list-dirs", "--recurse"];     Both => Err(Misfire::Conflict(&flags::RECURSE, &flags::LIST_DIRS)));
+    test!(error_2:         DirAction <- ["--list-dirs", "--tree"];        Both => Err(Misfire::Conflict(&flags::TREE,    &flags::LIST_DIRS)));
+    test!(underwaterlevel: DirAction <- ["--level=4"];                    Both => Err(Misfire::Useless2(&flags::LEVEL, &flags::RECURSE, &flags::TREE)));
+
+    // Overriding
+    test!(overriding_1:    DirAction <- ["-RL=6", "-L=7"];                Last => Ok(Recurse(RecurseOptions { tree: false, max_depth: Some(7) })));
+    test!(overriding_2:    DirAction <- ["-RL=6", "-L=7"];            Complain => Err(Misfire::Duplicate(Flag::Short(b'L'), Flag::Short(b'L'))));
 }

src/options/filter.rs

@@ -11,8 +11,8 @@ impl FileFilter {
     /// command-line arguments.
     pub fn deduce(matches: &MatchedFlags) -> Result<FileFilter, Misfire> {
         Ok(FileFilter {
-            list_dirs_first: matches.has(&flags::DIRS_FIRST),
-            reverse:         matches.has(&flags::REVERSE),
+            list_dirs_first: matches.has(&flags::DIRS_FIRST)?,
+            reverse:         matches.has(&flags::REVERSE)?,
             sort_field:      SortField::deduce(matches)?,
             dot_filter:      DotFilter::deduce(matches)?,
             ignore_patterns: IgnorePatterns::deduce(matches)?,
@@ -38,7 +38,7 @@ impl SortField {
     /// argument. This will return `Err` if the option is there, but does not
     /// correspond to a valid field.
     fn deduce(matches: &MatchedFlags) -> Result<SortField, Misfire> {
-        let word = match matches.get(&flags::SORT) {
+        let word = match matches.get(&flags::SORT)? {
             Some(w)  => w,
             None     => return Ok(SortField::default()),
         };
@@ -85,11 +85,22 @@ impl SortField {
 
 impl DotFilter {
     pub fn deduce(matches: &MatchedFlags) -> Result<DotFilter, Misfire> {
-        match matches.count(&flags::ALL) {
-            0 => Ok(DotFilter::JustFiles),
-            1 => Ok(DotFilter::Dotfiles),
-            _ => if matches.has(&flags::TREE) { Err(Misfire::TreeAllAll) }
-                                         else { Ok(DotFilter::DotfilesAndDots) }
+        let count = matches.count(&flags::ALL);
+
+        if count == 0 {
+            Ok(DotFilter::JustFiles)
+        }
+        else if count == 1 {
+            Ok(DotFilter::Dotfiles)
+        }
+        else if matches.count(&flags::TREE) > 0 {
+            Err(Misfire::TreeAllAll)
+        }
+        else if count >= 3 && matches.is_strict() {
+            Err(Misfire::Conflict(&flags::ALL, &flags::ALL))
+        }
+        else {
+            Ok(DotFilter::DotfilesAndDots)
         }
     }
 }
@@ -101,7 +112,7 @@ impl IgnorePatterns {
     /// command-line arguments.
     pub fn deduce(matches: &MatchedFlags) -> Result<IgnorePatterns, Misfire> {
 
-        let inputs = match matches.get(&flags::IGNORE_GLOB) {
+        let inputs = match matches.get(&flags::IGNORE_GLOB)? {
             None => return Ok(IgnorePatterns::empty()),
             Some(is) => is,
         };
@@ -126,6 +137,7 @@ mod test {
     use super::*;
     use std::ffi::OsString;
     use options::flags;
+    use options::parser::Flag;
 
     pub fn os(input: &'static str) -> OsString {
         let mut os = OsString::new();
@@ -134,17 +146,17 @@ mod test {
     }
 
     macro_rules! test {
-        ($name:ident: $type:ident <- $inputs:expr => $result:expr) => {
+        ($name:ident: $type:ident <- $inputs:expr; $stricts:expr => $result:expr) => {
             #[test]
             fn $name() {
-                use options::parser::{Args, Arg};
-                use std::ffi::OsString;
+                use options::parser::Arg;
+                use options::test::parse_for_test;
+                use options::test::Strictnesses::*;
 
                 static TEST_ARGS: &[&Arg] = &[ &flags::SORT, &flags::ALL, &flags::TREE, &flags::IGNORE_GLOB ];
-
-                let bits = $inputs.as_ref().into_iter().map(|&o| os(o)).collect::<Vec<OsString>>();
-                let results = Args(TEST_ARGS).parse(bits.iter());
-                assert_eq!($type::deduce(&results.unwrap().flags), $result);
+                for result in parse_for_test($inputs.as_ref(), TEST_ARGS, $stricts, |mf| $type::deduce(mf)) {
+                    assert_eq!(result, $result);
+                }
             }
         };
     }
@@ -153,38 +165,44 @@ mod test {
         use super::*;
 
         // Default behaviour
-        test!(empty:         SortField <- []                  => Ok(SortField::default()));
+        test!(empty:         SortField <- [];                  Both => Ok(SortField::default()));
 
         // Sort field arguments
-        test!(one_arg:       SortField <- ["--sort=cr"]       => Ok(SortField::CreatedDate));
-        test!(one_long:      SortField <- ["--sort=size"]     => Ok(SortField::Size));
-        test!(one_short:     SortField <- ["-saccessed"]      => Ok(SortField::AccessedDate));
-        test!(lowercase:     SortField <- ["--sort", "name"]  => Ok(SortField::Name(SortCase::Sensitive)));
-        test!(uppercase:     SortField <- ["--sort", "Name"]  => Ok(SortField::Name(SortCase::Insensitive)));
+        test!(one_arg:       SortField <- ["--sort=cr"];       Both => Ok(SortField::CreatedDate));
+        test!(one_long:      SortField <- ["--sort=size"];     Both => Ok(SortField::Size));
+        test!(one_short:     SortField <- ["-saccessed"];      Both => Ok(SortField::AccessedDate));
+        test!(lowercase:     SortField <- ["--sort", "name"];  Both => Ok(SortField::Name(SortCase::Sensitive)));
+        test!(uppercase:     SortField <- ["--sort", "Name"];  Both => Ok(SortField::Name(SortCase::Insensitive)));
 
         // Errors
-        test!(error:         SortField <- ["--sort=colour"]   => Err(Misfire::bad_argument(&flags::SORT, &os("colour"), super::SORTS)));
+        test!(error:         SortField <- ["--sort=colour"];   Both => Err(Misfire::bad_argument(&flags::SORT, &os("colour"), super::SORTS)));
 
         // Overriding
-        test!(overridden:    SortField <- ["--sort=cr",       "--sort", "mod"]     => Ok(SortField::ModifiedDate));
-        test!(overridden_2:  SortField <- ["--sort", "none",  "--sort=Extension"]  => Ok(SortField::Extension(SortCase::Insensitive)));
+        test!(overridden:    SortField <- ["--sort=cr",       "--sort", "mod"];     Last => Ok(SortField::ModifiedDate));
+        test!(overridden_2:  SortField <- ["--sort", "none",  "--sort=Extension"];  Last => Ok(SortField::Extension(SortCase::Insensitive)));
+        test!(overridden_3:  SortField <- ["--sort=cr",       "--sort", "mod"];     Complain => Err(Misfire::Duplicate(Flag::Long("sort"), Flag::Long("sort"))));
+        test!(overridden_4:  SortField <- ["--sort", "none",  "--sort=Extension"];  Complain => Err(Misfire::Duplicate(Flag::Long("sort"), Flag::Long("sort"))));
     }
 
 
     mod dot_filters {
         use super::*;
 
         // Default behaviour
-        test!(empty:      DotFilter <- []               => Ok(DotFilter::JustFiles));
+        test!(empty:      DotFilter <- [];               Both => Ok(DotFilter::JustFiles));
 
         // --all
-        test!(all:        DotFilter <- ["--all"]        => Ok(DotFilter::Dotfiles));
-        test!(all_all:    DotFilter <- ["--all", "-a"]  => Ok(DotFilter::DotfilesAndDots));
-        test!(all_all_2:  DotFilter <- ["-aa"]          => Ok(DotFilter::DotfilesAndDots));
+        test!(all:        DotFilter <- ["--all"];        Both => Ok(DotFilter::Dotfiles));
+        test!(all_all:    DotFilter <- ["--all", "-a"];  Both => Ok(DotFilter::DotfilesAndDots));
+        test!(all_all_2:  DotFilter <- ["-aa"];          Both => Ok(DotFilter::DotfilesAndDots));
+
+        test!(all_all_3:  DotFilter <- ["-aaa"];         Last => Ok(DotFilter::DotfilesAndDots));
+        test!(all_all_4:  DotFilter <- ["-aaa"];         Complain => Err(Misfire::Conflict(&flags::ALL, &flags::ALL)));
 
         // --all and --tree
-        test!(tree_a:     DotFilter <- ["-Ta"]          => Ok(DotFilter::Dotfiles));
-        test!(tree_aa:    DotFilter <- ["-Taa"]         => Err(Misfire::TreeAllAll));
+        test!(tree_a:     DotFilter <- ["-Ta"];          Both => Ok(DotFilter::Dotfiles));
+        test!(tree_aa:    DotFilter <- ["-Taa"];         Both => Err(Misfire::TreeAllAll));
+        test!(tree_aaa:   DotFilter <- ["-Taaa"];        Both => Err(Misfire::TreeAllAll));
     }
 
 
@@ -198,9 +216,15 @@ mod test {
         }
 
         // Various numbers of globs
-        test!(none:   IgnorePatterns <- []                             => Ok(IgnorePatterns::empty()));
-        test!(one:    IgnorePatterns <- ["--ignore-glob", "*.ogg"]     => Ok(IgnorePatterns::from_iter(vec![ pat("*.ogg") ])));
-        test!(two:    IgnorePatterns <- ["--ignore-glob=*.ogg|*.MP3"]  => Ok(IgnorePatterns::from_iter(vec![ pat("*.ogg"), pat("*.MP3") ])));
-        test!(loads:  IgnorePatterns <- ["-I*|?|.|*"]                  => Ok(IgnorePatterns::from_iter(vec![ pat("*"), pat("?"), pat("."), pat("*") ])));
+        test!(none:   IgnorePatterns <- [];                             Both => Ok(IgnorePatterns::empty()));
+        test!(one:    IgnorePatterns <- ["--ignore-glob", "*.ogg"];     Both => Ok(IgnorePatterns::from_iter(vec![ pat("*.ogg") ])));
+        test!(two:    IgnorePatterns <- ["--ignore-glob=*.ogg|*.MP3"];  Both => Ok(IgnorePatterns::from_iter(vec![ pat("*.ogg"), pat("*.MP3") ])));
+        test!(loads:  IgnorePatterns <- ["-I*|?|.|*"];                  Both => Ok(IgnorePatterns::from_iter(vec![ pat("*"), pat("?"), pat("."), pat("*") ])));
+
+        // Overriding
+        test!(overridden:   IgnorePatterns <- ["-I=*.ogg",    "-I", "*.mp3"];  Last => Ok(IgnorePatterns::from_iter(vec![ pat("*.mp3") ])));
+        test!(overridden_2: IgnorePatterns <- ["-I", "*.OGG", "-I*.MP3"];      Last => Ok(IgnorePatterns::from_iter(vec![ pat("*.MP3") ])));
+        test!(overridden_3: IgnorePatterns <- ["-I=*.ogg",    "-I", "*.mp3"];  Complain => Err(Misfire::Duplicate(Flag::Short(b'I'), Flag::Short(b'I'))));
+        test!(overridden_4: IgnorePatterns <- ["-I", "*.OGG", "-I*.MP3"];      Complain => Err(Misfire::Duplicate(Flag::Short(b'I'), Flag::Short(b'I'))));
     }
 }

src/options/help.rs

@@ -72,9 +72,13 @@ impl HelpString {
     /// Determines how to show help, if at all, based on the user’s
     /// command-line arguments. This one works backwards from the other
     /// ‘deduce’ functions, returning Err if help needs to be shown.
+    ///
+    /// We don’t do any strict-mode error checking here: it’s OK to give
+    /// the --help or --long flags more than once. Actually checking for
+    /// errors when the user wants help is kind of petty!
     pub fn deduce(matches: &MatchedFlags) -> Result<(), HelpString> {
-        if matches.has(&flags::HELP) {
-            let only_long = matches.has(&flags::LONG);
+        if matches.count(&flags::HELP) > 0 {
+            let only_long = matches.count(&flags::LONG) > 0;
             let git       = cfg!(feature="git");
             let xattrs    = xattr::ENABLED;
             Err(HelpString { only_long, git, xattrs })
@@ -126,21 +130,21 @@ mod test {
     #[test]
     fn help() {
         let args = [ os("--help") ];
-        let opts = Options::getopts(&args);
+        let opts = Options::parse(&args, None);
         assert!(opts.is_err())
     }
 
     #[test]
     fn help_with_file() {
         let args = [ os("--help"), os("me") ];
-        let opts = Options::getopts(&args);
+        let opts = Options::parse(&args, None);
         assert!(opts.is_err())
     }
 
     #[test]
     fn unhelpful() {
         let args = [];
-        let opts = Options::getopts(&args);
+        let opts = Options::parse(&args, None);
         assert!(opts.is_ok())  // no help when --help isn’t passed
     }
 }