From 41dbc641ccdd34ed05076b389d0d181b66baabbf Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Wed, 5 Jan 2022 11:26:01 +1100 Subject: [PATCH] Some cleanups for cd/PWD (#667) * Some cleanups for cd/PWD * Some cleanups for cd/PWD --- crates/nu-command/src/env/let_env.rs | 19 +++++++++++-- crates/nu-command/src/example_test.rs | 16 +++++++++++ crates/nu-command/src/path/expand.rs | 8 +++--- crates/nu-command/src/path/join.rs | 6 +--- crates/nu-command/src/path/relative_to.rs | 6 ++-- crates/nu-command/src/system/run_external.rs | 19 +++++++------ crates/nu-engine/src/env.rs | 1 + crates/nu-parser/src/parse_keywords.rs | 17 +++++++---- crates/nu-parser/src/parser.rs | 7 +++-- crates/nu-path/src/expansions.rs | 28 +++++++++---------- crates/nu-path/src/lib.rs | 2 +- crates/nu-protocol/src/engine/engine_state.rs | 11 ++++++++ src/main.rs | 9 +++--- src/tests.rs | 4 +++ 14 files changed, 104 insertions(+), 49 deletions(-) diff --git a/crates/nu-command/src/env/let_env.rs b/crates/nu-command/src/env/let_env.rs index b234a3c94..8b3da30d3 100644 --- a/crates/nu-command/src/env/let_env.rs +++ b/crates/nu-command/src/env/let_env.rs @@ -1,7 +1,7 @@ -use nu_engine::eval_expression; +use nu_engine::{current_dir, eval_expression}; use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; -use nu_protocol::{Category, PipelineData, Signature, SyntaxShape}; +use nu_protocol::{Category, PipelineData, Signature, SyntaxShape, Value}; #[derive(Clone)] pub struct LetEnv; @@ -43,7 +43,20 @@ impl Command for LetEnv { let rhs = eval_expression(engine_state, stack, keyword_expr)?; - stack.add_env_var(env_var, rhs); + if env_var == "PWD" { + let cwd = current_dir(engine_state, stack)?; + let rhs = rhs.as_string()?; + let rhs = nu_path::expand_path_with(rhs, cwd); + stack.add_env_var( + env_var, + Value::String { + val: rhs.to_string_lossy().to_string(), + span: call.head, + }, + ); + } else { + stack.add_env_var(env_var, rhs); + } Ok(PipelineData::new(call.head)) } } diff --git a/crates/nu-command/src/example_test.rs b/crates/nu-command/src/example_test.rs index a6570eeb9..ba6c949c7 100644 --- a/crates/nu-command/src/example_test.rs +++ b/crates/nu-command/src/example_test.rs @@ -57,6 +57,22 @@ pub fn test_examples(cmd: impl Command + 'static) { } let start = std::time::Instant::now(); + let mut stack = Stack::new(); + + // Set up PWD + stack.add_env_var( + "PWD".to_string(), + Value::String { + val: cwd.to_string_lossy().to_string(), + span: Span::test_data(), + }, + ); + let _ = engine_state.merge_delta( + StateWorkingSet::new(&*engine_state).render(), + Some(&mut stack), + &cwd, + ); + let (block, delta) = { let mut working_set = StateWorkingSet::new(&*engine_state); let (output, err) = parse(&mut working_set, None, example.example.as_bytes(), false); diff --git a/crates/nu-command/src/path/expand.rs b/crates/nu-command/src/path/expand.rs index 8600556a0..a4ae4c602 100644 --- a/crates/nu-command/src/path/expand.rs +++ b/crates/nu-command/src/path/expand.rs @@ -2,7 +2,7 @@ use std::path::Path; use nu_engine::env::current_dir_str; use nu_engine::CallExt; -use nu_path::{canonicalize_with, expand_path}; +use nu_path::{canonicalize_with, expand_path_with}; use nu_protocol::{engine::Command, Example, ShellError, Signature, Span, SyntaxShape, Value}; use super::PathSubcommandArguments; @@ -82,7 +82,7 @@ impl Command for SubCommand { Example { description: "Expand a relative path", example: r"'foo\..\bar' | path expand", - result: Some(Value::test_string("bar")), + result: None, }, ] } @@ -103,7 +103,7 @@ impl Command for SubCommand { Example { description: "Expand a relative path", example: "'foo/../bar' | path expand", - result: Some(Value::test_string("bar")), + result: None, }, ] } @@ -123,7 +123,7 @@ fn expand(path: &Path, span: Span, args: &Arguments) -> Value { ), } } else { - Value::string(expand_path(path).to_string_lossy(), span) + Value::string(expand_path_with(path, &args.cwd).to_string_lossy(), span) } } diff --git a/crates/nu-command/src/path/join.rs b/crates/nu-command/src/path/join.rs index 69e67be55..0a6e54d0b 100644 --- a/crates/nu-command/src/path/join.rs +++ b/crates/nu-command/src/path/join.rs @@ -38,11 +38,7 @@ impl Command for SubCommand { "Optionally operate by column path", Some('c'), ) - .optional( - "append", - SyntaxShape::Filepath, - "Path to append to the input", - ) + .optional("append", SyntaxShape::String, "Path to append to the input") } fn usage(&self) -> &str { diff --git a/crates/nu-command/src/path/relative_to.rs b/crates/nu-command/src/path/relative_to.rs index 8d2e15ec8..6f1913221 100644 --- a/crates/nu-command/src/path/relative_to.rs +++ b/crates/nu-command/src/path/relative_to.rs @@ -30,7 +30,7 @@ impl Command for SubCommand { Signature::build("path relative-to") .required( "path", - SyntaxShape::Filepath, + SyntaxShape::String, "Parent shared with the input path", ) .named( @@ -116,7 +116,9 @@ path."# fn relative_to(path: &Path, span: Span, args: &Arguments) -> Value { match path.strip_prefix(Path::new(&args.path.item)) { Ok(p) => Value::string(p.to_string_lossy(), span), - Err(_) => todo!(), + Err(e) => Value::Error { + error: ShellError::CantConvert(e.to_string(), "string".into(), span), + }, } } diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index bf5f93f95..e6708cfab 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -105,15 +105,16 @@ impl<'call> ExternalCommand<'call> { input: PipelineData, config: Config, ) -> Result { - let mut process = self.create_command(); let head = self.name.span; let ctrlc = engine_state.ctrlc.clone(); // TODO. We don't have a way to know the current directory // This should be information from the EvaluationContex or EngineState - if let Some(d) = self.env_vars.get("PWD") { + let mut process = if let Some(d) = self.env_vars.get("PWD") { + let mut process = self.create_command(d); process.current_dir(d); + process } else { return Err(ShellError::SpannedLabeledErrorHelp( "Current directory not found".to_string(), @@ -124,7 +125,7 @@ impl<'call> ExternalCommand<'call> { "It is required to define the current directory when running an external command." ).to_string(), )); - } + }; process.envs(&self.env_vars); @@ -239,7 +240,7 @@ impl<'call> ExternalCommand<'call> { } } - fn create_command(&self) -> CommandSys { + fn create_command(&self, cwd: &str) -> CommandSys { // in all the other cases shell out if cfg!(windows) { //TODO. This should be modifiable from the config file. @@ -248,22 +249,24 @@ impl<'call> ExternalCommand<'call> { if self.name.item.ends_with(".cmd") || self.name.item.ends_with(".bat") { self.spawn_cmd_command() } else { - self.spawn_simple_command() + self.spawn_simple_command(cwd) } } else if self.name.item.ends_with(".sh") { self.spawn_sh_command() } else { - self.spawn_simple_command() + self.spawn_simple_command(cwd) } } /// Spawn a command without shelling out to an external shell - fn spawn_simple_command(&self) -> std::process::Command { + fn spawn_simple_command(&self, cwd: &str) -> std::process::Command { let mut process = std::process::Command::new(&self.name.item); for arg in &self.args { let arg = trim_enclosing_quotes(arg); - let arg = nu_path::expand_path(arg).to_string_lossy().to_string(); + let arg = nu_path::expand_path_with(arg, cwd) + .to_string_lossy() + .to_string(); let arg = arg.replace("\\", "\\\\"); diff --git a/crates/nu-engine/src/env.rs b/crates/nu-engine/src/env.rs index 2f07d8d07..c75cba07d 100644 --- a/crates/nu-engine/src/env.rs +++ b/crates/nu-engine/src/env.rs @@ -144,6 +144,7 @@ pub fn current_dir_str(engine_state: &EngineState, stack: &Stack) -> Result= 2 { + let cwd = working_set.get_cwd(); for span in spans[1..].iter() { let (_, err) = parse_string(working_set, *span); error = error.or(err); @@ -657,7 +658,7 @@ pub fn parse_use( // TODO: Do not close over when loading module from file // It could be a file if let Ok(module_filename) = String::from_utf8(import_pattern.head.name) { - if let Ok(module_path) = canonicalize(&module_filename) { + if let Ok(module_path) = canonicalize_with(&module_filename, cwd) { let module_name = if let Some(stem) = module_path.file_stem() { stem.to_string_lossy().to_string() } else { @@ -1028,6 +1029,7 @@ pub fn parse_source( if name == b"source" { if let Some(decl_id) = working_set.find_decl(b"source") { + let cwd = working_set.get_cwd(); // Is this the right call to be using here? // Some of the others (`parse_let`) use it, some of them (`parse_hide`) don't. let (call, err) = parse_internal_call(working_set, spans[0], &spans[1..], decl_id); @@ -1038,7 +1040,7 @@ pub fn parse_source( let name_expr = working_set.get_span_contents(spans[1]); let name_expr = trim_quotes(name_expr); if let Ok(filename) = String::from_utf8(name_expr.to_vec()) { - if let Ok(path) = canonicalize(&filename) { + if let Ok(path) = canonicalize_with(&filename, cwd) { if let Ok(contents) = std::fs::read(&path) { // This will load the defs from the file into the // working set, if it was a successful parse. @@ -1124,6 +1126,7 @@ pub fn parse_register( ) -> (Statement, Option) { use nu_plugin::{get_signature, EncodingType, PluginDeclaration}; use nu_protocol::Signature; + let cwd = working_set.get_cwd(); // Checking that the function is used with the correct name // Maybe this is not necessary but it is a sanity check @@ -1176,6 +1179,7 @@ pub fn parse_register( // Extracting the required arguments from the call and keeping them together in a tuple // The ? operator is not used because the error has to be kept to be printed in the shell // For that reason the values are kept in a result that will be passed at the end of this call + let cwd_clone = cwd.clone(); let arguments = call .positional .get(0) @@ -1183,8 +1187,9 @@ pub fn parse_register( let name_expr = working_set.get_span_contents(expr.span); String::from_utf8(name_expr.to_vec()) .map_err(|_| ParseError::NonUtf8(expr.span)) - .and_then(|name| { - canonicalize(&name).map_err(|_| ParseError::FileNotFound(name, expr.span)) + .and_then(move |name| { + canonicalize_with(&name, cwd_clone) + .map_err(|_| ParseError::FileNotFound(name, expr.span)) }) .and_then(|path| { if path.exists() & path.is_file() { @@ -1231,7 +1236,7 @@ pub fn parse_register( String::from_utf8(shell_expr.to_vec()) .map_err(|_| ParseError::NonUtf8(expr.span)) .and_then(|name| { - canonicalize(&name).map_err(|_| ParseError::FileNotFound(name, expr.span)) + canonicalize_with(&name, cwd).map_err(|_| ParseError::FileNotFound(name, expr.span)) }) .and_then(|path| { if path.exists() & path.is_file() { diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 9095a990c..2bef82232 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1558,12 +1558,14 @@ pub fn parse_filepath( working_set: &mut StateWorkingSet, span: Span, ) -> (Expression, Option) { + let cwd = working_set.get_cwd(); + let bytes = working_set.get_span_contents(span); let bytes = trim_quotes(bytes); trace!("parsing: filepath"); if let Ok(token) = String::from_utf8(bytes.into()) { - let filepath = nu_path::expand_path(token); + let filepath = nu_path::expand_path_with(token, cwd); let filepath = filepath.to_string_lossy().to_string(); trace!("-- found {}", filepath); @@ -1789,9 +1791,10 @@ pub fn parse_glob_pattern( let bytes = trim_quotes(bytes); if let Ok(token) = String::from_utf8(bytes.into()) { + let cwd = working_set.get_cwd(); trace!("-- found {}", token); - let filepath = nu_path::expand_path(token); + let filepath = nu_path::expand_path_with(token, cwd); let filepath = filepath.to_string_lossy().to_string(); ( diff --git a/crates/nu-path/src/expansions.rs b/crates/nu-path/src/expansions.rs index 3393a5793..f23b6197f 100644 --- a/crates/nu-path/src/expansions.rs +++ b/crates/nu-path/src/expansions.rs @@ -26,19 +26,19 @@ where } } -/// Resolve all symbolic links and all components (tilde, ., .., ...+) and return the path in its -/// absolute form. -/// -/// Fails under the same conditions as -/// [std::fs::canonicalize](https://doc.rust-lang.org/std/fs/fn.canonicalize.html). -pub fn canonicalize(path: impl AsRef) -> io::Result { +fn canonicalize(path: impl AsRef) -> io::Result { let path = expand_tilde(path); let path = expand_ndots(path); dunce::canonicalize(path) } -/// Same as canonicalize() but the input path is specified relative to another path +/// Resolve all symbolic links and all components (tilde, ., .., ...+) and return the path in its +/// absolute form. +/// +/// Fails under the same conditions as +/// [std::fs::canonicalize](https://doc.rust-lang.org/std/fs/fn.canonicalize.html). +/// The input path is specified relative to another path pub fn canonicalize_with(path: P, relative_to: Q) -> io::Result where P: AsRef, @@ -49,6 +49,12 @@ where canonicalize(path) } +fn expand_path(path: impl AsRef) -> PathBuf { + let path = expand_tilde(path); + let path = expand_ndots(path); + expand_dots(path) +} + /// Resolve only path components (tilde, ., .., ...+), if possible. /// /// The function works in a "best effort" mode: It does not fail but rather returns the unexpanded @@ -57,13 +63,7 @@ where /// Furthermore, unlike canonicalize(), it does not use sys calls (such as readlink). /// /// Does not convert to absolute form nor does it resolve symlinks. -pub fn expand_path(path: impl AsRef) -> PathBuf { - let path = expand_tilde(path); - let path = expand_ndots(path); - expand_dots(path) -} - -/// Same as expand_path() but the input path is specified relative to another path +/// The input path is specified relative to another path pub fn expand_path_with(path: P, relative_to: Q) -> PathBuf where P: AsRef, diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index 5cd1edf1c..e6fbf4e6a 100644 --- a/crates/nu-path/src/lib.rs +++ b/crates/nu-path/src/lib.rs @@ -4,7 +4,7 @@ mod helpers; mod tilde; mod util; -pub use expansions::{canonicalize, canonicalize_with, expand_path, expand_path_with}; +pub use expansions::{canonicalize_with, expand_path_with}; pub use helpers::{config_dir, home_dir}; pub use tilde::expand_tilde; pub use util::trim_trailing_slash; diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index f1ac009f1..9c4ac5ba2 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -235,6 +235,8 @@ impl EngineState { } } + // FIXME: permanent state changes like this hopefully in time can be removed + // and be replaced by just passing the cwd in where needed std::env::set_current_dir(cwd)?; Ok(()) @@ -1009,6 +1011,15 @@ impl<'a> StateWorkingSet<'a> { last.aliases.insert(name, replacement); } + pub fn get_cwd(&self) -> String { + let pwd = self + .permanent_state + .env_vars + .get("PWD") + .expect("internal error: can't find PWD"); + pwd.as_string().expect("internal error: PWD not a string") + } + pub fn set_variable_type(&mut self, var_id: VarId, ty: Type) { let num_permanent_vars = self.permanent_state.num_vars(); if var_id < num_permanent_vars { diff --git a/src/main.rs b/src/main.rs index 7d899821d..5af6a4d5f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -117,6 +117,9 @@ fn main() -> Result<()> { // End ctrl-c protection section if let Some(path) = std::env::args().nth(1) { + // First, set up env vars as strings only + gather_parent_env_vars(&mut engine_state); + let file = std::fs::read(&path).into_diagnostic()?; let (block, delta) = { @@ -139,9 +142,6 @@ fn main() -> Result<()> { let mut stack = nu_protocol::engine::Stack::new(); - // First, set up env vars as strings only - gather_parent_env_vars(&mut engine_state); - // Set up our initial config to start from stack.vars.insert( CONFIG_VARIABLE_ID, @@ -436,6 +436,7 @@ fn main() -> Result<()> { // Check if this is a single call to a directory, if so auto-cd let cwd = nu_engine::env::current_dir_str(&engine_state, &stack)?; let path = nu_path::expand_path_with(&s, &cwd); + let orig = s.clone(); if (orig.starts_with('.') @@ -451,7 +452,7 @@ fn main() -> Result<()> { stack.add_env_var( "PWD".into(), Value::String { - val: s.clone(), + val: path.to_string_lossy().to_string(), span: Span { start: 0, end: 0 }, }, ); diff --git a/src/tests.rs b/src/tests.rs index dffc9aeb7..35cc4e966 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -53,6 +53,10 @@ pub fn fail_test(input: &str, expected: &str) -> TestResult { let mut cmd = Command::cargo_bin("engine-q")?; cmd.arg(name); + cmd.env( + "PWD", + std::env::current_dir().expect("Can't get current dir"), + ); writeln!(file, "{}", input)?;