`ls <dir>` no longer sorted by date

NotTheDr01ds commented 3 months ago

Describe the bug

This appears to have changed/broken in 0.94, but (once again), I left my long-running script running for too long under 0.93. When I skipped forward to 0.95, this broke my script, since it was trying to get the ls <dir> | last file from the directory. That filename was no longer the same as it had been in 0.93.

How to reproduce

> cd /
> ls
# sorted by filename
> ls (pwd)
# not sure what the sort-order is

Expected behavior

Sort ls output by filename, even when a directory path is specified.

Screenshots

No response

Configuration

key	value
version	0.95.0
major	0
minor	95
patch	0
branch
commit_hash
build_os	linux-x86_64
build_target	x86_64-unknown-linux-gnu
rust_version	rustc 1.77.2 (25ef9e3d8 2024-04-09)
rust_channel	1.77.2-x86_64-unknown-linux-gnu
cargo_version	cargo 1.77.2 (e52e36006 2024-03-26)
build_time	2024-06-28 06:28:34 -04:00
build_rust_channel	release
allocator	mimalloc
features	default, sqlite, system-clipboard, trash
installed_plugins

Additional context

No response

fdncred commented 3 months ago

A git bisect would be helpful to figure out what PR broke this functionality.

There is no sorting code in the ls.rs file so I wonder how it's getting sorted?

NotTheDr01ds commented 3 months ago

A git bisect would be helpful to figure out what PR broke this functionality.

Agreed - Just wanted to make sure I got the issue in since I didn't have enough time at that point to do the bisect.

Has anyone done a Nushell git bisect script that auto-runs based on assert's?

fdncred commented 3 months ago

I think @amtoine has a git bisect script. Maybe this? https://github.com/amtoine/nu-git-manager/blob/main/docs/nu-git-manager-sugar/git/gm-repo-bisect.md

NotTheDr01ds commented 3 months ago

Maybe this?

Excellent - That worked great.

Bisected to #12625 / https://github.com/nushell/nushell/commit/460a1c8f874b503cb4140619838be483600f00ce

cc: @WindSoilder

amtoine commented 3 months ago

i'm glad my script was useful to someone else than me :pray:

WindSoilder commented 3 months ago

Actually I don't think ls shold guarantee the order of output. It's sorted by date occasionally, maybe users shouldn't rely on it.

NotTheDr01ds commented 3 months ago

Actually I don't think ls shold guarantee the order of output. It's sorted by date occasionally, maybe users shouldn't rely on it.

I disagree. Historically (other shells/binaries) and AFAIK universally (other than Nushell 0.95), it is deterministic:

External ls on *nix:
```
> (^ls) == (^ls (pwd))
true
```
eza on *nix:
```
> (^eza) == (^eza (pwd))
true
```
lsd on *nix:
```
> (^lsd) == (^lsd (pwd))
true
```

On Windows, using CMD (using Nu for the assertion):

> (cmd.exe /C "dir /b") == (cmd.exe /C "dir /b %cd%")
true

On Windows, using PowerShell (using Nu for the assertion):

> (pwsh.exe -c "Get-ChildItem | Select-Object -ExpandProperty Name") == (pwsh.exe -c "Get-ChildItem (pwd) | Select-Object -ExpandProperty Name")
true

And for completeness:

Nushell 0.93:

> (ls -s | get name) == (ls -s (pwd) | get name)
true

Nushell 0.94.x/0.95:

> (ls -s | get name) == (ls -s (pwd) | get name)
false

It seems to me a completely normal expectation that the sort-order will remain the same regardless of whether ls was called with or without a path specifier. I think this is a regression.

fdncred commented 3 months ago

I think we should restore prior sort too, although I don't understand how that PR changed it. 🤔

userwiths commented 4 weeks ago

Hey, made a Pr for this that I tested locally. Seems that the use of read_dir was brought in the mentioned Bisect and from the rust docs we can see that it does not provide consistency in ordering. As far as I could see if we do not specify a directory/path we make no call to read_dir in the changed file (one type of order) and when we add an argument/path after ls we do call read_dir (hence the second order type).

The advised solution for consistency was to sort it manually. Gonna finish that of by saying that my grasp on rust aint the best yet, so improvements are welcome if someone has to offer.

fdncred commented 4 weeks ago

@userwiths The key to this change will be to ensure that it continues streaming and doesn't collect.

nushell / nushell