diff --git a/Cargo.lock b/Cargo.lock index 6dcf38f7cb..f55ed5ceb4 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2852,6 +2852,7 @@ dependencies = [ "reedline", "rstest", "sysinfo", + "tempfile", "unicode-segmentation", "uuid", "which", diff --git a/crates/nu-cli/Cargo.toml b/crates/nu-cli/Cargo.toml index 3423450cf3..e631f7ccf6 100644 --- a/crates/nu-cli/Cargo.toml +++ b/crates/nu-cli/Cargo.toml @@ -15,6 +15,7 @@ nu-cmd-lang = { path = "../nu-cmd-lang", version = "0.93.1" } nu-command = { path = "../nu-command", version = "0.93.1" } nu-test-support = { path = "../nu-test-support", version = "0.93.1" } rstest = { workspace = true, default-features = false } +tempfile = { workspace = true } [dependencies] nu-cmd-base = { path = "../nu-cmd-base", version = "0.93.1" } diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 3fe51db496..2482a7920e 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -870,7 +870,7 @@ fn parse_operation( let tokens = lex(s.as_bytes(), 0, &[], &[], false); // Check if this is a single call to a directory, if so auto-cd #[allow(deprecated)] - let cwd = nu_engine::env::current_dir_str(engine_state, stack)?; + let cwd = nu_engine::env::current_dir_str(engine_state, stack).unwrap_or_default(); let mut orig = s.clone(); if orig.starts_with('`') { orig = trim_quotes_str(&orig).to_string() @@ -927,7 +927,10 @@ fn do_auto_cd( //FIXME: this only changes the current scope, but instead this environment variable //should probably be a block that loads the information from the state in the overlay - stack.add_env_var("PWD".into(), Value::string(path.clone(), Span::unknown())); + if let Err(err) = stack.set_cwd(&path) { + report_error_new(engine_state, &err); + return; + }; let cwd = Value::string(cwd, span); let shells = stack.get_env_var(engine_state, "NUSHELL_SHELLS"); @@ -1479,3 +1482,136 @@ fn are_session_ids_in_sync() { engine_state.history_session_id ); } + +#[cfg(test)] +mod test_auto_cd { + use super::{do_auto_cd, parse_operation, ReplOperation}; + use nu_protocol::engine::{EngineState, Stack}; + use std::path::Path; + use tempfile::tempdir; + + /// Create a symlink. Works on both Unix and Windows. + #[cfg(any(unix, windows))] + fn symlink(original: impl AsRef, link: impl AsRef) -> std::io::Result<()> { + #[cfg(unix)] + { + std::os::unix::fs::symlink(original, link) + } + #[cfg(windows)] + { + if original.as_ref().is_dir() { + std::os::windows::fs::symlink_dir(original, link) + } else { + std::os::windows::fs::symlink_file(original, link) + } + } + } + + /// Run one test case on the auto-cd feature. PWD is initially set to + /// `before`, and after `input` is parsed and evaluated, PWD should be + /// changed to `after`. + #[track_caller] + fn check(before: impl AsRef, input: &str, after: impl AsRef) { + // Setup EngineState and Stack. + let mut engine_state = EngineState::new(); + let mut stack = Stack::new(); + stack.set_cwd(before).unwrap(); + + // Parse the input. It must be an auto-cd operation. + let op = parse_operation(input.to_string(), &engine_state, &stack).unwrap(); + let ReplOperation::AutoCd { cwd, target, span } = op else { + panic!("'{}' was not parsed into an auto-cd operation", input) + }; + + // Perform the auto-cd operation. + do_auto_cd(target, cwd, &mut stack, &mut engine_state, span); + let updated_cwd = engine_state.cwd(Some(&stack)).unwrap(); + + // Check that `updated_cwd` and `after` point to the same place. They + // don't have to be byte-wise equal (on Windows, the 8.3 filename + // conversion messes things up), + let updated_cwd = std::fs::canonicalize(updated_cwd).unwrap(); + let after = std::fs::canonicalize(after).unwrap(); + assert_eq!(updated_cwd, after); + } + + #[test] + fn auto_cd_root() { + let tempdir = tempdir().unwrap(); + let root = if cfg!(windows) { r"C:\" } else { "/" }; + check(&tempdir, root, root); + } + + #[test] + fn auto_cd_tilde() { + let tempdir = tempdir().unwrap(); + let home = nu_path::home_dir().unwrap(); + check(&tempdir, "~", home); + } + + #[test] + fn auto_cd_dot() { + let tempdir = tempdir().unwrap(); + check(&tempdir, ".", &tempdir); + } + + #[test] + fn auto_cd_double_dot() { + let tempdir = tempdir().unwrap(); + let dir = tempdir.path().join("foo"); + std::fs::create_dir_all(&dir).unwrap(); + check(dir, "..", &tempdir); + } + + #[test] + fn auto_cd_triple_dot() { + let tempdir = tempdir().unwrap(); + let dir = tempdir.path().join("foo").join("bar"); + std::fs::create_dir_all(&dir).unwrap(); + check(dir, "...", &tempdir); + } + + #[test] + fn auto_cd_relative() { + let tempdir = tempdir().unwrap(); + let foo = tempdir.path().join("foo"); + let bar = tempdir.path().join("bar"); + std::fs::create_dir_all(&foo).unwrap(); + std::fs::create_dir_all(&bar).unwrap(); + + let input = if cfg!(windows) { r"..\bar" } else { "../bar" }; + check(foo, input, bar); + } + + #[test] + fn auto_cd_trailing_slash() { + let tempdir = tempdir().unwrap(); + let dir = tempdir.path().join("foo"); + std::fs::create_dir_all(&dir).unwrap(); + + let input = if cfg!(windows) { r"foo\" } else { "foo/" }; + check(&tempdir, input, dir); + } + + #[test] + fn auto_cd_symlink() { + let tempdir = tempdir().unwrap(); + let dir = tempdir.path().join("foo"); + std::fs::create_dir_all(&dir).unwrap(); + let link = tempdir.path().join("link"); + symlink(&dir, &link).unwrap(); + + let input = if cfg!(windows) { r".\link" } else { "./link" }; + check(&tempdir, input, link); + } + + #[test] + #[should_panic(expected = "was not parsed into an auto-cd operation")] + fn auto_cd_nonexistent_directory() { + let tempdir = tempdir().unwrap(); + let dir = tempdir.path().join("foo"); + + let input = if cfg!(windows) { r"foo\" } else { "foo/" }; + check(&tempdir, input, dir); + } +} diff --git a/crates/nu-cmd-base/src/util.rs b/crates/nu-cmd-base/src/util.rs index 4d19fdb3c6..9c63dec836 100644 --- a/crates/nu-cmd-base/src/util.rs +++ b/crates/nu-cmd-base/src/util.rs @@ -1,6 +1,6 @@ use nu_protocol::{ engine::{EngineState, Stack}, - report_error_new, Range, ShellError, Span, Value, + Range, ShellError, Span, Value, }; use std::{ops::Bound, path::PathBuf}; @@ -13,10 +13,9 @@ pub fn get_init_cwd() -> PathBuf { } pub fn get_guaranteed_cwd(engine_state: &EngineState, stack: &Stack) -> PathBuf { - engine_state.cwd(Some(stack)).unwrap_or_else(|e| { - report_error_new(engine_state, &e); - crate::util::get_init_cwd() - }) + engine_state + .cwd(Some(stack)) + .unwrap_or(crate::util::get_init_cwd()) } type MakeRangeError = fn(&str, Span) -> ShellError; diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index f90e9bfd2a..57fe1c17e3 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -1,3 +1,4 @@ +use nu_cmd_base::util::get_init_cwd; use nu_engine::command_prelude::*; use nu_utils::filesystem::{have_permission, PermissionResult}; @@ -39,7 +40,10 @@ impl Command for Cd { ) -> Result { let physical = call.has_flag(engine_state, stack, "physical")?; let path_val: Option> = call.opt(engine_state, stack, 0)?; - let cwd = engine_state.cwd(Some(stack))?; + + // If getting PWD failed, default to the initial directory. This way, the + // user can use `cd` to recover PWD to a good state. + let cwd = engine_state.cwd(Some(stack)).unwrap_or(get_init_cwd()); let path_val = { if let Some(path) = path_val { @@ -52,13 +56,13 @@ impl Command for Cd { } }; - let (path, span) = match path_val { + let path = match path_val { Some(v) => { if v.item == "-" { if let Some(oldpwd) = stack.get_env_var(engine_state, "OLDPWD") { - (oldpwd.to_path()?, v.span) + oldpwd.to_path()? } else { - (cwd, v.span) + cwd } } else { // Trim whitespace from the end of path. @@ -66,7 +70,7 @@ impl Command for Cd { &v.item.trim_end_matches(|x| matches!(x, '\x09'..='\x0d')); // If `--physical` is specified, canonicalize the path; otherwise expand the path. - let path = if physical { + if physical { if let Ok(path) = nu_path::canonicalize_with(path_no_whitespace, &cwd) { if !path.is_dir() { return Err(ShellError::NotADirectory { span: v.span }); @@ -90,19 +94,12 @@ impl Command for Cd { return Err(ShellError::NotADirectory { span: v.span }); }; path - }; - (path, v.span) + } } } - None => { - let path = nu_path::expand_tilde("~"); - (path, call.head) - } + None => nu_path::expand_tilde("~"), }; - // Strip the trailing slash from the new path. This is required for PWD. - let path = nu_path::strip_trailing_slash(&path); - // Set OLDPWD. // We're using `Stack::get_env_var()` instead of `EngineState::cwd()` to avoid a conversion roundtrip. if let Some(oldpwd) = stack.get_env_var(engine_state, "PWD") { @@ -113,7 +110,7 @@ impl Command for Cd { //FIXME: this only changes the current scope, but instead this environment variable //should probably be a block that loads the information from the state in the overlay PermissionResult::PermissionOk => { - stack.add_env_var("PWD".into(), Value::string(path.to_string_lossy(), span)); + stack.set_cwd(path)?; Ok(PipelineData::empty()) } PermissionResult::PermissionDenied(reason) => Err(ShellError::IOError { diff --git a/crates/nu-command/tests/commands/cd.rs b/crates/nu-command/tests/commands/cd.rs index c7c30528e8..87af52aa4d 100644 --- a/crates/nu-command/tests/commands/cd.rs +++ b/crates/nu-command/tests/commands/cd.rs @@ -312,3 +312,17 @@ fn cd_permission_denied_folder() { assert!(actual.err.contains("Folder is not able to read")); }); } + +#[test] +#[cfg(unix)] +fn pwd_recovery() { + let nu = nu_test_support::fs::executable_path().display().to_string(); + let tmpdir = std::env::temp_dir().join("foobar").display().to_string(); + + // We `cd` into a temporary directory, then spawn another `nu` process to + // delete that directory. Then we attempt to recover by running `cd /`. + let cmd = format!("mkdir {tmpdir}; cd {tmpdir}; {nu} -c 'cd /; rm -r {tmpdir}'; cd /; pwd"); + let actual = nu!(cmd); + + assert_eq!(actual.out, "/"); +} diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 6324b35ae7..8b4333dc55 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -654,7 +654,7 @@ impl Eval for EvalRuntime { } else if quoted { Ok(Value::string(path, span)) } else { - let cwd = engine_state.cwd(Some(stack))?; + let cwd = engine_state.cwd(Some(stack)).unwrap_or_default(); let path = expand_path_with(path, cwd, true); Ok(Value::string(path.to_string_lossy(), span)) diff --git a/crates/nu-protocol/src/engine/engine_state.rs b/crates/nu-protocol/src/engine/engine_state.rs index 4a0ff4c4ae..bea49b5d6c 100644 --- a/crates/nu-protocol/src/engine/engine_state.rs +++ b/crates/nu-protocol/src/engine/engine_state.rs @@ -931,13 +931,12 @@ impl EngineState { /// directory on the stack that have yet to be merged into the engine state. pub fn cwd(&self, stack: Option<&Stack>) -> Result { // Helper function to create a simple generic error. - // Its messages are not especially helpful, but these errors don't occur often, so it's probably fine. - fn error(msg: &str) -> Result { + fn error(msg: &str, cwd: impl AsRef) -> Result { Err(ShellError::GenericError { error: msg.into(), - msg: "".into(), + msg: format!("$env.PWD = {}", cwd.as_ref().display()), span: None, - help: None, + help: Some("Use `cd` to reset $env.PWD into a good state".into()), inner: vec![], }) } @@ -967,21 +966,21 @@ impl EngineState { // Technically, a root path counts as "having trailing slashes", but // for the purpose of PWD, a root path is acceptable. if !is_root(&path) && has_trailing_slash(&path) { - error("$env.PWD contains trailing slashes") + error("$env.PWD contains trailing slashes", path) } else if !path.is_absolute() { - error("$env.PWD is not an absolute path") + error("$env.PWD is not an absolute path", path) } else if !path.exists() { - error("$env.PWD points to a non-existent directory") + error("$env.PWD points to a non-existent directory", path) } else if !path.is_dir() { - error("$env.PWD points to a non-directory") + error("$env.PWD points to a non-directory", path) } else { Ok(path) } } else { - error("$env.PWD is not a string") + error("$env.PWD is not a string", format!("{pwd:?}")) } } else { - error("$env.PWD not found") + error("$env.PWD not found", "") } } diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 2fa71f57fa..b577d55270 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -592,6 +592,40 @@ impl Stack { self.out_dest.pipe_stderr = None; self } + + /// Set the PWD environment variable to `path`. + /// + /// This method accepts `path` with trailing slashes, but they're removed + /// before writing the value into PWD. + pub fn set_cwd(&mut self, path: impl AsRef) -> Result<(), ShellError> { + // Helper function to create a simple generic error. + // Its messages are not especially helpful, but these errors don't occur often, so it's probably fine. + fn error(msg: &str) -> Result<(), ShellError> { + Err(ShellError::GenericError { + error: msg.into(), + msg: "".into(), + span: None, + help: None, + inner: vec![], + }) + } + + let path = path.as_ref(); + + if !path.is_absolute() { + error("Cannot set $env.PWD to a non-absolute path") + } else if !path.exists() { + error("Cannot set $env.PWD to a non-existent directory") + } else if !path.is_dir() { + error("Cannot set $env.PWD to a non-directory") + } else { + // Strip trailing slashes, if any. + let path = nu_path::strip_trailing_slash(path); + let value = Value::string(path.to_string_lossy(), Span::unknown()); + self.add_env_var("PWD".into(), value); + Ok(()) + } + } } #[cfg(test)]