From 23d44fcf2989560816e5d8be98d042bdf6b63cc8 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Thu, 5 Jul 2018 01:44:35 +0300 Subject: [PATCH 1/6] macronize repetitive tests in exec/input --- src/exec/input.rs | 98 +++++++++++++---------------------------------- 1 file changed, 27 insertions(+), 71 deletions(-) diff --git a/src/exec/input.rs b/src/exec/input.rs index 6a3710e7f..b465799f3 100644 --- a/src/exec/input.rs +++ b/src/exec/input.rs @@ -71,90 +71,46 @@ pub fn dirname(path: &str) -> &str { } #[cfg(test)] -mod tests { +mod path_tests { use super::*; fn correct(input: &str) -> String { input.replace('/', &MAIN_SEPARATOR.to_string()) } - #[test] - fn path_remove_ext_simple() { - assert_eq!(remove_extension("foo.txt"), "foo"); - } - - #[test] - fn path_remove_ext_dir() { - assert_eq!( - remove_extension(&correct("dir/foo.txt")), - correct("dir/foo") - ); - } - - #[test] - fn path_hidden() { - assert_eq!(remove_extension(".foo"), ".foo") - } - - #[test] - fn path_remove_ext_utf8() { - assert_eq!(remove_extension("💖.txt"), "💖"); - } - - #[test] - fn path_remove_ext_empty() { - assert_eq!(remove_extension(""), ""); - } - - #[test] - fn path_basename_simple() { - assert_eq!(basename("foo.txt"), "foo.txt"); - } - - #[test] - fn path_basename_no_ext() { - assert_eq!(remove_extension(basename("foo.txt")), "foo"); - } - - #[test] - fn path_basename_dir() { - assert_eq!(basename(&correct("dir/foo.txt")), "foo.txt"); - } - - #[test] - fn path_basename_empty() { - assert_eq!(basename(""), ""); - } - - #[test] - fn path_basename_utf8() { - assert_eq!(basename(&correct("💖/foo.txt")), "foo.txt"); - assert_eq!(basename(&correct("dir/💖.txt")), "💖.txt"); + macro_rules! func_tests { + ($($name:ident: $func:ident for $input:expr => $output:expr)+) => { + $( + #[test] + fn $name() { + assert_eq!($func(&correct($input)), correct($output)); + } + )+ + } } - #[test] - fn path_dirname_simple() { - assert_eq!(dirname("foo.txt"), "."); - } + func_tests! { + remove_ext_simple: remove_extension for "foo.txt" => "foo" + remove_ext_dir: remove_extension for "dir/foo.txt" => "dir/foo" + hidden: remove_extension for ".foo" => ".foo" + remove_ext_utf8: remove_extension for "💖.txt" => "💖" + remove_ext_empty: remove_extension for "" => "" - #[test] - fn path_dirname_dir() { - assert_eq!(dirname(&correct("dir/foo.txt")), "dir"); - } + basename_simple: basename for "foo.txt" => "foo.txt" + basename_dir: basename for "dir/foo.txt" => "foo.txt" + basename_empty: basename for "" => "" + basename_utf8_0: basename for "💖/foo.txt" => "foo.txt" + basename_utf8_1: basename for "dir/💖.txt" => "💖.txt" - #[test] - fn path_dirname_utf8() { - assert_eq!(dirname(&correct("💖/foo.txt")), "💖"); - assert_eq!(dirname(&correct("dir/💖.txt")), "dir"); - } - - #[test] - fn path_dirname_empty() { - assert_eq!(dirname(""), "."); + dirname_simple: dirname for "foo.txt" => "." + dirname_dir: dirname for "dir/foo.txt" => "dir" + dirname_utf8_0: dirname for "💖/foo.txt" => "💖" + dirname_utf8_1: dirname for "dir/💖.txt" => "dir" + dirname_empty: dirname for "" => "." } #[test] - fn path_dirname_root() { + fn dirname_root() { #[cfg(windows)] assert_eq!(dirname("C:\\"), "C:"); #[cfg(windows)] From a05312e4c1d899d0437f6edba0b0e2f36873ff87 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 26 Jan 2019 02:01:01 +0200 Subject: [PATCH 2/6] split spawn_receiver(..) and spawn_senders(..) from scan(..). This is just a split commit, refraining from renaming too much. The drop(tx) call is no longer necessary, as the first sender is dropped at the end of spawn_senders(..) --- src/walk.rs | 55 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 38 insertions(+), 17 deletions(-) diff --git a/src/walk.rs b/src/walk.rs index cda2e9db6..812c69707 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -17,7 +17,7 @@ use std::io; use std::path::PathBuf; use std::process; use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::mpsc::channel; +use std::sync::mpsc::{channel, Receiver, Sender}; use std::sync::{Arc, Mutex}; use std::thread; use std::time; @@ -54,7 +54,6 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { .expect("Error: Path vector can not be empty"); let (tx, rx) = channel(); let threads = config.threads; - let show_filesystem_errors = config.show_filesystem_errors; let mut override_builder = OverrideBuilder::new(first_path_buf.as_path()); @@ -121,8 +120,34 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { } // Spawn the thread that receives all results through the channel. - let rx_config = Arc::clone(&config); - let receiver_thread = thread::spawn(move || { + let receiver_thread = spawn_receiver(Arc::clone(&config), receiver_wtq, rx); + + // Spawn the sender threads. + spawn_senders( + Arc::clone(&config), + pattern, + sender_wtq, + parallel_walker, + tx, + ); + + // Wait for the receiver thread to print out all results. + receiver_thread.join().unwrap(); + + if wants_to_quit.load(Ordering::Relaxed) { + process::exit(ExitCode::KilledBySigint.into()); + } +} + +fn spawn_receiver( + rx_config: Arc, + receiver_wtq: Arc, + rx: Receiver, +) -> thread::JoinHandle<()> { + let show_filesystem_errors = rx_config.show_filesystem_errors; + let threads = rx_config.threads; + + thread::spawn(move || { // This will be set to `Some` if the `--exec` argument was supplied. if let Some(ref cmd) = rx_config.command { if cmd.in_batch_mode() { @@ -222,9 +247,16 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { } } } - }); + }) +} - // Spawn the sender threads. +fn spawn_senders( + config: Arc, + pattern: Arc, + sender_wtq: Arc, + parallel_walker: ignore::WalkParallel, + tx: Sender, +) { parallel_walker.run(|| { let config = Arc::clone(&config); let pattern = Arc::clone(&pattern); @@ -349,15 +381,4 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { ignore::WalkState::Continue }) }); - - // Drop the initial sender. If we don't do this, the receiver will block even - // if all threads have finished, since there is still one sender around. - drop(tx); - - // Wait for the receiver thread to print out all results. - receiver_thread.join().unwrap(); - - if wants_to_quit.load(Ordering::Relaxed) { - process::exit(ExitCode::KilledBySigint.into()); - } } From 210add81e87df3dc9df15c498be26d46d582fe41 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 26 Jan 2019 02:12:55 +0200 Subject: [PATCH 3/6] Uniform names for config and wants_to_quit. Pass Arc's by ref. --- src/walk.rs | 45 ++++++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 25 deletions(-) diff --git a/src/walk.rs b/src/walk.rs index 812c69707..cb561a7fa 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -109,10 +109,8 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { let parallel_walker = walker.threads(threads).build_parallel(); let wants_to_quit = Arc::new(AtomicBool::new(false)); - let receiver_wtq = Arc::clone(&wants_to_quit); - let sender_wtq = Arc::clone(&wants_to_quit); if config.ls_colors.is_some() && config.command.is_none() { - let wq = Arc::clone(&receiver_wtq); + let wq = Arc::clone(&wants_to_quit); ctrlc::set_handler(move || { wq.store(true, Ordering::Relaxed); }) @@ -120,16 +118,10 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { } // Spawn the thread that receives all results through the channel. - let receiver_thread = spawn_receiver(Arc::clone(&config), receiver_wtq, rx); + let receiver_thread = spawn_receiver(&config, &wants_to_quit, rx); // Spawn the sender threads. - spawn_senders( - Arc::clone(&config), - pattern, - sender_wtq, - parallel_walker, - tx, - ); + spawn_senders(&config, &wants_to_quit, pattern, parallel_walker, tx); // Wait for the receiver thread to print out all results. receiver_thread.join().unwrap(); @@ -140,16 +132,19 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { } fn spawn_receiver( - rx_config: Arc, - receiver_wtq: Arc, + config: &Arc, + wants_to_quit: &Arc, rx: Receiver, ) -> thread::JoinHandle<()> { - let show_filesystem_errors = rx_config.show_filesystem_errors; - let threads = rx_config.threads; + let config = Arc::clone(config); + let wants_to_quit = Arc::clone(wants_to_quit); + + let show_filesystem_errors = config.show_filesystem_errors; + let threads = config.threads; thread::spawn(move || { // This will be set to `Some` if the `--exec` argument was supplied. - if let Some(ref cmd) = rx_config.command { + if let Some(ref cmd) = config.command { if cmd.in_batch_mode() { exec::batch(rx, cmd, show_filesystem_errors); } else { @@ -192,7 +187,7 @@ fn spawn_receiver( let mut mode = ReceiverMode::Buffering; // Maximum time to wait before we start streaming to the console. - let max_buffer_time = rx_config + let max_buffer_time = config .max_buffer_time .unwrap_or_else(|| time::Duration::from_millis(100)); @@ -215,8 +210,8 @@ fn spawn_receiver( output::print_entry( &mut stdout, v, - &rx_config, - &receiver_wtq, + &config, + &wants_to_quit, ); } buffer.clear(); @@ -226,7 +221,7 @@ fn spawn_receiver( } } ReceiverMode::Streaming => { - output::print_entry(&mut stdout, &value, &rx_config, &receiver_wtq); + output::print_entry(&mut stdout, &value, &config, &wants_to_quit); } } } @@ -243,7 +238,7 @@ fn spawn_receiver( if !buffer.is_empty() { buffer.sort(); for value in buffer { - output::print_entry(&mut stdout, &value, &rx_config, &receiver_wtq); + output::print_entry(&mut stdout, &value, &config, &wants_to_quit); } } } @@ -251,17 +246,17 @@ fn spawn_receiver( } fn spawn_senders( - config: Arc, + config: &Arc, + wants_to_quit: &Arc, pattern: Arc, - sender_wtq: Arc, parallel_walker: ignore::WalkParallel, tx: Sender, ) { parallel_walker.run(|| { - let config = Arc::clone(&config); + let config = Arc::clone(config); let pattern = Arc::clone(&pattern); let tx_thread = tx.clone(); - let wants_to_quit = Arc::clone(&sender_wtq); + let wants_to_quit = Arc::clone(wants_to_quit); Box::new(move |entry_o| { if wants_to_quit.load(Ordering::Relaxed) { From eee02046da850a5b03ced980e100596c67e2f0c0 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 26 Jan 2019 02:16:53 +0200 Subject: [PATCH 4/6] inline value used only once --- src/walk.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/walk.rs b/src/walk.rs index cb561a7fa..de7ca377d 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -53,7 +53,6 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { .next() .expect("Error: Path vector can not be empty"); let (tx, rx) = channel(); - let threads = config.threads; let mut override_builder = OverrideBuilder::new(first_path_buf.as_path()); @@ -106,7 +105,7 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { walker.add(path_entry.as_path()); } - let parallel_walker = walker.threads(threads).build_parallel(); + let parallel_walker = walker.threads(config.threads).build_parallel(); let wants_to_quit = Arc::new(AtomicBool::new(false)); if config.ls_colors.is_some() && config.command.is_none() { From fbeff96a37363be475049f56e4c527e9442b13b4 Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 26 Jan 2019 02:22:06 +0200 Subject: [PATCH 5/6] save one indent level in error handling for add_ignore --- src/walk.rs | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/walk.rs b/src/walk.rs index de7ca377d..3a735f646 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -84,20 +84,19 @@ pub fn scan(path_vec: &[PathBuf], pattern: Arc, config: Arc) { for ignore_file in &config.ignore_files { let result = walker.add_ignore(ignore_file); - if let Some(err) = result { - match err { - ignore::Error::Partial(_) => (), - _ => { - print_error!( - "{}", - format!( - "Malformed pattern in custom ignore file '{}': {}.", - ignore_file.to_string_lossy(), - err.description() - ) - ); - } + match result { + Some(ignore::Error::Partial(_)) => (), + Some(err) => { + print_error!( + "{}", + format!( + "Malformed pattern in custom ignore file '{}': {}.", + ignore_file.to_string_lossy(), + err.description() + ) + ); } + None => (), } } From eb50c53aad051242fe24d6a86512d191bfa01e2b Mon Sep 17 00:00:00 2001 From: Alexandru Macovei Date: Sat, 26 Jan 2019 03:13:16 +0200 Subject: [PATCH 6/6] fix most clippy lints --- src/app.rs | 2 +- src/internal/filter/size.rs | 8 ++++---- src/main.rs | 2 +- src/output.rs | 6 +++--- src/walk.rs | 4 ++-- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/app.rs b/src/app.rs index ef3481483..950ec1ef6 100644 --- a/src/app.rs +++ b/src/app.rs @@ -219,7 +219,7 @@ pub fn build_app() -> App<'static, 'static> { ) } -#[cfg_attr(rustfmt, rustfmt_skip)] +#[rustfmt::skip] fn usage() -> HashMap<&'static str, Help> { let mut h = HashMap::new(); doc!(h, "hidden" diff --git a/src/internal/filter/size.rs b/src/internal/filter/size.rs index b534f8d3e..fafd4b518 100644 --- a/src/internal/filter/size.rs +++ b/src/internal/filter/size.rs @@ -24,7 +24,7 @@ const GIBI: u64 = MEBI * 1024; const TEBI: u64 = GIBI * 1024; impl SizeFilter { - pub fn from_string<'a>(s: &str) -> Option { + pub fn from_string(s: &str) -> Option { if !SIZE_CAPTURES.is_match(s) { return None; } @@ -56,9 +56,9 @@ impl SizeFilter { } pub fn is_within(&self, size: u64) -> bool { - match self { - &SizeFilter::Max(limit) => size <= limit, - &SizeFilter::Min(limit) => size >= limit, + match *self { + SizeFilter::Max(limit) => size <= limit, + SizeFilter::Min(limit) => size >= limit, } } } diff --git a/src/main.rs b/src/main.rs index 571bdc64e..8fad65b75 100644 --- a/src/main.rs +++ b/src/main.rs @@ -49,7 +49,7 @@ fn main() { // Get one or more root directories to search. let mut dir_vec: Vec<_> = match matches .values_of("path") - .or(matches.values_of("search-path")) + .or_else(|| matches.values_of("search-path")) { Some(paths) => paths .map(|path| { diff --git a/src/output.rs b/src/output.rs index ed62f3c80..a5d0c1e49 100644 --- a/src/output.rs +++ b/src/output.rs @@ -19,7 +19,7 @@ use std::sync::Arc; use ansi_term; /// Remove the `./` prefix from a path. -fn strip_current_dir<'a>(pathbuf: &'a PathBuf) -> &'a Path { +fn strip_current_dir(pathbuf: &PathBuf) -> &Path { let mut iter = pathbuf.components(); let mut iter_next = iter.clone(); if iter_next.next() == Some(Component::CurDir) { @@ -70,7 +70,7 @@ fn print_entry_colorized( write!(stdout, "{}", style.paint(component.to_string_lossy()))?; if wants_to_quit.load(Ordering::Relaxed) { - write!(stdout, "\n")?; + writeln!(stdout)?; process::exit(ExitCode::KilledBySigint.into()); } } @@ -78,7 +78,7 @@ fn print_entry_colorized( if config.null_separator { write!(stdout, "\0") } else { - writeln!(stdout, "") + writeln!(stdout) } } diff --git a/src/walk.rs b/src/walk.rs index 3a735f646..f085bda03 100644 --- a/src/walk.rs +++ b/src/walk.rs @@ -304,7 +304,7 @@ fn spawn_senders( // Filter out unwanted extensions. if let Some(ref exts_regex) = config.extensions { - if let Some(path_str) = entry_path.file_name().map_or(None, |s| s.to_str()) { + if let Some(path_str) = entry_path.file_name().and_then(|s| s.to_str()) { if !exts_regex.is_match(path_str) { return ignore::WalkState::Continue; } @@ -314,7 +314,7 @@ fn spawn_senders( } // Filter out unwanted sizes if it is a file and we have been given size constraints. - if config.size_constraints.len() > 0 { + if !config.size_constraints.is_empty() { if entry_path.is_file() { if let Ok(metadata) = entry_path.metadata() { let file_size = metadata.len();