Skip to content
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

Files with trailing dots (or spaces) break nushell on Windows #7869

Open
spf13 opened this issue Jan 27, 2023 · 7 comments
Open

Files with trailing dots (or spaces) break nushell on Windows #7869

spf13 opened this issue Jan 27, 2023 · 7 comments
Labels
🐛 bug Something isn't working file-system Related to commands and core nushell behavior around the file system platform-specific platform-specific issues

Comments

@spf13
Copy link

spf13 commented Jan 27, 2023

Describe the bug

While the Win32 API doesn't allow files or folders with trailing dots, the filesystems do allow it and programs do and can create files with trailing dots or spaces.

If this happens nushell reports the following

Error: nu::shell::error_reading_file (link)

  × Error trying to read file
   ╭─[entry #21:1:1]
 1 │ ls
   · ─┬
   ·  ╰── Could not read file metadata
   ╰────

How to reproduce

In Bash on Windows
touch "foo."

In Nushell on Windows
ls

Expected behavior

I expected nu to list the directory regardless of files that end in spaces or dots.... just like bash does.

Screenshots

No response

Configuration

key value
version 0.71.0
branch
commit_hash 6cc4ef6
build_os windows-x86_64
build_target x86_64-pc-windows-msvc
rust_version rustc 1.65.0 (897e37553 2022-11-02)
rust_channel stable-x86_64-pc-windows-msvc
cargo_version cargo 1.65.0 (4bc8f24d3 2022-10-20)
pkg_version 0.71.0
build_time 2022-11-08 18:03:10 +00:00
build_rust_channel release
features database, dataframe, default, trash, which, zip
installed_plugins

Additional context

No response

@spf13 spf13 changed the title Files with trailing dots break on Windows Files with trailing dots (or likely spaces) break nushell on Windows Jan 27, 2023
@spf13 spf13 changed the title Files with trailing dots (or likely spaces) break nushell on Windows Files with trailing dots (or spaces) break nushell on Windows Jan 27, 2023
@ChrisDenton
Copy link
Contributor

This is fundamentally a consequence of how Win32 APIs work. They strip trailing dots from paths when called.

MINGW bash uses lower level (NT) APIs that don't do this. It's also possible to use verbatim paths to workaround this issue.

@ChrisDenton
Copy link
Contributor

I guess the ls command specifically could be improved to not error in this case? E.g. if the file's metadata can't be retrieved for whatever reason it could have empty cells in the table or it could make use of whatever metadata is returned from fs::read_dir (I believe this is currently discarded by nu-glob).

@sholderbach sholderbach added 🐛 bug Something isn't working platform-specific platform-specific issues file-system Related to commands and core nushell behavior around the file system labels Jan 27, 2023
@rgwood
Copy link
Contributor

rgwood commented Jan 27, 2023

The error message is better in recent versions of Nu: #7348

It would be nice to handle this as well as MINGW bash but tbh it's not a high priority; IIRC Windows Explorer doesn't even show these files

edit: I misremembered, Explorer does show the files. If someone has time we should do this.

@spf13
Copy link
Author

spf13 commented Jan 27, 2023 via email

@rgwood
Copy link
Contributor

rgwood commented Jan 27, 2023

Yeah, I corrected my comment shortly after posting (currently travelling and on very little sleep)

@rgwood
Copy link
Contributor

rgwood commented Jan 29, 2023

If anyone wants to tackle this, this is the relevant place in ls:

/// A secondary way to get file info on Windows, for when std::fs::symlink_metadata() fails.
/// dir_entry_dict depends on metadata, but that can't be retrieved for some Windows system files:
/// https://github.com/rust-lang/rust/issues/96980
pub fn dir_entry_dict_windows_fallback(

It's pretty easy to give up and return nothing for metadata columns when FindFirstFileW fails, here's something I hacked together: image

That might be good enough as a quick-and-dirty improvement until someone has time to figure out a second fallback mechanism using Chris's suggestion:

MINGW bash uses lower level (NT) APIs that don't do this. It's also possible to use verbatim paths to workaround this issue.

I'm on vacation with a slow laptop but I will aim to put up a PR in a few weeks if nobody gets around to it earlier.

rgwood added a commit that referenced this issue Feb 7, 2023
This PR is an incremental improvement to `ls` when it encounters
'illegal' file paths on Windows. Related:
#7869

## Context

We have trouble with filenames that Windows doesn't like, for example
[files with a `.` at the end of their
name](https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions).
To make a long story short, the Rust stdlib and several Win32 APIs will
choke if asked to do something with an illegal filepath. This is a
problem because files with illegal names can be created via other means
(like `touch foo.` in MINGW bash).

Previously `ls` would fail completely in a directory with a bad file,
which isn't great. After this PR, bad files get included in `ls` results
but without any metadata columns. This is not quite where we want to be
— eventually we want to be able to display file metadata for _all_ files
(even naughty ones) — but it's an improvement on the status quo.

### Before


![image](https://user-images.githubusercontent.com/26268125/217340906-26afd6d3-0ec3-454f-bed4-2bfcc9cf3a2f.png)

### After


![image](https://user-images.githubusercontent.com/26268125/217344373-6b81cc39-50b8-4390-8061-3e570502a784.png)

## Future work

Try the workarounds @ChrisDenton suggested:
#7869 (comment)

Some info on verbatim paths:
https://users.rust-lang.org/t/understanding-windows-paths/58583

## Testing

I tried to write a test for this, but it looks like our testing sandbox
can't create files with illegal filenames.😔 Here's the code in case it
proves useful someday:

```rust
/// Windows doesn't like certain file names, like file names ending with a period:
/// https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions
/// However, those files can still be created with tools like MINGW bash.
/// We may not be able to get full metadata for those files, but we should test that we can at least include them in ls results
#[test]
#[cfg(windows)]
fn can_list_illegal_files() {
    Playground::setup("ls_test_all_columns", |dirs, sandbox| {
        sandbox.with_files(vec![
            EmptyFile("foo"),
            EmptyFile("bar."),
            EmptyFile("baz"),
        ]);

        let actual = nu!(
            cwd: dirs.test(),
            "ls | length"
        );
        assert_eq!(actual.out, "3");

        let actual = nu!(
            cwd: dirs.test(),
            "ls"
        );
        assert_eq!(actual.out, "1");

        let actual = nu!(
            cwd: dirs.test(),
            "ls | where {|f| $f.name | str ends-with 'bar.'} | length"
        );
        assert_eq!(actual.out, "1");
    })
}
```
@rgwood
Copy link
Contributor

rgwood commented Feb 7, 2023

I've just merged a fix that handles this a little better: #7999

image

Eventually we'd like to get to a place where Nu can read the metadata of files with 'illegal' names too. Chris's suggestion of verbatim paths seems like the most promising approach, IMO. Might as well leave this issue open until we get around to the full solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working file-system Related to commands and core nushell behavior around the file system platform-specific platform-specific issues
Projects
None yet
Development

No branches or pull requests

4 participants