Skip to content

Do not propagate glob creation error for external args#12955

Merged
WindSoilder merged 2 commits intonushell:mainfrom
IanManske:no-glob-error-propagation
May 25, 2024
Merged

Do not propagate glob creation error for external args#12955
WindSoilder merged 2 commits intonushell:mainfrom
IanManske:no-glob-error-propagation

Conversation

@IanManske
Copy link
Copy Markdown
Member

@IanManske IanManske commented May 24, 2024

Description

Instead of returning an error, this PR changes expand_glob in run_external.rs to return the original string arg if glob creation failed. This makes it so that, e.g.,

^echo `[`
^echo `***`

no longer fail with a shell error. (This follows from #12921.)

@WindSoilder
Copy link
Copy Markdown
Contributor

Thanks! Looks good

@WindSoilder WindSoilder merged commit 95977fa into nushell:main May 25, 2024
@devyn
Copy link
Copy Markdown
Contributor

devyn commented May 25, 2024

This is still happening:

  ~: stat .
/usr/bin/stat: cannot statx '': No such file or directory
# Eliza [exit:1]                                                                                                                                                                                2024-05-24 20:42:07
  ~: stat '.'
  File: .
  Size: 842             Blocks: 289        IO Block: 16384  directory
Device: 0,93    Inode: 34          Links: 284
Access: (0700/drwx------)  Uid: ( 1000/   devyn)   Gid: (  985/ mongodb)
Access: 2021-05-27 17:01:56.907115973 -0700
Modify: 2024-05-24 16:50:14.858521695 -0700
Change: 2024-05-24 16:50:14.858521695 -0700
 Birth: 2021-05-27 17:01:56.907115973 -0700

so I think #12950 is still necessary

@hustcer hustcer added this to the v0.94.0 milestone May 25, 2024
devyn added a commit that referenced this pull request Jun 20, 2024
…he parser (#13089)

# Description

We've had a lot of different issues and PRs related to arg handling with
externals since the rewrite of `run-external` in #12921:

- #12950
- #12955
- #13000
- #13001
- #13021
- #13027
- #13028
- #13073

Many of these are caused by the argument handling of external calls and
`run-external` being very special and involving the parser handing
quoted strings over to `run-external` so that it knows whether to expand
tildes and globs and so on. This is really unusual and also makes it
harder to use `run-external`, and also harder to understand it (and
probably is part of the reason why it was rewritten in the first place).

This PR moves a lot more of that work over to the parser, so that by the
time `run-external` gets it, it's dealing with much more normal Nushell
values. In particular:

- Unquoted strings are handled as globs with no expand
- The unescaped-but-quoted handling of strings was removed, and the
parser constructs normal looking strings instead, removing internal
quotes so that `run-external` doesn't have to do it
- Bare word interpolation is now supported and expansion is done in this
case
- Expressions typed as `Glob` containing `Expr::StringInterpolation` now
produce `Value::Glob` instead, with the quoted status from the expr
passed through so we know if it was a bare word
- Bare word interpolation for values typed as `glob` now possible, but
not implemented
- Because expansion is now triggered by `Value::Glob(_, false)` instead
of looking at the expr, externals now support glob types

# User-Facing Changes

- Bare word interpolation works for external command options, and
otherwise embedded in other strings:
  ```nushell
  ^echo --foo=(2 + 2) # prints --foo=4
  ^echo -foo=$"(2 + 2)" # prints -foo=4
  ^echo foo="(2 + 2)" # prints (no interpolation!) foo=(2 + 2)
  ^echo foo,(2 + 2),bar # prints foo,4,bar
  ```

- Bare word interpolation expands for external command head/args:
  ```nushell
  let name = "exa"
  ~/.cargo/bin/($name) # this works, and expands the tilde
  ^$"~/.cargo/bin/($name)" # this doesn't expand the tilde
  ^echo ~/($name)/* # this glob is expanded
  ^echo $"~/($name)/*" # this isn't expanded
  ```

- Ndots are now supported for the head of an external command
(`^.../foo` works)

- Glob values are now supported for head/args of an external command,
and expanded appropriately:
  ```nushell
  ^("~/.cargo/bin/exa" | into glob) # the tilde is expanded
  ^echo ("*.txt" | into glob) # this glob is expanded
  ```

- `run-external` now works more like any other command, without
expecting a special call convention
  for its args:
  ```nushell
  run-external echo "'foo'"
  # before PR: 'foo'
  # after PR:  foo
  run-external echo "*.txt"
  # before PR: (glob is expanded)
  # after PR:  *.txt
  ```

# Tests + Formatting
Lots of tests added and cleaned up. Some tests that weren't active on
Windows changed to use `nu --testbin cococo` so that they can work.
Added a test for Linux only to make sure tilde expansion of commands
works, because changing `HOME` there causes `~` to reliably change.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] release notes: make sure to mention the new syntaxes that are
supported
@IanManske IanManske deleted the no-glob-error-propagation branch June 23, 2024 01:45
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.

4 participants