Fix inconsistency in ls sort-order (#13875)

Fixes #13267 

As we can see from the bisect done in the comments.
Bisected to https://github.com/nushell/nushell/pull/12625 /
460a1c8f87

We can see that this update brought the use of `read_dir` and for it, it
is mentioned in the [rust
docs](https://doc.rust-lang.org/std/fs/fn.read_dir.html#platform-specific-behavior)
that it does **not** provide any specific order of files.
As was the advice there, I went and applied a manual `sort` to the
entries and tested it manually on my local machine.

If required I could probably try and add tests for the order
consistency, would need some time to find my way around them, so I'm
sending the PR first.
This commit is contained in:
Bark 2024-11-15 01:39:41 +02:00 committed by GitHub
parent 636bae2466
commit a84d410f11
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 37 additions and 3 deletions

View File

@ -339,7 +339,7 @@ fn ls_for_one_pattern(
let path = pattern_arg.into_spanned(p_tag); let path = pattern_arg.into_spanned(p_tag);
let (prefix, paths) = if just_read_dir { let (prefix, paths) = if just_read_dir {
let expanded = nu_path::expand_path_with(path.item.as_ref(), &cwd, path.item.is_expand()); let expanded = nu_path::expand_path_with(path.item.as_ref(), &cwd, path.item.is_expand());
let paths = read_dir(&expanded)?; let paths = read_dir(&expanded, use_threads)?;
// just need to read the directory, so prefix is path itself. // just need to read the directory, so prefix is path itself.
(Some(expanded), paths) (Some(expanded), paths)
} else { } else {
@ -979,10 +979,20 @@ mod windows_helper {
#[allow(clippy::type_complexity)] #[allow(clippy::type_complexity)]
fn read_dir( fn read_dir(
f: &Path, f: &Path,
use_threads: bool,
) -> Result<Box<dyn Iterator<Item = Result<PathBuf, ShellError>> + Send>, ShellError> { ) -> Result<Box<dyn Iterator<Item = Result<PathBuf, ShellError>> + Send>, ShellError> {
let iter = f.read_dir()?.map(|d| { let items = f.read_dir()?.map(|d| {
d.map(|r| r.path()) d.map(|r| r.path())
.map_err(|e| ShellError::IOError { msg: e.to_string() }) .map_err(|e| ShellError::IOError { msg: e.to_string() })
}); });
Ok(Box::new(iter)) if !use_threads {
let mut collected = items.collect::<Vec<_>>();
collected.sort_by(|a, b| {
let a = a.as_ref().expect("path should be valid");
let b = b.as_ref().expect("path should be valid");
a.cmp(b)
});
return Ok(Box::new(collected.into_iter()));
}
Ok(Box::new(items))
} }

View File

@ -833,3 +833,27 @@ fn list_symlink_with_full_path() {
); );
}) })
} }
#[test]
fn consistent_list_order() {
Playground::setup("ls_test_order", |dirs, sandbox| {
sandbox.with_files(&[
EmptyFile("los.txt"),
EmptyFile("tres.txt"),
EmptyFile("amigos.txt"),
EmptyFile("arepas.clu"),
]);
let no_arg = nu!(
cwd: dirs.test(), pipeline(
"ls"
));
let with_arg = nu!(
cwd: dirs.test(), pipeline(
"ls ."
));
assert_eq!(no_arg.out, with_arg.out);
})
}