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

@@ -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"
+source = "git+https://github.com/nushell/reedline.git?branch=main#b1344f6a653b7c37154a1dd79b3c4e34afe6cfc9"
-checksum = "971ac45071721aae18927f3feb7e4c2b95cce387d96af185c5103166d332e55c"
 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",
 ]

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

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 = []

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() {
+        let buffer_editor = get_editor(engine_state, stack, Span::unknown());
-            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())
-                })
-        };
 
-        line_editor = if let Some(buffer_editor) = buffer_editor {
+        line_editor = if let Ok((cmd, args)) = buffer_editor {
-            line_editor.with_buffer_editor(buffer_editor, "nu".into())
+            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
         };

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<String>), 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::<Result<_, ShellError>>()?;
+                    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<String>"),
+            "Specify an executable here".into(),
+            Some(value.span()),
+            Some(HELP_MSG.into()),
+            vec![],
+        )),
+        x => Err(ShellError::CantConvert {
+            to_type: "string or list<string>".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<String>), 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![],
+        ))
+    }
+}

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;

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;

crates/nu-command/src/env/config/utils.rs

@@ -1,48 +1,10 @@
 use std::collections::HashMap;
 use std::path::PathBuf;
 
-use nu_protocol::{
+use nu_protocol::{Span, Spanned};
-    engine::{EngineState, Stack},
-    ShellError, Span, Spanned,
-};
 
 use crate::ExternalCommand;
 
-pub(crate) fn get_editor(
-    engine_state: &EngineState,
-    stack: &mut Stack,
-    span: Span,
-) -> Result<(String, Vec<String>), 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::<Vec<String>>(),
-        ))
-    } else {
-        Ok((editor, Vec::new()))
-    }
-}
-
 pub(crate) fn gen_command(
     span: Span,
     config_path: PathBuf,

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" => {
+                    "buffer_editor" => match value {
-                        if let Ok(v) = value.as_string() {
+                        Value::Nothing { .. } | Value::String { .. } => {
-                            config.buffer_editor = v.to_lowercase();
+                            config.buffer_editor = value.clone();
-                        } else {
-                            invalid!(Some(span), "should be a string");
                         }
-                    }
+                        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<string>, or null");
+                        }
+                    },
                     "show_banner" => {
                         try_bool!(cols, vals, index, span, show_banner);
                     }