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

glob with ../ prefix now works; #10504

Merged
merged 11 commits into from Sep 29, 2023
Merged

glob with ../ prefix now works; #10504

merged 11 commits into from Sep 29, 2023

Conversation

bobhy
Copy link
Contributor

@bobhy bobhy commented Sep 26, 2023

Fixes #10503
Also improves link to metacharacter help;

Description

glob code was using pattern as provided by user. If that had leading ..\, wax::Glob is documented to treat them as literal chars to be matched.
Fix is to use wax::Glob.partition() to split such invariant prefixes off the pattern and tack them onto the working directory computed separately.

Before

> ls ..
╭───┬───────┬──────┬──────┬───────────────╮
│ # │ name  │ type │ size │   modified    │
├───┼───────┼──────┼──────┼───────────────┤
│ 0 │ ../r1 │ dir  │  7 B │ 3 hours ago   │
│ 1 │ ../r2 │ dir  │  3 B │ a day ago     │
│ 2 │ ../r3 │ dir  │ 13 B │ 4 minutes ago │
╰───┴───────┴──────┴──────┴───────────────╯
> glob ../r*
╭────────────╮
│ empty list │
╰────────────╯

After

> glob ../r*
╭───┬──────────────────────────────╮
│ 0 │ /home/bobhy/src/rust/work/r2 │
│ 1 │ /home/bobhy/src/rust/work/r1 │
│ 2 │ /home/bobhy/src/rust/work/r3 │
╰───┴──────────────────────────────╯

User-Facing Changes

None

Tests + Formatting

  • 🟢 toolkit fmt
  • 🟢 toolkit clippy
  • 🟢 toolkit test
  • 🟢 toolkit test stdlib

After Submitting

improve link to metacharacter help;
@fdncred
Copy link
Collaborator

fdncred commented Sep 26, 2023

While I agree with this sentiment, we'll have to do something different because it doesn't work right in Windows due to verbatim paths that canonicalize returns. I believe this is the reason dunce is used in there example. Not sure dunce is still supported or if we use it, but I think omnipath supports such things too.

