From 0e1553026e69adec2207d259d8513764deb5f87c Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Wed, 29 May 2024 18:48:29 -0700 Subject: [PATCH] Restore tilde expansion on external command names (#13001) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description Fix a regression introduced by #12921, where tilde expansion was no longer done on the external command name, breaking things like ```nushell > ~/.cargo/bin/exa ``` This properly handles quoted strings, so they don't expand: ```nushell > ^"~/.cargo/bin/exa" Error: nu::shell::external_command × External command failed ╭─[entry #1:1:2] 1 │ ^"~/.cargo/bin/exa" · ─────────┬──────── · ╰── Command `~/.cargo/bin/exa` not found ╰──── help: `~/.cargo/bin/exa` is neither a Nushell built-in or a known external command ``` This required a change to the parser, so the command name is also parsed in the same way the arguments are - i.e. the quotes on the outside remain in the expression. Hopefully that doesn't break anything else. 🤞 Fixes #13000. Should include in patch release 0.94.1 cc @YizhePKU # User-Facing Changes - Tilde expansion now works again for external commands - The `command` of `run-external` will now have its quotes removed like the other arguments if it is a literal string - The parser is changed to include quotes in the command expression of `ExternalCall` if they were present # Tests + Formatting I would like to add a regression test for this, but it's complicated because we need a well-known binary within the home directory, which just isn't a thing. We could drop one there, but that's kind of a bad behavior for a test to do. I also considered changing the home directory for the test, but that's so platform-specific - potentially could get it working on specific platforms though. Changing `HOME` env on Linux definitely works as far as tilde expansion works. - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` --- crates/nu-command/src/system/run_external.rs | 25 ++++++++++++++++++-- crates/nu-parser/src/parser.rs | 5 ++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 8531217d4b..69c4a319bb 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -46,15 +46,36 @@ impl Command for External { ) -> Result { let cwd = engine_state.cwd(Some(stack))?; + // Evaluate the command name in the same way the arguments are evaluated. Since this isn't + // a spread, it should return a one-element vec. + let name_expr = call + .positional_nth(0) + .ok_or_else(|| ShellError::MissingParameter { + param_name: "command".into(), + span: call.head, + })?; + let name = eval_argument(engine_state, stack, name_expr, false)? + .pop() + .expect("eval_argument returned zero-element vec") + .into_spanned(name_expr.span); + // Find the absolute path to the executable. On Windows, set the // executable to "cmd.exe" if it's is a CMD internal command. If the // command is not found, display a helpful error message. - let name: Spanned = call.req(engine_state, stack, 0)?; let executable = if cfg!(windows) && is_cmd_internal_command(&name.item) { PathBuf::from("cmd.exe") } else { + // Expand tilde on the name if it's a bare string (#13000) + let expanded_name = if is_bare_string(name_expr) { + expand_tilde(&name.item) + } else { + name.item.clone() + }; + + // Determine the PATH to be used and then use `which` to find it - though this has no + // effect if it's an absolute path already let paths = nu_engine::env::path_str(engine_state, stack, call.head)?; - let Some(executable) = which(&name.item, &paths, &cwd) else { + let Some(executable) = which(&expanded_name, &paths, &cwd) else { return Err(command_not_found( &name.item, call.head, diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 61a4261d8d..ad5e7d4f42 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -278,13 +278,14 @@ pub fn parse_external_call(working_set: &mut StateWorkingSet, spans: &[Span]) -> let arg = parse_expression(working_set, &[head_span]); Box::new(arg) } else { - let (contents, err) = unescape_unquote_string(&head_contents, head_span); + // Eval stage will unquote the string, so we don't bother with that here + let (contents, err) = unescape_string(&head_contents, head_span); if let Some(err) = err { working_set.error(err) } Box::new(Expression { - expr: Expr::String(contents), + expr: Expr::String(String::from_utf8_lossy(&contents).into_owned()), span: head_span, ty: Type::String, custom_completion: None,