From 739a7ea730e421b10106bfe3ebb0e2a0bf223964 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 14 Nov 2024 09:20:17 +0000 Subject: [PATCH 01/21] Bump mockito from 1.5.0 to 1.6.1 (#14336) --- Cargo.lock | 4 ++-- Cargo.toml | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 779364a7a3..51dfce7efc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2805,9 +2805,9 @@ dependencies = [ [[package]] name = "mockito" -version = "1.5.0" +version = "1.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09b34bd91b9e5c5b06338d392463e1318d683cf82ec3d3af4014609be6e2108d" +checksum = "652cd6d169a36eaf9d1e6bce1a221130439a966d7f27858af66a33a66e9c4ee2" dependencies = [ "assert-json-diff", "bytes", diff --git a/Cargo.toml b/Cargo.toml index 07a50d301a..2f0aaba5a7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -109,7 +109,7 @@ md5 = { version = "0.10", package = "md-5" } miette = "7.2" mime = "0.3.17" mime_guess = "2.0" -mockito = { version = "1.5", default-features = false } +mockito = { version = "1.6", default-features = false } multipart-rs = "0.1.11" native-tls = "0.2" nix = { version = "0.29", default-features = false } From 636bae24665b815a88f6253cc4b076cf1939a407 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu, 14 Nov 2024 09:32:55 +0000 Subject: [PATCH 02/21] Bump tempfile from 3.13.0 to 3.14.0 (#14326) --- Cargo.lock | 12 ++++++------ Cargo.toml | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51dfce7efc..7343d3c043 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2401,9 +2401,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.158" +version = "0.2.162" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d8adc4bb1803a324070e64a98ae98f38934d91957a99cfb3a43dcbc01bc56439" +checksum = "18d287de67fe55fd7e1581fe933d965a5a9477b38e949cfa9f8574ef01506398" [[package]] name = "libflate" @@ -5476,9 +5476,9 @@ dependencies = [ [[package]] name = "rustix" -version = "0.38.37" +version = "0.38.40" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8acb788b847c24f28525660c4d7758620a7210875711f79e7f663cc152726811" +checksum = "99e4ea3e1cdc4b559b8e5650f9c8e5998e3e5c1343b4eaf034565f32318d63c0" dependencies = [ "bitflags 2.6.0", "errno", @@ -6196,9 +6196,9 @@ checksum = "c1bbb9f3c5c463a01705937a24fdabc5047929ac764b2d5b9cf681c1f5041ed5" [[package]] name = "tempfile" -version = "3.13.0" +version = "3.14.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f0f2c9fc62d0beef6951ccffd757e241266a2c833136efbe35af6cd2567dca5b" +checksum = "28cce251fcbc87fac86a866eeb0d6c2d536fc16d06f184bb61aeae11aa4cee0c" dependencies = [ "cfg-if", "fastrand", diff --git a/Cargo.toml b/Cargo.toml index 2f0aaba5a7..e7318fa006 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -155,7 +155,7 @@ strip-ansi-escapes = "0.2.0" syn = "2.0" sysinfo = "0.32" tabled = { version = "0.16.0", default-features = false } -tempfile = "3.13" +tempfile = "3.14" terminal_size = "0.3" titlecase = "2.0" toml = "0.8" From a84d410f117d2247acb2b33452356c5c182572db Mon Sep 17 00:00:00 2001 From: Bark <32035685+userwiths@users.noreply.github.com> Date: Fri, 15 Nov 2024 01:39:41 +0200 Subject: [PATCH 03/21] Fix inconsistency in `ls` sort-order (#13875) Fixes #13267 As we can see from the bisect done in the comments. Bisected to https://github.com/nushell/nushell/pull/12625 / https://github.com/nushell/nushell/commit/460a1c8f874b503cb4140619838be483600f00ce We can see that this update brought the use of `read_dir` and for it, it is mentioned in the [rust docs](https://doc.rust-lang.org/std/fs/fn.read_dir.html#platform-specific-behavior) that it does **not** provide any specific order of files. As was the advice there, I went and applied a manual `sort` to the entries and tested it manually on my local machine. If required I could probably try and add tests for the order consistency, would need some time to find my way around them, so I'm sending the PR first. --- crates/nu-command/src/filesystem/ls.rs | 16 +++++++++++++--- crates/nu-command/tests/commands/ls.rs | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index 465fae16ff..b1b231a4a5 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -339,7 +339,7 @@ fn ls_for_one_pattern( let path = pattern_arg.into_spanned(p_tag); let (prefix, paths) = if just_read_dir { let expanded = nu_path::expand_path_with(path.item.as_ref(), &cwd, path.item.is_expand()); - let paths = read_dir(&expanded)?; + let paths = read_dir(&expanded, use_threads)?; // just need to read the directory, so prefix is path itself. (Some(expanded), paths) } else { @@ -979,10 +979,20 @@ mod windows_helper { #[allow(clippy::type_complexity)] fn read_dir( f: &Path, + use_threads: bool, ) -> Result> + Send>, ShellError> { - let iter = f.read_dir()?.map(|d| { + let items = f.read_dir()?.map(|d| { d.map(|r| r.path()) .map_err(|e| ShellError::IOError { msg: e.to_string() }) }); - Ok(Box::new(iter)) + if !use_threads { + let mut collected = items.collect::>(); + collected.sort_by(|a, b| { + let a = a.as_ref().expect("path should be valid"); + let b = b.as_ref().expect("path should be valid"); + a.cmp(b) + }); + return Ok(Box::new(collected.into_iter())); + } + Ok(Box::new(items)) } diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs index 7ac0063741..4140ea64bc 100644 --- a/crates/nu-command/tests/commands/ls.rs +++ b/crates/nu-command/tests/commands/ls.rs @@ -833,3 +833,27 @@ fn list_symlink_with_full_path() { ); }) } + +#[test] +fn consistent_list_order() { + Playground::setup("ls_test_order", |dirs, sandbox| { + sandbox.with_files(&[ + EmptyFile("los.txt"), + EmptyFile("tres.txt"), + EmptyFile("amigos.txt"), + EmptyFile("arepas.clu"), + ]); + + let no_arg = nu!( + cwd: dirs.test(), pipeline( + "ls" + )); + + let with_arg = nu!( + cwd: dirs.test(), pipeline( + "ls ." + )); + + assert_eq!(no_arg.out, with_arg.out); + }) +} From a04c90e22d7bc9ca8b2d023d5d771255d1a6e759 Mon Sep 17 00:00:00 2001 From: Solomon Date: Thu, 14 Nov 2024 21:09:02 -0700 Subject: [PATCH 04/21] make `ls` return "Permission denied" for CWD instead of empty results (#14310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #14265 # User-Facing Changes `ls` without a path argument now errors when the current working directory is unreadable due to missing permissions: ```diff mkdir foo chmod 100 foo cd foo ls | to nuon -[] +Error: × Permission denied ``` --- crates/nu-command/src/filesystem/ls.rs | 68 ++++++++++---------------- crates/nu-command/tests/commands/ls.rs | 23 +++++---- 2 files changed, 40 insertions(+), 51 deletions(-) diff --git a/crates/nu-command/src/filesystem/ls.rs b/crates/nu-command/src/filesystem/ls.rs index b1b231a4a5..c4944c6531 100644 --- a/crates/nu-command/src/filesystem/ls.rs +++ b/crates/nu-command/src/filesystem/ls.rs @@ -287,28 +287,10 @@ fn ls_for_one_pattern( nu_path::expand_path_with(pat.item.as_ref(), &cwd, pat.item.is_expand()); // Avoid checking and pushing "*" to the path when directory (do not show contents) flag is true if !directory && tmp_expanded.is_dir() { - if permission_denied(&tmp_expanded) { - #[cfg(unix)] - let error_msg = format!( - "The permissions of {:o} do not allow access for this user", - tmp_expanded - .metadata() - .expect("this shouldn't be called since we already know there is a dir") - .permissions() - .mode() - & 0o0777 - ); - #[cfg(not(unix))] - let error_msg = String::from("Permission denied"); - return Err(ShellError::GenericError { - error: "Permission denied".into(), - msg: error_msg, - span: Some(p_tag), - help: None, - inner: vec![], - }); - } - if is_empty_dir(&tmp_expanded) { + if read_dir(&tmp_expanded, p_tag, use_threads)? + .next() + .is_none() + { return Ok(Value::test_nothing().into_pipeline_data()); } just_read_dir = !(pat.item.is_expand() && pat.item.as_ref().contains(GLOB_CHARS)); @@ -327,7 +309,7 @@ fn ls_for_one_pattern( // Avoid pushing "*" to the default path when directory (do not show contents) flag is true if directory { (NuGlob::Expand(".".to_string()), false) - } else if is_empty_dir(&cwd) { + } else if read_dir(&cwd, p_tag, use_threads)?.next().is_none() { return Ok(Value::test_nothing().into_pipeline_data()); } else { (NuGlob::Expand("*".to_string()), false) @@ -339,7 +321,7 @@ fn ls_for_one_pattern( let path = pattern_arg.into_spanned(p_tag); let (prefix, paths) = if just_read_dir { let expanded = nu_path::expand_path_with(path.item.as_ref(), &cwd, path.item.is_expand()); - let paths = read_dir(&expanded, use_threads)?; + let paths = read_dir(&expanded, p_tag, use_threads)?; // just need to read the directory, so prefix is path itself. (Some(expanded), paths) } else { @@ -492,20 +474,6 @@ fn ls_for_one_pattern( .into_pipeline_data(call_span, signals.clone())) } -fn permission_denied(dir: impl AsRef) -> bool { - match dir.as_ref().read_dir() { - Err(e) => matches!(e.kind(), std::io::ErrorKind::PermissionDenied), - Ok(_) => false, - } -} - -fn is_empty_dir(dir: impl AsRef) -> bool { - match dir.as_ref().read_dir() { - Err(_) => true, - Ok(mut s) => s.next().is_none(), - } -} - fn is_hidden_dir(dir: impl AsRef) -> bool { #[cfg(windows)] { @@ -979,12 +947,28 @@ mod windows_helper { #[allow(clippy::type_complexity)] fn read_dir( f: &Path, + span: Span, use_threads: bool, ) -> Result> + Send>, ShellError> { - let items = f.read_dir()?.map(|d| { - d.map(|r| r.path()) - .map_err(|e| ShellError::IOError { msg: e.to_string() }) - }); + let items = f + .read_dir() + .map_err(|error| { + if error.kind() == std::io::ErrorKind::PermissionDenied { + return ShellError::GenericError { + error: "Permission denied".into(), + msg: "The permissions may not allow access for this user".into(), + span: Some(span), + help: None, + inner: vec![], + }; + } + + error.into() + })? + .map(|d| { + d.map(|r| r.path()) + .map_err(|e| ShellError::IOError { msg: e.to_string() }) + }); if !use_threads { let mut collected = items.collect::>(); collected.sort_by(|a, b| { diff --git a/crates/nu-command/tests/commands/ls.rs b/crates/nu-command/tests/commands/ls.rs index 4140ea64bc..416a6aaf7f 100644 --- a/crates/nu-command/tests/commands/ls.rs +++ b/crates/nu-command/tests/commands/ls.rs @@ -378,32 +378,37 @@ fn glob_with_hidden_directory() { #[test] #[cfg(unix)] -fn fails_with_ls_to_dir_without_permission() { +fn fails_with_permission_denied() { Playground::setup("ls_test_1", |dirs, sandbox| { sandbox .within("dir_a") .with_files(&[EmptyFile("yehuda.11.txt"), EmptyFile("jt10.txt")]); - let actual = nu!( + let actual_with_path_arg = nu!( cwd: dirs.test(), pipeline( " chmod 000 dir_a; ls dir_a " )); - let check_not_root = nu!( + let actual_in_cwd = nu!( + cwd: dirs.test(), pipeline( + " + chmod 100 dir_a; cd dir_a; ls + " + )); + + let get_uid = nu!( cwd: dirs.test(), pipeline( " id -u " )); + let is_root = get_uid.out == "0"; - assert!( - actual - .err - .contains("The permissions of 0 do not allow access for this user") - || check_not_root.out == "0" - ); + assert!(actual_with_path_arg.err.contains("Permission denied") || is_root); + + assert!(actual_in_cwd.err.contains("Permission denied") || is_root); }) } From 215ca6c5ca445dc56478c67ed91a37f8630732b5 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Thu, 14 Nov 2024 20:09:25 -0800 Subject: [PATCH 05/21] Remove the `NU_DISABLE_IR` option (#14293) # Description Removes the `NU_DISABLE_IR` option and some code related to evaluating blocks with the AST evaluator. Does not entirely remove the AST evaluator yet. We still have some dependencies on expression evaluation in a few minor places which will take a little bit of effort to fix. Also changes `debug profile` to always include instructions, because the output is a little confusing otherwise, and removes the different options for instructions/exprs. # User-Facing Changes - `NU_DISABLE_IR` no longer has any effect, and is removed. There is no way to use the AST evaluator. - `debug profile` no longer has `--exprs`, `--instructions` options. - `debug profile` lists `pc` and `instruction` columns by default now. # Tests + Formatting Eval tests fixed to only use IR. # After Submitting - [ ] release notes - [ ] finish removing AST evaluator, come up with solutions for the expression evaluation. --- benches/benchmarks.rs | 3 - crates/nu-cli/src/repl.rs | 3 - crates/nu-cmd-lang/src/core_commands/do_.rs | 3 - crates/nu-command/src/debug/profile.rs | 67 ++--- crates/nu-engine/src/eval.rs | 261 +------------------- crates/nu-protocol/src/engine/stack.rs | 6 - crates/nu-test-support/src/macros.rs | 10 - src/run.rs | 11 - src/test_bins.rs | 5 - tests/eval/mod.rs | 98 +++----- 10 files changed, 81 insertions(+), 386 deletions(-) diff --git a/benches/benchmarks.rs b/benches/benchmarks.rs index 6eb56532a4..d6581bc255 100644 --- a/benches/benchmarks.rs +++ b/benches/benchmarks.rs @@ -46,9 +46,6 @@ fn setup_stack_and_engine_from_command(command: &str) -> (Stack, EngineState) { let mut stack = Stack::new(); - // Support running benchmarks without IR mode - stack.use_ir = std::env::var_os("NU_DISABLE_IR").is_none(); - evaluate_commands( &commands, &mut engine, diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 08f00727f0..5440f9caa2 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -306,9 +306,6 @@ fn loop_iteration(ctx: LoopContext) -> (bool, Stack, Reedline) { if let Err(err) = engine_state.merge_env(&mut stack) { report_shell_error(engine_state, &err); } - // Check whether $env.NU_DISABLE_IR is set, so that the user can change it in the REPL - // Temporary while IR eval is optional - stack.use_ir = !stack.has_env_var(engine_state, "NU_DISABLE_IR"); perf!("merge env", start_time, use_color); start_time = std::time::Instant::now(); diff --git a/crates/nu-cmd-lang/src/core_commands/do_.rs b/crates/nu-cmd-lang/src/core_commands/do_.rs index 4fee08a79c..5f1b42cb66 100644 --- a/crates/nu-cmd-lang/src/core_commands/do_.rs +++ b/crates/nu-cmd-lang/src/core_commands/do_.rs @@ -82,9 +82,6 @@ impl Command for Do { bind_args_to(&mut callee_stack, &block.signature, rest, head)?; let eval_block_with_early_return = get_eval_block_with_early_return(engine_state); - // Applies to all block evaluation once set true - callee_stack.use_ir = !caller_stack.has_env_var(engine_state, "NU_DISABLE_IR"); - let result = eval_block_with_early_return(engine_state, &mut callee_stack, block, input); if has_env { diff --git a/crates/nu-command/src/debug/profile.rs b/crates/nu-command/src/debug/profile.rs index 41eee4c183..9f72d19d7c 100644 --- a/crates/nu-command/src/debug/profile.rs +++ b/crates/nu-command/src/debug/profile.rs @@ -30,8 +30,6 @@ impl Command for DebugProfile { "Collect pipeline element output values", Some('v'), ) - .switch("expr", "Collect expression types", Some('x')) - .switch("instructions", "Collect IR instructions", Some('i')) .switch("lines", "Collect line numbers", Some('l')) .named( "max-depth", @@ -48,37 +46,52 @@ impl Command for DebugProfile { } fn extra_description(&self) -> &str { - r#"The profiler profiles every evaluated pipeline element inside a closure, stepping into all + r#"The profiler profiles every evaluated instruction inside a closure, stepping into all commands calls and other blocks/closures. The output can be heavily customized. By default, the following columns are included: -- depth : Depth of the pipeline element. Each entered block adds one level of depth. How many +- depth : Depth of the instruction. Each entered block adds one level of depth. How many blocks deep to step into is controlled with the --max-depth option. -- id : ID of the pipeline element -- parent_id : ID of the parent element -- source : Source code of the pipeline element. If the element has multiple lines, only the - first line is used and `...` is appended to the end. Full source code can be shown - with the --expand-source flag. -- duration_ms : How long it took to run the pipeline element in milliseconds. -- (optional) span : Span of the element. Can be viewed via the `view span` command. Enabled with - the --spans flag. -- (optional) expr : The type of expression of the pipeline element. Enabled with the --expr flag. -- (optional) output : The output value of the pipeline element. Enabled with the --values flag. +- id : ID of the instruction +- parent_id : ID of the instruction that created the parent scope +- source : Source code that generated the instruction. If the source code has multiple lines, + only the first line is used and `...` is appended to the end. Full source code can + be shown with the --expand-source flag. +- pc : The index of the instruction within the block. +- instruction : The pretty printed instruction being evaluated. +- duration_ms : How long it took to run the instruction in milliseconds. +- (optional) span : Span associated with the instruction. Can be viewed via the `view span` + command. Enabled with the --spans flag. +- (optional) output : The output value of the instruction. Enabled with the --values flag. -To illustrate the depth and IDs, consider `debug profile { if true { echo 'spam' } }`. There are -three pipeline elements: +To illustrate the depth and IDs, consider `debug profile { do { if true { echo 'spam' } } }`. A unique ID is generated each time an instruction is executed, and there are two levels of depth: -depth id parent_id - 0 0 0 debug profile { do { if true { 'spam' } } } - 1 1 0 if true { 'spam' } - 2 2 1 'spam' +``` +depth id parent_id source pc instruction + 0 0 0 debug profile { do { if true { 'spam' } } } 0 + 1 1 0 { if true { 'spam' } } 0 load-literal %1, closure(2164) + 1 2 0 { if true { 'spam' } } 1 push-positional %1 + 1 3 0 { do { if true { 'spam' } } } 2 redirect-out caller + 1 4 0 { do { if true { 'spam' } } } 3 redirect-err caller + 1 5 0 do 4 call decl 7 "do", %0 + 2 6 5 true 0 load-literal %1, bool(true) + 2 7 5 if 1 not %1 + 2 8 5 if 2 branch-if %1, 5 + 2 9 5 'spam' 3 load-literal %0, string("spam") + 2 10 5 if 4 jump 6 + 2 11 5 { if true { 'spam' } } 6 return %0 + 1 12 0 { do { if true { 'spam' } } } 5 return %0 +``` Each block entered increments depth by 1 and each block left decrements it by one. This way you can -control the profiling granularity. Passing --max-depth=1 to the above would stop at -`if true { 'spam' }`. The id is used to identify each element. The parent_id tells you that 'spam' -was spawned from `if true { 'spam' }` which was spawned from the root `debug profile { ... }`. +control the profiling granularity. Passing --max-depth=1 to the above would stop inside the `do` +at `if true { 'spam' }`. The id is used to identify each element. The parent_id tells you that the +instructions inside the block are being executed because of `do` (5), which in turn was spawned from +the root `debug profile { ... }`. -Note: In some cases, the ordering of piepeline elements might not be intuitive. For example, +For a better understanding of how instructions map to source code, see the `view ir` command. + +Note: In some cases, the ordering of pipeline elements might not be intuitive. For example, `[ a bb cc ] | each { $in | str length }` involves some implicit collects and lazy evaluation confusing the id/parent_id hierarchy. The --expr flag is helpful for investigating these issues."# } @@ -94,8 +107,6 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati let collect_spans = call.has_flag(engine_state, stack, "spans")?; let collect_expanded_source = call.has_flag(engine_state, stack, "expanded-source")?; let collect_values = call.has_flag(engine_state, stack, "values")?; - let collect_exprs = call.has_flag(engine_state, stack, "expr")?; - let collect_instructions = call.has_flag(engine_state, stack, "instructions")?; let collect_lines = call.has_flag(engine_state, stack, "lines")?; let max_depth = call .get_flag(engine_state, stack, "max-depth")? @@ -108,8 +119,8 @@ confusing the id/parent_id hierarchy. The --expr flag is helpful for investigati collect_source: true, collect_expanded_source, collect_values, - collect_exprs, - collect_instructions, + collect_exprs: false, + collect_instructions: true, collect_lines, }, call.span(), diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 3e5ad239fe..fc4a88f83e 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -1,20 +1,17 @@ use crate::eval_ir_block; #[allow(deprecated)] -use crate::{current_dir, get_full_help}; +use crate::get_full_help; use nu_path::{expand_path_with, AbsolutePathBuf}; use nu_protocol::{ - ast::{ - Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember, PipelineElement, - PipelineRedirection, RedirectionSource, RedirectionTarget, - }, + ast::{Assignment, Block, Call, Expr, Expression, ExternalArgument, PathMember}, debugger::DebugContext, - engine::{Closure, EngineState, Redirection, Stack, StateWorkingSet}, + engine::{Closure, EngineState, Stack}, eval_base::Eval, - BlockId, ByteStreamSource, Config, DataSource, FromValue, IntoPipelineData, OutDest, - PipelineData, PipelineMetadata, ShellError, Span, Spanned, Type, Value, VarId, ENV_VARIABLE_ID, + BlockId, Config, DataSource, IntoPipelineData, PipelineData, PipelineMetadata, ShellError, + Span, Type, Value, VarId, ENV_VARIABLE_ID, }; use nu_utils::IgnoreCaseExt; -use std::{fs::OpenOptions, path::PathBuf, sync::Arc}; +use std::sync::Arc; pub fn eval_call( engine_state: &EngineState, @@ -301,177 +298,6 @@ pub fn eval_expression_with_input( Ok(input) } -fn eval_redirection( - engine_state: &EngineState, - stack: &mut Stack, - target: &RedirectionTarget, - next_out: Option, -) -> Result { - match target { - RedirectionTarget::File { expr, append, .. } => { - #[allow(deprecated)] - let cwd = current_dir(engine_state, stack)?; - let value = eval_expression::(engine_state, stack, expr)?; - let path = Spanned::::from_value(value)?.item; - let path = expand_path_with(path, cwd, true); - - let mut options = OpenOptions::new(); - if *append { - options.append(true); - } else { - options.write(true).truncate(true); - } - Ok(Redirection::file(options.create(true).open(path)?)) - } - RedirectionTarget::Pipe { .. } => { - let dest = match next_out { - None | Some(OutDest::PipeSeparate) => OutDest::Pipe, - Some(next) => next, - }; - Ok(Redirection::Pipe(dest)) - } - } -} - -fn eval_element_redirection( - engine_state: &EngineState, - stack: &mut Stack, - element_redirection: Option<&PipelineRedirection>, - pipe_redirection: (Option, Option), -) -> Result<(Option, Option), ShellError> { - let (next_out, next_err) = pipe_redirection; - - if let Some(redirection) = element_redirection { - match redirection { - PipelineRedirection::Single { - source: RedirectionSource::Stdout, - target, - } => { - let stdout = eval_redirection::(engine_state, stack, target, next_out)?; - Ok((Some(stdout), next_err.map(Redirection::Pipe))) - } - PipelineRedirection::Single { - source: RedirectionSource::Stderr, - target, - } => { - let stderr = eval_redirection::(engine_state, stack, target, None)?; - if matches!(stderr, Redirection::Pipe(OutDest::Pipe)) { - let dest = match next_out { - None | Some(OutDest::PipeSeparate) => OutDest::Pipe, - Some(next) => next, - }; - // e>| redirection, don't override current stack `stdout` - Ok((None, Some(Redirection::Pipe(dest)))) - } else { - Ok((next_out.map(Redirection::Pipe), Some(stderr))) - } - } - PipelineRedirection::Single { - source: RedirectionSource::StdoutAndStderr, - target, - } => { - let stream = eval_redirection::(engine_state, stack, target, next_out)?; - Ok((Some(stream.clone()), Some(stream))) - } - PipelineRedirection::Separate { out, err } => { - let stdout = eval_redirection::(engine_state, stack, out, None)?; // `out` cannot be `RedirectionTarget::Pipe` - let stderr = eval_redirection::(engine_state, stack, err, next_out)?; - Ok((Some(stdout), Some(stderr))) - } - } - } else { - Ok(( - next_out.map(Redirection::Pipe), - next_err.map(Redirection::Pipe), - )) - } -} - -fn eval_element_with_input_inner( - engine_state: &EngineState, - stack: &mut Stack, - element: &PipelineElement, - input: PipelineData, -) -> Result { - let data = eval_expression_with_input::(engine_state, stack, &element.expr, input)?; - - let is_external = if let PipelineData::ByteStream(stream, ..) = &data { - matches!(stream.source(), ByteStreamSource::Child(..)) - } else { - false - }; - - if let Some(redirection) = element.redirection.as_ref() { - if !is_external { - match redirection { - &PipelineRedirection::Single { - source: RedirectionSource::Stderr, - target: RedirectionTarget::Pipe { span }, - } - | &PipelineRedirection::Separate { - err: RedirectionTarget::Pipe { span }, - .. - } => { - return Err(ShellError::GenericError { - error: "`e>|` only works on external commands".into(), - msg: "`e>|` only works on external commands".into(), - span: Some(span), - help: None, - inner: vec![], - }); - } - &PipelineRedirection::Single { - source: RedirectionSource::StdoutAndStderr, - target: RedirectionTarget::Pipe { span }, - } => { - return Err(ShellError::GenericError { - error: "`o+e>|` only works on external commands".into(), - msg: "`o+e>|` only works on external commands".into(), - span: Some(span), - help: None, - inner: vec![], - }); - } - _ => {} - } - } - } - - let data = if let Some(OutDest::File(file)) = stack.pipe_stdout() { - match &data { - PipelineData::Value(..) | PipelineData::ListStream(..) => { - data.write_to(file.as_ref())?; - PipelineData::Empty - } - PipelineData::ByteStream(..) => { - if !is_external { - data.write_to(file.as_ref())?; - PipelineData::Empty - } else { - data - } - } - PipelineData::Empty => PipelineData::Empty, - } - } else { - data - }; - - Ok(data) -} - -fn eval_element_with_input( - engine_state: &EngineState, - stack: &mut Stack, - element: &PipelineElement, - input: PipelineData, -) -> Result { - D::enter_element(engine_state, element); - let result = eval_element_with_input_inner::(engine_state, stack, element, input); - D::leave_element(engine_state, element, &result); - result -} - pub fn eval_block_with_early_return( engine_state: &EngineState, stack: &mut Stack, @@ -484,86 +310,13 @@ pub fn eval_block_with_early_return( } } -fn eval_block_inner( - engine_state: &EngineState, - stack: &mut Stack, - block: &Block, - mut input: PipelineData, -) -> Result { - // Remove once IR is the default. - if stack.use_ir { - return eval_ir_block::(engine_state, stack, block, input); - } - - let num_pipelines = block.len(); - - for (pipeline_idx, pipeline) in block.pipelines.iter().enumerate() { - let last_pipeline = pipeline_idx >= num_pipelines - 1; - - let Some((last, elements)) = pipeline.elements.split_last() else { - debug_assert!(false, "pipelines should have at least one element"); - continue; - }; - - for (i, element) in elements.iter().enumerate() { - let next = elements.get(i + 1).unwrap_or(last); - let (next_out, next_err) = next.pipe_redirection(&StateWorkingSet::new(engine_state)); - let (stdout, stderr) = eval_element_redirection::( - engine_state, - stack, - element.redirection.as_ref(), - (next_out.or(Some(OutDest::Pipe)), next_err), - )?; - let stack = &mut stack.push_redirection(stdout, stderr); - input = eval_element_with_input::(engine_state, stack, element, input)?; - } - - if last_pipeline { - let (stdout, stderr) = eval_element_redirection::( - engine_state, - stack, - last.redirection.as_ref(), - (stack.pipe_stdout().cloned(), stack.pipe_stderr().cloned()), - )?; - let stack = &mut stack.push_redirection(stdout, stderr); - input = eval_element_with_input::(engine_state, stack, last, input)?; - } else { - let (stdout, stderr) = eval_element_redirection::( - engine_state, - stack, - last.redirection.as_ref(), - (None, None), - )?; - let stack = &mut stack.push_redirection(stdout, stderr); - match eval_element_with_input::(engine_state, stack, last, input)? { - PipelineData::ByteStream(stream, ..) => { - let span = stream.span(); - if let Err(err) = stream.drain() { - stack.set_last_error(&err); - return Err(err); - } else { - stack.set_last_exit_code(0, span); - } - } - PipelineData::ListStream(stream, ..) => stream.drain()?, - PipelineData::Value(..) | PipelineData::Empty => {} - } - input = PipelineData::Empty; - } - } - - Ok(input) -} - pub fn eval_block( engine_state: &EngineState, stack: &mut Stack, block: &Block, input: PipelineData, ) -> Result { - D::enter_block(engine_state, block); - let result = eval_block_inner::(engine_state, stack, block, input); - D::leave_block(engine_state, block); + let result = eval_ir_block::(engine_state, stack, block, input); if let Err(err) = &result { stack.set_last_error(err); } diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 3102e3a84e..ddd1169513 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -45,8 +45,6 @@ pub struct Stack { pub arguments: ArgumentStack, /// Error handler stack for IR evaluation pub error_handlers: ErrorHandlerStack, - /// Set true to always use IR mode - pub use_ir: bool, pub recursion_count: u64, pub parent_stack: Option>, /// Variables that have been deleted (this is used to hide values from parent stack lookups) @@ -78,7 +76,6 @@ impl Stack { active_overlays: vec![DEFAULT_OVERLAY_NAME.to_string()], arguments: ArgumentStack::new(), error_handlers: ErrorHandlerStack::new(), - use_ir: true, recursion_count: 0, parent_stack: None, parent_deletions: vec![], @@ -99,7 +96,6 @@ impl Stack { active_overlays: parent.active_overlays.clone(), arguments: ArgumentStack::new(), error_handlers: ErrorHandlerStack::new(), - use_ir: parent.use_ir, recursion_count: parent.recursion_count, vars: vec![], parent_deletions: vec![], @@ -317,7 +313,6 @@ impl Stack { active_overlays: self.active_overlays.clone(), arguments: ArgumentStack::new(), error_handlers: ErrorHandlerStack::new(), - use_ir: self.use_ir, recursion_count: self.recursion_count, parent_stack: None, parent_deletions: vec![], @@ -351,7 +346,6 @@ impl Stack { active_overlays: self.active_overlays.clone(), arguments: ArgumentStack::new(), error_handlers: ErrorHandlerStack::new(), - use_ir: self.use_ir, recursion_count: self.recursion_count, parent_stack: None, parent_deletions: vec![], diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index fdfdabd0b5..f41c8cbc9d 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -248,7 +248,6 @@ pub struct NuOpts { pub locale: Option, pub envs: Option>, pub collapse_output: Option, - pub use_ir: Option, // Note: At the time this was added, passing in a file path was more convenient. However, // passing in file contents seems like a better API - consider this when adding new uses of // this field. @@ -301,15 +300,6 @@ pub fn nu_run_test(opts: NuOpts, commands: impl AsRef, with_std: bool) -> O .stdout(Stdio::piped()) .stderr(Stdio::piped()); - // Explicitly set NU_DISABLE_IR - if let Some(use_ir) = opts.use_ir { - if !use_ir { - command.env("NU_DISABLE_IR", "1"); - } else { - command.env_remove("NU_DISABLE_IR"); - } - } - // Uncomment to debug the command being run: // println!("=== command\n{command:?}\n"); diff --git a/src/run.rs b/src/run.rs index 14c737c613..1cbfbc03a2 100644 --- a/src/run.rs +++ b/src/run.rs @@ -26,9 +26,6 @@ pub(crate) fn run_commands( let ask_to_create_config = nu_path::nu_config_dir().map_or(false, |p| !p.exists()); let mut stack = Stack::new(); - if stack.has_env_var(engine_state, "NU_DISABLE_IR") { - stack.use_ir = false; - } // if the --no-config-file(-n) option is NOT passed, load the plugin file, // load the default env file or custom (depending on parsed_nu_cli_args.env_file), @@ -119,10 +116,6 @@ pub(crate) fn run_file( trace!("run_file"); let mut stack = Stack::new(); - if stack.has_env_var(engine_state, "NU_DISABLE_IR") { - stack.use_ir = false; - } - // if the --no-config-file(-n) option is NOT passed, load the plugin file, // load the default env file or custom (depending on parsed_nu_cli_args.env_file), // and maybe a custom config file (depending on parsed_nu_cli_args.config_file) @@ -191,10 +184,6 @@ pub(crate) fn run_repl( let mut stack = Stack::new(); let start_time = std::time::Instant::now(); - if stack.has_env_var(engine_state, "NU_DISABLE_IR") { - stack.use_ir = false; - } - if parsed_nu_cli_args.no_config_file.is_none() { setup_config( engine_state, diff --git a/src/test_bins.rs b/src/test_bins.rs index 9752114977..7e684d5794 100644 --- a/src/test_bins.rs +++ b/src/test_bins.rs @@ -236,11 +236,6 @@ pub fn nu_repl() { engine_state.add_env_var("PWD".into(), Value::test_string(cwd.to_string_lossy())); engine_state.add_env_var("PATH".into(), Value::test_string("")); - // Disable IR in tests if set - if std::env::var_os("NU_DISABLE_IR").is_some() { - Arc::make_mut(&mut top_stack).use_ir = false; - } - let mut last_output = String::new(); load_standard_library(&mut engine_state).expect("Could not load the standard library."); diff --git a/tests/eval/mod.rs b/tests/eval/mod.rs index f8996f0a3e..191b19eb89 100644 --- a/tests/eval/mod.rs +++ b/tests/eval/mod.rs @@ -31,71 +31,43 @@ enum ExpectedOut<'a> { use self::ExpectedOut::*; fn test_eval(source: &str, expected_out: ExpectedOut) { - Playground::setup("test_eval_ast", |ast_dirs, _playground| { - Playground::setup("test_eval_ir", |ir_dirs, _playground| { - let actual_ast = nu!( - cwd: ast_dirs.test(), - use_ir: false, - source, - ); - let actual_ir = nu!( - cwd: ir_dirs.test(), - use_ir: true, - source, - ); + Playground::setup("test_eval", |dirs, _playground| { + let actual = nu!( + cwd: dirs.test(), + source, + ); - match expected_out { - Eq(eq) => { - assert_eq!(actual_ast.out, eq); - assert_eq!(actual_ir.out, eq); - assert!(actual_ast.status.success()); - assert!(actual_ir.status.success()); - } - Matches(regex) => { - let compiled_regex = Regex::new(regex).expect("regex failed to compile"); - assert!( - compiled_regex.is_match(&actual_ast.out), - "AST eval out does not match: {}\n{}", - regex, - actual_ast.out - ); - assert!( - compiled_regex.is_match(&actual_ir.out), - "IR eval out does not match: {}\n{}", - regex, - actual_ir.out, - ); - assert!(actual_ast.status.success()); - assert!(actual_ir.status.success()); - } - Error(regex) => { - let compiled_regex = Regex::new(regex).expect("regex failed to compile"); - assert!( - compiled_regex.is_match(&actual_ast.err), - "AST eval err does not match: {}", - regex - ); - assert!( - compiled_regex.is_match(&actual_ir.err), - "IR eval err does not match: {}", - regex - ); - assert!(!actual_ast.status.success()); - assert!(!actual_ir.status.success()); - } - FileEq(path, contents) => { - let ast_contents = std::fs::read_to_string(ast_dirs.test().join(path)) - .expect("failed to read AST file"); - let ir_contents = std::fs::read_to_string(ir_dirs.test().join(path)) - .expect("failed to read IR file"); - assert_eq!(ast_contents.trim(), contents); - assert_eq!(ir_contents.trim(), contents); - assert!(actual_ast.status.success()); - assert!(actual_ir.status.success()); - } + match expected_out { + Eq(eq) => { + assert_eq!(actual.out, eq); + assert!(actual.status.success()); } - assert_eq!(actual_ast.out, actual_ir.out); - }) + Matches(regex) => { + let compiled_regex = Regex::new(regex).expect("regex failed to compile"); + assert!( + compiled_regex.is_match(&actual.out), + "eval out does not match: {}\n{}", + regex, + actual.out, + ); + assert!(actual.status.success()); + } + Error(regex) => { + let compiled_regex = Regex::new(regex).expect("regex failed to compile"); + assert!( + compiled_regex.is_match(&actual.err), + "eval err does not match: {}", + regex + ); + assert!(!actual.status.success()); + } + FileEq(path, contents) => { + let read_contents = + std::fs::read_to_string(dirs.test().join(path)).expect("failed to read file"); + assert_eq!(read_contents.trim(), contents); + assert!(actual.status.success()); + } + } }); } From 9d0f69ac507b82500a71453035d7fd4f78a897bd Mon Sep 17 00:00:00 2001 From: Jack Wright <56345+ayax79@users.noreply.github.com> Date: Thu, 14 Nov 2024 20:10:38 -0800 Subject: [PATCH 06/21] Add support for converting polars decimal values to nushell values (#14343) Adds support for converting from polars decimal type to nushell values. This fix works by first converting a polars decimal series to an f64 series, then converting to Value::Float Co-authored-by: Jack Wright --- .../src/dataframe/values/nu_dataframe/conversion.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/crates/nu_plugin_polars/src/dataframe/values/nu_dataframe/conversion.rs b/crates/nu_plugin_polars/src/dataframe/values/nu_dataframe/conversion.rs index a920aac0a3..c2d64741f4 100644 --- a/crates/nu_plugin_polars/src/dataframe/values/nu_dataframe/conversion.rs +++ b/crates/nu_plugin_polars/src/dataframe/values/nu_dataframe/conversion.rs @@ -1201,6 +1201,18 @@ fn series_to_values( Ok(values) } + DataType::Decimal(_precision, _scale) => { + let casted = series + .cast(&DataType::Float64) + .map_err(|e| ShellError::GenericError { + error: "Errors casting decimal column to float".into(), + msg: "".into(), + span: None, + help: Some(e.to_string()), + inner: vec![], + })?; + series_to_values(&casted, maybe_from_row, maybe_size, span) + } e => Err(ShellError::GenericError { error: "Error creating Dataframe".into(), msg: "".to_string(), From 8c1ab7e0a3b803279e09c93c70a02e3636e0146b Mon Sep 17 00:00:00 2001 From: Douglas <32344964+NotTheDr01ds@users.noreply.github.com> Date: Thu, 14 Nov 2024 23:27:26 -0500 Subject: [PATCH 07/21] Add proper config defaults for hooks (#14341) # Release Notes Excerpt * Hooks now default to an empty value of the proper type (e.g., `[]` or `{}`) when not otherwise specified # Description ```nushell # Start with no config nu -n # Populate with defaults $env.config = {} $env.config.hooks ``` * Before: All hooks other than `display_output` were set to `null`. Attempting to append a hook using `++=` would fail unless it had already been assigned. * After: * `pre_prompt`, `pre_execution`, and `command_not_found` are set to empty lists. This allows the user to simply append new hooks using `++=`. * `env_change` is set to an empty record. This allows the user to add new hooks using `merge`, although a "helper" command would still be useful (TODO: stdlib). Also fixed a typo in an error message. # User-Facing Changes There shouldn't be any breaking changes since (before) there were no guarantees of the hook's value/type. Previously, users would have to check for `null` and `default` to an empty list before appending. Any user-strategies for dealing with the problem should continue to work after this change. # Tests + Formatting - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` Note that, for reasons I cannot ascertain, this PR appears to have *fixed* the `command_not_found_error_recognizes_non_executable_file` test that was previously broken by #12953. That PR essentially rewrote the test to match the new behavior, but it no longer tested what it was intended to test. Now, the test is working again as designed (and as it works in the REPL). # After Submitting This will be covered in the Configuration update for #14249. This PR will simplify several examples in the doc. --- crates/nu-command/src/system/run_external.rs | 2 +- crates/nu-protocol/src/config/hooks.rs | 9 +++++---- tests/shell/pipeline/commands/external.rs | 6 +++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 013d3d8374..32543716f1 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -484,7 +484,7 @@ pub fn command_not_found( if Path::new(name).is_file() { return ShellError::ExternalCommand { label: format!("Command `{name}` not found"), - help: format!("`{name}` refers to a file that is not executable. Did you forget to to set execute permissions?"), + help: format!("`{name}` refers to a file that is not executable. Did you forget to set execute permissions?"), span, }; } diff --git a/crates/nu-protocol/src/config/hooks.rs b/crates/nu-protocol/src/config/hooks.rs index 9b9860a87d..374b3818d6 100644 --- a/crates/nu-protocol/src/config/hooks.rs +++ b/crates/nu-protocol/src/config/hooks.rs @@ -1,5 +1,6 @@ use super::prelude::*; use crate as nu_protocol; +use crate::Record; /// Definition of a parsed hook from the config object #[derive(Clone, Debug, IntoValue, PartialEq, Serialize, Deserialize)] @@ -14,14 +15,14 @@ pub struct Hooks { impl Hooks { pub fn new() -> Self { Self { - pre_prompt: None, - pre_execution: None, - env_change: None, + pre_prompt: Some(Value::list(vec![], Span::unknown())), + pre_execution: Some(Value::list(vec![], Span::unknown())), + env_change: Some(Value::record(Record::default(), Span::unknown())), display_output: Some(Value::string( "if (term size).columns >= 100 { table -e } else { table }", Span::unknown(), )), - command_not_found: None, + command_not_found: Some(Value::list(vec![], Span::unknown())), } } } diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index 207a94766b..172ac8cbfc 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -131,9 +131,9 @@ fn command_not_found_error_suggests_typo_fix() { #[test] fn command_not_found_error_recognizes_non_executable_file() { let actual = nu!("./Cargo.toml"); - assert!(actual - .err - .contains("is neither a Nushell built-in or a known external command")); + assert!(actual.err.contains( + "refers to a file that is not executable. Did you forget to set execute permissions?" + )); } #[test] From f7832c0e822a3db7546b2ddb9b6ebb952954338b Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Fri, 15 Nov 2024 06:39:42 -0600 Subject: [PATCH 08/21] allow nuscripts to be run again on windows with assoc/ftype (#14318) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description This PR tries to correct the problem of nushell scripts being made executable on Windows systems. In order to do this, these steps need to take place. 1. `assoc .nu=nuscript` 2. `ftype nuscript=C:\path\to\nu.exe '%1' %*` 3. modify the env var PATHEXT by appending `;.NU` at the end   Once those steps are done and this PR is landed, one should be able to create a script such as this. ```nushell ❯ open im_exe.nu def main [arg] { print $"Hello ($arg)!" } ``` Then they should be able to do this to run the nushell script. ```nushell ❯ im_exe Nushell Hello Nushell! ``` Under-the-hood, nushell is shelling out to cmd.exe in order to run the nushell script. # User-Facing Changes closes #13020 # Tests + Formatting # After Submitting --- crates/nu-command/src/system/run_external.rs | 31 ++++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 32543716f1..67b848915d 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -51,7 +51,6 @@ impl Command for External { input: PipelineData, ) -> Result { let cwd = engine_state.cwd(Some(stack))?; - let name: Value = call.req(engine_state, stack, 0)?; let name_str: Cow = match &name { @@ -68,10 +67,36 @@ impl Command for External { _ => Path::new(&*name_str).to_owned(), }; + // On Windows, the user could have run the cmd.exe built-in "assoc" command + // Example: "assoc .nu=nuscript" and then run the cmd.exe built-in "ftype" command + // Example: "ftype nuscript=C:\path\to\nu.exe '%1' %*" and then added the nushell + // script extension ".NU" to the PATHEXT environment variable. In this case, we use + // the which command, which will find the executable with or without the extension. + // If it "which" returns true, that means that we've found the nushell script and we + // believe the user wants to use the windows association to run the script. The only + // easy way to do this is to run cmd.exe with the script as an argument. + let potential_nuscript_in_windows = if cfg!(windows) { + // let's make sure it's a .nu scrtipt + if let Some(executable) = which(&expanded_name, "", cwd.as_ref()) { + let ext = executable + .extension() + .unwrap_or_default() + .to_string_lossy() + .to_uppercase(); + ext == "NU" + } else { + false + } + } else { + false + }; + // Find the absolute path to the executable. On Windows, set the // executable to "cmd.exe" if it's a CMD internal command. If the // command is not found, display a helpful error message. - let executable = if cfg!(windows) && is_cmd_internal_command(&name_str) { + let executable = if cfg!(windows) + && (is_cmd_internal_command(&name_str) || potential_nuscript_in_windows) + { PathBuf::from("cmd.exe") } else { // Determine the PATH to be used and then use `which` to find it - though this has no @@ -97,7 +122,7 @@ impl Command for External { // Configure args. let args = eval_arguments_from_call(engine_state, stack, call)?; #[cfg(windows)] - if is_cmd_internal_command(&name_str) { + if is_cmd_internal_command(&name_str) || potential_nuscript_in_windows { use std::os::windows::process::CommandExt; // The /D flag disables execution of AutoRun commands from registry. From b6e84879b6506c1a1f4ac0b900b36c90da489d7a Mon Sep 17 00:00:00 2001 From: Bahex Date: Fri, 15 Nov 2024 15:40:49 +0300 Subject: [PATCH 09/21] add multiple grouper support to `group-by` (#14337) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - closes #14330 Related: - #2607 - #14019 - #14316 # Description This PR changes `group-by` to support grouping by multiple `grouper` arguments. # Changes - No grouper: no change in behavior - Single grouper - `--to-table=false`: no change in behavior - `--to-table=true`: - closure grouper: named group0 - cell-path grouper: named after the cell-path - Multiple groupers: - `--to-table=false`: nested groups - `--to-table=true`: one column for each grouper argument, followed by the `items` column - columns corresponding to cell-paths are named after them - columns corresponding to closure groupers are named `group{i}` where `i` is the index of the grouper argument # Examples ```nushell > [1 3 1 3 2 1 1] | group-by ╭───┬───────────╮ │ │ ╭───┬───╮ │ │ 1 │ │ 0 │ 1 │ │ │ │ │ 1 │ 1 │ │ │ │ │ 2 │ 1 │ │ │ │ │ 3 │ 1 │ │ │ │ ╰───┴───╯ │ │ │ ╭───┬───╮ │ │ 3 │ │ 0 │ 3 │ │ │ │ │ 1 │ 3 │ │ │ │ ╰───┴───╯ │ │ │ ╭───┬───╮ │ │ 2 │ │ 0 │ 2 │ │ │ │ ╰───┴───╯ │ ╰───┴───────────╯ > [1 3 1 3 2 1 1] | group-by --to-table ╭─#─┬─group─┬───items───╮ │ 0 │ 1 │ ╭───┬───╮ │ │ │ │ │ 0 │ 1 │ │ │ │ │ │ 1 │ 1 │ │ │ │ │ │ 2 │ 1 │ │ │ │ │ │ 3 │ 1 │ │ │ │ │ ╰───┴───╯ │ │ 1 │ 3 │ ╭───┬───╮ │ │ │ │ │ 0 │ 3 │ │ │ │ │ │ 1 │ 3 │ │ │ │ │ ╰───┴───╯ │ │ 2 │ 2 │ ╭───┬───╮ │ │ │ │ │ 0 │ 2 │ │ │ │ │ ╰───┴───╯ │ ╰─#─┴─group─┴───items───╯ > [1 3 1 3 2 1 1] | group-by { $in >= 2 } ╭───────┬───────────╮ │ │ ╭───┬───╮ │ │ false │ │ 0 │ 1 │ │ │ │ │ 1 │ 1 │ │ │ │ │ 2 │ 1 │ │ │ │ │ 3 │ 1 │ │ │ │ ╰───┴───╯ │ │ │ ╭───┬───╮ │ │ true │ │ 0 │ 3 │ │ │ │ │ 1 │ 3 │ │ │ │ │ 2 │ 2 │ │ │ │ ╰───┴───╯ │ ╰───────┴───────────╯ > [1 3 1 3 2 1 1] | group-by { $in >= 2 } --to-table ╭─#─┬─group0─┬───items───╮ │ 0 │ false │ ╭───┬───╮ │ │ │ │ │ 0 │ 1 │ │ │ │ │ │ 1 │ 1 │ │ │ │ │ │ 2 │ 1 │ │ │ │ │ │ 3 │ 1 │ │ │ │ │ ╰───┴───╯ │ │ 1 │ true │ ╭───┬───╮ │ │ │ │ │ 0 │ 3 │ │ │ │ │ │ 1 │ 3 │ │ │ │ │ │ 2 │ 2 │ │ │ │ │ ╰───┴───╯ │ ╰─#─┴─group0─┴───items───╯ ``` ```nushell let data = [ [name, lang, year]; [andres, rb, "2019"], [jt, rs, "2019"], [storm, rs, "2021"] ] > $data ╭─#─┬──name──┬─lang─┬─year─╮ │ 0 │ andres │ rb │ 2019 │ │ 1 │ jt │ rs │ 2019 │ │ 2 │ storm │ rs │ 2021 │ ╰─#─┴──name──┴─lang─┴─year─╯ ``` ```nushell > $data | group-by lang ╭────┬──────────────────────────────╮ │ │ ╭─#─┬──name──┬─lang─┬─year─╮ │ │ rb │ │ 0 │ andres │ rb │ 2019 │ │ │ │ ╰─#─┴──name──┴─lang─┴─year─╯ │ │ │ ╭─#─┬─name──┬─lang─┬─year─╮ │ │ rs │ │ 0 │ jt │ rs │ 2019 │ │ │ │ │ 1 │ storm │ rs │ 2021 │ │ │ │ ╰─#─┴─name──┴─lang─┴─year─╯ │ ╰────┴──────────────────────────────╯ ``` Group column is now named after the grouper, to allow multiple groupers. ```nushell > $data | group-by lang --to-table # column names changed! ╭─#─┬─lang─┬────────────items─────────────╮ │ 0 │ rb │ ╭─#─┬──name──┬─lang─┬─year─╮ │ │ │ │ │ 0 │ andres │ rb │ 2019 │ │ │ │ │ ╰─#─┴──name──┴─lang─┴─year─╯ │ │ 1 │ rs │ ╭─#─┬─name──┬─lang─┬─year─╮ │ │ │ │ │ 0 │ jt │ rs │ 2019 │ │ │ │ │ │ 1 │ storm │ rs │ 2021 │ │ │ │ │ ╰─#─┴─name──┴─lang─┴─year─╯ │ ╰─#─┴─lang─┴────────────items─────────────╯ ``` Grouping by multiple columns makes finer grained aggregations possible. ```nushell > $data | group-by lang year --to-table ╭─#─┬─lang─┬─year─┬────────────items─────────────╮ │ 0 │ rb │ 2019 │ ╭─#─┬──name──┬─lang─┬─year─╮ │ │ │ │ │ │ 0 │ andres │ rb │ 2019 │ │ │ │ │ │ ╰─#─┴──name──┴─lang─┴─year─╯ │ │ 1 │ rs │ 2019 │ ╭─#─┬─name─┬─lang─┬─year─╮ │ │ │ │ │ │ 0 │ jt │ rs │ 2019 │ │ │ │ │ │ ╰─#─┴─name─┴─lang─┴─year─╯ │ │ 2 │ rs │ 2021 │ ╭─#─┬─name──┬─lang─┬─year─╮ │ │ │ │ │ │ 0 │ storm │ rs │ 2021 │ │ │ │ │ │ ╰─#─┴─name──┴─lang─┴─year─╯ │ ╰─#─┴─lang─┴─year─┴────────────items─────────────╯ ``` Grouping by multiple columns, without `--to-table` returns a nested structure. This is equivalent to `$data | group-by year | split-by lang`, making `split-by` obsolete. ```nushell > $data | group-by lang year ╭────┬─────────────────────────────────────────╮ │ │ ╭──────┬──────────────────────────────╮ │ │ rb │ │ │ ╭─#─┬──name──┬─lang─┬─year─╮ │ │ │ │ │ 2019 │ │ 0 │ andres │ rb │ 2019 │ │ │ │ │ │ │ ╰─#─┴──name──┴─lang─┴─year─╯ │ │ │ │ ╰──────┴──────────────────────────────╯ │ │ │ ╭──────┬─────────────────────────────╮ │ │ rs │ │ │ ╭─#─┬─name─┬─lang─┬─year─╮ │ │ │ │ │ 2019 │ │ 0 │ jt │ rs │ 2019 │ │ │ │ │ │ │ ╰─#─┴─name─┴─lang─┴─year─╯ │ │ │ │ │ │ ╭─#─┬─name──┬─lang─┬─year─╮ │ │ │ │ │ 2021 │ │ 0 │ storm │ rs │ 2021 │ │ │ │ │ │ │ ╰─#─┴─name──┴─lang─┴─year─╯ │ │ │ │ ╰──────┴─────────────────────────────╯ │ ╰────┴─────────────────────────────────────────╯ ``` From #2607: > Here's a couple more examples without much explanation. This one shows adding two grouping keys. I'm always wanting to add more columns when using group-by and it just-work:tm: `gb.exe -f movies-2.csv -k 3,2 -s 7 --skip_header` > > ``` > k:3 | k:2 | count | sum:7 > -----------------------+-----------+-------+-------------------- > 20th Century Fox | Drama | 1 | 117.09 > 20th Century Fox | Romance | 1 | 39.66 > CBS | Comedy | 1 | 77.09 > Disney | Animation | 4 | 1264.23 > Disney | Comedy | 4 | 950.27 > Fox | Comedy | 5 | 661.85 > Independent | Comedy | 7 | 399.07 > Independent | Drama | 4 | 69.75 > Independent | Romance | 7 | 1048.75 > Independent | romance | 1 | 29.37 > ... > ``` This example can be achieved like this: ```nushell > open movies-2.csv | group-by "Lead Studio" Genre --to-table | insert count {get items | length} | insert sum { get items."Worldwide Gross" | math sum} | reject items | sort-by "Lead Studio" Genre ╭─#──┬──────Lead Studio──────┬───Genre───┬─count─┬───sum───╮ │ 0 │ 20th Century Fox │ Drama │ 1 │ 117.09 │ │ 1 │ 20th Century Fox │ Romance │ 1 │ 39.66 │ │ 2 │ CBS │ Comedy │ 1 │ 77.09 │ │ 3 │ Disney │ Animation │ 4 │ 1264.23 │ │ 4 │ Disney │ Comedy │ 4 │ 950.27 │ │ 5 │ Fox │ Comedy │ 5 │ 661.85 │ │ 6 │ Fox │ comedy │ 1 │ 60.72 │ │ 7 │ Independent │ Comedy │ 7 │ 399.07 │ │ 8 │ Independent │ Drama │ 4 │ 69.75 │ │ 9 │ Independent │ Romance │ 7 │ 1048.75 │ │ 10 │ Independent │ romance │ 1 │ 29.37 │ ... ``` --- crates/nu-command/src/filters/group_by.rs | 284 +++++++++++++++++----- crates/nu-std/testing.nu | 2 +- 2 files changed, 226 insertions(+), 60 deletions(-) diff --git a/crates/nu-command/src/filters/group_by.rs b/crates/nu-command/src/filters/group_by.rs index 74a19c38e3..9396de4781 100644 --- a/crates/nu-command/src/filters/group_by.rs +++ b/crates/nu-command/src/filters/group_by.rs @@ -1,6 +1,6 @@ use indexmap::IndexMap; use nu_engine::{command_prelude::*, ClosureEval}; -use nu_protocol::engine::Closure; +use nu_protocol::{engine::Closure, IntoValue}; #[derive(Clone)] pub struct GroupBy; @@ -22,7 +22,7 @@ impl Command for GroupBy { "Return a table with \"groups\" and \"items\" columns", None, ) - .optional( + .rest( "grouper", SyntaxShape::OneOf(vec![ SyntaxShape::CellPath, @@ -135,7 +135,89 @@ impl Command for GroupBy { Value::test_string("false"), ]), })), - } + }, + Example { + description: "Group items by multiple columns' values", + example: r#"[ + [name, lang, year]; + [andres, rb, "2019"], + [jt, rs, "2019"], + [storm, rs, "2021"] + ] + | group-by lang year"#, + result: Some(Value::test_record(record! { + "rb" => Value::test_record(record! { + "2019" => Value::test_list( + vec![Value::test_record(record! { + "name" => Value::test_string("andres"), + "lang" => Value::test_string("rb"), + "year" => Value::test_string("2019"), + })], + ), + }), + "rs" => Value::test_record(record! { + "2019" => Value::test_list( + vec![Value::test_record(record! { + "name" => Value::test_string("jt"), + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2019"), + })], + ), + "2021" => Value::test_list( + vec![Value::test_record(record! { + "name" => Value::test_string("storm"), + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2021"), + })], + ), + }), + })) + }, + Example { + description: "Group items by multiple columns' values", + example: r#"[ + [name, lang, year]; + [andres, rb, "2019"], + [jt, rs, "2019"], + [storm, rs, "2021"] + ] + | group-by lang year --to-table"#, + result: Some(Value::test_list(vec![ + Value::test_record(record! { + "lang" => Value::test_string("rb"), + "year" => Value::test_string("2019"), + "items" => Value::test_list(vec![ + Value::test_record(record! { + "name" => Value::test_string("andres"), + "lang" => Value::test_string("rb"), + "year" => Value::test_string("2019"), + }) + ]), + }), + Value::test_record(record! { + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2019"), + "items" => Value::test_list(vec![ + Value::test_record(record! { + "name" => Value::test_string("jt"), + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2019"), + }) + ]), + }), + Value::test_record(record! { + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2021"), + "items" => Value::test_list(vec![ + Value::test_record(record! { + "name" => Value::test_string("storm"), + "lang" => Value::test_string("rs"), + "year" => Value::test_string("2021"), + }) + ]), + }), + ])) + }, ] } } @@ -147,7 +229,7 @@ pub fn group_by( input: PipelineData, ) -> Result { let head = call.head; - let grouper: Option = call.opt(engine_state, stack, 0)?; + let groupers: Vec = call.rest(engine_state, stack, 0)?; let to_table = call.has_flag(engine_state, stack, "to-table")?; let config = engine_state.get_config(); @@ -156,29 +238,22 @@ pub fn group_by( return Ok(Value::record(Record::new(), head).into_pipeline_data()); } - let groups = match grouper { - Some(grouper) => { - let span = grouper.span(); - match grouper { - Value::CellPath { val, .. } => group_cell_path(val, values, config)?, - Value::Closure { val, .. } => { - group_closure(values, span, *val, engine_state, stack)? - } - _ => { - return Err(ShellError::TypeMismatch { - err_message: "unsupported grouper type".to_string(), - span, - }) - } - } + let mut groupers = groupers.into_iter(); + + let grouped = if let Some(grouper) = groupers.next() { + let mut groups = Grouped::new(&grouper, values, config, engine_state, stack)?; + for grouper in groupers { + groups.subgroup(&grouper, config, engine_state, stack)?; } - None => group_no_grouper(values, config)?, + groups + } else { + Grouped::empty(values, config) }; let value = if to_table { - groups_to_table(groups, head) + grouped.into_table(head) } else { - groups_to_record(groups, head) + grouped.into_record(head) }; Ok(value.into_pipeline_data()) @@ -207,20 +282,6 @@ fn group_cell_path( Ok(groups) } -fn group_no_grouper( - values: Vec, - config: &nu_protocol::Config, -) -> Result>, ShellError> { - let mut groups = IndexMap::<_, Vec<_>>::new(); - - for value in values.into_iter() { - let key = value.to_abbreviated_string(config); - groups.entry(key).or_default().push(value); - } - - Ok(groups) -} - fn group_closure( values: Vec, span: Span, @@ -244,32 +305,137 @@ fn group_closure( Ok(groups) } -fn groups_to_record(groups: IndexMap>, span: Span) -> Value { - Value::record( - groups - .into_iter() - .map(|(k, v)| (k, Value::list(v, span))) - .collect(), - span, - ) +struct Grouped { + grouper: Option, + groups: Tree, } -fn groups_to_table(groups: IndexMap>, span: Span) -> Value { - Value::list( - groups - .into_iter() - .map(|(group, items)| { - Value::record( - record! { - "group" => Value::string(group, span), - "items" => Value::list(items, span), - }, +enum Tree { + Leaf(IndexMap>), + Branch(IndexMap), +} + +impl Grouped { + fn empty(values: Vec, config: &nu_protocol::Config) -> Self { + let mut groups = IndexMap::<_, Vec<_>>::new(); + + for value in values.into_iter() { + let key = value.to_abbreviated_string(config); + groups.entry(key).or_default().push(value); + } + + Self { + grouper: Some("group".into()), + groups: Tree::Leaf(groups), + } + } + + fn new( + grouper: &Value, + values: Vec, + config: &nu_protocol::Config, + engine_state: &EngineState, + stack: &mut Stack, + ) -> Result { + let span = grouper.span(); + let groups = match grouper { + Value::CellPath { val, .. } => group_cell_path(val.clone(), values, config)?, + Value::Closure { val, .. } => { + group_closure(values, span, Closure::clone(val), engine_state, stack)? + } + _ => { + return Err(ShellError::TypeMismatch { + err_message: "unsupported grouper type".to_string(), span, - ) - }) - .collect(), - span, - ) + }) + } + }; + let grouper = grouper.as_cell_path().ok().map(CellPath::to_column_name); + Ok(Self { + grouper, + groups: Tree::Leaf(groups), + }) + } + + fn subgroup( + &mut self, + grouper: &Value, + config: &nu_protocol::Config, + engine_state: &EngineState, + stack: &mut Stack, + ) -> Result<(), ShellError> { + let groups = match &mut self.groups { + Tree::Leaf(groups) => std::mem::take(groups) + .into_iter() + .map(|(key, values)| -> Result<_, ShellError> { + let leaf = Self::new(grouper, values, config, engine_state, stack)?; + Ok((key, leaf)) + }) + .collect::, ShellError>>()?, + Tree::Branch(nested_groups) => { + let mut nested_groups = std::mem::take(nested_groups); + for v in nested_groups.values_mut() { + v.subgroup(grouper, config, engine_state, stack)?; + } + nested_groups + } + }; + self.groups = Tree::Branch(groups); + Ok(()) + } + + fn into_table(self, head: Span) -> Value { + self._into_table(head, 0) + .into_iter() + .map(|row| row.into_iter().rev().collect::().into_value(head)) + .collect::>() + .into_value(head) + } + + fn _into_table(self, head: Span, index: usize) -> Vec { + let grouper = self.grouper.unwrap_or_else(|| format!("group{index}")); + match self.groups { + Tree::Leaf(leaf) => leaf + .into_iter() + .map(|(group, values)| { + [ + ("items".to_string(), values.into_value(head)), + (grouper.clone(), group.into_value(head)), + ] + .into_iter() + .collect() + }) + .collect::>(), + Tree::Branch(branch) => branch + .into_iter() + .flat_map(|(group, items)| { + let mut inner = items._into_table(head, index + 1); + for row in &mut inner { + row.insert(grouper.clone(), group.clone().into_value(head)); + } + inner + }) + .collect(), + } + } + + fn into_record(self, head: Span) -> Value { + match self.groups { + Tree::Leaf(leaf) => Value::record( + leaf.into_iter() + .map(|(k, v)| (k, v.into_value(head))) + .collect(), + head, + ), + Tree::Branch(branch) => { + let values = branch + .into_iter() + .map(|(k, v)| (k, v.into_record(head))) + .collect(); + Value::record(values, head) + } + } + } } #[cfg(test)] diff --git a/crates/nu-std/testing.nu b/crates/nu-std/testing.nu index 003e1d1ebc..7052983a21 100644 --- a/crates/nu-std/testing.nu +++ b/crates/nu-std/testing.nu @@ -79,7 +79,7 @@ def create-test-record [] nothing -> record Date: Fri, 15 Nov 2024 13:18:01 -0500 Subject: [PATCH 10/21] Update rstest from 0.18 to 0.23 (the current version) (#14350) --- Cargo.lock | 92 +++++++++++++++++++++++++++--------------------------- Cargo.toml | 2 +- 2 files changed, 47 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7343d3c043..2754f5bd23 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -275,7 +275,7 @@ checksum = "16e62a023e7c117e27523144c5d2459f4397fcc3cab0085af8e2224f643a0193" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -286,7 +286,7 @@ checksum = "6e0c28dcc82d7c8ead5cb13beb15405b57b8546e93215673ff8ca0349a028107" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -383,7 +383,7 @@ dependencies = [ "regex", "rustc-hash", "shlex", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -466,7 +466,7 @@ dependencies = [ "proc-macro-crate", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", "syn_derive", ] @@ -570,7 +570,7 @@ checksum = "0cc8b54b395f2fcfbb3d90c47b01c7f444d94d05bdeb775811dec868ac3bbc26" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -803,7 +803,7 @@ dependencies = [ "heck", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1108,7 +1108,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "13b588ba4ac1a99f7f2964d24b3d896ddc6bf847ee3855dbd4366f058cfcd331" dependencies = [ "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1195,7 +1195,7 @@ checksum = "d150dea618e920167e5973d70ae6ece4385b7164e0d799fe7c122dd0a5d912ad" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1206,7 +1206,7 @@ checksum = "5f33878137e4dafd7fa914ad4e259e18a4e8e532b9617a2d0150262bf53abfce" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1388,7 +1388,7 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1680,7 +1680,7 @@ checksum = "87750cf4b7a4c0625b1529e4c543c2182106e4dedc60a2a6455e00d212c489ac" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1942,7 +1942,7 @@ dependencies = [ "markup5ever 0.12.1", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -1956,7 +1956,7 @@ dependencies = [ "markup5ever 0.14.0", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -2726,7 +2726,7 @@ checksum = "dcf09caffaac8068c346b6df2a7fc27a177fd20b39421a39ce0a211bde679a6c" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -3248,7 +3248,7 @@ dependencies = [ "proc-macro-error", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -3954,7 +3954,7 @@ checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -4136,7 +4136,7 @@ dependencies = [ "pest_meta", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -4210,7 +4210,7 @@ dependencies = [ "phf_shared 0.11.2", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5147,7 +5147,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "76009fbe0614077fc1a2ce255e3a1881a2e3a3527097d5dc6d8212c585e7e38b" dependencies = [ "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5216,7 +5216,7 @@ checksum = "bcc303e793d3734489387d205e9b186fac9c6cfacedd98cbb2e8a5943595f3e6" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5352,9 +5352,9 @@ checksum = "3cd14fd5e3b777a7422cca79358c57a8f6e3a703d9ac187448d0daf220c2407f" [[package]] name = "rstest" -version = "0.18.2" +version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "97eeab2f3c0a199bc4be135c36c924b6590b88c377d416494288c14f2db30199" +checksum = "0a2c585be59b6b5dd66a9d2084aa1d8bd52fbdb806eafdeffb52791147862035" dependencies = [ "rstest_macros", "rustc_version", @@ -5362,9 +5362,9 @@ dependencies = [ [[package]] name = "rstest_macros" -version = "0.18.2" +version = "0.23.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d428f8247852f894ee1be110b375111b586d4fa431f6c46e64ba5a0dcccbe605" +checksum = "825ea780781b15345a146be27eaefb05085e337e869bff01b4306a4fd4a9ad5a" dependencies = [ "cfg-if", "glob", @@ -5373,7 +5373,7 @@ dependencies = [ "regex", "relative-path", "rustc_version", - "syn 2.0.75", + "syn 2.0.87", "unicode-ident", ] @@ -5412,7 +5412,7 @@ dependencies = [ "proc-macro2", "quote", "rust-embed-utils", - "syn 2.0.75", + "syn 2.0.87", "walkdir", ] @@ -5467,9 +5467,9 @@ checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" [[package]] name = "rustc_version" -version = "0.4.0" +version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +checksum = "cfcb3a22ef46e85b45de6ee7e79d063319ebb6594faafcf1c225ea92ab6e9b92" dependencies = [ "semver", ] @@ -5570,7 +5570,7 @@ checksum = "1db149f81d46d2deba7cd3c50772474707729550221e69588478ebf9ada425ae" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5650,7 +5650,7 @@ checksum = "24008e81ff7613ed8e5ba0cfaf24e2c2f1e5b8a0495711e44fcd4882fca62bcf" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5674,7 +5674,7 @@ checksum = "6c64451ba24fc7a6a2d60fc75dd9c83c90903b19028d4eff35e88fc1e86564e9" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5733,7 +5733,7 @@ checksum = "5d69265a08751de7844521fd15003ae0a888e035773ba05695c5c759a6f89eef" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -5910,7 +5910,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d904e7009df136af5297832a3ace3370cd14ff1546a232f4f185036c2736fcac" dependencies = [ "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -6025,7 +6025,7 @@ dependencies = [ "proc-macro2", "quote", "rustversion", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -6093,9 +6093,9 @@ dependencies = [ [[package]] name = "syn" -version = "2.0.75" +version = "2.0.87" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6af063034fc1935ede7be0122941bafa9bacb949334d090b77ca98b5817c7d9" +checksum = "25aa4ce346d03a6dcd68dd8b4010bcb74e54e62c90c573f394c46eae99aba32d" dependencies = [ "proc-macro2", "quote", @@ -6111,7 +6111,7 @@ dependencies = [ "proc-macro-error", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -6271,7 +6271,7 @@ checksum = "a4558b58466b9ad7ca0f102865eccc95938dca1a74a856f2b57b6629050da261" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -6547,7 +6547,7 @@ checksum = "70b20a22c42c8f1cd23ce5e34f165d4d37038f5b663ad20fb6adbdf029172483" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -6579,9 +6579,9 @@ checksum = "08f95100a766bf4f8f28f90d77e0a5461bbdb219042e7679bebe79004fed8d75" [[package]] name = "unicode-ident" -version = "1.0.12" +version = "1.0.13" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" +checksum = "e91b56cd4cadaeb79bbf1a5645f6b4f8dc5bde8834ad5894a8db35fda9efa1fe" [[package]] name = "unicode-linebreak" @@ -6925,7 +6925,7 @@ dependencies = [ "once_cell", "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", "wasm-bindgen-shared", ] @@ -6947,7 +6947,7 @@ checksum = "afc340c74d9005395cf9dd098506f7f44e38f2b4a21c6aaacf9a105ea5e1e836" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", "wasm-bindgen-backend", "wasm-bindgen-shared", ] @@ -7157,7 +7157,7 @@ checksum = "f6fc35f58ecd95a9b71c4f2329b911016e6bec66b3f2e6a4aad86bd2e99e2f9b" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -7168,7 +7168,7 @@ checksum = "08990546bf4edef8f431fa6326e032865f27138718c587dc21bc0265bbcb57cc" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] @@ -7513,7 +7513,7 @@ checksum = "fa4f8080344d4671fb4e831a13ad1e68092748387dfc4f55e356242fae12ce3e" dependencies = [ "proc-macro2", "quote", - "syn 2.0.75", + "syn 2.0.87", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e7318fa006..a976aeec17 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -143,7 +143,7 @@ rmp = "0.8" rmp-serde = "1.3" ropey = "1.6.1" roxmltree = "0.19" -rstest = { version = "0.18", default-features = false } +rstest = { version = "0.23", default-features = false } rusqlite = "0.31" rust-embed = "8.5.0" serde = { version = "1.0" } From 455d32d9e5512096b27c167ca57bd53dc4faf2b9 Mon Sep 17 00:00:00 2001 From: Stefan Holderbach Date: Fri, 15 Nov 2024 19:24:39 +0100 Subject: [PATCH 11/21] Cut down unnecessary lint allows (#14335) Trying to reduce lint allows either by checking if they are former false positives or by fixing the underlying warning. - **Remove dead `allow(dead_code)`** - **Remove recursive dead code** - **Remove dead code** - **Move test only functions to test module** The unit tests that use them, themselves are somewhat sus in that they mock the usage and not test specificly used methods of the implementation, so there is a risk for divergence - **Remove `clippy::uninit_vec` allow.** May have been a false positive, or the impl has changed somewhat. We certainly want to look at the unsafe code here to vet for correctness. --- crates/nu-cli/src/prompt_update.rs | 9 --- crates/nu-cli/src/reedline_config.rs | 4 -- crates/nu-command/src/progress_bar.rs | 5 -- crates/nu-command/src/strings/guess_width.rs | 65 ++++++++++---------- crates/nu-system/src/windows.rs | 2 - 5 files changed, 31 insertions(+), 54 deletions(-) diff --git a/crates/nu-cli/src/prompt_update.rs b/crates/nu-cli/src/prompt_update.rs index 95280af828..63dd9ce19d 100644 --- a/crates/nu-cli/src/prompt_update.rs +++ b/crates/nu-cli/src/prompt_update.rs @@ -30,30 +30,21 @@ pub(crate) const TRANSIENT_PROMPT_MULTILINE_INDICATOR: &str = pub(crate) const PRE_PROMPT_MARKER: &str = "\x1b]133;A\x1b\\"; pub(crate) const POST_PROMPT_MARKER: &str = "\x1b]133;B\x1b\\"; pub(crate) const PRE_EXECUTION_MARKER: &str = "\x1b]133;C\x1b\\"; -#[allow(dead_code)] pub(crate) const POST_EXECUTION_MARKER_PREFIX: &str = "\x1b]133;D;"; -#[allow(dead_code)] pub(crate) const POST_EXECUTION_MARKER_SUFFIX: &str = "\x1b\\"; // OSC633 is the same as OSC133 but specifically for VSCode pub(crate) const VSCODE_PRE_PROMPT_MARKER: &str = "\x1b]633;A\x1b\\"; pub(crate) const VSCODE_POST_PROMPT_MARKER: &str = "\x1b]633;B\x1b\\"; -#[allow(dead_code)] pub(crate) const VSCODE_PRE_EXECUTION_MARKER: &str = "\x1b]633;C\x1b\\"; -#[allow(dead_code)] //"\x1b]633;D;{}\x1b\\" pub(crate) const VSCODE_POST_EXECUTION_MARKER_PREFIX: &str = "\x1b]633;D;"; -#[allow(dead_code)] pub(crate) const VSCODE_POST_EXECUTION_MARKER_SUFFIX: &str = "\x1b\\"; -#[allow(dead_code)] //"\x1b]633;E;{}\x1b\\" pub(crate) const VSCODE_COMMANDLINE_MARKER_PREFIX: &str = "\x1b]633;E;"; -#[allow(dead_code)] pub(crate) const VSCODE_COMMANDLINE_MARKER_SUFFIX: &str = "\x1b\\"; -#[allow(dead_code)] // "\x1b]633;P;Cwd={}\x1b\\" pub(crate) const VSCODE_CWD_PROPERTY_MARKER_PREFIX: &str = "\x1b]633;P;Cwd="; -#[allow(dead_code)] pub(crate) const VSCODE_CWD_PROPERTY_MARKER_SUFFIX: &str = "\x1b\\"; pub(crate) const RESET_APPLICATION_MODE: &str = "\x1b[?1l"; diff --git a/crates/nu-cli/src/reedline_config.rs b/crates/nu-cli/src/reedline_config.rs index 4c1594389e..4012876ac0 100644 --- a/crates/nu-cli/src/reedline_config.rs +++ b/crates/nu-cli/src/reedline_config.rs @@ -711,7 +711,6 @@ pub(crate) fn create_keybindings(config: &Config) -> Result Result, mode: &Value, keybinding: &ParsedKeybinding, config: &Config, @@ -755,7 +752,6 @@ fn add_keybinding( Value::List { vals, .. } => { for inner_mode in vals { add_keybinding( - name, inner_mode, keybinding, config, diff --git a/crates/nu-command/src/progress_bar.rs b/crates/nu-command/src/progress_bar.rs index db4d4e23b1..b700c1e2d9 100644 --- a/crates/nu-command/src/progress_bar.rs +++ b/crates/nu-command/src/progress_bar.rs @@ -45,11 +45,6 @@ impl NuProgressBar { self.pb.set_position(bytes_processed); } - #[allow(dead_code)] - pub fn finished_msg(&self, msg: String) { - self.pb.finish_with_message(msg); - } - pub fn abandoned_msg(&self, msg: String) { self.pb.abandon_with_message(msg); } diff --git a/crates/nu-command/src/strings/guess_width.rs b/crates/nu-command/src/strings/guess_width.rs index 59d666dd02..2eefb7d3f0 100644 --- a/crates/nu-command/src/strings/guess_width.rs +++ b/crates/nu-command/src/strings/guess_width.rs @@ -291,42 +291,39 @@ fn positions_helper(blanks: &[usize], min_lines: usize) -> Vec { pos } -// to_rows returns rows separated by columns. -#[allow(dead_code)] -fn to_rows(lines: Vec, pos: Vec, trim_space: bool) -> Vec> { - let mut rows: Vec> = Vec::with_capacity(lines.len()); - for line in lines { - let columns = split(&line, &pos, trim_space); - rows.push(columns); - } - rows -} - -// to_table parses a slice of lines and returns a table. -#[allow(dead_code)] -pub fn to_table(lines: Vec, header: usize, trim_space: bool) -> Vec> { - let pos = positions(&lines, header, 2); - to_rows(lines, pos, trim_space) -} - -// to_table_n parses a slice of lines and returns a table, but limits the number of splits. -#[allow(dead_code)] -pub fn to_table_n( - lines: Vec, - header: usize, - num_split: usize, - trim_space: bool, -) -> Vec> { - let mut pos = positions(&lines, header, 2); - if pos.len() > num_split { - pos.truncate(num_split); - } - to_rows(lines, pos, trim_space) -} - #[cfg(test)] mod tests { - use super::{to_table, to_table_n, GuessWidth}; + use super::*; + + /// to_rows returns rows separated by columns. + fn to_rows(lines: Vec, pos: Vec, trim_space: bool) -> Vec> { + let mut rows: Vec> = Vec::with_capacity(lines.len()); + for line in lines { + let columns = split(&line, &pos, trim_space); + rows.push(columns); + } + rows + } + + /// to_table parses a slice of lines and returns a table. + pub fn to_table(lines: Vec, header: usize, trim_space: bool) -> Vec> { + let pos = positions(&lines, header, 2); + to_rows(lines, pos, trim_space) + } + + /// to_table_n parses a slice of lines and returns a table, but limits the number of splits. + pub fn to_table_n( + lines: Vec, + header: usize, + num_split: usize, + trim_space: bool, + ) -> Vec> { + let mut pos = positions(&lines, header, 2); + if pos.len() > num_split { + pos.truncate(num_split); + } + to_rows(lines, pos, trim_space) + } #[test] fn test_guess_width_ps_trim() { diff --git a/crates/nu-system/src/windows.rs b/crates/nu-system/src/windows.rs index 59e2a77751..cc0ab99559 100644 --- a/crates/nu-system/src/windows.rs +++ b/crates/nu-system/src/windows.rs @@ -471,7 +471,6 @@ unsafe fn null_terminated_wchar_to_string(slice: &[u16]) -> String { } } -#[allow(clippy::uninit_vec)] unsafe fn get_process_data( handle: HANDLE, ptr: *const c_void, @@ -518,7 +517,6 @@ unsafe fn get_region_size(handle: HANDLE, ptr: *const c_void) -> Result Date: Sat, 16 Nov 2024 01:35:29 +0530 Subject: [PATCH 12/21] Seq char update will work on all char (#14261) # Description - fixes #14174 This PR addresses a bug in the `seq char` command where the command's behavior did not align with its help description, which stated that it prints a sequence of ASCII characters. The initial implementation only allowed alphabetic characters, leading to user confusion when non-alphabetic characters (e.g., digits, punctuation) were rejected or when unexpected behavior occurred for certain input ranges. ### Changes Made: - **Updated the input validation**: Modified the `is_single_character` function to accept any ASCII character instead of restricting to alphabetic characters. - **Enhanced error messages**: Clarified error messages to specify that any single ASCII character is acceptable. - **Expanded functionality**: Ensured that the command can now generate sequences that include non-alphabetic ASCII characters. - **Updated tests**: Added tests to cover new use cases involving non-alphabetic characters and improved validation. ### Examples After Fix: - `seq char '0' '9'` now outputs `['0', '1', '2', '3', '4', '5', '6', '7', '8', '9']` - `seq char ' ' '/'` outputs a list of characters from space to `/` - `seq char 'A' 'z'` correctly includes alphabetic and non-alphabetic characters between `A` and `z` # User-Facing Changes - Users can now input any single ASCII character for the `start` and `end` parameters of `seq char`. - The output will accurately include all characters within the specified ASCII range, including digits and punctuation. # Tests + Formatting - Added new tests to ensure the `seq char` command supports sequences including non-alphabetic ASCII characters. --- crates/nu-command/src/generators/seq_char.rs | 30 ++++++++----- crates/nu-command/tests/commands/seq_char.rs | 47 +++++++++++++++++++- 2 files changed, 65 insertions(+), 12 deletions(-) diff --git a/crates/nu-command/src/generators/seq_char.rs b/crates/nu-command/src/generators/seq_char.rs index c0f16593d7..0f51815067 100644 --- a/crates/nu-command/src/generators/seq_char.rs +++ b/crates/nu-command/src/generators/seq_char.rs @@ -45,9 +45,10 @@ impl Command for SeqChar { )), }, Example { - description: "sequence a to e, and put the characters in a pipe-separated string", + description: "Sequence a to e, and join the characters with a pipe", example: "seq char a e | str join '|'", // TODO: it would be nice to test this example, but it currently breaks the input/output type tests + // result: Some(Value::test_string("a|b|c|d|e")), result: None, }, ] @@ -65,7 +66,7 @@ impl Command for SeqChar { } fn is_single_character(ch: &str) -> bool { - ch.is_ascii() && ch.len() == 1 && ch.chars().all(char::is_alphabetic) + ch.is_ascii() && (ch.len() == 1) } fn seq_char( @@ -79,7 +80,7 @@ fn seq_char( if !is_single_character(&start.item) { return Err(ShellError::GenericError { error: "seq char only accepts individual ASCII characters as parameters".into(), - msg: "should be 1 character long".into(), + msg: "input should be a single ASCII character".into(), span: Some(start.span), help: None, inner: vec![], @@ -89,7 +90,7 @@ fn seq_char( if !is_single_character(&end.item) { return Err(ShellError::GenericError { error: "seq char only accepts individual ASCII characters as parameters".into(), - msg: "should be 1 character long".into(), + msg: "input should be a single ASCII character".into(), span: Some(end.span), help: None, inner: vec![], @@ -115,18 +116,27 @@ fn seq_char( } fn run_seq_char(start_ch: char, end_ch: char, span: Span) -> Result { - let mut result_vec = vec![]; - for current_ch in start_ch as u8..end_ch as u8 + 1 { - result_vec.push((current_ch as char).to_string()) - } - + let start = start_ch as u8; + let end = end_ch as u8; + let range = if start <= end { + start..=end + } else { + end..=start + }; + let result_vec = if start <= end { + range.map(|c| (c as char).to_string()).collect::>() + } else { + range + .rev() + .map(|c| (c as char).to_string()) + .collect::>() + }; let result = result_vec .into_iter() .map(|x| Value::string(x, span)) .collect::>(); Ok(Value::list(result, span).into_pipeline_data()) } - #[cfg(test)] mod tests { use super::*; diff --git a/crates/nu-command/tests/commands/seq_char.rs b/crates/nu-command/tests/commands/seq_char.rs index 6c78084b80..b59f45d39d 100644 --- a/crates/nu-command/tests/commands/seq_char.rs +++ b/crates/nu-command/tests/commands/seq_char.rs @@ -4,12 +4,55 @@ use nu_test_support::nu; fn fails_when_first_arg_is_multiple_chars() { let actual = nu!("seq char aa z"); - assert!(actual.err.contains("should be 1 character long")); + assert!(actual + .err + .contains("input should be a single ASCII character")); } #[test] fn fails_when_second_arg_is_multiple_chars() { let actual = nu!("seq char a zz"); - assert!(actual.err.contains("should be 1 character long")); + assert!(actual + .err + .contains("input should be a single ASCII character")); +} + +#[test] +fn generates_sequence_from_a_to_e() { + let actual = nu!("seq char a e | str join ''"); + + assert_eq!(actual.out, "abcde"); +} + +#[test] +fn generates_sequence_from_e_to_a() { + let actual = nu!("seq char e a | str join ''"); + + assert_eq!(actual.out, "edcba"); +} + +#[test] +fn fails_when_non_ascii_character_is_used_in_first_arg() { + let actual = nu!("seq char ñ z"); + + assert!(actual + .err + .contains("input should be a single ASCII character")); +} + +#[test] +fn fails_when_non_ascii_character_is_used_in_second_arg() { + let actual = nu!("seq char a ñ"); + + assert!(actual + .err + .contains("input should be a single ASCII character")); +} + +#[test] +fn joins_sequence_with_pipe() { + let actual = nu!("seq char a e | str join '|'"); + + assert_eq!(actual.out, "a|b|c|d|e"); } From 029c586717cf335ce87f187de8820942a7380a38 Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Sun, 17 Nov 2024 05:47:09 -0600 Subject: [PATCH 13/21] fix ansi bleed over on right prompt (#14357) # Description In certain situations, we had ansi bleed on the right prompt. This PR fixes that by prefixing the right prompt with an ansi reset `\x1b[0m`. This PR also adds some --log-level warn logging so we can see the ansi escapes that form the prompts. Closes https://github.com/nushell/nushell/issues/14268 --- crates/nu-cli/src/prompt_update.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/nu-cli/src/prompt_update.rs b/crates/nu-cli/src/prompt_update.rs index 63dd9ce19d..c033475586 100644 --- a/crates/nu-cli/src/prompt_update.rs +++ b/crates/nu-cli/src/prompt_update.rs @@ -1,5 +1,5 @@ use crate::NushellPrompt; -use log::trace; +use log::{trace, warn}; use nu_engine::ClosureEvalOnce; use nu_protocol::{ engine::{EngineState, Stack}, @@ -80,8 +80,13 @@ fn get_prompt_string( }) .and_then(|pipeline_data| { let output = pipeline_data.collect_string("", config).ok(); + let ansi_output = output.map(|mut x| { + // Always reset the color at the start of the right prompt + // to ensure there is no ansi bleed over + if x.is_empty() && prompt == PROMPT_COMMAND_RIGHT { + x.insert_str(0, "\x1b[0m") + }; - output.map(|mut x| { // Just remove the very last newline. if x.ends_with('\n') { x.pop(); @@ -91,7 +96,11 @@ fn get_prompt_string( x.pop(); } x - }) + }); + // Let's keep this for debugging purposes with nu --log-level warn + warn!("{}:{}:{} {:?}", file!(), line!(), column!(), ansi_output); + + ansi_output }) } From 6c36bd822cd36ea871befb14c3a448bf42c7e366 Mon Sep 17 00:00:00 2001 From: Jan Klass Date: Sun, 17 Nov 2024 19:17:35 +0100 Subject: [PATCH 14/21] Fix doc and code comment typos (#14366) # User-Facing Changes * Fixes `polars value-counts --column` help text typo * Fixes `polars agg-groups` help text typo --- crates/nu-command/src/system/run_external.rs | 2 +- .../src/dataframe/command/aggregation/agg_groups.rs | 2 +- .../src/dataframe/command/aggregation/value_counts.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 67b848915d..604278c676 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/crates/nu-command/src/system/run_external.rs @@ -76,7 +76,7 @@ impl Command for External { // believe the user wants to use the windows association to run the script. The only // easy way to do this is to run cmd.exe with the script as an argument. let potential_nuscript_in_windows = if cfg!(windows) { - // let's make sure it's a .nu scrtipt + // let's make sure it's a .nu script if let Some(executable) = which(&expanded_name, "", cwd.as_ref()) { let ext = executable .extension() diff --git a/crates/nu_plugin_polars/src/dataframe/command/aggregation/agg_groups.rs b/crates/nu_plugin_polars/src/dataframe/command/aggregation/agg_groups.rs index ae12239800..a8d58bd6f0 100644 --- a/crates/nu_plugin_polars/src/dataframe/command/aggregation/agg_groups.rs +++ b/crates/nu_plugin_polars/src/dataframe/command/aggregation/agg_groups.rs @@ -34,7 +34,7 @@ impl PluginCommand for ExprAggGroups { fn examples(&self) -> Vec { vec![Example { - description: "Get the groiup index of the group by operations.", + description: "Get the group index of the group by operations.", example: r#"[[group value]; [one 94] [one 95] [one 96] [two 97] [two 98] [two 99]] | polars into-df | polars group-by group diff --git a/crates/nu_plugin_polars/src/dataframe/command/aggregation/value_counts.rs b/crates/nu_plugin_polars/src/dataframe/command/aggregation/value_counts.rs index 064e308e80..57f7f3381a 100644 --- a/crates/nu_plugin_polars/src/dataframe/command/aggregation/value_counts.rs +++ b/crates/nu_plugin_polars/src/dataframe/command/aggregation/value_counts.rs @@ -28,7 +28,7 @@ impl PluginCommand for ValueCount { .named( "column", SyntaxShape::String, - "Provide a custom name for the coutn column", + "Provide a custom name for the count column", Some('c'), ) .switch("sort", "Whether or not values should be sorted", Some('s')) From e5cec8f4ebad5796a2ce9b8155e6a0bec53b45c9 Mon Sep 17 00:00:00 2001 From: Bahex Date: Mon, 18 Nov 2024 02:25:53 +0300 Subject: [PATCH 15/21] fix(group-by): re #14337 name collision prevention (#14360) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A more involved solution to the issue pointed out [here](https://github.com/nushell/nushell/pull/14337#issuecomment-2480392373) # Description With `--to-table` - cell-path groupers are used to create column names, similar to `select` - closure groupers result in columns named `closure_{i}` where `i` is the index of argument, with regards to other closures i.e. first closure grouper results in a column named `closure_0` Previously - `group-by foo {...} {...}` => `table` - `group-by {...} foo {...}` => `table` With this PR - `group-by foo {...} {...}` => `table` - `group-by {...} foo {...}` => `table` - no grouper argument results in a `table` as previously On naming conflicts caused by cell-path groupers named `items` or `closure_{i}`, an error is thrown, suggesting to use a closure in place of a cell-path. ```nushell ❯ ls | rename items | group-by items --to-table Error: × grouper arguments can't be named `items` ╭─[entry #3:1:29] 1 │ ls | rename items | group-by items --to-table · ────────┬──────── · ╰── contains `items` ╰──── help: instead of a cell-path, try using a closure ``` And following the suggestion: ```nushell ❯ ls | rename items | group-by { get items } --to-table ╭─#──┬──────closure_0──────┬───────────────────────────items────────────────────────────╮ │ 0 │ CITATION.cff │ ╭─#─┬────items─────┬─type─┬─size──┬───modified───╮ │ │ │ │ │ 0 │ CITATION.cff │ file │ 812 B │ 3 months ago │ │ │ │ │ ╰─#─┴────items─────┴─type─┴─size──┴───modified───╯ │ │ 1 │ CODE_OF_CONDUCT.md │ ╭─#─┬───────items────────┬─type─┬──size───┬───modified───╮ │ ... ``` --- crates/nu-command/src/filters/group_by.rs | 167 +++++++++++++++------- 1 file changed, 116 insertions(+), 51 deletions(-) diff --git a/crates/nu-command/src/filters/group_by.rs b/crates/nu-command/src/filters/group_by.rs index 9396de4781..0f56152834 100644 --- a/crates/nu-command/src/filters/group_by.rs +++ b/crates/nu-command/src/filters/group_by.rs @@ -1,6 +1,6 @@ use indexmap::IndexMap; use nu_engine::{command_prelude::*, ClosureEval}; -use nu_protocol::{engine::Closure, IntoValue}; +use nu_protocol::{engine::Closure, FromValue, IntoValue}; #[derive(Clone)] pub struct GroupBy; @@ -12,10 +12,6 @@ impl Command for GroupBy { fn signature(&self) -> Signature { Signature::build("group-by") - // TODO: It accepts Table also, but currently there is no Table - // example. Perhaps Table should be a subtype of List, in which case - // the current signature would suffice even when a Table example - // exists. .input_output_types(vec![(Type::List(Box::new(Type::Any)), Type::Any)]) .switch( "to-table", @@ -229,7 +225,7 @@ pub fn group_by( input: PipelineData, ) -> Result { let head = call.head; - let groupers: Vec = call.rest(engine_state, stack, 0)?; + let groupers: Vec> = call.rest(engine_state, stack, 0)?; let to_table = call.has_flag(engine_state, stack, "to-table")?; let config = engine_state.get_config(); @@ -238,20 +234,20 @@ pub fn group_by( return Ok(Value::record(Record::new(), head).into_pipeline_data()); } - let mut groupers = groupers.into_iter(); - - let grouped = if let Some(grouper) = groupers.next() { - let mut groups = Grouped::new(&grouper, values, config, engine_state, stack)?; - for grouper in groupers { - groups.subgroup(&grouper, config, engine_state, stack)?; + let grouped = match &groupers[..] { + [first, rest @ ..] => { + let mut grouped = Grouped::new(first.as_ref(), values, config, engine_state, stack)?; + for grouper in rest { + grouped.subgroup(grouper.as_ref(), config, engine_state, stack)?; + } + grouped } - groups - } else { - Grouped::empty(values, config) + [] => Grouped::empty(values, config), }; let value = if to_table { - grouped.into_table(head) + let column_names = groupers_to_column_names(&groupers)?; + grouped.into_table(&column_names, head) } else { grouped.into_record(head) }; @@ -259,8 +255,67 @@ pub fn group_by( Ok(value.into_pipeline_data()) } +fn groupers_to_column_names(groupers: &[Spanned]) -> Result, ShellError> { + if groupers.is_empty() { + return Ok(vec!["group".into(), "items".into()]); + } + + let mut closure_idx: usize = 0; + let grouper_names = groupers.iter().map(|grouper| { + grouper.as_ref().map(|item| match item { + Grouper::CellPath { val } => val.to_column_name(), + Grouper::Closure { .. } => { + closure_idx += 1; + format!("closure_{}", closure_idx - 1) + } + }) + }); + + let mut name_set: Vec> = Vec::with_capacity(grouper_names.len()); + + for name in grouper_names { + if name.item == "items" { + return Err(ShellError::GenericError { + error: "grouper arguments can't be named `items`".into(), + msg: "here".into(), + span: Some(name.span), + help: Some("instead of a cell-path, try using a closure: { get items }".into()), + inner: vec![], + }); + } + + if let Some(conflicting_name) = name_set + .iter() + .find(|elem| elem.as_ref().item == name.item.as_str()) + { + return Err(ShellError::GenericError { + error: "grouper arguments result in colliding column names".into(), + msg: "duplicate column names".into(), + span: Some(conflicting_name.span.append(name.span)), + help: Some( + "instead of a cell-path, try using a closure or renaming columns".into(), + ), + inner: vec![ShellError::ColumnDefinedTwice { + col_name: conflicting_name.item.clone(), + first_use: conflicting_name.span, + second_use: name.span, + }], + }); + } + + name_set.push(name); + } + + let column_names: Vec = name_set + .into_iter() + .map(|elem| elem.item) + .chain(["items".into()]) + .collect(); + Ok(column_names) +} + fn group_cell_path( - column_name: CellPath, + column_name: &CellPath, values: Vec, config: &nu_protocol::Config, ) -> Result>, ShellError> { @@ -305,8 +360,25 @@ fn group_closure( Ok(groups) } +enum Grouper { + CellPath { val: CellPath }, + Closure { val: Box }, +} + +impl FromValue for Grouper { + fn from_value(v: Value) -> Result { + match v { + Value::CellPath { val, .. } => Ok(Grouper::CellPath { val }), + Value::Closure { val, .. } => Ok(Grouper::Closure { val }), + _ => Err(ShellError::TypeMismatch { + err_message: "unsupported grouper type".to_string(), + span: v.span(), + }), + } + } +} + struct Grouped { - grouper: Option, groups: Tree, } @@ -325,41 +397,35 @@ impl Grouped { } Self { - grouper: Some("group".into()), groups: Tree::Leaf(groups), } } fn new( - grouper: &Value, + grouper: Spanned<&Grouper>, values: Vec, config: &nu_protocol::Config, engine_state: &EngineState, stack: &mut Stack, ) -> Result { - let span = grouper.span(); - let groups = match grouper { - Value::CellPath { val, .. } => group_cell_path(val.clone(), values, config)?, - Value::Closure { val, .. } => { - group_closure(values, span, Closure::clone(val), engine_state, stack)? - } - _ => { - return Err(ShellError::TypeMismatch { - err_message: "unsupported grouper type".to_string(), - span, - }) - } + let groups = match grouper.item { + Grouper::CellPath { val } => group_cell_path(val, values, config)?, + Grouper::Closure { val } => group_closure( + values, + grouper.span, + Closure::clone(val), + engine_state, + stack, + )?, }; - let grouper = grouper.as_cell_path().ok().map(CellPath::to_column_name); Ok(Self { - grouper, groups: Tree::Leaf(groups), }) } fn subgroup( &mut self, - grouper: &Value, + grouper: Spanned<&Grouper>, config: &nu_protocol::Config, engine_state: &EngineState, stack: &mut Stack, @@ -384,34 +450,33 @@ impl Grouped { Ok(()) } - fn into_table(self, head: Span) -> Value { - self._into_table(head, 0) + fn into_table(self, column_names: &[String], head: Span) -> Value { + self._into_table(head) .into_iter() - .map(|row| row.into_iter().rev().collect::().into_value(head)) + .map(|row| { + row.into_iter() + .rev() + .zip(column_names) + .map(|(val, key)| (key.clone(), val)) + .collect::() + .into_value(head) + }) .collect::>() .into_value(head) } - fn _into_table(self, head: Span, index: usize) -> Vec { - let grouper = self.grouper.unwrap_or_else(|| format!("group{index}")); + fn _into_table(self, head: Span) -> Vec> { match self.groups { Tree::Leaf(leaf) => leaf .into_iter() - .map(|(group, values)| { - [ - ("items".to_string(), values.into_value(head)), - (grouper.clone(), group.into_value(head)), - ] - .into_iter() - .collect() - }) - .collect::>(), + .map(|(group, values)| vec![(values.into_value(head)), (group.into_value(head))]) + .collect::>>(), Tree::Branch(branch) => branch .into_iter() .flat_map(|(group, items)| { - let mut inner = items._into_table(head, index + 1); + let mut inner = items._into_table(head); for row in &mut inner { - row.insert(grouper.clone(), group.clone().into_value(head)); + row.push(group.clone().into_value(head)); } inner }) From 6e1118681deb507670994508b648117dc435867b Mon Sep 17 00:00:00 2001 From: Solomon Date: Sun, 17 Nov 2024 17:01:52 -0700 Subject: [PATCH 16/21] make command signature parsing more strict (#14309) # User-Facing Changes The parser now errors on more invalid command signatures: ```nushell # expected parameter or flag def foo [ bar: int: ] {} # expected type def foo [ bar: = ] {} def foo [ bar: ] {} # expected default value def foo [ bar = ] {} ``` --- crates/nu-command/tests/commands/def.rs | 29 ++++++++++++++++++++++++- crates/nu-parser/src/parser.rs | 25 ++++++++++++++++----- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index c66869b479..0705c0085f 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -2,6 +2,13 @@ use nu_test_support::nu; use nu_test_support::playground::Playground; use std::fs; +#[test] +fn def_with_trailing_comma() { + let actual = nu!("def test-command [ foo: int, ] { $foo }; test-command 1"); + + assert!(actual.out == "1"); +} + #[test] fn def_with_comment() { Playground::setup("def_with_comment", |dirs, _| { @@ -72,6 +79,13 @@ fn def_errors_with_comma_before_equals() { assert!(actual.err.contains("expected parameter")); } +#[test] +fn def_errors_with_colon_before_equals() { + let actual = nu!("def test-command [ foo: = 1 ] {}"); + + assert!(actual.err.contains("expected type")); +} + #[test] fn def_errors_with_comma_before_colon() { let actual = nu!("def test-command [ foo, : int ] {}"); @@ -85,7 +99,6 @@ fn def_errors_with_multiple_colons() { assert!(actual.err.contains("expected type")); } -#[ignore = "This error condition is not implemented yet"] #[test] fn def_errors_with_multiple_types() { let actual = nu!("def test-command [ foo:int:string ] {}"); @@ -93,6 +106,20 @@ fn def_errors_with_multiple_types() { assert!(actual.err.contains("expected parameter")); } +#[test] +fn def_errors_with_trailing_colon() { + let actual = nu!("def test-command [ foo: int: ] {}"); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_trailing_default_value() { + let actual = nu!("def test-command [ foo: int = ] {}"); + + assert!(actual.err.contains("expected default value")); +} + #[test] fn def_errors_with_multiple_commas() { let actual = nu!("def test-command [ foo,,bar ] {}"); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index e4d3765e17..7db2e112bf 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3392,6 +3392,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> Arg, AfterCommaArg, Type, + AfterType, DefaultValue, } @@ -3425,7 +3426,9 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> let mut args: Vec = vec![]; let mut parse_mode = ParseMode::Arg; - for token in &output { + for (index, token) in output.iter().enumerate() { + let last_token = index == output.len() - 1; + match token { Token { contents: crate::TokenContents::Item | crate::TokenContents::AssignmentOperator, @@ -3437,10 +3440,12 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The : symbol separates types if contents == b":" { match parse_mode { + ParseMode::Arg if last_token => working_set + .error(ParseError::Expected("type", Span::new(span.end, span.end))), ParseMode::Arg => { parse_mode = ParseMode::Type; } - ParseMode::AfterCommaArg => { + ParseMode::AfterCommaArg | ParseMode::AfterType => { working_set.error(ParseError::Expected("parameter or flag", span)); } ParseMode::Type | ParseMode::DefaultValue => { @@ -3452,9 +3457,15 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The = symbol separates a variable from its default value else if contents == b"=" { match parse_mode { - ParseMode::Type | ParseMode::Arg => { + ParseMode::Arg | ParseMode::AfterType if last_token => working_set.error( + ParseError::Expected("default value", Span::new(span.end, span.end)), + ), + ParseMode::Arg | ParseMode::AfterType => { parse_mode = ParseMode::DefaultValue; } + ParseMode::Type => { + working_set.error(ParseError::Expected("type", span)); + } ParseMode::AfterCommaArg => { working_set.error(ParseError::Expected("parameter or flag", span)); } @@ -3467,7 +3478,9 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The , symbol separates params only else if contents == b"," { match parse_mode { - ParseMode::Arg => parse_mode = ParseMode::AfterCommaArg, + ParseMode::Arg | ParseMode::AfterType => { + parse_mode = ParseMode::AfterCommaArg + } ParseMode::AfterCommaArg => { working_set.error(ParseError::Expected("parameter or flag", span)); } @@ -3480,7 +3493,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> } } else { match parse_mode { - ParseMode::Arg | ParseMode::AfterCommaArg => { + ParseMode::Arg | ParseMode::AfterCommaArg | ParseMode::AfterType => { // Long flag with optional short form following with no whitespace, e.g. --output, --age(-a) if contents.starts_with(b"--") && contents.len() > 2 { // Split the long flag from the short flag with the ( character as delimiter. @@ -3790,7 +3803,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> } } } - parse_mode = ParseMode::Arg; + parse_mode = ParseMode::AfterType; } ParseMode::DefaultValue => { if let Some(last) = args.last_mut() { From f63f8cb15406c9f3f94e271f71767444763aa9a8 Mon Sep 17 00:00:00 2001 From: Yash Thakur <45539777+ysthakur@users.noreply.github.com> Date: Sun, 17 Nov 2024 19:03:21 -0500 Subject: [PATCH 17/21] Add utouch command from uutils/coreutils (#11817) Part of https://github.com/nushell/nushell/issues/11549 # Description This PR adds a `utouch` command that uses the `touch` command from https://github.com/uutils/coreutils. Eventually, `utouch` may be able to replace `touch`. The conflicts in Cargo.lock and Cargo.toml are because I'm using the uutils/coreutils main rather than the latest release, since the changes that expose `uu_touch`'s internal functionality aren't available in the latest release. # User-Facing Changes Users will have access to a new `utouch` command with the following flags: todo # Tests + Formatting # After Submitting --- Cargo.lock | 26 + Cargo.toml | 1 + crates/nu-command/Cargo.toml | 1 + crates/nu-command/src/default_context.rs | 1 + crates/nu-command/src/filesystem/mod.rs | 2 + crates/nu-command/src/filesystem/utouch.rs | 268 ++++++++ crates/nu-command/tests/commands/mod.rs | 1 + crates/nu-command/tests/commands/utouch.rs | 740 +++++++++++++++++++++ 8 files changed, 1040 insertions(+) create mode 100644 crates/nu-command/src/filesystem/utouch.rs create mode 100644 crates/nu-command/tests/commands/utouch.rs diff --git a/Cargo.lock b/Cargo.lock index 2754f5bd23..884f1993d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3229,6 +3229,7 @@ dependencies = [ "uu_mkdir", "uu_mktemp", "uu_mv", + "uu_touch", "uu_uname", "uu_whoami", "uucore", @@ -4081,6 +4082,17 @@ dependencies = [ "regex", ] +[[package]] +name = "parse_datetime" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a8720474e3dd4af20cea8716703498b9f3b690f318fa9d9d9e2e38eaf44b96d0" +dependencies = [ + "chrono", + "nom", + "regex", +] + [[package]] name = "paste" version = "1.0.15" @@ -6745,6 +6757,20 @@ dependencies = [ "uucore", ] +[[package]] +name = "uu_touch" +version = "0.0.28" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55476bec11d5b70c578233a2e94f685058e0d65fc5d66c7ed465877c15124c7c" +dependencies = [ + "chrono", + "clap", + "filetime", + "parse_datetime", + "uucore", + "windows-sys 0.59.0", +] + [[package]] name = "uu_uname" version = "0.0.27" diff --git a/Cargo.toml b/Cargo.toml index a976aeec17..0297f5fdb5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -169,6 +169,7 @@ uu_cp = "0.0.27" uu_mkdir = "0.0.27" uu_mktemp = "0.0.27" uu_mv = "0.0.27" +uu_touch = "0.0.28" uu_whoami = "0.0.27" uu_uname = "0.0.27" uucore = "0.0.27" diff --git a/crates/nu-command/Cargo.toml b/crates/nu-command/Cargo.toml index e9c3b9a784..c37954e9c1 100644 --- a/crates/nu-command/Cargo.toml +++ b/crates/nu-command/Cargo.toml @@ -96,6 +96,7 @@ uu_cp = { workspace = true } uu_mkdir = { workspace = true } uu_mktemp = { workspace = true } uu_mv = { workspace = true } +uu_touch = { workspace = true } uu_uname = { workspace = true } uu_whoami = { workspace = true } uuid = { workspace = true, features = ["v4"] } diff --git a/crates/nu-command/src/default_context.rs b/crates/nu-command/src/default_context.rs index ac6fb46631..cbbee717d4 100644 --- a/crates/nu-command/src/default_context.rs +++ b/crates/nu-command/src/default_context.rs @@ -230,6 +230,7 @@ pub fn add_shell_command_context(mut engine_state: EngineState) -> EngineState { Rm, Save, Touch, + UTouch, Glob, Watch, }; diff --git a/crates/nu-command/src/filesystem/mod.rs b/crates/nu-command/src/filesystem/mod.rs index acfa54fee3..089e899dda 100644 --- a/crates/nu-command/src/filesystem/mod.rs +++ b/crates/nu-command/src/filesystem/mod.rs @@ -12,6 +12,7 @@ mod ucp; mod umkdir; mod umv; mod util; +mod utouch; mod watch; pub use self::open::Open; @@ -27,4 +28,5 @@ pub use touch::Touch; pub use ucp::UCp; pub use umkdir::UMkdir; pub use umv::UMv; +pub use utouch::UTouch; pub use watch::Watch; diff --git a/crates/nu-command/src/filesystem/utouch.rs b/crates/nu-command/src/filesystem/utouch.rs new file mode 100644 index 0000000000..f32364b28a --- /dev/null +++ b/crates/nu-command/src/filesystem/utouch.rs @@ -0,0 +1,268 @@ +use std::io::ErrorKind; +use std::path::PathBuf; + +use chrono::{DateTime, FixedOffset}; +use filetime::FileTime; + +use nu_engine::CallExt; +use nu_path::expand_path_with; +use nu_protocol::engine::{Call, Command, EngineState, Stack}; +use nu_protocol::{ + Category, Example, NuGlob, PipelineData, ShellError, Signature, Spanned, SyntaxShape, Type, +}; +use uu_touch::error::TouchError; +use uu_touch::{ChangeTimes, InputFile, Options, Source}; + +use super::util::get_rest_for_glob_pattern; + +#[derive(Clone)] +pub struct UTouch; + +impl Command for UTouch { + fn name(&self) -> &str { + "utouch" + } + + fn search_terms(&self) -> Vec<&str> { + vec!["create", "file"] + } + + fn signature(&self) -> Signature { + Signature::build("utouch") + .input_output_types(vec![ (Type::Nothing, Type::Nothing) ]) + .rest( + "files", + SyntaxShape::OneOf(vec![SyntaxShape::GlobPattern, SyntaxShape::Filepath]), + "The file(s) to create. '-' is used to represent stdout." + ) + .named( + "reference", + SyntaxShape::Filepath, + "Use the access and modification times of the reference file/directory instead of the current time", + Some('r'), + ) + .named( + "timestamp", + SyntaxShape::DateTime, + "Use the given timestamp instead of the current time", + Some('t') + ) + .named( + "date", + SyntaxShape::String, + "Use the given time instead of the current time. This can be a full timestamp or it can be relative to either the current time or reference file time (if given). For more information, see https://www.gnu.org/software/coreutils/manual/html_node/touch-invocation.html", + Some('d') + ) + .switch( + "modified", + "Change only the modification time (if used with -a, access time is changed too)", + Some('m'), + ) + .switch( + "access", + "Change only the access time (if used with -m, modification time is changed too)", + Some('a'), + ) + .switch( + "no-create", + "Don't create the file if it doesn't exist", + Some('c'), + ) + .switch( + "no-deref", + "Affect each symbolic link instead of any referenced file (only for systems that can change the timestamps of a symlink). Ignored if touching stdout", + Some('s'), + ) + .category(Category::FileSystem) + } + + fn description(&self) -> &str { + "Creates one or more files." + } + + fn run( + &self, + engine_state: &EngineState, + stack: &mut Stack, + call: &Call, + _input: PipelineData, + ) -> Result { + let change_mtime: bool = call.has_flag(engine_state, stack, "modified")?; + let change_atime: bool = call.has_flag(engine_state, stack, "access")?; + let no_create: bool = call.has_flag(engine_state, stack, "no-create")?; + let no_deref: bool = call.has_flag(engine_state, stack, "no-dereference")?; + let file_globs: Vec> = + get_rest_for_glob_pattern(engine_state, stack, call, 0)?; + let cwd = engine_state.cwd(Some(stack))?; + + if file_globs.is_empty() { + return Err(ShellError::MissingParameter { + param_name: "requires file paths".to_string(), + span: call.head, + }); + } + + let (reference_file, reference_span) = if let Some(reference) = + call.get_flag::>(engine_state, stack, "reference")? + { + (Some(reference.item), Some(reference.span)) + } else { + (None, None) + }; + let (date_str, date_span) = + if let Some(date) = call.get_flag::>(engine_state, stack, "date")? { + (Some(date.item), Some(date.span)) + } else { + (None, None) + }; + let timestamp: Option>> = + call.get_flag(engine_state, stack, "timestamp")?; + + let source = if let Some(timestamp) = timestamp { + if let Some(reference_span) = reference_span { + return Err(ShellError::IncompatibleParameters { + left_message: "timestamp given".to_string(), + left_span: timestamp.span, + right_message: "reference given".to_string(), + right_span: reference_span, + }); + } + if let Some(date_span) = date_span { + return Err(ShellError::IncompatibleParameters { + left_message: "timestamp given".to_string(), + left_span: timestamp.span, + right_message: "date given".to_string(), + right_span: date_span, + }); + } + Source::Timestamp(FileTime::from_unix_time( + timestamp.item.timestamp(), + timestamp.item.timestamp_subsec_nanos(), + )) + } else if let Some(reference_file) = reference_file { + let reference_file = expand_path_with(reference_file, &cwd, true); + Source::Reference(reference_file) + } else { + Source::Now + }; + + let change_times = if change_atime && !change_mtime { + ChangeTimes::AtimeOnly + } else if change_mtime && !change_atime { + ChangeTimes::MtimeOnly + } else { + ChangeTimes::Both + }; + + let mut input_files = Vec::new(); + for file_glob in &file_globs { + if file_glob.item.as_ref() == "-" { + input_files.push(InputFile::Stdout); + } else { + let path = + expand_path_with(file_glob.item.as_ref(), &cwd, file_glob.item.is_expand()); + input_files.push(InputFile::Path(path)); + } + } + + if let Err(err) = uu_touch::touch( + &input_files, + &Options { + no_create, + no_deref, + source, + date: date_str, + change_times, + strict: true, + }, + ) { + let nu_err = match err { + TouchError::TouchFileError { path, index, error } => ShellError::GenericError { + error: format!("Could not touch {}", path.display()), + msg: error.to_string(), + span: Some(file_globs[index].span), + help: None, + inner: Vec::new(), + }, + TouchError::InvalidDateFormat(date) => ShellError::IncorrectValue { + msg: format!("Invalid date: {}", date), + val_span: date_span.expect("utouch should've been given a date"), + call_span: call.head, + }, + TouchError::ReferenceFileInaccessible(reference_path, io_err) => { + let span = + reference_span.expect("utouch should've been given a reference file"); + if io_err.kind() == ErrorKind::NotFound { + ShellError::FileNotFound { + span, + file: reference_path.display().to_string(), + } + } else { + ShellError::GenericError { + error: io_err.to_string(), + msg: format!("Failed to read metadata of {}", reference_path.display()), + span: Some(span), + help: None, + inner: Vec::new(), + } + } + } + _ => ShellError::GenericError { + error: err.to_string(), + msg: err.to_string(), + span: Some(call.head), + help: None, + inner: Vec::new(), + }, + }; + return Err(nu_err); + } + + Ok(PipelineData::empty()) + } + + fn examples(&self) -> Vec { + vec![ + Example { + description: "Creates \"fixture.json\"", + example: "utouch fixture.json", + result: None, + }, + Example { + description: "Creates files a, b and c", + example: "utouch a b c", + result: None, + }, + Example { + description: r#"Changes the last modified time of "fixture.json" to today's date"#, + example: "utouch -m fixture.json", + result: None, + }, + Example { + description: "Changes the last accessed and modified times of files a, b and c to the current time but yesterday", + example: r#"utouch -d "yesterday" a b c"#, + result: None, + }, + Example { + description: r#"Changes the last modified time of files d and e to "fixture.json"'s last modified time"#, + example: r#"utouch -m -r fixture.json d e"#, + result: None, + }, + Example { + description: r#"Changes the last accessed time of "fixture.json" to a datetime"#, + example: r#"utouch -a -t 2019-08-24T12:30:30 fixture.json"#, + result: None, + }, + Example { + description: r#"Change the last accessed and modified times of stdout"#, + example: r#"utouch -"#, + result: None, + }, + Example { + description: r#"Changes the last accessed and modified times of file a to 1 month before "fixture.json"'s last modified time"#, + example: r#"utouch -r fixture.json -d "-1 month" a"#, + result: None, + }, + ] + } +} diff --git a/crates/nu-command/tests/commands/mod.rs b/crates/nu-command/tests/commands/mod.rs index 63911ebfbc..678b8d8896 100644 --- a/crates/nu-command/tests/commands/mod.rs +++ b/crates/nu-command/tests/commands/mod.rs @@ -127,6 +127,7 @@ mod update; mod upsert; mod url; mod use_; +mod utouch; mod where_; mod which; mod while_; diff --git a/crates/nu-command/tests/commands/utouch.rs b/crates/nu-command/tests/commands/utouch.rs new file mode 100644 index 0000000000..062ec7ddfc --- /dev/null +++ b/crates/nu-command/tests/commands/utouch.rs @@ -0,0 +1,740 @@ +use chrono::{DateTime, Days, Local, TimeDelta, Utc}; +use filetime::FileTime; +use nu_test_support::fs::{files_exist_at, Stub}; +use nu_test_support::nu; +use nu_test_support::playground::{Dirs, Playground}; +use std::path::Path; + +// Use 1 instead of 0 because 0 has a special meaning in Windows +const TIME_ONE: FileTime = FileTime::from_unix_time(1, 0); + +fn file_times(file: impl AsRef) -> (FileTime, FileTime) { + ( + file.as_ref().metadata().unwrap().accessed().unwrap().into(), + file.as_ref().metadata().unwrap().modified().unwrap().into(), + ) +} + +fn symlink_times(path: &nu_path::AbsolutePath) -> (filetime::FileTime, filetime::FileTime) { + let metadata = path.symlink_metadata().unwrap(); + + ( + filetime::FileTime::from_system_time(metadata.accessed().unwrap()), + filetime::FileTime::from_system_time(metadata.modified().unwrap()), + ) +} + +// From https://github.com/nushell/nushell/pull/14214 +fn setup_symlink_fs(dirs: &Dirs, sandbox: &mut Playground<'_>) { + sandbox.mkdir("d"); + sandbox.with_files(&[Stub::EmptyFile("f"), Stub::EmptyFile("d/f")]); + sandbox.symlink("f", "fs"); + sandbox.symlink("d", "ds"); + sandbox.symlink("d/f", "fds"); + + // sandbox.symlink does not handle symlinks to missing files well. It panics + // But they are useful, and they should be tested. + #[cfg(unix)] + { + std::os::unix::fs::symlink(dirs.test().join("m"), dirs.test().join("fms")).unwrap(); + } + + #[cfg(windows)] + { + std::os::windows::fs::symlink_file(dirs.test().join("m"), dirs.test().join("fms")).unwrap(); + } + + // Change the file times to a known "old" value for comparison + filetime::set_symlink_file_times(dirs.test().join("f"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("d"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("d/f"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("ds"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fs"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fds"), TIME_ONE, TIME_ONE).unwrap(); + filetime::set_symlink_file_times(dirs.test().join("fms"), TIME_ONE, TIME_ONE).unwrap(); +} + +#[test] +fn creates_a_file_when_it_doesnt_exist() { + Playground::setup("create_test_1", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "utouch i_will_be_created.txt" + ); + + let path = dirs.test().join("i_will_be_created.txt"); + assert!(path.exists()); + }) +} + +#[test] +fn creates_two_files() { + Playground::setup("create_test_2", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "utouch a b" + ); + + let path = dirs.test().join("a"); + assert!(path.exists()); + + let path2 = dirs.test().join("b"); + assert!(path2.exists()); + }) +} + +#[test] +fn change_modified_time_of_file_to_today() { + Playground::setup("change_time_test_9", |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + // Set file.txt's times to the past before the test to make sure `utouch` actually changes the mtime to today + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -m file.txt" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + + // Check that atime remains unchanged + assert_eq!( + TIME_ONE, + FileTime::from_system_time(metadata.accessed().unwrap()) + ); + }) +} + +#[test] +fn change_access_time_of_file_to_today() { + Playground::setup("change_time_test_18", |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + // Set file.txt's times to the past before the test to make sure `utouch` actually changes the atime to today + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -a file.txt" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, atime_day); + + // Check that mtime remains unchanged + assert_eq!( + TIME_ONE, + FileTime::from_system_time(metadata.modified().unwrap()) + ); + }) +} + +#[test] +fn change_modified_and_access_time_of_file_to_today() { + Playground::setup("change_time_test_27", |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -a -m file.txt" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }) +} + +#[test] +fn not_create_file_if_it_not_exists() { + Playground::setup("change_time_test_28", |dirs, _sandbox| { + let outcome = nu!( + cwd: dirs.test(), + "utouch -c file.txt" + ); + + let path = dirs.test().join("file.txt"); + + assert!(!path.exists()); + + // If --no-create is improperly handled `utouch` may error when trying to change the times of a nonexistent file + assert!(outcome.status.success()) + }) +} + +#[test] +fn change_file_times_if_exists_with_no_create() { + Playground::setup( + "change_file_times_if_exists_with_no_create", + |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("file.txt")]); + let path = dirs.test().join("file.txt"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -c file.txt" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }, + ) +} + +#[test] +fn creates_file_three_dots() { + Playground::setup("create_test_1", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "utouch file..." + ); + + let path = dirs.test().join("file..."); + assert!(path.exists()); + }) +} + +#[test] +fn creates_file_four_dots() { + Playground::setup("create_test_1", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "utouch file...." + ); + + let path = dirs.test().join("file...."); + assert!(path.exists()); + }) +} + +#[test] +fn creates_file_four_dots_quotation_marks() { + Playground::setup("create_test_1", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "utouch 'file....'" + ); + + let path = dirs.test().join("file...."); + assert!(path.exists()); + }) +} + +#[test] +fn change_file_times_to_reference_file() { + Playground::setup("change_dir_times_to_reference_dir", |dirs, sandbox| { + sandbox.with_files(&[ + Stub::EmptyFile("reference_file"), + Stub::EmptyFile("target_file"), + ]); + + let reference = dirs.test().join("reference_file"); + let target = dirs.test().join("target_file"); + + // Change the times for reference + filetime::set_file_times(&reference, FileTime::from_unix_time(1337, 0), TIME_ONE).unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + nu!( + cwd: dirs.test(), + "utouch -r reference_file target_file" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + }) +} + +#[test] +fn change_file_mtime_to_reference() { + Playground::setup("change_file_mtime_to_reference", |dirs, sandbox| { + sandbox.with_files(&[ + Stub::EmptyFile("reference_file"), + Stub::EmptyFile("target_file"), + ]); + + let reference = dirs.test().join("reference_file"); + let target = dirs.test().join("target_file"); + + // Change the times for reference + filetime::set_file_times(&reference, TIME_ONE, FileTime::from_unix_time(1337, 0)).unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!(file_times(&reference), file_times(&target)); + + // Save target's current atime to make sure it is preserved + let target_original_atime = target.metadata().unwrap().accessed().unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -mr reference_file target_file" + ); + + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + target_original_atime, + target.metadata().unwrap().accessed().unwrap() + ); + }) +} + +// TODO when https://github.com/uutils/coreutils/issues/6629 is fixed, +// unignore this test +#[test] +#[ignore] +fn change_file_times_to_reference_file_with_date() { + Playground::setup( + "change_file_times_to_reference_file_with_date", + |dirs, sandbox| { + sandbox.with_files(&[ + Stub::EmptyFile("reference_file"), + Stub::EmptyFile("target_file"), + ]); + + let reference = dirs.test().join("reference_file"); + let target = dirs.test().join("target_file"); + + let now = Utc::now(); + + let ref_atime = now; + let ref_mtime = now.checked_sub_days(Days::new(5)).unwrap(); + + // Change the times for reference + filetime::set_file_times( + reference, + FileTime::from_unix_time(ref_atime.timestamp(), ref_atime.timestamp_subsec_nanos()), + FileTime::from_unix_time(ref_mtime.timestamp(), ref_mtime.timestamp_subsec_nanos()), + ) + .unwrap(); + + nu!( + cwd: dirs.test(), + r#"utouch -r reference_file -d "yesterday" target_file"# + ); + + let (got_atime, got_mtime) = file_times(target); + let got = ( + DateTime::from_timestamp(got_atime.seconds(), got_atime.nanoseconds()).unwrap(), + DateTime::from_timestamp(got_mtime.seconds(), got_mtime.nanoseconds()).unwrap(), + ); + assert_eq!( + ( + now.checked_sub_days(Days::new(1)).unwrap(), + now.checked_sub_days(Days::new(6)).unwrap() + ), + got + ); + }, + ) +} + +#[test] +fn change_file_times_to_timestamp() { + Playground::setup("change_file_times_to_timestamp", |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("target_file")]); + + let target = dirs.test().join("target_file"); + let timestamp = DateTime::from_timestamp(TIME_ONE.unix_seconds(), TIME_ONE.nanoseconds()) + .unwrap() + .to_rfc3339(); + + nu!(cwd: dirs.test(), format!("utouch --timestamp {} target_file", timestamp)); + + assert_eq!((TIME_ONE, TIME_ONE), file_times(target)); + }) +} + +#[test] +fn change_modified_time_of_dir_to_today() { + Playground::setup("change_dir_mtime", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_mtime(&path, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -m test_dir" + ); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = + DateTime::::from(path.metadata().unwrap().modified().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + }) +} + +#[test] +fn change_access_time_of_dir_to_today() { + Playground::setup("change_dir_atime", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_atime(&path, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -a test_dir" + ); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let atime_day = + DateTime::::from(path.metadata().unwrap().accessed().unwrap()).date_naive(); + + assert_eq!(today, atime_day); + }) +} + +#[test] +fn change_modified_and_access_time_of_dir_to_today() { + Playground::setup("change_dir_times", |dirs, sandbox| { + sandbox.mkdir("test_dir"); + let path = dirs.test().join("test_dir"); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -a -m test_dir" + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }) +} + +// TODO when https://github.com/uutils/coreutils/issues/6629 is fixed, +// unignore this test +#[test] +#[ignore] +fn change_file_times_to_date() { + Playground::setup("change_file_times_to_date", |dirs, sandbox| { + sandbox.with_files(&[Stub::EmptyFile("target_file")]); + + let expected = Utc::now().checked_sub_signed(TimeDelta::hours(2)).unwrap(); + nu!(cwd: dirs.test(), "utouch -d '-2 hours' target_file"); + + let (got_atime, got_mtime) = file_times(dirs.test().join("target_file")); + let got_atime = + DateTime::from_timestamp(got_atime.seconds(), got_atime.nanoseconds()).unwrap(); + let got_mtime = + DateTime::from_timestamp(got_mtime.seconds(), got_mtime.nanoseconds()).unwrap(); + let threshold = TimeDelta::minutes(1); + assert!( + got_atime.signed_duration_since(expected).lt(&threshold) + && got_mtime.signed_duration_since(expected).lt(&threshold), + "Expected: {}. Got: atime={}, mtime={}", + expected, + got_atime, + got_mtime + ); + assert!(got_mtime.signed_duration_since(expected).lt(&threshold)); + }) +} + +#[test] +fn change_dir_three_dots_times() { + Playground::setup("change_dir_three_dots_times", |dirs, sandbox| { + sandbox.mkdir("test_dir..."); + let path = dirs.test().join("test_dir..."); + + filetime::set_file_times(&path, TIME_ONE, TIME_ONE).unwrap(); + + nu!( + cwd: dirs.test(), + "utouch test_dir..." + ); + + let metadata = path.metadata().unwrap(); + + // Check only the date since the time may not match exactly + let today = Local::now().date_naive(); + let mtime_day = DateTime::::from(metadata.modified().unwrap()).date_naive(); + let atime_day = DateTime::::from(metadata.accessed().unwrap()).date_naive(); + + assert_eq!(today, mtime_day); + assert_eq!(today, atime_day); + }) +} + +#[test] +fn change_dir_times_to_reference_dir() { + Playground::setup("change_dir_times_to_reference_dir", |dirs, sandbox| { + sandbox.mkdir("reference_dir"); + sandbox.mkdir("target_dir"); + + let reference = dirs.test().join("reference_dir"); + let target = dirs.test().join("target_dir"); + + // Change the times for reference + filetime::set_file_times(&reference, FileTime::from_unix_time(1337, 0), TIME_ONE).unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + nu!( + cwd: dirs.test(), + "utouch -r reference_dir target_dir" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_eq!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + }) +} + +#[test] +fn change_dir_atime_to_reference() { + Playground::setup("change_dir_atime_to_reference", |dirs, sandbox| { + sandbox.mkdir("reference_dir"); + sandbox.mkdir("target_dir"); + + let reference = dirs.test().join("reference_dir"); + let target = dirs.test().join("target_dir"); + + // Change the times for reference + filetime::set_file_times(&reference, FileTime::from_unix_time(1337, 0), TIME_ONE).unwrap(); + + // target should have today's date since it was just created, but reference should be different + assert_ne!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + + // Save target's current mtime to make sure it is preserved + let target_original_mtime = target.metadata().unwrap().modified().unwrap(); + + nu!( + cwd: dirs.test(), + "utouch -ar reference_dir target_dir" + ); + + assert_eq!( + reference.metadata().unwrap().accessed().unwrap(), + target.metadata().unwrap().accessed().unwrap() + ); + assert_ne!( + reference.metadata().unwrap().modified().unwrap(), + target.metadata().unwrap().modified().unwrap() + ); + assert_eq!( + target_original_mtime, + target.metadata().unwrap().modified().unwrap() + ); + }) +} + +#[test] +fn create_a_file_with_tilde() { + Playground::setup("utouch with tilde", |dirs, _| { + let actual = nu!(cwd: dirs.test(), "utouch '~tilde'"); + assert!(actual.err.is_empty()); + assert!(files_exist_at(&[Path::new("~tilde")], dirs.test())); + + // pass variable + let actual = nu!(cwd: dirs.test(), "let f = '~tilde2'; utouch $f"); + assert!(actual.err.is_empty()); + assert!(files_exist_at(&[Path::new("~tilde2")], dirs.test())); + }) +} + +#[test] +fn respects_cwd() { + Playground::setup("utouch_respects_cwd", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "mkdir 'dir'; cd 'dir'; utouch 'i_will_be_created.txt'" + ); + + let path = dirs.test().join("dir/i_will_be_created.txt"); + assert!(path.exists()); + }) +} + +#[test] +fn reference_respects_cwd() { + Playground::setup("utouch_reference_respects_cwd", |dirs, _sandbox| { + nu!( + cwd: dirs.test(), + "mkdir 'dir'; cd 'dir'; utouch 'ref.txt'; utouch --reference 'ref.txt' 'foo.txt'" + ); + + let path = dirs.test().join("dir/foo.txt"); + assert!(path.exists()); + }) +} + +#[test] +fn recognizes_stdout() { + Playground::setup("utouch_recognizes_stdout", |dirs, _sandbox| { + nu!(cwd: dirs.test(), "utouch -"); + assert!(!dirs.test().join("-").exists()); + }) +} + +#[test] +fn follow_symlinks() { + Playground::setup("touch_follows_symlinks", |dirs, sandbox| { + setup_symlink_fs(&dirs, sandbox); + + let missing = dirs.test().join("m"); + assert!(!missing.exists()); + + nu!( + cwd: dirs.test(), + " + touch fds + touch ds + touch fs + touch fms + " + ); + + // We created the missing symlink target + assert!(missing.exists()); + + // The timestamps for files and directories were changed from TIME_ONE + let file_times = symlink_times(&dirs.test().join("f")); + let dir_times = symlink_times(&dirs.test().join("d")); + let dir_file_times = symlink_times(&dirs.test().join("d/f")); + + assert_ne!(file_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_file_times, (TIME_ONE, TIME_ONE)); + + // For symlinks, they remain (mostly) the same + // We can't test accessed times, since to reach the target file, the symlink must be accessed! + let file_symlink_times = symlink_times(&dirs.test().join("fs")); + let dir_symlink_times = symlink_times(&dirs.test().join("ds")); + let dir_file_symlink_times = symlink_times(&dirs.test().join("fds")); + let file_missing_symlink_times = symlink_times(&dirs.test().join("fms")); + + assert_eq!(file_symlink_times.1, TIME_ONE); + assert_eq!(dir_symlink_times.1, TIME_ONE); + assert_eq!(dir_file_symlink_times.1, TIME_ONE); + assert_eq!(file_missing_symlink_times.1, TIME_ONE); + }) +} + +#[test] +fn no_follow_symlinks() { + Playground::setup("touch_touches_symlinks", |dirs, sandbox| { + setup_symlink_fs(&dirs, sandbox); + + let missing = dirs.test().join("m"); + assert!(!missing.exists()); + + nu!( + cwd: dirs.test(), + " + touch fds -s + touch ds -s + touch fs -s + touch fms -s + " + ); + + // We did not create the missing symlink target + assert!(!missing.exists()); + + // The timestamps for files and directories remain the same + let file_times = symlink_times(&dirs.test().join("f")); + let dir_times = symlink_times(&dirs.test().join("d")); + let dir_file_times = symlink_times(&dirs.test().join("d/f")); + + assert_eq!(file_times, (TIME_ONE, TIME_ONE)); + assert_eq!(dir_times, (TIME_ONE, TIME_ONE)); + assert_eq!(dir_file_times, (TIME_ONE, TIME_ONE)); + + // For symlinks, everything changed. (except their targets, and paths, and personality) + let file_symlink_times = symlink_times(&dirs.test().join("fs")); + let dir_symlink_times = symlink_times(&dirs.test().join("ds")); + let dir_file_symlink_times = symlink_times(&dirs.test().join("fds")); + let file_missing_symlink_times = symlink_times(&dirs.test().join("fms")); + + assert_ne!(file_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(dir_file_symlink_times, (TIME_ONE, TIME_ONE)); + assert_ne!(file_missing_symlink_times, (TIME_ONE, TIME_ONE)); + }) +} From 13ce9e4f64ba324e9cd62fc3780951ab5683c205 Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Sun, 17 Nov 2024 19:31:36 -0600 Subject: [PATCH 18/21] update uutils crates (#14371) # Description This PR updates the uutils/coreutils crates to the latest version. I hard-coded debug to false, a new uu_mv parameter. It may be interesting to add that but I just wanted to get all the uu crates on the same version. I had to update the tests because --no-clobber works but doesn't say anything when it's not clobbering and previously we were checking for an error message. # User-Facing Changes # Tests + Formatting # After Submitting --- Cargo.lock | 34 +++++++++---------- Cargo.toml | 14 ++++---- crates/nu-command/src/filesystem/umv.rs | 1 + crates/nu-command/tests/commands/move_/umv.rs | 9 +++-- crates/nu-command/tests/commands/ucp.rs | 11 +++--- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 884f1993d9..e6cef1232b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6709,9 +6709,9 @@ checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" [[package]] name = "uu_cp" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6fb99d355ccb02e8c514e4a1d93e4aa4eedea9837de24635dfd24c165971444e" +checksum = "e0eff79f5eacf6bb88c9afc19f3cec2ab14ad31317be1369100658b46d41e410" dependencies = [ "clap", "filetime", @@ -6725,9 +6725,9 @@ dependencies = [ [[package]] name = "uu_mkdir" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "219588fbc146f18188781208ac4034616c51cf151677b4e1f9caf63ca8a7f2cf" +checksum = "feba7cf875eecbb746b1c5a5a8a031ab3a00e5f44f5441643a06b78577780d3a" dependencies = [ "clap", "uucore", @@ -6735,9 +6735,9 @@ dependencies = [ [[package]] name = "uu_mktemp" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b1e79ad2c5911908fce23a6069c52ca82e1997e2ed4bf6abf2d867c79c3dc73f" +checksum = "1a9cfd389f60e667c5ee6659beaad50bada7e710d76082c7d77ab91e04307c8f" dependencies = [ "clap", "rand", @@ -6747,9 +6747,9 @@ dependencies = [ [[package]] name = "uu_mv" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cd57c8d02f8a99ed56ed9f6fddab403ee0e2bf9e8f3a5ca8f0f9e4d6e3e392a0" +checksum = "bf932231fccdf108f75443bab0ce17acfe49b5825d731b8a358251833be7da20" dependencies = [ "clap", "fs_extra", @@ -6773,9 +6773,9 @@ dependencies = [ [[package]] name = "uu_uname" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ad1ca90f9b292bccaad0de70e6feccac5182c6713a5e1ca72d97bf3555b608b4" +checksum = "182b4071a2e6f7288cbbc1b1ff05c74e9dc7527b4735583d9e3cd92802b06910" dependencies = [ "clap", "platform-info", @@ -6784,27 +6784,27 @@ dependencies = [ [[package]] name = "uu_whoami" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc7c52e42e0425710461700adc1063f468f2ba8a8ff83ee69ba661095ab7b77a" +checksum = "5d15200414428c65f95d0b1d1226fc84f74ae80376bfe59959d93ddf57f944f5" dependencies = [ "clap", "libc", "uucore", - "windows-sys 0.48.0", + "windows-sys 0.59.0", ] [[package]] name = "uucore" -version = "0.0.27" +version = "0.0.28" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7b54aad02cf7e96f5fafabb6b836efa73eef934783b17530095a29ffd4fdc154" +checksum = "04ea43050c46912575654c5181f4135529e8d4003fca80803af10cdef3ca6412" dependencies = [ "clap", "dunce", "glob", "libc", - "nix 0.28.0", + "nix 0.29.0", "number_prefix", "once_cell", "os_display", @@ -6812,7 +6812,7 @@ dependencies = [ "walkdir", "wild", "winapi-util", - "windows-sys 0.48.0", + "windows-sys 0.59.0", "xattr", ] diff --git a/Cargo.toml b/Cargo.toml index 0297f5fdb5..0b796573c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -165,14 +165,14 @@ unicode-segmentation = "1.12" unicode-width = "0.1" ureq = { version = "2.10", default-features = false } url = "2.2" -uu_cp = "0.0.27" -uu_mkdir = "0.0.27" -uu_mktemp = "0.0.27" -uu_mv = "0.0.27" +uu_cp = "0.0.28" +uu_mkdir = "0.0.28" +uu_mktemp = "0.0.28" +uu_mv = "0.0.28" uu_touch = "0.0.28" -uu_whoami = "0.0.27" -uu_uname = "0.0.27" -uucore = "0.0.27" +uu_whoami = "0.0.28" +uu_uname = "0.0.28" +uucore = "0.0.28" uuid = "1.11.0" v_htmlescape = "0.15.0" wax = "0.6" diff --git a/crates/nu-command/src/filesystem/umv.rs b/crates/nu-command/src/filesystem/umv.rs index 0f49a7d97d..929fc7bcbb 100644 --- a/crates/nu-command/src/filesystem/umv.rs +++ b/crates/nu-command/src/filesystem/umv.rs @@ -188,6 +188,7 @@ impl Command for UMv { target_dir: None, no_target_dir: false, strip_slashes: false, + debug: false, }; if let Err(error) = uu_mv::mv(&files, &options) { return Err(ShellError::GenericError { diff --git a/crates/nu-command/tests/commands/move_/umv.rs b/crates/nu-command/tests/commands/move_/umv.rs index b193d5b7cb..98e2a753bf 100644 --- a/crates/nu-command/tests/commands/move_/umv.rs +++ b/crates/nu-command/tests/commands/move_/umv.rs @@ -513,13 +513,18 @@ fn test_mv_no_clobber() { sandbox.with_files(&[EmptyFile(file_a)]); sandbox.with_files(&[EmptyFile(file_b)]); - let actual = nu!( + let _ = nu!( cwd: dirs.test(), "mv -n {} {}", file_a, file_b, ); - assert!(actual.err.contains("not replacing")); + + let file_count = nu!( + cwd: dirs.test(), + "ls test_mv* | length | to nuon" + ); + assert_eq!(file_count.out, "2"); }) } diff --git a/crates/nu-command/tests/commands/ucp.rs b/crates/nu-command/tests/commands/ucp.rs index eb2363e68c..e3fa2ca931 100644 --- a/crates/nu-command/tests/commands/ucp.rs +++ b/crates/nu-command/tests/commands/ucp.rs @@ -841,14 +841,13 @@ fn test_cp_arg_no_clobber() { let target = dirs.fixtures.join("cp").join(TEST_HOW_ARE_YOU_SOURCE); let target_hash = get_file_hash(target.display()); - let actual = nu!( - cwd: dirs.root(), - "cp {} {} --no-clobber", - src.display(), - target.display() + let _ = nu!( + cwd: dirs.root(), + "cp {} {} --no-clobber", + src.display(), + target.display() ); let after_cp_hash = get_file_hash(target.display()); - assert!(actual.err.contains("not replacing")); // Check content was not clobbered assert_eq!(after_cp_hash, target_hash); }); From 9ec152d06d78875f436472a3787c5eaff69f5a59 Mon Sep 17 00:00:00 2001 From: "pegasus.cadence@gmail.com" Date: Mon, 18 Nov 2024 18:35:36 -0800 Subject: [PATCH 19/21] DriverPwdMap --- crates/nu-path/src/tilde.rs | 12 + .../src/engine/state_driver_pwd.rs | 292 ++++++++++++++++++ 2 files changed, 304 insertions(+) create mode 100644 crates/nu-protocol/src/engine/state_driver_pwd.rs diff --git a/crates/nu-path/src/tilde.rs b/crates/nu-path/src/tilde.rs index ac48bdfc3f..02324ea8da 100644 --- a/crates/nu-path/src/tilde.rs +++ b/crates/nu-path/src/tilde.rs @@ -21,6 +21,18 @@ fn expand_tilde_with_home(path: impl AsRef, home: Option) -> Path let path = path.as_ref(); if !path.starts_with("~") { + use nu_protocol::engine::state_driver_pwd:: { + need_expand_current_directory, + get_windows_absolute_path, + }; + if need_expand_current_directory(path) { + if let Some(current_dir) = get_windows_absolute_path(path) { + //println!("Absolute path for {} is: {}", path.display(), current_dir); + return PathBuf::from(¤t_dir) + } else { + println!("Failed to get absolute path for {}", path.display()); + } + } let string = path.to_string_lossy(); let mut path_as_string = string.as_ref().bytes(); return match path_as_string.next() { diff --git a/crates/nu-protocol/src/engine/state_driver_pwd.rs b/crates/nu-protocol/src/engine/state_driver_pwd.rs new file mode 100644 index 0000000000..dfdbe0ebc3 --- /dev/null +++ b/crates/nu-protocol/src/engine/state_driver_pwd.rs @@ -0,0 +1,292 @@ +use std::path::Path; + +pub struct DrivePwdMap { + map: [Option; 26], // Fixed-size array for A-Z +} + +impl DrivePwdMap { + /// Create a new DrivePwdMap + pub fn new() -> Self { + DrivePwdMap { + map: Default::default(), // Initialize all to `None` + } + } + + /// Set the current working directory based on the drive letter in the path + pub fn set_pwd(&mut self, path: &Path) -> Result<(), String> { + if let Some(drive_letter) = Self::extract_drive_letter(path) { + if let Some(index) = Self::drive_to_index(drive_letter) { + self.map[index] = Some(path.to_string_lossy().into_owned()); + Ok(()) + } else { + Err(format!("Invalid drive letter: {}", drive_letter)) + } + } else { + Err(format!("Invalid path: {}", path.display())) + } + } + + /// Get the current working directory for a drive letter + /// If no PWD is set, return the root of the drive (e.g., `C:\`) + pub fn get_pwd(&self, drive: char) -> Option { + Self::drive_to_index(drive).map(|index| { + self.map[index] + .clone() + .unwrap_or_else(|| format!("{}:\\", drive.to_ascii_uppercase())) + }) + } + + /// Expand a relative path using the current working directory of the drive + pub fn expand_path(&self, path: &Path) -> Option { + let path_str = path.to_str()?; + if let Some(drive_letter) = Self::extract_drive_letter(path) { + let is_absolute = path_str.contains(":\\") || path_str.starts_with("\\"); + if is_absolute { + // Already an absolute path + Some(PathBuf::from(path_str)) + } else if let Some(pwd) = self.get_pwd(drive_letter) { + // Combine current PWD with the relative path + let mut base = PathBuf::from(pwd); + base.push(path_str.split_at(2).1); // Skip the "C:" part of the relative path + Some(base) + } else { + None // Drive letter not found + } + } else { + None // Invalid or no drive letter + } + } + + /// Helper to convert a drive letter to an array index + fn drive_to_index(drive: char) -> Option { + let drive = drive.to_ascii_uppercase(); + if ('A'..='Z').contains(&drive) { + Some((drive as usize) - ('A' as usize)) + } else { + None + } + } + + /// Extract the drive letter from a path (e.g., `C:\Users` -> `C`) + fn extract_drive_letter(path: &Path) -> Option { + path.to_str() + .and_then(|s| s.chars().next()) + .filter(|c| c.is_ascii_alphabetic()) + } +} + +use once_cell::sync::Lazy; +use std::sync::Mutex; +use crate::ShellError; + +/// Global singleton instance of DrivePwdMap +static DRIVE_PWD_MAP: Lazy> = Lazy::new(|| Mutex::new(DrivePwdMap::new())); + +/// Public API to access the singleton instance +pub fn get_drive_pwd_map() -> &'static Mutex { + &DRIVE_PWD_MAP +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn test_expand_path() { + let mut drive_map = DrivePwdMap::new(); + + // Set PWD for drive C + drive_map.set_pwd(Path::new("C:\\Users\\Home")).unwrap(); + + // Expand a relative path + let expanded = drive_map.expand_path(Path::new("C:test")); + assert_eq!(expanded, Some(PathBuf::from("C:\\Users\\Home\\test"))); + + // Expand an absolute path + let expanded = drive_map.expand_path(Path::new("C:\\absolute\\path")); + assert_eq!(expanded, Some(PathBuf::from("C:\\absolute\\path"))); + + // Expand with no drive letter + let expanded = drive_map.expand_path(Path::new("\\no_drive")); + assert_eq!(expanded, None); + + // Expand with no PWD set for the drive + let expanded = drive_map.expand_path(Path::new("D:test")); + assert_eq!(expanded, Some(PathBuf::from("D:\\test"))); + } + #[test] + fn test_singleton_set_and_get_pwd() { + let drive_pwd_map = get_drive_pwd_map(); + { + let mut map = drive_pwd_map.lock().unwrap(); + + // Set PWD for drive C + assert!(map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); + } + + { + let map = drive_pwd_map.lock().unwrap(); + + // Get PWD for drive C + assert_eq!(map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); + + // Get PWD for drive E (not set, should return E:\) + assert_eq!(map.get_pwd('E'), Some("E:\\".to_string())); + } + } + + #[test] + fn test_set_and_get_pwd() { + let mut drive_map = DrivePwdMap::new(); + + // Set PWD for drive C + assert!(drive_map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); + assert_eq!(drive_map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); + + // Set PWD for drive D + assert!(drive_map.set_pwd(Path::new("D:\\Projects")).is_ok()); + assert_eq!(drive_map.get_pwd('D'), Some("D:\\Projects".to_string())); + + // Get PWD for drive E (not set, should return E:\) + assert_eq!(drive_map.get_pwd('E'), Some("E:\\".to_string())); + } + + #[test] + fn test_set_pwd_invalid_path() { + let mut drive_map = DrivePwdMap::new(); + + // Invalid path (no drive letter) + let result = drive_map.set_pwd(Path::new("\\InvalidPath")); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "Invalid path: \\InvalidPath"); + } + + #[test] + fn test_get_pwd_invalid_drive() { + let drive_map = DrivePwdMap::new(); + + // Get PWD for a drive not set (e.g., Z) + assert_eq!(drive_map.get_pwd('Z'), Some("Z:\\".to_string())); + + // Invalid drive letter (non-alphabetic) + assert_eq!(drive_map.get_pwd('1'), None); + } + + #[test] + fn test_drive_to_index() { + // Valid drive letters + assert_eq!(DrivePwdMap::drive_to_index('A'), Some(0)); + assert_eq!(DrivePwdMap::drive_to_index('Z'), Some(25)); + // Valid drive letters + assert_eq!(DrivePwdMap::drive_to_index('a'), Some(0)); + assert_eq!(DrivePwdMap::drive_to_index('z'), Some(25)); + for i in 1..25 { + assert_eq!(DrivePwdMap::drive_to_index(std::char::from_u32(('A' as usize + i) as u32).unwrap()), Some(i)); + assert_eq!(DrivePwdMap::drive_to_index(std::char::from_u32(('a' as usize + i) as u32).unwrap()), Some(i)); + } + + // Invalid drive letters + assert_eq!(DrivePwdMap::drive_to_index('1'), None); + assert_eq!(DrivePwdMap::drive_to_index('$'), None); + } +} + +mod current_directory_specific { + use crate::ShellError; + use std::path::Path; + + #[cfg(target_os = "windows")] + fn need_expand_current_directory(path: &Path) -> bool { + if let Some(path_str) = path.to_str() { + let chars: Vec = path_str.chars().collect(); + if chars.len() >= 2 { + return chars[1] == ':' && (chars.len() == 2 || (chars[2] != '/' && chars[2] != '\\')); + } + } + false + } + + #[cfg(not(target_os = "windows"))] + fn need_expand_current_directory(path: &Path) -> bool { + false + } + + #[cfg(target_os = "windows")] + fn get_windows_absolute_path(path: &Path) -> Option { + use std::ffi::OsString; + use std::os::windows::ffi::OsStringExt; + use std::os::windows::ffi::OsStrExt; + use winapi::um::fileapi::GetFullPathNameW; + use winapi::um::winnt::WCHAR; + + const MAX_PATH : usize = 260; + let mut buffer: [WCHAR; MAX_PATH] = [0; MAX_PATH]; + + if let Some(path_str) = path.to_str() { + unsafe { + // Convert input to wide string. + let wide_path: Vec = OsString::from(path_str).encode_wide().chain(Some(0)).collect(); + let length = GetFullPathNameW( + wide_path.as_ptr(), + buffer.len() as u32, + buffer.as_mut_ptr(), + std::ptr::null_mut(), + ); + + if length > 0 { + let abs_path = OsString::from_wide(&buffer[..length as usize]); + if let Some(abs_path_str) = abs_path.to_str() { + let abs_path_string = abs_path_str.to_string(); + return Some(abs_path_string); + } + } + } + } + + None + } + + #[cfg(not(target_os = "windows"))] + fn get_windows_absolute_path(path: &Path) -> Option { + None + } + #[cfg(target_os = "windows")] + pub fn set_current_directory_windows(path: &Path) -> Result<(), ShellError> { + use std::ffi::OsString; + use std::os::windows::ffi::OsStrExt; + use windows_sys::Win32::System::Environment::SetCurrentDirectoryW; + + if let Some(path_str) = path.to_str() { + unsafe { + // Convert input to wide string. + let wide_path: Vec = OsString::from(path_str).encode_wide().chain(Some(0)).collect(); + if SetCurrentDirectoryW(wide_path.as_ptr()) != 0 { + println!("Successfully changed the current directory to {}", path_str); + return Ok(()) + } else { + return + Err(ShellError::GenericError { + error: "Failed to set current directory".into(), + msg: "SetCurrentDirectoryW() failed".into(), + span: None, + help: None, + inner: vec![], + }) + }; + } + } + Err(ShellError::GenericError { + error: "Failed to set current directory".into(), + msg: "Invalid path".into(), + span: None, + help: None, + inner: vec![], + }) + } + + #[cfg(not(target_os = "windows"))] + pub fn set_current_directory_windows(_path: &Path) -> Result<(), ShellError>{ + Ok(()) + } +} \ No newline at end of file From 2a216ea363a39cd633d3be5658c92c18dd59af9a Mon Sep 17 00:00:00 2001 From: "pegasus.cadence@gmail.com" Date: Mon, 18 Nov 2024 20:42:45 -0800 Subject: [PATCH 20/21] CWDperDrive --- Cargo.lock | 3 + Cargo.toml | 3 + crates/nu-command/src/filesystem/cd.rs | 12 ++++ crates/nu-path/Cargo.toml | 3 + crates/nu-path/src/lib.rs | 1 + .../src}/state_driver_pwd.rs | 67 +++++++++++-------- crates/nu-path/src/tilde.rs | 22 +++--- 7 files changed, 73 insertions(+), 38 deletions(-) rename crates/{nu-protocol/src/engine => nu-path/src}/state_driver_pwd.rs (85%) diff --git a/Cargo.lock b/Cargo.lock index 2754f5bd23..a00e672645 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3352,7 +3352,10 @@ version = "0.100.1" dependencies = [ "dirs", "omnipath", + "once_cell", "pwd", + "winapi", + "windows-sys 0.48.0", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index a976aeec17..394565702e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -321,3 +321,6 @@ bench = false [[bench]] name = "benchmarks" harness = false + +[profile.dev] +incremental = true diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 2af932bcb4..67ee4a6e9d 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -114,7 +114,19 @@ 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 => { + let path_for_scd = path.clone(); stack.set_cwd(path)?; + use nu_path::state_driver_pwd::get_drive_pwd_map; + get_drive_pwd_map().lock().unwrap().set_pwd(path_for_scd.as_path()) + .map_err(|_e| { + Err(ShellError::GenericError{ + error: "Set PWD for drive failed".into(), + msg: "System error".into(), + span: None, + help: None, + inner: vec![], + }) + })?; Ok(PipelineData::empty()) } PermissionResult::PermissionDenied(reason) => Err(ShellError::IOError { diff --git a/crates/nu-path/Cargo.toml b/crates/nu-path/Cargo.toml index 22ab716d14..59de73e5e4 100644 --- a/crates/nu-path/Cargo.toml +++ b/crates/nu-path/Cargo.toml @@ -16,6 +16,9 @@ dirs = { workspace = true } [target.'cfg(windows)'.dependencies] omnipath = { workspace = true } +once_cell = "1.20.1" +winapi = { version = "0.3.9", features = ["fileapi"] } +windows-sys = { workspace = true, features = ["Win32_System_Environment"] } [target.'cfg(all(unix, not(target_os = "macos"), not(target_os = "android")))'.dependencies] pwd = { workspace = true } diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index cf31a5789f..2d4ecb7d88 100644 --- a/crates/nu-path/src/lib.rs +++ b/crates/nu-path/src/lib.rs @@ -8,6 +8,7 @@ mod helpers; mod path; mod tilde; mod trailing_slash; +pub mod state_driver_pwd; pub use components::components; pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs}; diff --git a/crates/nu-protocol/src/engine/state_driver_pwd.rs b/crates/nu-path/src/state_driver_pwd.rs similarity index 85% rename from crates/nu-protocol/src/engine/state_driver_pwd.rs rename to crates/nu-path/src/state_driver_pwd.rs index dfdbe0ebc3..267438954a 100644 --- a/crates/nu-protocol/src/engine/state_driver_pwd.rs +++ b/crates/nu-path/src/state_driver_pwd.rs @@ -1,11 +1,10 @@ -use std::path::Path; +use std::path::{ Path, PathBuf }; pub struct DrivePwdMap { map: [Option; 26], // Fixed-size array for A-Z } impl DrivePwdMap { - /// Create a new DrivePwdMap pub fn new() -> Self { DrivePwdMap { map: Default::default(), // Initialize all to `None` @@ -16,7 +15,8 @@ impl DrivePwdMap { pub fn set_pwd(&mut self, path: &Path) -> Result<(), String> { if let Some(drive_letter) = Self::extract_drive_letter(path) { if let Some(index) = Self::drive_to_index(drive_letter) { - self.map[index] = Some(path.to_string_lossy().into_owned()); + let normalized = Self::normalize_path(path); + self.map[index] = Some(normalized.to_string_lossy().into_owned()); Ok(()) } else { Err(format!("Invalid drive letter: {}", drive_letter)) @@ -27,7 +27,6 @@ impl DrivePwdMap { } /// Get the current working directory for a drive letter - /// If no PWD is set, return the root of the drive (e.g., `C:\`) pub fn get_pwd(&self, drive: char) -> Option { Self::drive_to_index(drive).map(|index| { self.map[index] @@ -43,10 +42,10 @@ impl DrivePwdMap { let is_absolute = path_str.contains(":\\") || path_str.starts_with("\\"); if is_absolute { // Already an absolute path - Some(PathBuf::from(path_str)) + Some(PathBuf::from(Self::ensure_trailing_separator(path_str))) } else if let Some(pwd) = self.get_pwd(drive_letter) { // Combine current PWD with the relative path - let mut base = PathBuf::from(pwd); + let mut base = PathBuf::from(Self::ensure_trailing_separator(&pwd)); base.push(path_str.split_at(2).1); // Skip the "C:" part of the relative path Some(base) } else { @@ -67,17 +66,35 @@ impl DrivePwdMap { } } - /// Extract the drive letter from a path (e.g., `C:\Users` -> `C`) + /// Extract the drive letter from a path (e.g., `C:test` -> `C`) fn extract_drive_letter(path: &Path) -> Option { path.to_str() .and_then(|s| s.chars().next()) .filter(|c| c.is_ascii_alphabetic()) } + + /// Normalize a path by removing any trailing `\` or `/` + fn normalize_path(path: &Path) -> PathBuf { + let mut normalized = path.to_path_buf(); + while normalized.to_string_lossy().ends_with(&['\\', '/'][..]) { + normalized.pop(); + } + normalized + } + + /// Ensure a path has a trailing `\` + fn ensure_trailing_separator(path: &str) -> String { + if !path.ends_with('\\') && !path.ends_with('/') { + format!("{}\\", path) + } else { + path.to_string() + } + } } use once_cell::sync::Lazy; use std::sync::Mutex; -use crate::ShellError; +//use nu_protocol::errors::shell_error::ShellError; /// Global singleton instance of DrivePwdMap static DRIVE_PWD_MAP: Lazy> = Lazy::new(|| Mutex::new(DrivePwdMap::new())); @@ -192,12 +209,11 @@ mod tests { } } -mod current_directory_specific { - use crate::ShellError; +pub mod current_directory_specific { use std::path::Path; #[cfg(target_os = "windows")] - fn need_expand_current_directory(path: &Path) -> bool { + pub fn need_expand_current_directory_per_drive(path: &Path) -> bool { if let Some(path_str) = path.to_str() { let chars: Vec = path_str.chars().collect(); if chars.len() >= 2 { @@ -208,12 +224,12 @@ mod current_directory_specific { } #[cfg(not(target_os = "windows"))] - fn need_expand_current_directory(path: &Path) -> bool { + pub fn need_expand_current_directory(path: &Path) -> bool { false } #[cfg(target_os = "windows")] - fn get_windows_absolute_path(path: &Path) -> Option { + pub fn get_windows_absolute_path(path: &Path) -> Option { use std::ffi::OsString; use std::os::windows::ffi::OsStringExt; use std::os::windows::ffi::OsStrExt; @@ -247,12 +263,16 @@ mod current_directory_specific { None } + pub enum DrivePwdError { + InvalidPath, + SystemError, + } #[cfg(not(target_os = "windows"))] fn get_windows_absolute_path(path: &Path) -> Option { None } #[cfg(target_os = "windows")] - pub fn set_current_directory_windows(path: &Path) -> Result<(), ShellError> { + pub fn set_current_directory_windows(path: &Path) -> Result<(), DrivePwdError> { use std::ffi::OsString; use std::os::windows::ffi::OsStrExt; use windows_sys::Win32::System::Environment::SetCurrentDirectoryW; @@ -266,27 +286,16 @@ mod current_directory_specific { return Ok(()) } else { return - Err(ShellError::GenericError { - error: "Failed to set current directory".into(), - msg: "SetCurrentDirectoryW() failed".into(), - span: None, - help: None, - inner: vec![], - }) + Err(DrivePwdError::SystemError) + }; } } - Err(ShellError::GenericError { - error: "Failed to set current directory".into(), - msg: "Invalid path".into(), - span: None, - help: None, - inner: vec![], - }) + Err(DrivePwdError::InvalidPath) } #[cfg(not(target_os = "windows"))] - pub fn set_current_directory_windows(_path: &Path) -> Result<(), ShellError>{ + pub fn set_current_directory_windows(_path: &Path) -> Result<(), Error>{ Ok(()) } } \ No newline at end of file diff --git a/crates/nu-path/src/tilde.rs b/crates/nu-path/src/tilde.rs index 02324ea8da..360810b86c 100644 --- a/crates/nu-path/src/tilde.rs +++ b/crates/nu-path/src/tilde.rs @@ -21,17 +21,21 @@ fn expand_tilde_with_home(path: impl AsRef, home: Option) -> Path let path = path.as_ref(); if !path.starts_with("~") { - use nu_protocol::engine::state_driver_pwd:: { - need_expand_current_directory, - get_windows_absolute_path, + use crate::state_driver_pwd:: { + current_directory_specific::need_expand_current_directory_per_drive, + //current_directory_specific::get_windows_absolute_path, + get_drive_pwd_map }; - if need_expand_current_directory(path) { - if let Some(current_dir) = get_windows_absolute_path(path) { - //println!("Absolute path for {} is: {}", path.display(), current_dir); - return PathBuf::from(¤t_dir) - } else { - println!("Failed to get absolute path for {}", path.display()); + if need_expand_current_directory_per_drive(path) { + if let Some(expanded_dir) = get_drive_pwd_map().lock().unwrap().expand_path(path) { + return PathBuf::from(&expanded_dir) } + // if let Some(current_dir) = get_windows_absolute_path(path) { + // //println!("Absolute path for {} is: {}", path.display(), current_dir); + // return PathBuf::from(¤t_dir) + // } else { + // println!("Failed to get absolute path for {}", path.display()); + // } } let string = path.to_string_lossy(); let mut path_as_string = string.as_ref().bytes(); From e48daf8da2caa3fe4c6e9d33de8fda35c8b478aa Mon Sep 17 00:00:00 2001 From: "pegasus.cadence@gmail.com" Date: Tue, 19 Nov 2024 04:12:31 -0800 Subject: [PATCH 21/21] PWD per drive --- crates/nu-cli/src/repl.rs | 2 + crates/nu-command/src/filesystem/cd.rs | 14 +- crates/nu-path/Cargo.toml | 2 +- crates/nu-path/src/lib.rs | 3 +- crates/nu-path/src/pwd_per_drive.rs | 348 +++++++++++++++++++++++++ crates/nu-path/src/state_driver_pwd.rs | 301 --------------------- crates/nu-path/src/tilde.rs | 18 +- 7 files changed, 359 insertions(+), 329 deletions(-) create mode 100644 crates/nu-path/src/pwd_per_drive.rs delete mode 100644 crates/nu-path/src/state_driver_pwd.rs diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 5440f9caa2..7884d7fab9 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -858,6 +858,8 @@ fn do_auto_cd( report_shell_error(engine_state, &err); return; }; + use nu_path::pwd_per_drive::pwd_per_drive::set_pwd_per_drive; + let _as_is = set_pwd_per_drive(PathBuf::from(path.clone()).as_path()); let cwd = Value::string(cwd, span); let shells = stack.get_env_var(engine_state, "NUSHELL_SHELLS"); diff --git a/crates/nu-command/src/filesystem/cd.rs b/crates/nu-command/src/filesystem/cd.rs index 67ee4a6e9d..81e93e6ec0 100644 --- a/crates/nu-command/src/filesystem/cd.rs +++ b/crates/nu-command/src/filesystem/cd.rs @@ -114,19 +114,9 @@ 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 => { - let path_for_scd = path.clone(); + use nu_path::pwd_per_drive::pwd_per_drive::set_pwd_per_drive; + let _as_is = set_pwd_per_drive(path.as_path()); stack.set_cwd(path)?; - use nu_path::state_driver_pwd::get_drive_pwd_map; - get_drive_pwd_map().lock().unwrap().set_pwd(path_for_scd.as_path()) - .map_err(|_e| { - Err(ShellError::GenericError{ - error: "Set PWD for drive failed".into(), - msg: "System error".into(), - span: None, - help: None, - inner: vec![], - }) - })?; Ok(PipelineData::empty()) } PermissionResult::PermissionDenied(reason) => Err(ShellError::IOError { diff --git a/crates/nu-path/Cargo.toml b/crates/nu-path/Cargo.toml index 59de73e5e4..762859f8cb 100644 --- a/crates/nu-path/Cargo.toml +++ b/crates/nu-path/Cargo.toml @@ -13,12 +13,12 @@ bench = false [dependencies] dirs = { workspace = true } +cfg-if = "1.0.0" [target.'cfg(windows)'.dependencies] omnipath = { workspace = true } once_cell = "1.20.1" winapi = { version = "0.3.9", features = ["fileapi"] } -windows-sys = { workspace = true, features = ["Win32_System_Environment"] } [target.'cfg(all(unix, not(target_os = "macos"), not(target_os = "android")))'.dependencies] pwd = { workspace = true } diff --git a/crates/nu-path/src/lib.rs b/crates/nu-path/src/lib.rs index 2d4ecb7d88..145013e7fe 100644 --- a/crates/nu-path/src/lib.rs +++ b/crates/nu-path/src/lib.rs @@ -4,11 +4,12 @@ mod components; pub mod dots; pub mod expansions; pub mod form; +#[cfg(target_os="windows")] +pub mod pwd_per_drive; mod helpers; mod path; mod tilde; mod trailing_slash; -pub mod state_driver_pwd; pub use components::components; pub use expansions::{canonicalize_with, expand_path_with, expand_to_real_path, locate_in_dirs}; diff --git a/crates/nu-path/src/pwd_per_drive.rs b/crates/nu-path/src/pwd_per_drive.rs new file mode 100644 index 0000000000..a9f8238319 --- /dev/null +++ b/crates/nu-path/src/pwd_per_drive.rs @@ -0,0 +1,348 @@ +cfg_if::cfg_if! { + if #[cfg(target_os="windows")] { + +use once_cell::sync::Lazy; +use std::path::{ Path, PathBuf }; +use std::sync::Mutex; + +struct DrivePWDmap { + map: [Option; 26], // Fixed-size array for A-Z +} + +impl DrivePWDmap { + pub fn new() -> Self { + DrivePWDmap { + map: Default::default(), // Initialize all to `None` + } + } + + /// Set the PWD for the drive letter in the path which is an absolute path + pub fn set_pwd(&mut self, path: &Path) -> Result<(), String> { + if let Some(drive_letter) = Self::extract_drive_letter(path) { + if let Some(index) = Self::drive_to_index(drive_letter) { + if let Some(path_str) = path.to_str() { + self.map[index] = Some(path_str.to_string()); + Ok(()) + } else { + Err(format!("Invalid path: {}", path.display())) + } + } else { + Err(format!("Invalid drive letter: {}", drive_letter)) + } + } else { + Err(format!("Invalid path: {}", path.display())) + } + } + + /// Get the PWD for a drive letter, if not yet, try using + /// winapi GetFullPathNameW to get "T:", "T:/" can be default + pub fn get_pwd(&mut self, drive: char) -> Option { + Self::drive_to_index(drive).map(|index| { + self.map[index] + .clone() + .unwrap_or_else(|| + if let Some(system_pwd) = get_full_path_name_w(&format!("{}:", drive.to_ascii_uppercase())) { + self.map[index] = Some(system_pwd.clone()); + system_pwd + } else { + format!("{}:/", drive.to_ascii_uppercase()) + } + ) + }) + } + + /// Expand a relative path using the PWD of the drive + pub fn expand_path(&mut self, path: &Path) -> Option { + let path_str = path.to_str()?; + if let Some(drive_letter) = Self::extract_drive_letter(path) { + if let Some(pwd) = self.get_pwd(drive_letter) { + // Combine current PWD with the relative path + let mut base = PathBuf::from(Self::ensure_trailing_separator(&pwd)); + base.push(path_str.split_at(2).1); // Skip the "C:" part of the relative path + Some(base) + } else { + None // PWD on Drive letter not found + } + } else { + None // Invalid or no drive letter + } + } + + /// Helper to convert a drive letter to an array index + fn drive_to_index(drive: char) -> Option { + let drive = drive.to_ascii_uppercase(); + if ('A'..='Z').contains(&drive) { + Some((drive as usize) - ('A' as usize)) + } else { + None + } + } + + /// Extract the drive letter from a path (e.g., `C:test` -> `C`) + fn extract_drive_letter(path: &Path) -> Option { + path.to_str() + .and_then(|s| s.chars().next()) + .filter(|c| c.is_ascii_alphabetic()) + } + + /// Ensure a path has a trailing `\` + fn ensure_trailing_separator(path: &str) -> String { + if !path.ends_with('\\') && !path.ends_with('/') { + format!("{}/", path) + } else { + path.to_string() + } + } +} + +// GetFullPathW +fn get_full_path_name_w(path_str: &str) -> Option { + use std::ffi::OsString; + use std::os::windows::ffi::OsStringExt; + use std::os::windows::ffi::OsStrExt; + use winapi::um::fileapi::GetFullPathNameW; + use winapi::um::winnt::WCHAR; + + const MAX_PATH : usize = 260; + let mut buffer: [WCHAR; MAX_PATH] = [0; MAX_PATH]; + + unsafe { + // Convert input to wide string. + let wide_path: Vec = OsString::from(path_str).encode_wide().chain(Some(0)).collect(); + let length = GetFullPathNameW( + wide_path.as_ptr(), + buffer.len() as u32, + buffer.as_mut_ptr(), + std::ptr::null_mut(), + ); + + if length > 0 && (length as usize) < MAX_PATH { + let path = OsString::from_wide(&buffer[..length as usize]); + if let Some(path_str) = path.to_str() { + let path_string = path_str.to_string(); + { + return Some(path_string); + } + } + } + } + None +} + +/// Global singleton instance of DrivePwdMap +static DRIVE_PWD_MAP: Lazy> = Lazy::new(|| Mutex::new(DrivePWDmap::new())); + +/// Public API to access the singleton instance +fn get_drive_pwd_map() -> &'static Mutex { + &DRIVE_PWD_MAP +} + +/// Test for DrivePWD map +#[cfg(test)] +mod tests { + use super::*; + use std::path::Path; + + #[test] + fn test_singleton_set_and_get_pwd() { + let drive_pwd_map = get_drive_pwd_map(); + { + let mut map = drive_pwd_map.lock().unwrap(); + + // Set PWD for drive C + assert!(map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); + } + + { + let map = drive_pwd_map.lock().unwrap(); + + // Get PWD for drive C + assert_eq!(map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); + + // Get PWD for drive E (not set, should return E:\) + assert_eq!(map.get_pwd('E'), Some("E:\\".to_string())); + } + } + #[test] + fn test_expand_path() { + let mut drive_map = DrivePWDmap::new(); + + // Set PWD for drive C + drive_map.set_pwd(Path::new("C:\\Users\\Home")).unwrap(); + + // Expand a relative path + let expanded = drive_map.expand_path(Path::new("C:test")); + assert_eq!(expanded, Some(PathBuf::from("C:\\Users\\Home\\test"))); + + // Expand an absolute path + let expanded = drive_map.expand_path(Path::new("C:\\absolute\\path")); + assert_eq!(expanded, Some(PathBuf::from("C:\\absolute\\path"))); + + // Expand with no drive letter + let expanded = drive_map.expand_path(Path::new("\\no_drive")); + assert_eq!(expanded, None); + + // Expand with no PWD set for the drive + let expanded = drive_map.expand_path(Path::new("D:test")); + assert_eq!(expanded, Some(PathBuf::from("D:\\test"))); + } + + #[test] + fn test_set_and_get_pwd() { + let mut drive_map = DrivePWDmap::new(); + + // Set PWD for drive C + assert!(drive_map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); + assert_eq!(drive_map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); + + // Set PWD for drive D + assert!(drive_map.set_pwd(Path::new("D:\\Projects")).is_ok()); + assert_eq!(drive_map.get_pwd('D'), Some("D:\\Projects".to_string())); + + // Get PWD for drive E (not set, should return E:\) + assert_eq!(drive_map.get_pwd('E'), Some("E:\\".to_string())); + } + + #[test] + fn test_set_pwd_invalid_path() { + let mut drive_map = DrivePWDmap::new(); + + // Invalid path (no drive letter) + let result = drive_map.set_pwd(Path::new("\\InvalidPath")); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), "Invalid path: \\InvalidPath"); + } + + #[test] + fn test_get_pwd_invalid_drive() { + let drive_map = DrivePWDmap::new(); + + // Get PWD for a drive not set (e.g., Z) + assert_eq!(drive_map.get_pwd('Z'), Some("Z:\\".to_string())); + + // Invalid drive letter (non-alphabetic) + assert_eq!(drive_map.get_pwd('1'), None); + } + + #[test] + fn test_drive_to_index() { + // Valid drive letters + assert_eq!(DrivePWDmap::drive_to_index('A'), Some(0)); + assert_eq!(DrivePWDmap::drive_to_index('Z'), Some(25)); + // Valid drive letters + assert_eq!(DrivePWDmap::drive_to_index('a'), Some(0)); + assert_eq!(DrivePWDmap::drive_to_index('z'), Some(25)); + for i in 1..25 { + assert_eq!(DrivePWDmap::drive_to_index(std::char::from_u32(('A' as usize + i) as u32).unwrap()), Some(i)); + assert_eq!(DrivePWDmap::drive_to_index(std::char::from_u32(('a' as usize + i) as u32).unwrap()), Some(i)); + } + + // Invalid drive letters + assert_eq!(DrivePWDmap::drive_to_index('1'), None); + assert_eq!(DrivePWDmap::drive_to_index('$'), None); + } +}}} + +/// Usage for pwd_per_drive +/// +/// Upon change PWD, call set_pwd_per_drive() with absolute path +/// +/// Call expand_pwd_per_drive() with relative path to get absolution path +/// +/// Doctest +/// ```Rust +/// // Set PWD for drive C +/// set_pwd_per_drive(Path::new("C:\\Users\\Home")).unwrap(); +/// +/// // Expand a relative path +/// let expanded = expand_pwd_per_drive(Path::new("C:test")); +/// assert_eq!(expanded, Some(PathBuf::from("C:\\Users\\Home\\test"))); +/// +/// // Will NOT expand an absolute path +/// let expanded = expand_pwd_per_drive(Path::new("C:\\absolute\\path")); +/// assert_eq!(expanded, None); +/// +/// // Expand with no drive letter +/// let expanded = expand_pwd_per_drive(Path::new("\\no_drive")); +/// assert_eq!(expanded, None); +/// +/// // Expand with no PWD set for the drive +/// let expanded = expand_pwd_per_drive(Path::new("D:test")); +/// assert_eq!(expanded, Some(PathBuf::from("D:\\test"))); +/// ``` +pub mod pwd_per_drive { + use std::path::{ Path, PathBuf }; + use super::{get_drive_pwd_map}; + + /// set_pwd_per_drive + /// record PWD for drive, path must be absolute path + /// return Ok(()) if succeeded, otherwise error message + #[cfg(target_os = "windows")] + pub fn set_pwd_per_drive(path: &Path) -> Result<(), String> { + get_drive_pwd_map().lock().unwrap().set_pwd(path) + } + + #[cfg(not(target_os = "windows"))] + pub fn set_pwd_per_drive(path: &Path) -> Result<(), String> { + Ok(()) + } + + /// expand_pwe_per_drive + /// input relative path, expand PWD to construct absolute path + /// return PathBuf for absolute path, None if input path is invalid + #[cfg(target_os = "windows")] + pub fn expand_pwd_per_drive(path: &Path) -> Option { + if need_expand_pwd_per_drive(path) { + get_drive_pwd_map().lock().unwrap().expand_path(path) + } else { + None + } + } + + /// expand_pwd_per_drive will return None on non-windows platform + #[cfg(not(target_os = "windows"))] + pub fn expand_pwd_per_drive(_path: &Path) -> Option { + None + } + + /// If input path is relative path with drive letter, + /// it can be expanded with PWD per drive + #[cfg(target_os = "windows")] + fn need_expand_pwd_per_drive(path: &Path) -> bool { + if let Some(path_str) = path.to_str() { + let chars: Vec = path_str.chars().collect(); + if chars.len() >= 2 { + return chars[1] == ':' && (chars.len() == 2 || (chars[2] != '/' && chars[2] != '\\')); + } + } + false + } + + /// On non-windows platform, will not expand + #[cfg(not(target_os = "windows"))] + fn need_expand_pwd_per_drive(path: &Path) -> bool { + false + } + + #[test] + fn test_usage_for_pwd_per_drive() { + // Set PWD for drive C + set_pwd_per_drive(Path::new("C:\\Users\\Home")).unwrap(); + + // Expand a relative path + let expanded = expand_pwd_per_drive(Path::new("C:test")); + assert_eq!(expanded, Some(PathBuf::from("C:\\Users\\Home\\test"))); + + // Will NOT expand an absolute path + let expanded = expand_pwd_per_drive(Path::new("C:\\absolute\\path")); + assert_eq!(expanded, None); + + // Expand with no drive letter + let expanded = expand_pwd_per_drive(Path::new("\\no_drive")); + assert_eq!(expanded, None); + + // Expand with no PWD set for the drive + let expanded = expand_pwd_per_drive(Path::new("D:test")); + assert_eq!(expanded, Some(PathBuf::from("D:\\test"))); + } +} \ No newline at end of file diff --git a/crates/nu-path/src/state_driver_pwd.rs b/crates/nu-path/src/state_driver_pwd.rs deleted file mode 100644 index 267438954a..0000000000 --- a/crates/nu-path/src/state_driver_pwd.rs +++ /dev/null @@ -1,301 +0,0 @@ -use std::path::{ Path, PathBuf }; - -pub struct DrivePwdMap { - map: [Option; 26], // Fixed-size array for A-Z -} - -impl DrivePwdMap { - pub fn new() -> Self { - DrivePwdMap { - map: Default::default(), // Initialize all to `None` - } - } - - /// Set the current working directory based on the drive letter in the path - pub fn set_pwd(&mut self, path: &Path) -> Result<(), String> { - if let Some(drive_letter) = Self::extract_drive_letter(path) { - if let Some(index) = Self::drive_to_index(drive_letter) { - let normalized = Self::normalize_path(path); - self.map[index] = Some(normalized.to_string_lossy().into_owned()); - Ok(()) - } else { - Err(format!("Invalid drive letter: {}", drive_letter)) - } - } else { - Err(format!("Invalid path: {}", path.display())) - } - } - - /// Get the current working directory for a drive letter - pub fn get_pwd(&self, drive: char) -> Option { - Self::drive_to_index(drive).map(|index| { - self.map[index] - .clone() - .unwrap_or_else(|| format!("{}:\\", drive.to_ascii_uppercase())) - }) - } - - /// Expand a relative path using the current working directory of the drive - pub fn expand_path(&self, path: &Path) -> Option { - let path_str = path.to_str()?; - if let Some(drive_letter) = Self::extract_drive_letter(path) { - let is_absolute = path_str.contains(":\\") || path_str.starts_with("\\"); - if is_absolute { - // Already an absolute path - Some(PathBuf::from(Self::ensure_trailing_separator(path_str))) - } else if let Some(pwd) = self.get_pwd(drive_letter) { - // Combine current PWD with the relative path - let mut base = PathBuf::from(Self::ensure_trailing_separator(&pwd)); - base.push(path_str.split_at(2).1); // Skip the "C:" part of the relative path - Some(base) - } else { - None // Drive letter not found - } - } else { - None // Invalid or no drive letter - } - } - - /// Helper to convert a drive letter to an array index - fn drive_to_index(drive: char) -> Option { - let drive = drive.to_ascii_uppercase(); - if ('A'..='Z').contains(&drive) { - Some((drive as usize) - ('A' as usize)) - } else { - None - } - } - - /// Extract the drive letter from a path (e.g., `C:test` -> `C`) - fn extract_drive_letter(path: &Path) -> Option { - path.to_str() - .and_then(|s| s.chars().next()) - .filter(|c| c.is_ascii_alphabetic()) - } - - /// Normalize a path by removing any trailing `\` or `/` - fn normalize_path(path: &Path) -> PathBuf { - let mut normalized = path.to_path_buf(); - while normalized.to_string_lossy().ends_with(&['\\', '/'][..]) { - normalized.pop(); - } - normalized - } - - /// Ensure a path has a trailing `\` - fn ensure_trailing_separator(path: &str) -> String { - if !path.ends_with('\\') && !path.ends_with('/') { - format!("{}\\", path) - } else { - path.to_string() - } - } -} - -use once_cell::sync::Lazy; -use std::sync::Mutex; -//use nu_protocol::errors::shell_error::ShellError; - -/// Global singleton instance of DrivePwdMap -static DRIVE_PWD_MAP: Lazy> = Lazy::new(|| Mutex::new(DrivePwdMap::new())); - -/// Public API to access the singleton instance -pub fn get_drive_pwd_map() -> &'static Mutex { - &DRIVE_PWD_MAP -} - -#[cfg(test)] -mod tests { - use super::*; - use std::path::Path; - - #[test] - fn test_expand_path() { - let mut drive_map = DrivePwdMap::new(); - - // Set PWD for drive C - drive_map.set_pwd(Path::new("C:\\Users\\Home")).unwrap(); - - // Expand a relative path - let expanded = drive_map.expand_path(Path::new("C:test")); - assert_eq!(expanded, Some(PathBuf::from("C:\\Users\\Home\\test"))); - - // Expand an absolute path - let expanded = drive_map.expand_path(Path::new("C:\\absolute\\path")); - assert_eq!(expanded, Some(PathBuf::from("C:\\absolute\\path"))); - - // Expand with no drive letter - let expanded = drive_map.expand_path(Path::new("\\no_drive")); - assert_eq!(expanded, None); - - // Expand with no PWD set for the drive - let expanded = drive_map.expand_path(Path::new("D:test")); - assert_eq!(expanded, Some(PathBuf::from("D:\\test"))); - } - #[test] - fn test_singleton_set_and_get_pwd() { - let drive_pwd_map = get_drive_pwd_map(); - { - let mut map = drive_pwd_map.lock().unwrap(); - - // Set PWD for drive C - assert!(map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); - } - - { - let map = drive_pwd_map.lock().unwrap(); - - // Get PWD for drive C - assert_eq!(map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); - - // Get PWD for drive E (not set, should return E:\) - assert_eq!(map.get_pwd('E'), Some("E:\\".to_string())); - } - } - - #[test] - fn test_set_and_get_pwd() { - let mut drive_map = DrivePwdMap::new(); - - // Set PWD for drive C - assert!(drive_map.set_pwd(Path::new("C:\\Users\\Example")).is_ok()); - assert_eq!(drive_map.get_pwd('C'), Some("C:\\Users\\Example".to_string())); - - // Set PWD for drive D - assert!(drive_map.set_pwd(Path::new("D:\\Projects")).is_ok()); - assert_eq!(drive_map.get_pwd('D'), Some("D:\\Projects".to_string())); - - // Get PWD for drive E (not set, should return E:\) - assert_eq!(drive_map.get_pwd('E'), Some("E:\\".to_string())); - } - - #[test] - fn test_set_pwd_invalid_path() { - let mut drive_map = DrivePwdMap::new(); - - // Invalid path (no drive letter) - let result = drive_map.set_pwd(Path::new("\\InvalidPath")); - assert!(result.is_err()); - assert_eq!(result.unwrap_err(), "Invalid path: \\InvalidPath"); - } - - #[test] - fn test_get_pwd_invalid_drive() { - let drive_map = DrivePwdMap::new(); - - // Get PWD for a drive not set (e.g., Z) - assert_eq!(drive_map.get_pwd('Z'), Some("Z:\\".to_string())); - - // Invalid drive letter (non-alphabetic) - assert_eq!(drive_map.get_pwd('1'), None); - } - - #[test] - fn test_drive_to_index() { - // Valid drive letters - assert_eq!(DrivePwdMap::drive_to_index('A'), Some(0)); - assert_eq!(DrivePwdMap::drive_to_index('Z'), Some(25)); - // Valid drive letters - assert_eq!(DrivePwdMap::drive_to_index('a'), Some(0)); - assert_eq!(DrivePwdMap::drive_to_index('z'), Some(25)); - for i in 1..25 { - assert_eq!(DrivePwdMap::drive_to_index(std::char::from_u32(('A' as usize + i) as u32).unwrap()), Some(i)); - assert_eq!(DrivePwdMap::drive_to_index(std::char::from_u32(('a' as usize + i) as u32).unwrap()), Some(i)); - } - - // Invalid drive letters - assert_eq!(DrivePwdMap::drive_to_index('1'), None); - assert_eq!(DrivePwdMap::drive_to_index('$'), None); - } -} - -pub mod current_directory_specific { - use std::path::Path; - - #[cfg(target_os = "windows")] - pub fn need_expand_current_directory_per_drive(path: &Path) -> bool { - if let Some(path_str) = path.to_str() { - let chars: Vec = path_str.chars().collect(); - if chars.len() >= 2 { - return chars[1] == ':' && (chars.len() == 2 || (chars[2] != '/' && chars[2] != '\\')); - } - } - false - } - - #[cfg(not(target_os = "windows"))] - pub fn need_expand_current_directory(path: &Path) -> bool { - false - } - - #[cfg(target_os = "windows")] - pub fn get_windows_absolute_path(path: &Path) -> Option { - use std::ffi::OsString; - use std::os::windows::ffi::OsStringExt; - use std::os::windows::ffi::OsStrExt; - use winapi::um::fileapi::GetFullPathNameW; - use winapi::um::winnt::WCHAR; - - const MAX_PATH : usize = 260; - let mut buffer: [WCHAR; MAX_PATH] = [0; MAX_PATH]; - - if let Some(path_str) = path.to_str() { - unsafe { - // Convert input to wide string. - let wide_path: Vec = OsString::from(path_str).encode_wide().chain(Some(0)).collect(); - let length = GetFullPathNameW( - wide_path.as_ptr(), - buffer.len() as u32, - buffer.as_mut_ptr(), - std::ptr::null_mut(), - ); - - if length > 0 { - let abs_path = OsString::from_wide(&buffer[..length as usize]); - if let Some(abs_path_str) = abs_path.to_str() { - let abs_path_string = abs_path_str.to_string(); - return Some(abs_path_string); - } - } - } - } - - None - } - - pub enum DrivePwdError { - InvalidPath, - SystemError, - } - #[cfg(not(target_os = "windows"))] - fn get_windows_absolute_path(path: &Path) -> Option { - None - } - #[cfg(target_os = "windows")] - pub fn set_current_directory_windows(path: &Path) -> Result<(), DrivePwdError> { - use std::ffi::OsString; - use std::os::windows::ffi::OsStrExt; - use windows_sys::Win32::System::Environment::SetCurrentDirectoryW; - - if let Some(path_str) = path.to_str() { - unsafe { - // Convert input to wide string. - let wide_path: Vec = OsString::from(path_str).encode_wide().chain(Some(0)).collect(); - if SetCurrentDirectoryW(wide_path.as_ptr()) != 0 { - println!("Successfully changed the current directory to {}", path_str); - return Ok(()) - } else { - return - Err(DrivePwdError::SystemError) - - }; - } - } - Err(DrivePwdError::InvalidPath) - } - - #[cfg(not(target_os = "windows"))] - pub fn set_current_directory_windows(_path: &Path) -> Result<(), Error>{ - Ok(()) - } -} \ No newline at end of file diff --git a/crates/nu-path/src/tilde.rs b/crates/nu-path/src/tilde.rs index 360810b86c..fe44fa07aa 100644 --- a/crates/nu-path/src/tilde.rs +++ b/crates/nu-path/src/tilde.rs @@ -21,21 +21,11 @@ fn expand_tilde_with_home(path: impl AsRef, home: Option) -> Path let path = path.as_ref(); if !path.starts_with("~") { - use crate::state_driver_pwd:: { - current_directory_specific::need_expand_current_directory_per_drive, - //current_directory_specific::get_windows_absolute_path, - get_drive_pwd_map + use crate::pwd_per_drive:: { + pwd_per_drive::expand_pwd_per_drive, }; - if need_expand_current_directory_per_drive(path) { - if let Some(expanded_dir) = get_drive_pwd_map().lock().unwrap().expand_path(path) { - return PathBuf::from(&expanded_dir) - } - // if let Some(current_dir) = get_windows_absolute_path(path) { - // //println!("Absolute path for {} is: {}", path.display(), current_dir); - // return PathBuf::from(¤t_dir) - // } else { - // println!("Failed to get absolute path for {}", path.display()); - // } + if let Some(expanded_dir) = expand_pwd_per_drive(path) { + return expanded_dir; } let string = path.to_string_lossy(); let mut path_as_string = string.as_ref().bytes();