Skip to content

ls: C sort order and fix subdirectory listing #2161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/uu/ls/BENCHMARKING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
41 changes: 24 additions & 17 deletions src/uu/ls/src/ls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1122,14 +1122,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
.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,
Expand Down Expand Up @@ -1192,7 +1199,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()
Expand Down Expand Up @@ -1237,14 +1244,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| {
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()
Expand Down Expand Up @@ -1283,8 +1284,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(dir.p_buf.join(".."), None, 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 {
vec![]
Expand All @@ -1293,7 +1300,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);
Expand Down
72 changes: 61 additions & 11 deletions tests/by-util/test_ls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 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()
.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);

scene
.ucmd()
.arg("-A")
.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);

let result = scene.ucmd().arg("-A").succeeds();
result.stdout_contains(".test-1");
let stdout = result.stdout_str();
assert!(!stdout.contains(".."));
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]
Expand Down Expand Up @@ -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");
Expand All @@ -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]
Expand Down