From 28c7800f73ec2712a296fd930a9e7706efa6a9bd Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sun, 2 May 2021 10:03:01 +0200 Subject: [PATCH 1/4] ls: fix subdirectory name --- src/uu/ls/src/ls.rs | 25 ++++++++++------ tests/by-util/test_ls.rs | 63 ++++++++++++++++++++++++++++++++++------ 2 files changed, 70 insertions(+), 18 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index dc7ea4c08..4ebcc479c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1122,14 +1122,21 @@ impl PathData { fn new( p_buf: PathBuf, file_type: Option>, + file_name: Option, config: &Config, command_line: bool, ) -> Self { - let name = p_buf - .file_name() - .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) - .to_string_lossy() - .into_owned(); + // We cannot use `Path::ends_with` or `Path::Components`, because they remove occurrences of '.' + // For '..', the filename is None + let name = if let Some(name) = file_name { + name + } else { + p_buf + .file_name() + .unwrap_or_else(|| p_buf.iter().next_back().unwrap()) + .to_string_lossy() + .into_owned() + }; let must_dereference = match &config.dereference { Dereference::All => true, Dereference::Args => command_line, @@ -1192,7 +1199,7 @@ fn list(locs: Vec, config: Config) -> i32 { continue; } - let path_data = PathData::new(p, None, &config, true); + let path_data = PathData::new(p, None, None, &config, true); let show_dir_contents = if let Some(ft) = path_data.file_type() { !config.directory && ft.is_dir() @@ -1283,8 +1290,8 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) { let mut entries: Vec<_> = if config.files == Files::All { vec![ - PathData::new(dir.p_buf.join("."), None, config, false), - PathData::new(dir.p_buf.join(".."), None, config, false), + PathData::new(dir.p_buf.join("."), None, Some(".".into()), config, false), + PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false), ] } else { vec![] @@ -1293,7 +1300,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) let mut temp: Vec<_> = safe_unwrap!(fs::read_dir(&dir.p_buf)) .map(|res| safe_unwrap!(res)) .filter(|e| should_display(e, config)) - .map(|e| PathData::new(DirEntry::path(&e), Some(e.file_type()), config, false)) + .map(|e| PathData::new(DirEntry::path(&e), Some(e.file_type()), None, config, false)) .collect(); sort_entries(&mut temp, config); diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 2b8f311d1..0331d0214 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -43,23 +43,68 @@ fn test_ls_a() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; at.touch(".test-1"); + at.mkdir("some-dir"); + at.touch( + Path::new("some-dir") + .join(".test-2") + .as_os_str() + .to_str() + .unwrap(), + ); - let result = scene.ucmd().succeeds(); - let stdout = result.stdout_str(); - assert!(!stdout.contains(".test-1")); - assert!(!stdout.contains("..")); + let re_pwd = Regex::new(r"^\.\n").unwrap(); + + // Using the present working directory + scene + .ucmd() + .succeeds() + .stdout_does_not_contain(".test-1") + .stdout_does_not_contain("..") + .stdout_does_not_match(&re_pwd); scene .ucmd() .arg("-a") .succeeds() .stdout_contains(&".test-1") - .stdout_contains(&".."); + .stdout_contains(&"..") + .stdout_matches(&re_pwd); - let result = scene.ucmd().arg("-A").succeeds(); - result.stdout_contains(".test-1"); - let stdout = result.stdout_str(); - assert!(!stdout.contains("..")); + scene + .ucmd() + .arg("-A") + .succeeds() + .stdout_contains(".test-1") + .stdout_does_not_contain("..") + .stdout_does_not_match(&re_pwd); + + // Using a subdirectory + scene + .ucmd() + .arg("some-dir") + .succeeds() + .stdout_does_not_contain(".test-2") + .stdout_does_not_contain("..") + .stdout_does_not_match(&re_pwd); + + scene + .ucmd() + .arg("-a") + .arg("some-dir") + .succeeds() + .stdout_contains(&".test-2") + .stdout_contains(&"..") + .no_stderr() + .stdout_matches(&re_pwd); + + scene + .ucmd() + .arg("-A") + .arg("some-dir") + .succeeds() + .stdout_contains(".test-2") + .stdout_does_not_contain("..") + .stdout_does_not_match(&re_pwd); } #[test] From 361408cbe53e37e5b42a4d2b496cb03b2334f406 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sun, 2 May 2021 10:04:11 +0200 Subject: [PATCH 2/4] ls: remove case-insensitivity and leading period of name sort --- src/uu/ls/BENCHMARKING.md | 2 ++ src/uu/ls/src/ls.rs | 10 ++-------- tests/by-util/test_ls.rs | 3 +-- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/src/uu/ls/BENCHMARKING.md b/src/uu/ls/BENCHMARKING.md index 852220496..b009b703a 100644 --- a/src/uu/ls/BENCHMARKING.md +++ b/src/uu/ls/BENCHMARKING.md @@ -36,6 +36,8 @@ args="$@" hyperfine "ls $args" "target/release/coreutils ls $args" ``` +**Note**: No localization is currently implemented. This means that the comparison above is not really fair. We can fix this by setting `LC_ALL=C`, so GNU `ls` can ignore localization. + ## Checking system call count - Another thing to look at would be system calls count using strace (on linux) or equivalent on other operating systems. diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 4ebcc479c..2e8a5e5ed 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1244,14 +1244,8 @@ fn sort_entries(entries: &mut Vec, config: &Config) { entries.sort_by_key(|k| Reverse(k.md().as_ref().map(|md| md.len()).unwrap_or(0))) } // The default sort in GNU ls is case insensitive - Sort::Name => entries.sort_by_cached_key(|k| { - let has_dot: bool = k.file_name.starts_with('.'); - let filename_nodot: &str = &k.file_name[if has_dot { 1 } else { 0 }..]; - // We want hidden files to appear before regular files of the same - // name, so we need to negate the "has_dot" variable. - (filename_nodot.to_lowercase(), !has_dot) - }), - Sort::Version => entries.sort_by(|k, j| version_cmp::version_cmp(&k.p_buf, &j.p_buf)), + Sort::Name => entries.sort_by(|a, b| a.file_name.cmp(&b.file_name)), + Sort::Version => entries.sort_by(|a, b| version_cmp::version_cmp(&a.p_buf, &b.p_buf)), Sort::Extension => entries.sort_by(|a, b| { a.p_buf .extension() diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 0331d0214..4be24a99a 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -527,7 +527,6 @@ fn test_ls_sort_name() { .succeeds() .stdout_is(["test-1", "test-2", "test-3\n"].join(sep)); - // Order of a named sort ignores leading dots. let scene_dot = TestScenario::new(util_name!()); let at = &scene_dot.fixtures; at.touch(".a"); @@ -540,7 +539,7 @@ fn test_ls_sort_name() { .arg("--sort=name") .arg("-A") .succeeds() - .stdout_is([".a", "a", ".b", "b\n"].join(sep)); + .stdout_is([".a", ".b", "a", "b\n"].join(sep)); } #[test] From eb3206737b61dab810e7c115ac3fbd077f348702 Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sun, 2 May 2021 10:20:14 +0200 Subject: [PATCH 3/4] ls: give '.' a file_type --- src/uu/ls/src/ls.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 2e8a5e5ed..482648e79 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1284,7 +1284,13 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool { fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter) { let mut entries: Vec<_> = if config.files == Files::All { vec![ - PathData::new(dir.p_buf.join("."), None, Some(".".into()), config, false), + PathData::new( + dir.p_buf.clone(), + Some(Ok(*dir.file_type().unwrap())), + Some(".".into()), + config, + false, + ), PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false), ] } else { From fba245b176d47b9e9d25975dd87f5d4babfaf5dd Mon Sep 17 00:00:00 2001 From: Terts Diepraam Date: Sun, 2 May 2021 14:26:23 +0200 Subject: [PATCH 4/4] ls: add -1 to tests to separate names with \n on windows --- tests/by-util/test_ls.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 4be24a99a..65dd51c8b 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -57,6 +57,7 @@ fn test_ls_a() { // Using the present working directory scene .ucmd() + .arg("-1") .succeeds() .stdout_does_not_contain(".test-1") .stdout_does_not_contain("..") @@ -65,6 +66,7 @@ fn test_ls_a() { scene .ucmd() .arg("-a") + .arg("-1") .succeeds() .stdout_contains(&".test-1") .stdout_contains(&"..") @@ -73,6 +75,7 @@ fn test_ls_a() { scene .ucmd() .arg("-A") + .arg("-1") .succeeds() .stdout_contains(".test-1") .stdout_does_not_contain("..") @@ -81,6 +84,7 @@ fn test_ls_a() { // Using a subdirectory scene .ucmd() + .arg("-1") .arg("some-dir") .succeeds() .stdout_does_not_contain(".test-2") @@ -90,6 +94,7 @@ fn test_ls_a() { scene .ucmd() .arg("-a") + .arg("-1") .arg("some-dir") .succeeds() .stdout_contains(&".test-2") @@ -100,6 +105,7 @@ fn test_ls_a() { scene .ucmd() .arg("-A") + .arg("-1") .arg("some-dir") .succeeds() .stdout_contains(".test-2")