Skip to content

do not attempt to glob expand if the file path is wrapped in quotes#11569

Merged
WindSoilder merged 8 commits intonushell:mainfrom
WindSoilder:no_auto_expand
Jan 21, 2024
Merged

do not attempt to glob expand if the file path is wrapped in quotes#11569
WindSoilder merged 8 commits intonushell:mainfrom
WindSoilder:no_auto_expand

Conversation

@WindSoilder
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder commented Jan 18, 2024

Description

Fixes: #11455

For arguments which is annotated with :path/:directory/:glob

To fix the issue, we need to have a way to know if a path is originally quoted during runtime. So the information needed to be added at several levels:

  • parse time (from user input to expression)
    We need to add quoted information into Expr::Filepath, Expr::Directory, Expr::GlobPattern
  • eval time
    When convert from Expr::Filepath, Expr::Directory, Expr::GlobPattern to Value::String during runtime, we won't auto expanded the path if it's quoted

For ls

It's really special, because it accepts a String as a pattern, and it generates glob expression inside the command itself.

So the idea behind the change is introducing a special SyntaxShape to ls: SyntaxShape::LsGlobPattern. So we can track if the pattern is originally quoted easier, and we don't auto expand the path either.

Then when constructing a glob pattern inside ls, we check if input pattern is quoted, if so: we escape the input pattern, so we can run ls a[123]b, because it's already escaped.
Finally, to accomplish the checking process, we also need to introduce a new value type called Value::QuotedString to differ from Value::String, it's used to generate an enum called NuPath, which is finally used in ls function. ls learned from NuPath to know if user input is quoted.

User-Facing Changes

Actually it contains several changes

For arguments which is annotated with :path/:directory/:glob

Before

> def foo [p: path] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a
> def foo [p: directory] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a
> def foo [p: glob] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a

After

> def foo [p: path] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a
> def foo [p: directory] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a
> def foo [p: glob] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a

For ls command

touch '[uwu]'

Before

