Skip to content

Fixes . expanded incorrectly as external argument#12950

Merged
WindSoilder merged 4 commits intonushell:mainfrom
yizhepku:fix-external-dot
May 25, 2024
Merged

Fixes . expanded incorrectly as external argument#12950
WindSoilder merged 4 commits intonushell:mainfrom
yizhepku:fix-external-dot

Conversation

@yizhepku
Copy link
Copy Markdown
Contributor

This PR fixes a bug where . is expanded into an empty string when used as an argument to external commands. Fixes #12948.

@yizhepku yizhepku marked this pull request as draft May 24, 2024 10:30
@yizhepku yizhepku marked this pull request as ready for review May 24, 2024 10:32
@WindSoilder
Copy link
Copy Markdown
Contributor

Thank you for the quick fix!
I'm thinking that, can we expand only if input argument contains *? It can solve relative issue, and I think it can also improve performance.

@yizhepku
Copy link
Copy Markdown
Contributor Author

yizhepku commented May 24, 2024

I'm thinking that, can we expand only if input argument contains *? It can solve relative issue

I don't that fixes the bug, because you can refer to PWD even when the input contains *. For example, if PWD is /tmp/foobar, then ^echo /tmp/foo* should print . as part of its output.

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 24, 2024

I just found that code . doesn't work in nushell anymore either. I'm hoping this PR fixes that too, but I haven't tried it yet.

@yizhepku
Copy link
Copy Markdown
Contributor Author

I just found that code . doesn't work in nushell anymore either. I'm hoping this PR fixes that too, but I haven't tried it yet.

It does fix that too.

@WindSoilder
Copy link
Copy Markdown
Contributor

I don't that fixes the bug, because you can refer to PWD even when the input contains . For example, if PWD is /tmp/foobar, then ^echo /tmp/foo should print . as part of its output.

Ah, OK, that's a good example, sorry for another question, is there any reason to use nu_glob::glob_with_parent rather than nu_engine::glob_from? After reading the docstring, it's provided primarily for testability.

I also think it's good to add another test at higher level, in tests/shell/pipeline/commands/external.rs

let actual = nu!("nu --testbin cococo .");
assert_eq!(actual.out, ".")

Co-authored-by: Ian Manske <ian.manske@pm.me>
@yizhepku
Copy link
Copy Markdown
Contributor Author

Is there any reason to use nu_glob::glob_with_parent rather than nu_engine::glob_from? After reading the docstring, it's provided primarily for testability.

To be honest, the reason I didn't use nu_engine::glob_from() was that I couldn't understand what it's doing after reading its implementation several times. It doesn't seem to add much value compared to using nu_glob::glob_with_parent() directly.

I know nu_glob::glob_with_parent() says it's provided for testability, but it's surprisingly useful given how Nushell has its own PWD semantics (to the point where using nu_glob::glob() without specifying PWD is almost certainly a bug).

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 25, 2024

@WindSoilder are you ready to land this? We only have another day or so before we freeze before the release and I'd like to get this one in.

@WindSoilder
Copy link
Copy Markdown
Contributor

Sure! I'm ok with the change, for the usage of glob_with_parent, I don't have any objections

@WindSoilder WindSoilder merged commit f38f88d into nushell:main May 25, 2024
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 25, 2024

if it needs to be fixed, and changed to glob_from to be more correct, let's do it after the release.

@WindSoilder
Copy link
Copy Markdown
Contributor

The behavior is slightly different between nu_engine::glob_from + diff_paths and nu_glob::glob_with_parent:
If you are in nushell directory, ^echo ../nushell//.nu will show different output.

On 0.93.0:

〉^ls ../nushell/*/*.nu
scripts/build-all.nu scripts/coverage-local.nu scripts/register-plugins.nu scripts/test_virtualenv.nu

On latest main:

^ls ../nushell/*/*.nu
../nushell/scripts/build-all.nu         ../nushell/scripts/register-plugins.nu
../nushell/scripts/coverage-local.nu    ../nushell/scripts/test_virtualenv.nu

Currently bash and fish output the same to latest main, Personally I think what 0.93.0 do is better.

But I think they are both correct.

@hustcer hustcer added this to the v0.94.0 milestone May 26, 2024
@yizhepku yizhepku deleted the fix-external-dot branch May 26, 2024 04:18
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
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.

git add cannot add file with path .

5 participants