Skip to content

Add nullglob functionality to nu-glob#9307

Closed
casuallyblue wants to merge 4 commits intonushell:mainfrom
casuallyblue:main
Closed

Add nullglob functionality to nu-glob#9307
casuallyblue wants to merge 4 commits intonushell:mainfrom
casuallyblue:main

Conversation

@casuallyblue
Copy link
Copy Markdown

@casuallyblue casuallyblue commented May 27, 2023

Description

This pull request fixes an issue where files with square brackets in their names would be treated as a glob pattern rather than the filename, even if there were no matches to the glob pattern. This is similar to bash nullglob.

User-Facing Changes

Glob would now use the literal glob pattern as a result if there were no other results from matching

@WindSoilder
Copy link
Copy Markdown
Contributor

WindSoilder commented May 27, 2023

Hi, thanks for your contribution, I wonder is ls command also meet the same issue like this? #9232

Would you please add a relative test case for relative commands you changed?

@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented May 27, 2023 •
edited
Loading

Hi, thanks for your contribution, I wonder is ls command also meet the same issue like this? #9232

I think that is likely the issue that is causing that, and I've found that I need to fix my pull request because its acting on the pattern as a whole and it should be working by path segment instead.

Would you please add a relative test case for relative commands you changed?

I'm not quite sure on how to do test cases since this patch works on the results from actually running the glob patterns against a filesystem. Do you have any pointers on how I could do this from a test environment?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 27, 2023 •
edited
Loading

Cool! Thanks for working on this! I'm definitely hoping for a fix for ls related to #9232.

To answer your question about tests, you can search for glob in our tests and find hits like this for ls

nushell/crates/nu-command/tests/commands/ls.rs

Lines 174 to 206 in 3005fe1

fn lists_all_hidden_files_when_glob_contains_dot() {
Playground::setup("ls_test_8", |dirs, sandbox| {
sandbox
.with_files(vec![
EmptyFile("root1.txt"),
EmptyFile("root2.txt"),
EmptyFile(".dotfile1"),
])
.within("dir_a")
.with_files(vec![
EmptyFile("yehuda.10.txt"),
EmptyFile("jt10.txt"),
EmptyFile(".dotfile2"),
])
.within("dir_b")
.with_files(vec![
EmptyFile("andres.10.txt"),
EmptyFile("chicken_not_to_be_picked_up.100.txt"),
EmptyFile(".dotfile3"),
]);
let actual = nu!(
cwd: dirs.test(), pipeline(
r#"
ls **/.*
| length
"#
));
assert_eq!(actual.out, "3");
})
}

and glob

nushell/crates/nu-command/tests/commands/glob.rs

Lines 6 to 22 in 3005fe1

fn empty_glob_pattern_triggers_error() {
Playground::setup("glob_test_1", |dirs, sandbox| {
sandbox.with_files(vec![
EmptyFile("yehuda.txt"),
EmptyFile("jttxt"),
EmptyFile("andres.txt"),
]);
let actual = nu!(
cwd: dirs.test(),
"glob ''",
);
assert!(actual.err.contains("must not be empty"));
})
}

Just note that the glob command does not use the nu-glob crate, it uses wax just to give some different functionality (although we'd like to integrate them back together someday)

@WindSoilder WindSoilder mentioned this pull request May 28, 2023
Closed
@amtoine amtoine linked an issue May 28, 2023 that may be closed by this pull request
Error on the commands 'cp', 'mv', 'rm', and 'open' on files that have [ ] on its path #9310
Closed
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 1, 2023

@casually-blue are you going to be able to have time for this?

@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented Jun 1, 2023

I've been a bit busy the beginning of this week but I've got some time today to work on it.

@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented Jun 2, 2023

Initially I was thinking that it would be possible to just check if there were no matches for the pattern as a whole but I've realized that nullglob needs to actually be implemented so it gets checked for each path segment which requires a bit more understanding of how the glob library is actually working internally. I should have something soon but if anyone who knows a bit more about it has any pointers that would be greatly appreciated.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 2, 2023

Thanks for your help, we appreciate the dedication to making nushell better!

@fdncred fdncred mentioned this pull request Jun 5, 2023
Closed
@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented Jun 5, 2023

After some more looking at how the code is working it looks like this would not actually fix the problem with ls, cp, etc., in a real way. Their main problem when dealing with folders with those names seems to be that they aren't escaping the pattern when they use their current working directory. While nullglob would probably fix them for simple cases, they would probably break again if any part of a path had a file that matched the glob.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 5, 2023

The tricky part is supporting both cases. Sometimes you want [blah] to work like a range/glob and other times it's literal. I'm not sure there's a way to fix it automatically, but we should allow escaping like ls \[blah\], at least that's an opt-in approach.

Our previous approach was to totally remove [] from nu-glob entirely. We'd still love to have some type of fix so that we don't have to remove it again.

@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented Jun 5, 2023

The tricky part is supporting both cases. Sometimes you want [blah] to work like a range/glob and other times it's literal. I'm not sure there's a way to fix it automatically, but we should allow escaping like ls \[blah\], at least that's an opt-in approach.

Yeah, I'm talking about the relative path that ls is putting in when it canonicalizes the path. The current version will treat characters in that as part of the glob when they are meant to be read literally.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 6, 2023

any ideas for a fix?

@casuallyblue
Copy link
Copy Markdown
Author

casuallyblue commented Jun 6, 2023

Looking at the code I got it somewhat working by just escaping the filename and then converting back to a PathBuf for pushing the rest of the glob, but that both breaks the prefix calculation for removing the relative parts of the path and isn't really a good idea because then the PathBufs that ls uses in its code aren't valid paths anymore since escaping is its own thing.

@fdncred fdncred mentioned this pull request Jun 12, 2023
Merged
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 12, 2023

I may be close with #9416, if you're interested in testing, please let me know how it goes.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jun 13, 2023

closing in favor of #9416 (which is pretty hacky. would love to have a better fix still)

@fdncred fdncred closed this Jun 13, 2023
fdncred added a commit that referenced this pull request Jun 13, 2023
@fdncred
allow paths to have brackets (#9416)
8a52085 
# Description

This PR is trying to allow you to have `[blah]` in your path and yet
still have `ls` work. This is done by trying to separate the path from
the pattern to be searched for. It may still need more work.

I've tested it with:
- mkdir "[test]"
- cd "[test]"
- ls

Related to #9307 
Hopefully fixes #9232 

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect -A clippy::result_large_err` to check that
you're using the standard code style
- `cargo test --workspace` to check that all tests pass
- `cargo run -- crates/nu-std/tests/run.nu` to run the tests for the
standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error on the commands 'cp', 'mv', 'rm', and 'open' on files that have [ ] on its path

3 participants

@casuallyblue @WindSoilder @fdncred