From 80a183dde219acde37064539ef8073f265f9a007 Mon Sep 17 00:00:00 2001 From: Stefan Holderbach Date: Fri, 29 Sep 2023 16:36:03 +0200 Subject: [PATCH] Fix editor config for reedline and `config nu/env` (#10535) # Description This merges @horasal 's changes from #10246 and #10269 Closes #10205 Closes #8714 Fixes the bug that editor paths with spaces are unusable Closes #10210 Closes #10269 # User-Facing Changes You can now either pass a string with the name of the executable or a list with the executable and any flags to `$env.config.buffer_editor`/`$env.EDITOR`/`$env.VISUAL` Both the external buffer editor of reedline (by default bound to `Ctrl-o`) and the commands `config nu` and `config env` will respect those variables in the following order: 1. `$env.config.buffer_editor` 2. `$env.EDITOR` 3. `$env.VISUAL` Example: ``` $env.EDITOR = "nvim" # The system-wide EDITOR is neovim $env.config.buffer_editor = ["vim" "-p2"] # Force vim to open two tabs (not particularly useful) $env.config.buffer_editor = null # Unset `buffer_editor` -> Uses `$env.EDITOR` ergo nvim ``` # Tests + Formatting None --------- Co-authored-by: Horasal <1991933+horasal@users.noreply.github.com> --- Cargo.lock | 8 +-- Cargo.toml | 2 +- crates/nu-cli/Cargo.toml | 1 + crates/nu-cli/src/repl.rs | 30 ++++---- crates/nu-cmd-base/src/util.rs | 69 +++++++++++++++++++ .../nu-command/src/env/config/config_env.rs | 3 +- crates/nu-command/src/env/config/config_nu.rs | 3 +- crates/nu-command/src/env/config/utils.rs | 40 +---------- crates/nu-protocol/src/config.rs | 23 ++++--- 9 files changed, 108 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dc0ed24472..92fee234b5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2746,6 +2746,7 @@ dependencies = [ "rstest", "sysinfo", "unicode-segmentation", + "uuid", ] [[package]] @@ -4291,8 +4292,7 @@ dependencies = [ [[package]] name = "reedline" version = "0.24.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "971ac45071721aae18927f3feb7e4c2b95cce387d96af185c5103166d332e55c" +source = "git+https://github.com/nushell/reedline.git?branch=main#b1344f6a653b7c37154a1dd79b3c4e34afe6cfc9" dependencies = [ "chrono", "crossterm 0.27.0", @@ -5705,9 +5705,9 @@ checksum = "37db35583cf0ad896592892034f281ef0906c893b3b6d901acf3918357f28199" [[package]] name = "uuid" -version = "1.4.0" +version = "1.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d023da39d1fde5a8a3fe1f3e01ca9632ada0a63e9797de55a879d6e2236277be" +checksum = "79daa5ed5740825c40b389c5e50312b9c86df53fccd33f281df655642b43869d" dependencies = [ "getrandom", ] diff --git a/Cargo.toml b/Cargo.toml index d823a1b2ed..759ed1c0cb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -164,7 +164,7 @@ bench = false # To use a development version of a dependency please use a global override here # changing versions in each sub-crate of the workspace is tedious [patch.crates-io] -# reedline = { git = "https://github.com/nushell/reedline.git", branch = "main"} +reedline = { git = "https://github.com/nushell/reedline.git", branch = "main"} # nu-ansi-term = {git = "https://github.com/nushell/nu-ansi-term.git", branch = "main"} # Criterion benchmarking setup diff --git a/crates/nu-cli/Cargo.toml b/crates/nu-cli/Cargo.toml index 447a18b01c..4b1df657f5 100644 --- a/crates/nu-cli/Cargo.toml +++ b/crates/nu-cli/Cargo.toml @@ -38,6 +38,7 @@ once_cell = "1.18" percent-encoding = "2" sysinfo = "0.29" unicode-segmentation = "1.10" +uuid = { version = "1.4.1", features = ["v4"] } [features] plugin = [] diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 9f34021e00..ab926b2f40 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -8,8 +8,8 @@ use crate::{ use crossterm::cursor::SetCursorStyle; use log::{trace, warn}; use miette::{ErrReport, IntoDiagnostic, Result}; -use nu_cmd_base::hook::eval_hook; use nu_cmd_base::util::get_guaranteed_cwd; +use nu_cmd_base::{hook::eval_hook, util::get_editor}; use nu_color_config::StyleComputer; use nu_engine::convert_env_values; use nu_parser::{lex, parse, trim_quotes_str}; @@ -26,6 +26,7 @@ use reedline::{ SqliteBackedHistory, Vi, }; use std::{ + env::temp_dir, io::{self, IsTerminal, Write}, path::Path, sync::atomic::Ordering, @@ -94,6 +95,7 @@ pub fn evaluate_repl( let mut start_time = std::time::Instant::now(); let mut line_editor = Reedline::create(); + let temp_file = temp_dir().join(format!("{}.nu", uuid::Uuid::new_v4())); // Now that reedline is created, get the history session id and store it in engine_state store_history_id_in_engine(engine_state, &line_editor); @@ -330,23 +332,17 @@ pub fn evaluate_repl( ); start_time = std::time::Instant::now(); - let buffer_editor = if !config.buffer_editor.is_empty() { - Some(config.buffer_editor.clone()) - } else { - stack - .get_env_var(engine_state, "EDITOR") - .map(|v| v.as_string().unwrap_or_default()) - .filter(|v| !v.is_empty()) - .or_else(|| { - stack - .get_env_var(engine_state, "VISUAL") - .map(|v| v.as_string().unwrap_or_default()) - .filter(|v| !v.is_empty()) - }) - }; + let buffer_editor = get_editor(engine_state, stack, Span::unknown()); - line_editor = if let Some(buffer_editor) = buffer_editor { - line_editor.with_buffer_editor(buffer_editor, "nu".into()) + line_editor = if let Ok((cmd, args)) = buffer_editor { + let mut command = std::process::Command::new(&cmd); + command.args(args).envs( + engine_state + .render_env_vars() + .into_iter() + .filter_map(|(k, v)| v.as_string().ok().map(|v| (k, v))), + ); + line_editor.with_buffer_editor(command, temp_file.clone()) } else { line_editor }; diff --git a/crates/nu-cmd-base/src/util.rs b/crates/nu-cmd-base/src/util.rs index 62f52a6690..dc927b5484 100644 --- a/crates/nu-cmd-base/src/util.rs +++ b/crates/nu-cmd-base/src/util.rs @@ -55,3 +55,72 @@ pub fn process_range(range: &Range) -> Result<(isize, isize), MakeRangeError> { Ok((start, end)) } + +const HELP_MSG: &str = "Nushell's config file can be found with the command: $nu.config-path. \ +For more help: (https://nushell.sh/book/configuration.html#configurations-with-built-in-commands)"; + +fn get_editor_commandline( + value: &Value, + var_name: &str, +) -> Result<(String, Vec), ShellError> { + match value { + Value::String { val, .. } if !val.is_empty() => Ok((val.to_string(), Vec::new())), + Value::List { vals, .. } if !vals.is_empty() => { + let mut editor_cmd = vals.iter().map(|l| l.as_string()); + match editor_cmd.next().transpose()? { + Some(editor) if !editor.is_empty() => { + let params = editor_cmd.collect::>()?; + Ok((editor, params)) + } + _ => Err(ShellError::GenericError( + "Editor executable is missing".into(), + "Set the first element to an executable".into(), + Some(value.span()), + Some(HELP_MSG.into()), + vec![], + )), + } + } + Value::String { .. } | Value::List { .. } => Err(ShellError::GenericError( + format!("{var_name} should be a non-empty string or list"), + "Specify an executable here".into(), + Some(value.span()), + Some(HELP_MSG.into()), + vec![], + )), + x => Err(ShellError::CantConvert { + to_type: "string or list".into(), + from_type: x.get_type().to_string(), + span: value.span(), + help: None, + }), + } +} + +pub fn get_editor( + engine_state: &EngineState, + stack: &mut Stack, + span: Span, +) -> Result<(String, Vec), ShellError> { + let config = engine_state.get_config(); + let env_vars = stack.get_env_vars(engine_state); + + if let Ok(buff_editor) = + get_editor_commandline(&config.buffer_editor, "$env.config.buffer_editor") + { + Ok(buff_editor) + } else if let Some(value) = env_vars.get("EDITOR") { + get_editor_commandline(value, "$env.EDITOR") + } else if let Some(value) = env_vars.get("VISUAL") { + get_editor_commandline(value, "$env.VISUAL") + } else { + Err(ShellError::GenericError( + "No editor configured".into(), + "Please specify one via `$env.config.buffer_editor` or `$env.EDITOR`/`$env.VISUAL`" + .into(), + Some(span), + Some(HELP_MSG.into()), + vec![], + )) + } +} diff --git a/crates/nu-command/src/env/config/config_env.rs b/crates/nu-command/src/env/config/config_env.rs index 45245b2492..86f54abee0 100644 --- a/crates/nu-command/src/env/config/config_env.rs +++ b/crates/nu-command/src/env/config/config_env.rs @@ -5,7 +5,8 @@ use nu_protocol::{ Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Type, Value, }; -use super::utils::{gen_command, get_editor}; +use super::utils::gen_command; +use nu_cmd_base::util::get_editor; #[derive(Clone)] pub struct ConfigEnv; diff --git a/crates/nu-command/src/env/config/config_nu.rs b/crates/nu-command/src/env/config/config_nu.rs index 7746a68ca2..90c04fd92e 100644 --- a/crates/nu-command/src/env/config/config_nu.rs +++ b/crates/nu-command/src/env/config/config_nu.rs @@ -5,7 +5,8 @@ use nu_protocol::{ Category, Example, IntoPipelineData, PipelineData, ShellError, Signature, Type, Value, }; -use super::utils::{gen_command, get_editor}; +use super::utils::gen_command; +use nu_cmd_base::util::get_editor; #[derive(Clone)] pub struct ConfigNu; diff --git a/crates/nu-command/src/env/config/utils.rs b/crates/nu-command/src/env/config/utils.rs index a7d50eab2c..0c6e5d4ff7 100644 --- a/crates/nu-command/src/env/config/utils.rs +++ b/crates/nu-command/src/env/config/utils.rs @@ -1,48 +1,10 @@ use std::collections::HashMap; use std::path::PathBuf; -use nu_protocol::{ - engine::{EngineState, Stack}, - ShellError, Span, Spanned, -}; +use nu_protocol::{Span, Spanned}; use crate::ExternalCommand; -pub(crate) fn get_editor( - engine_state: &EngineState, - stack: &mut Stack, - span: Span, -) -> Result<(String, Vec), ShellError> { - let config = engine_state.get_config(); - let env_vars = stack.get_env_vars(engine_state); - let editor = if !config.buffer_editor.is_empty() { - Ok(config.buffer_editor.clone()) - } else if let Some(value) = env_vars.get("EDITOR") { - value.as_string() - } else if let Some(value) = env_vars.get("VISUAL") { - value.as_string() - } else { - Err(ShellError::GenericError( - "No editor configured".into(), - "Please specify one via environment variables $EDITOR or $VISUAL".into(), - Some(span), - Some( - "Nushell's config file can be found with the command: $nu.config-path. For more help: (https://nushell.sh/book/configuration.html#configurations-with-built-in-commands)" - .into(), - ), - vec![], - )) - }?; - if let Some((a, b)) = editor.split_once(' ') { - Ok(( - a.to_string(), - b.split(' ').map(|s| s.to_string()).collect::>(), - )) - } else { - Ok((editor, Vec::new())) - } -} - pub(crate) fn gen_command( span: Span, config_path: PathBuf, diff --git a/crates/nu-protocol/src/config.rs b/crates/nu-protocol/src/config.rs index 3b3f4297cc..7d268cf3c3 100644 --- a/crates/nu-protocol/src/config.rs +++ b/crates/nu-protocol/src/config.rs @@ -98,7 +98,7 @@ pub struct Config { pub hooks: Hooks, pub rm_always_trash: bool, pub shell_integration: bool, - pub buffer_editor: String, + pub buffer_editor: Value, pub table_index_mode: TableIndexMode, pub cd_with_abbreviations: bool, pub case_sensitive_completions: bool, @@ -167,7 +167,7 @@ impl Default for Config { use_grid_icons: true, footer_mode: FooterMode::RowCount(25), float_precision: 2, - buffer_editor: String::new(), + buffer_editor: Value::nothing(Span::unknown()), use_ansi_coloring: true, bracketed_paste: true, edit_mode: "emacs".into(), @@ -1187,13 +1187,20 @@ impl Value { "shell_integration" => { try_bool!(cols, vals, index, span, shell_integration); } - "buffer_editor" => { - if let Ok(v) = value.as_string() { - config.buffer_editor = v.to_lowercase(); - } else { - invalid!(Some(span), "should be a string"); + "buffer_editor" => match value { + Value::Nothing { .. } | Value::String { .. } => { + config.buffer_editor = value.clone(); } - } + Value::List { vals, .. } + if vals.iter().all(|val| matches!(val, Value::String { .. })) => + { + config.buffer_editor = value.clone(); + } + _ => { + dbg!(value); + invalid!(Some(span), "should be a string, list, or null"); + } + }, "show_banner" => { try_bool!(cols, vals, index, span, show_banner); }