❯ ls -D "[uwu]"
Error:   × No matches found for [uwu]
   ╭─[entry #6:1:1]
 1 │ ls -D "[uwu]"
   ·       ───┬───
   ·          ╰── Pattern, file or folder not found
   ╰────
  help: no matches found

After

❯ ls -D "[uwu]"
╭───┬───────┬──────┬──────┬──────────╮
│ # │ name  │ type │ size │ modified │
├───┼───────┼──────┼──────┼──────────┤
│ 0 │ [uwu] │ file │  0 B │ now      │
╰───┴───────┴──────┴──────┴──────────╯

Tests + Formatting

Done

After Submitting

NaN

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 20, 2024

I think this is a creative solution @WindSoilder. Nice work. I have a couple questions.

  1. Does this work with any quotes (single, double, backtick)?
  2. Does this (or should this) revert bob's PR that checks if the glob exists file first, then if not, checks for a file?
  3. Related to 2, I'm curious how this works for cp, mv, umv, rm, touch` etc. (commands that may make use of globbing)

@WindSoilder
Copy link
Copy Markdown
Contributor Author

  1. Does this work with any quotes (single, double, backtick)?
    Nope: it doesn't work with backtick, because a backtick wrapped string is a bare word.
  2. Does this (or should this) revert bob's Allow filesystem commands to access files with glob metachars in name #10694 that checks if the glob exists file first, then if not, checks for a file?
    I've tested examples under 10694, and found it doesn't revert bob's pr.
  3. Related to 2, I'm curious how this works for cp, mv, umv, rm, touch etc. (commands that may make use of globbing)
    For cp, mv, rmv, rm, touch, the behavior is still the same, it checks if the file exists firstly, if not exists, apply glob operation. This logic is handled by arg_glob function.

The changes on SyntaxShape::GlobPattern only affects custom command which annotated with :glob.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

I'm thinking that do we need to apply the same logic on cp, mv, rm. Which is: if quoted, get rid of globbing. If not quoted, apply the glob operation.

If we do so, nushell will get better consistency on globbing rules across different fs commands. But it belongs to another problem.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 20, 2024

What i'm suggesting then is 2/bob's pr should be reverted since it has extra overhead to check things twice (in some cases).

@WindSoilder
Copy link
Copy Markdown
Contributor Author

Yeah, that's a good point. Maybe I can try to handle it in another pr?
Revert the logic and apply the logic in this pr to mv, cp, rv will require a larger change, I need to think how to achieve the bahavior

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 20, 2024

ya, i'd be fine with a separate pr for that.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

I'll merge it if there is no objections, any thought?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 21, 2024

no objections from me. i think it's worth a try.

@WindSoilder
Copy link
Copy Markdown
Contributor Author

OK, thanks for your input, you guide me to a good direction to improve some other commands

@WindSoilder WindSoilder merged commit c59d6d3 into nushell:main Jan 21, 2024
@WindSoilder WindSoilder deleted the no_auto_expand branch January 21, 2024 15:37
WindSoilder added a commit that referenced this pull request Jan 26, 2024
…`du` commands (#11621)

# Description
This pr is a follow up to
[#11569](#11569 (comment))
> Revert the logic in #10694 and
apply the logic in this pr to mv, cp, rv will require a larger change, I
need to think how to achieve the bahavior

And sorry @bobhy for reverting some of your changes.

This pr is going to unify glob behavior on the given commands:
* open
* rm
* cp-old
* mv
* umv
* cp
* du

So they have the same behavior to `ls`, which is:
If given parameter is quoted by single quote(`'`) or double quote(`"`),
don't auto-expand the glob pattern. If not quoted, auto-expand the glob
pattern.

Fixes: #9558  Fixes: #10211 Fixes: #9310 Fixes: #10364 

# TODO
But there is one thing remains: if we give a variable to the command, it
will always auto-expand the glob pattern, e.g:
```nushell
let path = "a[123]b"
rm $path
```
I don't think it's expected. But I also think user might want to
auto-expand the glob pattern in variables.

So I'll introduce a new command called `glob escape`, then if user
doesn't want to auto-expand the glob pattern, he can just do this: `rm
($path | glob escape)`

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

# Tests + Formatting
Done

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

## NOTE
This pr changes the semantic of `GlobPattern`, before this pr, it will
`expand path` after evaluated, this makes `nu_engine::glob_from` have no
chance to glob things right if a path contains glob pattern.

e.g: [#9310
](#9310 (comment))
#10211

I think changing the semantic is fine, because it makes glob works if
path contains something like '*'.

It maybe a breaking change if a custom command's argument are annotated
by `: glob`.
@crides
Copy link
Copy Markdown
Contributor

crides commented Jan 28, 2024

After this PR, how are we supposed to deal with globs containing spaces? in POSIX shells one can just escape the space directly, but that's not the case in nushell

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Jan 28, 2024

@crides do backticks work? they seem to work for me

ls `/Library/Application Support/*`

@crides
Copy link
Copy Markdown
Contributor

crides commented Jan 28, 2024

oh okay, backticks are not included. Thanks

@hustcer hustcer added this to the v0.90.0 milestone Feb 3, 2024
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
…ushell#11569)

# Description
Fixes: nushell#11455

### For arguments which is annotated with `:path/:directory/:glob`
To fix the issue, we need to have a way to know if a path is originally
quoted during runtime. So the information needed to be added at several
levels:
* parse time (from user input to expression)
We need to add quoted information into `Expr::Filepath`,
`Expr::Directory`, `Expr::GlobPattern`
* eval time
When convert from `Expr::Filepath`, `Expr::Directory`,
`Expr::GlobPattern` to `Value::String` during runtime, we won't auto
expanded the path if it's quoted

### For `ls`
It's really special, because it accepts a `String` as a pattern, and it
generates `glob` expression inside the command itself.

So the idea behind the change is introducing a special SyntaxShape to
ls: `SyntaxShape::LsGlobPattern`. So we can track if the pattern is
originally quoted easier, and we don't auto expand the path either.

Then when constructing a glob pattern inside ls, we check if input
pattern is quoted, if so: we escape the input pattern, so we can run `ls
a[123]b`, because it's already escaped.
Finally, to accomplish the checking process, we also need to introduce a
new value type called `Value::QuotedString` to differ from
`Value::String`, it's used to generate an enum called `NuPath`, which is
finally used in `ls` function. `ls` learned from `NuPath` to know if
user input is quoted.

# User-Facing Changes
Actually it contains several changes
### For arguments which is annotated with `:path/:directory/:glob`
#### Before
```nushell
> def foo [p: path] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a
> def foo [p: directory] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a
> def foo [p: glob] { echo $p }; print (foo "~/a"); print (foo '~/a')
/home/windsoilder/a
/home/windsoilder/a
```
#### After
```nushell
> def foo [p: path] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a
> def foo [p: directory] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a
> def foo [p: glob] { echo $p }; print (foo "~/a"); print (foo '~/a')
~/a
~/a
```
### For ls command
`touch '[uwu]'`
#### Before
```
❯ ls -D "[uwu]"
Error:   × No matches found for [uwu]
   ╭─[entry nushell#6:1:1]
 1 │ ls -D "[uwu]"
   ·       ───┬───
   ·          ╰── Pattern, file or folder not found
   ╰────
  help: no matches found
```

#### After
```
❯ ls -D "[uwu]"
╭───┬───────┬──────┬──────┬──────────╮
│ # │ name  │ type │ size │ modified │
├───┼───────┼──────┼──────┼──────────┤
│ 0 │ [uwu] │ file │  0 B │ now      │
╰───┴───────┴──────┴──────┴──────────╯
```

# Tests + Formatting
Done

# After Submitting
NaN
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
…`du` commands (nushell#11621)

# Description
This pr is a follow up to
[nushell#11569](nushell#11569 (comment))
> Revert the logic in nushell#10694 and
apply the logic in this pr to mv, cp, rv will require a larger change, I
need to think how to achieve the bahavior

And sorry @bobhy for reverting some of your changes.

This pr is going to unify glob behavior on the given commands:
* open
* rm
* cp-old
* mv
* umv
* cp
* du

So they have the same behavior to `ls`, which is:
If given parameter is quoted by single quote(`'`) or double quote(`"`),
don't auto-expand the glob pattern. If not quoted, auto-expand the glob
pattern.

Fixes: nushell#9558  Fixes: nushell#10211 Fixes: nushell#9310 Fixes: nushell#10364 

# TODO
But there is one thing remains: if we give a variable to the command, it
will always auto-expand the glob pattern, e.g:
```nushell
let path = "a[123]b"
rm $path
```
I don't think it's expected. But I also think user might want to
auto-expand the glob pattern in variables.

So I'll introduce a new command called `glob escape`, then if user
doesn't want to auto-expand the glob pattern, he can just do this: `rm
($path | glob escape)`

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

# Tests + Formatting
Done

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

## NOTE
This pr changes the semantic of `GlobPattern`, before this pr, it will
`expand path` after evaluated, this makes `nu_engine::glob_from` have no
chance to glob things right if a path contains glob pattern.

e.g: [nushell#9310
](nushell#9310 (comment))
nushell#10211

I think changing the semantic is fine, because it makes glob works if
path contains something like '*'.

It maybe a breaking change if a custom command's argument are annotated
by `: glob`.
@fdncred fdncred added the A:glob Behavior around file-system globbing with regular commands or `glob`. See also A:quoting/expansion label Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A:glob Behavior around file-system globbing with regular commands or `glob`. See also A:quoting/expansion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

For internal commands, do not attempt to glob expand if the file path is wrapped in quotes.

4 participants