Some cleanups for cd/PWD (#667)

* Some cleanups for cd/PWD

* Some cleanups for cd/PWD

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))
     }
 }

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);

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)
     }
 }
 

crates/nu-command/src/path/join.rs

@@ -38,11 +38,7 @@ impl Command for SubCommand {
                 "Optionally operate by column path",
                 Some('c'),
             )
-            .optional(
+            .optional("append", SyntaxShape::String, "Path to append to the input")
-                "append",
-                SyntaxShape::Filepath,
-                "Path to append to the input",
-            )
     }
 
     fn usage(&self) -> &str {

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),
+        },
     }
 }
 

crates/nu-command/src/system/run_external.rs

@@ -105,15 +105,16 @@ impl<'call> ExternalCommand<'call> {
         input: PipelineData,
         config: Config,
     ) -> Result<PipelineData, ShellError> {
-        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("\\", "\\\\");
 

crates/nu-engine/src/env.rs

@@ -144,6 +144,7 @@ pub fn current_dir_str(engine_state: &EngineState, stack: &Stack) -> Result<Stri
                 if Path::new(&cwd).is_absolute() {
                     Ok(cwd)
                 } else {
+                    println!("cwd is: {}", cwd);
                     Err(ShellError::LabeledError(
                             "Invalid current directory".to_string(),
                             format!("The 'PWD' environment variable must be set to an absolute path. Found: '{}'", cwd)

crates/nu-parser/src/parse_keywords.rs

@@ -1,4 +1,4 @@
-use nu_path::canonicalize;
+use nu_path::canonicalize_with;
 use nu_protocol::{
     ast::{
         Block, Call, Expr, Expression, ImportPattern, ImportPatternHead, ImportPatternMember,
@@ -640,6 +640,7 @@ pub fn parse_use(
     let bytes = working_set.get_span_contents(spans[0]);
 
     if bytes == b"use" && spans.len() >= 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<ParseError>) {
     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| {
+                .and_then(move |name| {
-                    canonicalize(&name).map_err(|_| ParseError::FileNotFound(name, expr.span))
+                    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() {

crates/nu-parser/src/parser.rs

@@ -1558,12 +1558,14 @@ pub fn parse_filepath(
     working_set: &mut StateWorkingSet,
     span: Span,
 ) -> (Expression, Option<ParseError>) {
+    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();
 
         (

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
+fn canonicalize(path: impl AsRef<Path>) -> io::Result<PathBuf> {
-/// 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<Path>) -> io::Result<PathBuf> {
     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<P, Q>(path: P, relative_to: Q) -> io::Result<PathBuf>
 where
     P: AsRef<Path>,
@@ -49,6 +49,12 @@ where
     canonicalize(path)
 }
 
+fn expand_path(path: impl AsRef<Path>) -> 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<Path>) -> PathBuf {
+/// The input path is specified relative to another path
-    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
 pub fn expand_path_with<P, Q>(path: P, relative_to: Q) -> PathBuf
 where
     P: AsRef<Path>,

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;

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 {

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 },
                             },
                         );

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)?;