From 5535478fe8bf157e430a7f7dcce684216cd27ba6 Mon Sep 17 00:00:00 2001 From: "andy.boot" Date: Wed, 4 Dec 2019 21:58:29 +0000 Subject: [PATCH] Fix substring bug When one directory was a substring of another the directory would appear as a subdirectory instead of a sibling Fixed originally by this: 6e03dd77e61d7a479e653dc5e5031372f05fe0c8 Broken by this: db6c8a019d1bf76a99e7c8b46c513538a52b9efe Added integration test as this has bitten me before --- src/main.rs | 7 +++-- src/test_dir2/dir/hello | 1 + src/test_dir2/dir_name_clash | 1 + src/test_dir2/dir_substring/hello | 1 + src/tests.rs | 46 +++++++++++++++++++++++++-- src/utils/mod.rs | 52 +++++++++++++++++++++++-------- 6 files changed, 90 insertions(+), 18 deletions(-) create mode 100644 src/test_dir2/dir/hello create mode 100644 src/test_dir2/dir_name_clash create mode 100644 src/test_dir2/dir_substring/hello diff --git a/src/main.rs b/src/main.rs index fb28148..29c4642 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,6 +2,7 @@ extern crate clap; use self::display::draw_it; +use crate::utils::is_a_parent_of; use clap::{App, AppSettings, Arg}; use utils::{find_big_ones, get_dir_tree, simplify_dir_names, sort, trim_deep_ones, Node}; @@ -106,7 +107,6 @@ fn main() { } }; let tree = build_tree(biggest_ones, depth); - //println!("{:?}", tree); draw_it( permissions, @@ -126,7 +126,8 @@ fn build_tree(biggest_ones: Vec<(String, u64)>, depth: Option) -> Node { size: b.1, children: Vec::default(), }; - recursively_build_tree(&mut top_parent, n, depth) + recursively_build_tree(&mut top_parent, n, depth); + top_parent.children.sort_unstable() } top_parent } @@ -140,7 +141,7 @@ fn recursively_build_tree(parent_node: &mut Node, new_node: Node, depth: Option< if let Some(c) = parent_node .children .iter_mut() - .find(|c| new_node.name.starts_with(&c.name)) + .find(|c| is_a_parent_of(&c.name, &new_node.name)) { recursively_build_tree(c, new_node, new_depth); } else { diff --git a/src/test_dir2/dir/hello b/src/test_dir2/dir/hello new file mode 100644 index 0000000..b6fc4c6 --- /dev/null +++ b/src/test_dir2/dir/hello @@ -0,0 +1 @@ +hello \ No newline at end of file diff --git a/src/test_dir2/dir_name_clash b/src/test_dir2/dir_name_clash new file mode 100644 index 0000000..b6fc4c6 --- /dev/null +++ b/src/test_dir2/dir_name_clash @@ -0,0 +1 @@ +hello \ No newline at end of file diff --git a/src/test_dir2/dir_substring/hello b/src/test_dir2/dir_substring/hello new file mode 100644 index 0000000..ce01362 --- /dev/null +++ b/src/test_dir2/dir_substring/hello @@ -0,0 +1 @@ +hello diff --git a/src/tests.rs b/src/tests.rs index 12a7463..53c1c36 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -138,7 +138,7 @@ pub fn test_d_flag_works() { .unwrap(); } -fn build_temp_file(dir: &TempDir) -> (PathBuf) { +fn build_temp_file(dir: &TempDir) -> PathBuf { let file_path = dir.path().join("notes.txt"); let mut file = File::create(&file_path).unwrap(); writeln!(file, "I am a temp file").unwrap(); @@ -164,7 +164,7 @@ pub fn test_soft_sym_link() { let r = soft_sym_link_output(dir_s, file_path_s, link_name_s); // We cannot guarantee which version will appear first. - // TODO: Consider adding predictable itteration order (sort file entries by name?) + // TODO: Consider adding predictable iteration order (sort file entries by name?) assert_cli::Assert::main_binary() .with_args(&[dir_s]) .stdout() @@ -308,3 +308,45 @@ fn recursive_sym_link_output(dir: &str, link_name: &str) -> String { format_string(link_name, true, true, " 0B", " └──",), ) } + +// Check against directories and files whos names are substrings of each other +#[test] +#[cfg(target_os = "macos")] +pub fn test_substring_of_names() { + assert_cli::Assert::main_binary() + .with_args(&["src/test_dir2"]) + .stdout() + .contains(" ─┬ test_dir2") + .stdout() + .contains(" ├─┬ dir") + .stdout() + .contains(" │ └── hello") + .stdout() + .contains(" ├── dir_name_clash") + .stdout() + .contains(" └─┬ dir_substring") + .stdout() + .contains(" └── hello") + .unwrap(); +} + +// Check against directories and files whos names are substrings of each other +#[test] +#[cfg(target_os = "linux")] +pub fn test_substring_of_names() { + assert_cli::Assert::main_binary() + .with_args(&["src/test_dir2"]) + .stdout() + .contains(" ─┬ test_dir2") + .stdout() + .contains(" ├─┬ dir") + .stdout() + .contains(" │ └── hello") + .stdout() + .contains(" ├─┬ dir_substring") + .stdout() + .contains(" │ └── hello") + .stdout() + .contains(" └── dir_name_clash") + .unwrap(); +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 6774b25..cfe84ce 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -7,26 +7,52 @@ use jwalk::WalkDir; mod platform; use self::platform::*; -#[derive(Debug, Default)] +#[derive(Debug, Default, Eq)] pub struct Node { pub name: String, pub size: u64, pub children: Vec, } +impl Ord for Node { + fn cmp(&self, other: &Self) -> Ordering { + if self.size == other.size { + self.name.cmp(&other.name) + } else { + self.size.cmp(&other.size) + } + } +} + +impl PartialOrd for Node { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl PartialEq for Node { + fn eq(&self, other: &Self) -> bool { + self.name == other.name && self.size == other.size && self.children == other.children + } +} + +pub fn is_a_parent_of(parent: &str, child: &str) -> bool { + child.starts_with(parent) && child.chars().nth(parent.chars().count()) == Some('/') +} + pub fn simplify_dir_names(filenames: Vec<&str>) -> HashSet { let mut top_level_names: HashSet = HashSet::with_capacity(filenames.len()); let mut to_remove: Vec = Vec::with_capacity(filenames.len()); for t in filenames { - let top_level_name = ensure_end_slash(t); + let top_level_name = strip_end_slash(t); let mut can_add = true; for tt in top_level_names.iter() { - if top_level_name.starts_with(tt) { - can_add = false; - } else if tt.starts_with(&top_level_name) { + if is_a_parent_of(&top_level_name, tt) { to_remove.push(tt.to_string()); + } else if is_a_parent_of(tt, &top_level_name) { + can_add = false; } } to_remove.sort_unstable(); @@ -62,14 +88,6 @@ pub fn get_dir_tree( (permissions == 0, data) } -pub fn ensure_end_slash(s: &str) -> String { - let mut new_name = String::from(s); - while new_name.ends_with('/') || new_name.ends_with("/.") { - new_name.pop(); - } - new_name + "/" -} - pub fn strip_end_slash(mut new_name: &str) -> &str { while (new_name.ends_with('/') || new_name.ends_with("/.")) && new_name.len() > 1 { new_name = &new_name[..new_name.len() - 1]; @@ -206,4 +224,12 @@ mod tests { correct.insert("src".to_string()); assert_eq!(simplify_dir_names(vec!["src/."]), correct); } + + #[test] + fn test_simplify_dir_substring_names() { + let mut correct = HashSet::new(); + correct.insert("src".to_string()); + correct.insert("src_v2".to_string()); + assert_eq!(simplify_dir_names(vec!["src/", "src_v2"]), correct); + } }