when execute external command, escape args if possible #6560
Conversation
|
Does this PR change any of these tests? https://hackmd.io/9PZKGOs0Rc-5kf8GuVY8rw?view |
|
Nope, it only handle for double quoted string with escape character like: |
|
I get this on Windows, which I'm guessing is wrong. And using Window's version of I wonder how we make this work for all |
|
I don't have a windows pc(again), I'll try to have a look on weekend |
WindSoilder
force-pushed
the
escape_arg
branch
from
ff04bf9
to
6ef3bbd
Compare
|
@fdncred Sorry, currently I have to give up for make it compatible with all I'd like to share what I get:
Windows echo and git-bash's echo are two built-in commands for each shell, the idle solution would make To handle this, we need to know if the given command is a cmd built-in command, or git-bash built-in command. But I don't think we can handle both, on windows, if we spawn external command failed, currently we think this maybe a cmd built-in command, and try to spawn it with Though, I'd propose to make this pr ready to review again, at least it solves external command (not includes other shell's built-in command) with escaping like this example: #6420 (comment) For other complicate part, it's tracking by the following issue: |
|
@WindSoilder ok. We could also check which echo it is by invoking |
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
Fixes: #6559
When passing argument to external command, try escape it.
Relative history:
#6014, in this change, we move
trim quote logicintorun_externaleval stage, butescape logicis also removed.This pr is going to keep
escape logicwhen parsing arguments.Tests
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