Conditionally disable expansion for external command#6014
Conditionally disable expansion for external command#6014fdncred merged 22 commits intonushell:mainfrom
Conversation
|
With this, will we still expand globs if there aren't quotes? Some externals will expand the shell to do the expansion of globs for them. |
No, external( |
|
Unfortunately, we need both expansion and non-expansion. We may need to design something that works a bit better here (possibly following something like bash-style shells that have a way to opt-out of expansion) |
|
@jntrnr If you could point me out what external might need expansion, that would be helpful to understand this. I tried to ask the same question in our implementation chat. Regards to the opt-out, my first thought is to create an opt-out list that could prevent the expansion |
|
@Kangaxx-0 - any external that takes in a glob likely will break. |
Ok, this was the same thing I worried. I am going to start a new thread(Discord) for further discussion on this :) |
|
did we decide anything on this? |
|
I don't think so. To be honest, I don't know if I should abandon this PR |
|
I'll try to bring this up later today with the team. |
|
@Kangaxx-0 The core team discussed this today and we decided on this approach. Would you be willing to update this PR to support this below? Also, how do you feel about this decision? Do you think it's a good way to go?
|
One quick question, double quotes usually do the expansion, e.g - run this in zsh |
Hmm, interesting - can we think through variable expansion separate from glob expansion? I think we might want to do some design work specifically for that case so that we can see the trade-offs more clearly. I noticed that Bash also treats these differently: So maybe yeah we can focus on just the path glob expansion first and then we'll do some work to figure out variable expansion. |
|
In regards to the decision above, what about variables, if we have It this a case of losing context, I wonder if to avoid opt-in/opt-out we need to make bare words a more first-class concept or be able to attach/wrap it in something so the consumer knows it was declared without quotes and should be treated as a bare word and glob expanded, Can we have a |
|
Can someone change my pr to draft please, I will be working on the changes:
|
|
Please do not merge the latest iteration, I will come up with some test scenarios, and also add more tests :) |
|
@Kangaxx-0 how's it going on this PR? Looks like CI is green, are we ready to merge or are we missing some windows tests still? |
Yes, I did a few tests, and also add some unit tests. Now, single and double quotes will prevent expansion, backtick and bare word does path expand |
|
CI and my local, windows gave me different error message. Nonetheless, it did work fine |
|
@Kangaxx-0 If you're done, I guess we can probably merge but it looks like there's some tests missing from the Windows side, if I'm reading things right, like backticks quoting. I can understand if you're just fed up with working on this. |
My local windows was running super slow. I have switched to another issue, I will add some unit tests after i am done with that issue |
|
Interesting.... The failed two unit tests, they worked as expected in my local(Win 11), I did not see any error whatsoever, the image CI use is win server 2022. |
|
are we ready to land this? appreciate all your hard work. |
I am going to add some comment, will ping you when it is ready |
I think it is ready now |
|
ok, let's land this and see if it makes things better. thanks for sticking with it @Kangaxx-0! |
This PR is a complete rewrite of `run_external.rs`. The main goal of the rewrite is improving readability, but it also fixes some bugs related to argument handling and the PATH variable (fixes #6011). I'll discuss some technical details to make reviewing easier. ## Argument handling Quoting arguments for external commands is hard. Like, *really* hard. We've had more than a dozen issues and PRs dedicated to quoting arguments (see Appendix) but the current implementation is still buggy. Here's a demonstration of the buggy behavior: ```nu let foo = "'bar'" ^touch $foo # This creates a file named `bar`, but it should be `'bar'` ^touch ...[ "'bar'" ] # Same ``` I'll describe how this PR deals with argument handling. First, we'll introduce the concept of **bare strings**. Bare strings are **string literals** that are either **unquoted** or **quoted by backticks** [^1]. Strings within a list literal are NOT considered bare strings, even if they are unquoted or quoted by backticks. When a bare string is used as an argument to external process, we need to perform tilde-expansion, glob-expansion, and inner-quotes-removal, in that order. "Inner-quotes-removal" means transforming from `--option="value"` into `--option=value`. ## `.bat` files and CMD built-ins On Windows, `.bat` files and `.cmd` files are considered executable, but they need `CMD.exe` as the interpreter. The Rust standard library supports running `.bat` files directly and will spawn `CMD.exe` under the hood (see [documentation](https://doc.rust-lang.org/std/process/index.html#windows-argument-splitting)). However, other extensions are not supported [^2]. Nushell also supports a selected number of CMD built-ins. The problem with CMD is that it uses a different set of quoting rules. Correctly quoting for CMD requires using [Command::raw_arg()](https://doc.rust-lang.org/std/os/windows/process/trait.CommandExt.html#tymethod.raw_arg) and manually quoting CMD special characters, on top of quoting from the Nushell side. ~~I decided that this is too complex and chose to reject special characters in CMD built-ins instead [^3]. Hopefully this will not affact real-world use cases.~~ I've implemented escaping that works reasonably well. ## `which-support` feature The `which` crate is now a hard dependency of `nu-command`, making the `which-support` feature essentially useless. The `which` crate is already a hard dependency of `nu-cli`, and we should consider removing the `which-support` feature entirely. ## Appendix Here's a list of quoting-related issues and PRs in rough chronological order. * #4609 * #4631 * #4601 * #5846 * #5978 * #6014 * #6154 * #6161 * #6399 * #6420 * #6426 * #6465 * #6559 * #6560 [^1]: The idea that backtick-quoted strings act like bare strings was introduced by Kubouch and briefly mentioned in [the language reference](https://www.nushell.sh/lang-guide/chapters/strings_and_text.html#backtick-quotes). [^2]: The documentation also said "running .bat scripts in this way may be removed in the future and so should not be relied upon", which is another reason to move away from this. But again, quoting for CMD is hard. [^3]: If anyone wants to try, the best resource I found on the topic is [this](https://daviddeley.com/autohotkey/parameters/parameters.htm).
Description
Fix #5978, with this change, external command will do expansion based on
^ls *.nu): will expand glob for external^ls*.nu``): will also expand glob^ls '*.nu'won't do the path expansionTests
Make sure you've done the following:
Make sure you've run and fixed any issues with these commands:
cargo fmt --all -- --checkto check standard code formatting (cargo fmt --allapplies these changes)cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collectto check that you're using the standard code stylecargo test --workspace --features=extrato check that all the tests pass