❯ glob **/*.{rs,toml} --depth 2
╭──┬──────────────────────────────────────────────────────────────────────╮
│ 0│\\?\C:\Users\username\source\repos\forks\nushell\.cargo\config.toml   │
│ 1│\\?\C:\Users\username\source\repos\forks\nushell\.github\.typos.toml  │
│ 2│\\?\C:\Users\username\source\repos\forks\nushell\benches\benchmarks.rs│
│ 3│\\?\C:\Users\username\source\repos\forks\nushell\Cargo.toml           │
│ 4│\\?\C:\Users\username\source\repos\forks\nushell\Cross.toml           │
│ 5│\\?\C:\Users\username\source\repos\forks\nushell\rust-toolchain.toml  │
│ 6│\\?\C:\Users\username\source\repos\forks\nushell\scripts\build.rs     │
│ 7│\\?\C:\Users\username\source\repos\forks\nushell\src\command.rs       │
│ 8│\\?\C:\Users\username\source\repos\forks\nushell\src\config_files.rs  │
│ 9│\\?\C:\Users\username\source\repos\forks\nushell\src\ide.rs           │
│10│\\?\C:\Users\username\source\repos\forks\nushell\src\logger.rs        │
│11│\\?\C:\Users\username\source\repos\forks\nushell\src\main.rs          │
│12│\\?\C:\Users\username\source\repos\forks\nushell\src\run.rs           │
│13│\\?\C:\Users\username\source\repos\forks\nushell\src\signals.rs       │
│14│\\?\C:\Users\username\source\repos\forks\nushell\src\terminal.rs      │
│15│\\?\C:\Users\username\source\repos\forks\nushell\src\tests.rs         │
│16│\\?\C:\Users\username\source\repos\forks\nushell\src\test_bins.rs     │
│17│\\?\C:\Users\username\source\repos\forks\nushell\tests\main.rs        │
╰──┴──────────────────────────────────────────────────────────────────────╯

@fdncred fdncred marked this pull request as draft September 26, 2023 12:44
@bobhy
Copy link
Contributor Author

bobhy commented Sep 28, 2023

Whew! Ready for review!

@bobhy bobhy marked this pull request as ready for review September 28, 2023 03:15
@fdncred
Copy link
Collaborator

fdncred commented Sep 28, 2023

wow, that was a hard-fought battle. ⚔️

@fdncred
Copy link
Collaborator

fdncred commented Sep 28, 2023

Looks much better on Windows

╭──┬──────────────────────────────────────────────────────────────────────╮
│ 0│C:\Users\username\source\repos\forks.bak\nushell\.cargo\config.toml   │
│ 1│C:\Users\username\source\repos\forks.bak\nushell\.github\.typos.toml  │
│ 2│C:\Users\username\source\repos\forks.bak\nushell\benches\benchmarks.rs│
│ 3│C:\Users\username\source\repos\forks.bak\nushell\Cargo.toml           │
│ 4│C:\Users\username\source\repos\forks.bak\nushell\Cross.toml           │
│ 5│C:\Users\username\source\repos\forks.bak\nushell\rust-toolchain.toml  │
│ 6│C:\Users\username\source\repos\forks.bak\nushell\scripts\build.rs     │
│ 7│C:\Users\username\source\repos\forks.bak\nushell\src\command.rs       │
│ 8│C:\Users\username\source\repos\forks.bak\nushell\src\config_files.rs  │
│ 9│C:\Users\username\source\repos\forks.bak\nushell\src\ide.rs           │
│10│C:\Users\username\source\repos\forks.bak\nushell\src\logger.rs        │
│11│C:\Users\username\source\repos\forks.bak\nushell\src\main.rs          │
│12│C:\Users\username\source\repos\forks.bak\nushell\src\run.rs           │
│13│C:\Users\username\source\repos\forks.bak\nushell\src\signals.rs       │
│14│C:\Users\username\source\repos\forks.bak\nushell\src\terminal.rs      │
│15│C:\Users\username\source\repos\forks.bak\nushell\src\tests.rs         │
│16│C:\Users\username\source\repos\forks.bak\nushell\src\test_bins.rs     │
│17│C:\Users\username\source\repos\forks.bak\nushell\tests\main.rs        │
╰──┴──────────────────────────────────────────────────────────────────────╯

Copy link
Collaborator

@fdncred fdncred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to see you finally got this working. I just removed some comments but I think it's ready too.

@bobhy
Copy link
Contributor Author

bobhy commented Sep 28, 2023 via email

@kubouch
Copy link
Contributor

kubouch commented Sep 29, 2023

Ah yeah, wrong link. You could point at https://crates.io/crates/wax, for example (or maybe https://docs.rs/crate/wax) to show the README immediately. Not a big deal, though, I just thought the link was too long for a help message and the # reference might be prone to change.

@fdncred
Copy link
Collaborator

fdncred commented Sep 29, 2023

My changes were reverted by force pushes. So, I made the changes again to change the link and remove some excess comments. We can land this when it's green again.

@fdncred fdncred merged commit 9a0c6f2 into nushell:main Sep 29, 2023
19 checks passed
@bobhy bobhy deleted the wax_glob branch October 13, 2023 15:38
hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
Fixes nushell#10503 
Also improves link to metacharacter help;

# Description
`glob` code was using pattern as provided by user. If that had leading
`..\`, `wax::Glob` is documented to treat them as literal chars to be
matched.
Fix is to use `wax::Glob.partition()` to split such invariant prefixes
off the pattern and tack them onto the working directory computed
separately.

Before
```
> ls ..
╭───┬───────┬──────┬──────┬───────────────╮
│ # │ name  │ type │ size │   modified    │
├───┼───────┼──────┼──────┼───────────────┤
│ 0 │ ../r1 │ dir  │  7 B │ 3 hours ago   │
│ 1 │ ../r2 │ dir  │  3 B │ a day ago     │
│ 2 │ ../r3 │ dir  │ 13 B │ 4 minutes ago │
╰───┴───────┴──────┴──────┴───────────────╯
> glob ../r*
╭────────────╮
│ empty list │
╰────────────╯
```
After 
```
> glob ../r*
╭───┬──────────────────────────────╮
│ 0 │ /home/bobhy/src/rust/work/r2 │
│ 1 │ /home/bobhy/src/rust/work/r1 │
│ 2 │ /home/bobhy/src/rust/work/r3 │
╰───┴──────────────────────────────╯
```

# User-Facing Changes
None

# Tests + Formatting
- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# 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.
-->

---------

Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

glob doesn't match files if pattern starts with ../ or ./
3 participants