Merge pull request #2161 from tertsdiepraam/ls/sort_order_and_subdirectory_listing

`ls`: C sort order and fix subdirectory listing

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.

src/uu/ls/src/ls.rs

@@ -1120,14 +1120,21 @@ impl PathData {
     fn new(
         p_buf: PathBuf,
         file_type: Option<std::io::Result<FileType>>,
+        file_name: Option<String>,
         config: &Config,
         command_line: bool,
     ) -> Self {
-        let name = p_buf
+        // We cannot use `Path::ends_with` or `Path::Components`, because they remove occurrences of '.'
-            .file_name()
+        // For '..', the filename is None
-            .unwrap_or_else(|| p_buf.iter().next_back().unwrap())
+        let name = if let Some(name) = file_name {
-            .to_string_lossy()
+            name
-            .into_owned();
+        } 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,
@@ -1190,7 +1197,7 @@ fn list(locs: Vec<String>, 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()
@@ -1235,14 +1242,8 @@ fn sort_entries(entries: &mut Vec<PathData>, 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| {
+        Sort::Name => entries.sort_by(|a, b| a.file_name.cmp(&b.file_name)),
-            let has_dot: bool = k.file_name.starts_with('.');
+        Sort::Version => entries.sort_by(|a, b| version_cmp::version_cmp(&a.p_buf, &b.p_buf)),
-            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::Extension => entries.sort_by(|a, b| {
             a.p_buf
                 .extension()
@@ -1281,8 +1282,14 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool {
 fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>) {
     let mut entries: Vec<_> = if config.files == Files::All {
         vec![
-            PathData::new(dir.p_buf.join("."), None, config, false),
+            PathData::new(
-            PathData::new(dir.p_buf.join(".."), None, config, false),
+                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 {
         vec![]
@@ -1291,7 +1298,7 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>)
     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);

tests/by-util/test_ls.rs

@@ -43,23 +43,74 @@ 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 re_pwd = Regex::new(r"^\.\n").unwrap();
-    let stdout = result.stdout_str();
+
-    assert!(!stdout.contains(".test-1"));
+    // Using the present working directory
-    assert!(!stdout.contains(".."));
+    scene
+        .ucmd()
+        .arg("-1")
+        .succeeds()
+        .stdout_does_not_contain(".test-1")
+        .stdout_does_not_contain("..")
+        .stdout_does_not_match(&re_pwd);
 
     scene
         .ucmd()
         .arg("-a")
+        .arg("-1")
         .succeeds()
         .stdout_contains(&".test-1")
-        .stdout_contains(&"..");
+        .stdout_contains(&"..")
+        .stdout_matches(&re_pwd);
 
-    let result = scene.ucmd().arg("-A").succeeds();
+    scene
-    result.stdout_contains(".test-1");
+        .ucmd()
-    let stdout = result.stdout_str();
+        .arg("-A")
-    assert!(!stdout.contains(".."));
+        .arg("-1")
+        .succeeds()
+        .stdout_contains(".test-1")
+        .stdout_does_not_contain("..")
+        .stdout_does_not_match(&re_pwd);
+
+    // Using a subdirectory
+    scene
+        .ucmd()
+        .arg("-1")
+        .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("-1")
+        .arg("some-dir")
+        .succeeds()
+        .stdout_contains(&".test-2")
+        .stdout_contains(&"..")
+        .no_stderr()
+        .stdout_matches(&re_pwd);
+
+    scene
+        .ucmd()
+        .arg("-A")
+        .arg("-1")
+        .arg("some-dir")
+        .succeeds()
+        .stdout_contains(".test-2")
+        .stdout_does_not_contain("..")
+        .stdout_does_not_match(&re_pwd);
 }
 
 #[test]
@@ -482,7 +533,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");
@@ -495,7 +545,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]