From b9a7faad5adb59e1a9fae444586caa27f9e4058a Mon Sep 17 00:00:00 2001 From: YizhePKU Date: Sat, 11 May 2024 00:06:33 +0800 Subject: [PATCH] Implement PWD recovery (#12779) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR has two parts. The first part is the addition of the `Stack::set_pwd()` API. It strips trailing slashes from paths for convenience, but will reject otherwise bad paths, leaving PWD in a good state. This should reduce the impact of faulty code incorrectly trying to set PWD. (https://github.com/nushell/nushell/pull/12760#issuecomment-2095393012) The second part is implementing a PWD recovery mechanism. PWD can become bad even when we did nothing wrong. For example, Unix allows you to remove any directory when another process might still be using it, which means PWD can just "disappear" under our nose. This PR makes it possible to use `cd` to reset PWD into a good state. Here's a demonstration: ```sh mkdir /tmp/foo cd /tmp/foo # delete "/tmp/foo" in a subshell, because Nushell is smart and refuse to delete PWD nu -c 'cd /; rm -r /tmp/foo' ls # Error: × $env.PWD points to a non-existent directory # help: Use `cd` to reset $env.PWD into a good state cd / pwd # prints / ``` Also, auto-cd should be working again. --- Cargo.lock | 1 + crates/nu-cli/Cargo.toml | 1 + crates/nu-cli/src/repl.rs | 140 +++++++++++++++++- crates/nu-cmd-base/src/util.rs | 9 +- crates/nu-command/src/filesystem/cd.rs | 27 ++-- crates/nu-command/tests/commands/cd.rs | 14 ++ crates/nu-engine/src/eval.rs | 2 +- crates/nu-protocol/src/engine/engine_state.rs | 19 ++- crates/nu-protocol/src/engine/stack.rs | 34 +++++ 9 files changed, 214 insertions(+), 33 deletions(-) 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)]