mirror of
https://github.com/nushell/nushell.git
synced 2025-07-01 07:00:37 +02:00
Migrate to a new PWD API (#12603)
This is the first PR towards migrating to a new `$env.PWD` API that returns potentially un-canonicalized paths. Refer to PR #12515 for motivations. ## New API: `EngineState::cwd()` The goal of the new API is to cover both parse-time and runtime use case, and avoid unintentional misuse. It takes an `Option<Stack>` as argument, which if supplied, will search for `$env.PWD` on the stack in additional to the engine state. I think with this design, there's less confusion over parse-time and runtime environments. If you have access to a stack, just supply it; otherwise supply `None`. ## Deprecation of other PWD-related APIs Other APIs are re-implemented using `EngineState::cwd()` and properly documented. They're marked deprecated, but their behavior is unchanged. Unused APIs are deleted, and code that accesses `$env.PWD` directly without using an API is rewritten. Deprecated APIs: * `EngineState::current_work_dir()` * `StateWorkingSet::get_cwd()` * `env::current_dir()` * `env::current_dir_str()` * `env::current_dir_const()` * `env::current_dir_str_const()` Other changes: * `EngineState::get_cwd()` (deleted) * `StateWorkingSet::list_env()` (deleted) * `repl::do_run_cmd()` (rewritten with `env::current_dir_str()`) ## `cd` and `pwd` now use logical paths by default This pulls the changes from PR #12515. It's currently somewhat broken because using non-canonicalized paths exposed a bug in our path normalization logic (Issue #12602). Once that is fixed, this should work. ## Future plans This PR needs some tests. Which test helpers should I use, and where should I put those tests? I noticed that unquoted paths are expanded within `eval_filepath()` and `eval_directory()` before they even reach the `cd` command. This means every paths is expanded twice. Is this intended? Once this PR lands, the plan is to review all usages of the deprecated APIs and migrate them to `EngineState::cwd()`. In the meantime, these usages are annotated with `#[allow(deprecated)]` to avoid breaking CI. --------- Co-authored-by: Jakub Žádník <kubouch@gmail.com>
This commit is contained in:
YizhePKU
GitHub
@ -46,6 +46,7 @@ strum_macros = "0.26"
|
||||
nu-test-support = { path = "../nu-test-support", version = "0.93.1" }
|
||||
pretty_assertions = { workspace = true }
|
||||
rstest = { workspace = true }
|
||||
tempfile = { workspace = true }
|
||||
|
||||
[package.metadata.docs.rs]
|
||||
all-features = true
|
||||
|
||||
@ -26,8 +26,6 @@ type PoisonDebuggerError<'a> = PoisonError<MutexGuard<'a, Box<dyn Debugger>>>;
|
||||
#[cfg(feature = "plugin")]
|
||||
use crate::{PluginRegistryFile, PluginRegistryItem, RegisteredPlugin};
|
||||
|
||||
pub static PWD_ENV: &str = "PWD";
|
||||
|
||||
#[derive(Clone, Debug)]
|
||||
pub enum VirtualPath {
|
||||
File(FileId),
|
||||
@ -893,14 +891,6 @@ impl EngineState {
|
||||
self.num_files() - 1
|
||||
}
|
||||
|
||||
pub fn get_cwd(&self) -> Option<String> {
|
||||
if let Some(pwd_value) = self.get_env_var(PWD_ENV) {
|
||||
pwd_value.coerce_string().ok()
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
pub fn set_config_path(&mut self, key: &str, val: PathBuf) {
|
||||
self.config_path.insert(key.to_string(), val);
|
||||
}
|
||||
@ -922,12 +912,71 @@ impl EngineState {
|
||||
.map(|comment_spans| self.build_usage(comment_spans))
|
||||
}
|
||||
|
||||
/// Returns the current working directory, which is guaranteed to be canonicalized.
|
||||
///
|
||||
/// Returns an empty String if $env.PWD doesn't exist.
|
||||
#[deprecated(since = "0.92.3", note = "please use `EngineState::cwd()` instead")]
|
||||
pub fn current_work_dir(&self) -> String {
|
||||
self.get_env_var("PWD")
|
||||
.map(|d| d.coerce_string().unwrap_or_default())
|
||||
self.cwd(None)
|
||||
.map(|path| path.to_string_lossy().to_string())
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// Returns the current working directory, which is guaranteed to be an
|
||||
/// absolute path without trailing slashes, but might contain symlink
|
||||
/// components.
|
||||
///
|
||||
/// If `stack` is supplied, also considers modifications to the working
|
||||
/// directory on the stack that have yet to be merged into the engine state.
|
||||
pub fn cwd(&self, stack: Option<&Stack>) -> Result<PathBuf, 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<PathBuf, ShellError> {
|
||||
Err(ShellError::GenericError {
|
||||
error: msg.into(),
|
||||
msg: "".into(),
|
||||
span: None,
|
||||
help: None,
|
||||
inner: vec![],
|
||||
})
|
||||
}
|
||||
|
||||
// Helper function to check if a path has trailing slashes.
|
||||
fn has_trailing_slash(path: &Path) -> bool {
|
||||
nu_path::components(path).last()
|
||||
== Some(std::path::Component::Normal(std::ffi::OsStr::new("")))
|
||||
}
|
||||
|
||||
// Retrieve $env.PWD from the stack or the engine state.
|
||||
let pwd = if let Some(stack) = stack {
|
||||
stack.get_env_var(self, "PWD")
|
||||
} else {
|
||||
self.get_env_var("PWD").map(ToOwned::to_owned)
|
||||
};
|
||||
|
||||
if let Some(pwd) = pwd {
|
||||
if let Value::String { val, .. } = pwd {
|
||||
let path = PathBuf::from(val);
|
||||
|
||||
if has_trailing_slash(&path) {
|
||||
error("$env.PWD contains trailing slashes")
|
||||
} else if !path.is_absolute() {
|
||||
error("$env.PWD is not an absolute path")
|
||||
} else if !path.exists() {
|
||||
error("$env.PWD points to a non-existent directory")
|
||||
} else if !path.is_dir() {
|
||||
error("$env.PWD points to a non-directory")
|
||||
} else {
|
||||
Ok(path)
|
||||
}
|
||||
} else {
|
||||
error("$env.PWD is not a string")
|
||||
}
|
||||
} else {
|
||||
error("$env.PWD not found")
|
||||
}
|
||||
}
|
||||
|
||||
// TODO: see if we can completely get rid of this
|
||||
pub fn get_file_contents(&self) -> &[CachedFile] {
|
||||
&self.files
|
||||
@ -1077,3 +1126,213 @@ mod engine_state_tests {
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod test_cwd {
|
||||
//! Here're the test cases we need to cover:
|
||||
//!
|
||||
//! `EngineState::cwd()` computes the result from `self.env_vars["PWD"]` and
|
||||
//! optionally `stack.env_vars["PWD"]`.
|
||||
//!
|
||||
//! PWD may be unset in either `env_vars`.
|
||||
//! PWD should NOT be an empty string.
|
||||
//! PWD should NOT be a non-string value.
|
||||
//! PWD should NOT be a relative path.
|
||||
//! PWD should NOT contain trailing slashes.
|
||||
//! PWD may point to a directory or a symlink to directory.
|
||||
//! PWD should NOT point to a file or a symlink to file.
|
||||
//! PWD should NOT point to non-existent entities in the filesystem.
|
||||
|
||||
use crate::{
|
||||
engine::{EngineState, Stack},
|
||||
Span, Value,
|
||||
};
|
||||
use nu_path::assert_path_eq;
|
||||
use std::path::Path;
|
||||
use tempfile::{NamedTempFile, TempDir};
|
||||
|
||||
/// Creates a symlink. Works on both Unix and Windows.
|
||||
#[cfg(any(unix, windows))]
|
||||
fn symlink(original: impl AsRef<Path>, link: impl AsRef<Path>) -> 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)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Create an engine state initialized with the given PWD.
|
||||
fn engine_state_with_pwd(path: impl AsRef<Path>) -> EngineState {
|
||||
let mut engine_state = EngineState::new();
|
||||
engine_state.add_env_var(
|
||||
"PWD".into(),
|
||||
Value::String {
|
||||
val: path.as_ref().to_string_lossy().to_string(),
|
||||
internal_span: Span::unknown(),
|
||||
},
|
||||
);
|
||||
engine_state
|
||||
}
|
||||
|
||||
/// Create a stack initialized with the given PWD.
|
||||
fn stack_with_pwd(path: impl AsRef<Path>) -> Stack {
|
||||
let mut stack = Stack::new();
|
||||
stack.add_env_var(
|
||||
"PWD".into(),
|
||||
Value::String {
|
||||
val: path.as_ref().to_string_lossy().to_string(),
|
||||
internal_span: Span::unknown(),
|
||||
},
|
||||
);
|
||||
stack
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_not_set() {
|
||||
let engine_state = EngineState::new();
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_is_empty_string() {
|
||||
let engine_state = engine_state_with_pwd("");
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_is_non_string_value() {
|
||||
let mut engine_state = EngineState::new();
|
||||
engine_state.add_env_var(
|
||||
"PWD".into(),
|
||||
Value::Glob {
|
||||
val: "*".into(),
|
||||
no_expand: false,
|
||||
internal_span: Span::unknown(),
|
||||
},
|
||||
);
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_is_relative_path() {
|
||||
let engine_state = engine_state_with_pwd("./foo");
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_has_trailing_slash() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(dir.path().join(""));
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_normal_file() {
|
||||
let file = NamedTempFile::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(file.path());
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_normal_directory() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(dir.path());
|
||||
|
||||
let cwd = engine_state.cwd(None).unwrap();
|
||||
assert_path_eq!(cwd, dir.path());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_symlink_to_file() {
|
||||
let file = NamedTempFile::new().unwrap();
|
||||
let dir = TempDir::new().unwrap();
|
||||
let link = dir.path().join("link");
|
||||
symlink(file.path(), &link).unwrap();
|
||||
let engine_state = engine_state_with_pwd(&link);
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_symlink_to_directory() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let link = dir.path().join("link");
|
||||
symlink(dir.path(), &link).unwrap();
|
||||
let engine_state = engine_state_with_pwd(&link);
|
||||
|
||||
let cwd = engine_state.cwd(None).unwrap();
|
||||
assert_path_eq!(cwd, link);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_broken_symlink() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let link = dir.path().join("link");
|
||||
symlink(TempDir::new().unwrap().path(), &link).unwrap();
|
||||
let engine_state = engine_state_with_pwd(&link);
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn pwd_points_to_nonexistent_entity() {
|
||||
let engine_state = engine_state_with_pwd(TempDir::new().unwrap().path());
|
||||
|
||||
engine_state.cwd(None).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stack_pwd_not_set() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(dir.path());
|
||||
let stack = Stack::new();
|
||||
|
||||
let cwd = engine_state.cwd(Some(&stack)).unwrap();
|
||||
assert_eq!(cwd, dir.path());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stack_pwd_is_empty_string() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(dir.path());
|
||||
let stack = stack_with_pwd("");
|
||||
|
||||
engine_state.cwd(Some(&stack)).unwrap_err();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stack_pwd_points_to_normal_directory() {
|
||||
let dir1 = TempDir::new().unwrap();
|
||||
let dir2 = TempDir::new().unwrap();
|
||||
let engine_state = engine_state_with_pwd(dir1.path());
|
||||
let stack = stack_with_pwd(dir2.path());
|
||||
|
||||
let cwd = engine_state.cwd(Some(&stack)).unwrap();
|
||||
assert_path_eq!(cwd, dir2.path());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn stack_pwd_points_to_normal_directory_with_symlink_components() {
|
||||
// `/tmp/dir/link` points to `/tmp/dir`, then we set PWD to `/tmp/dir/link/foo`
|
||||
let dir = TempDir::new().unwrap();
|
||||
let link = dir.path().join("link");
|
||||
symlink(dir.path(), &link).unwrap();
|
||||
let foo = link.join("foo");
|
||||
std::fs::create_dir(dir.path().join("foo")).unwrap();
|
||||
let engine_state = EngineState::new();
|
||||
let stack = stack_with_pwd(&foo);
|
||||
|
||||
let cwd = engine_state.cwd(Some(&stack)).unwrap();
|
||||
assert_path_eq!(cwd, foo);
|
||||
}
|
||||
}
|
||||
|
||||
@ -2,7 +2,7 @@ use crate::{
|
||||
ast::Block,
|
||||
engine::{
|
||||
usage::build_usage, CachedFile, Command, CommandType, EngineState, OverlayFrame,
|
||||
StateDelta, Variable, VirtualPath, Visibility, PWD_ENV,
|
||||
StateDelta, Variable, VirtualPath, Visibility,
|
||||
},
|
||||
BlockId, Category, Config, DeclId, FileId, Module, ModuleId, ParseError, ParseWarning, Span,
|
||||
Type, Value, VarId, VirtualPathId,
|
||||
@ -601,13 +601,16 @@ impl<'a> StateWorkingSet<'a> {
|
||||
next_id
|
||||
}
|
||||
|
||||
/// Returns the current working directory as a String, which is guaranteed to be canonicalized.
|
||||
/// Returns an empty string if $env.PWD doesn't exist, is not a String, or is not an absolute path.
|
||||
///
|
||||
/// It does NOT consider modifications to the working directory made on a stack.
|
||||
#[deprecated(since = "0.92.3", note = "please use `EngineState::cwd()` instead")]
|
||||
pub fn get_cwd(&self) -> String {
|
||||
let pwd = self
|
||||
.permanent_state
|
||||
.get_env_var(PWD_ENV)
|
||||
.expect("internal error: can't find PWD");
|
||||
pwd.coerce_string()
|
||||
.expect("internal error: PWD not a string")
|
||||
self.permanent_state
|
||||
.cwd(None)
|
||||
.map(|path| path.to_string_lossy().to_string())
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
pub fn get_env_var(&self, name: &str) -> Option<&Value> {
|
||||
@ -622,16 +625,6 @@ impl<'a> StateWorkingSet<'a> {
|
||||
&self.permanent_state.config
|
||||
}
|
||||
|
||||
pub fn list_env(&self) -> Vec<String> {
|
||||
let mut env_vars = vec![];
|
||||
|
||||
for env_var in self.permanent_state.env_vars.iter() {
|
||||
env_vars.push(env_var.0.clone());
|
||||
}
|
||||
|
||||
env_vars
|
||||
}
|
||||
|
||||
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 {
|
||||
|
||||
@ -14,6 +14,7 @@ use std::{
|
||||
/// Create a Value for `$nu`.
|
||||
pub fn create_nu_constant(engine_state: &EngineState, span: Span) -> Result<Value, ShellError> {
|
||||
fn canonicalize_path(engine_state: &EngineState, path: &Path) -> PathBuf {
|
||||
#[allow(deprecated)]
|
||||
let cwd = engine_state.current_work_dir();
|
||||
|
||||
if path.exists() {
|
||||
|
||||
Reference in New Issue
Block a user