From 86f12ffe619ba8ff6f64a9dc2c9db19a01c2b802 Mon Sep 17 00:00:00 2001 From: Bram Geron Date: Fri, 16 Jun 2023 10:42:50 +0200 Subject: [PATCH] Fix bug in std dirs drop; improve documentation (#9449) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description When the directory stack contains only two directories, then std dirs drop used to misbehave and (essentially) drop the other directory, instead of dropping the current directory. This is fixed by always cd'ing for std dirs drop. Before: /tmp〉enter .. /〉dexit /〉 After: /tmp〉enter .. /〉dexit /tmp〉 Additionally, I propose to explain the relevant environment variables a bit more thoroughly. # User-Facing Changes - Fix bug in dexit (std dirs drop) when two directories are remaining # Tests + Formatting Added a regression test. Made the existing test easier to understand. --- crates/nu-std/std/dirs.nu | 41 ++++++++++++++++++++++---------- crates/nu-std/tests/test_dirs.nu | 12 +++++++++- 2 files changed, 39 insertions(+), 14 deletions(-) diff --git a/crates/nu-std/std/dirs.nu b/crates/nu-std/std/dirs.nu index 315441993..c8bc14827 100644 --- a/crates/nu-std/std/dirs.nu +++ b/crates/nu-std/std/dirs.nu @@ -1,8 +1,21 @@ # Maintain a list of working directories and navigate them -# the directory stack -# current slot is DIRS_POSITION, but that entry doesn't hold $PWD (until leaving it for some other) -# till then, we let CD change PWD freely +# The directory stack. +# +# Exception: the entry for the current directory contains an +# irrelevant value. Instead, the source of truth for the working +# directory is $env.PWD. It has to be this way because cd doesn't +# know about this module. +# +# Example: the following state represents a user-facing directory +# stack of [/a, /var/tmp, /c], and we are currently in /var/tmp . +# +# PWD = /var/tmp +# DIRS_POSITION = 1 +# DIRS_LIST = [/a, /b, /c] +# +# This situation could arise if we started with [/a, /b, /c], then +# we changed directories from /b to /var/tmp. export-env { let-env DIRS_POSITION = 0 let-env DIRS_LIST = [($env.PWD | path expand)] @@ -20,9 +33,9 @@ export def-env add [ let span = (metadata $p).span error make {msg: "not a directory", label: {text: "not a directory", start: $span.start, end: $span.end } } } - $abspaths = ($abspaths | append $exp) - + $abspaths = ($abspaths | append $exp) } + let-env DIRS_LIST = ($env.DIRS_LIST | insert ($env.DIRS_POSITION + 1) $abspaths | flatten) @@ -35,7 +48,7 @@ export alias enter = add export def-env next [ N:int = 1 # number of positions to move. ] { - _fetch $N + _fetch $N } export alias n = next @@ -44,7 +57,7 @@ export alias n = next export def-env prev [ N:int = 1 # number of positions to move. ] { - _fetch (-1 * $N) + _fetch (-1 * $N) } export alias p = prev @@ -57,7 +70,8 @@ export def-env drop [] { if ($env.DIRS_POSITION >= ($env.DIRS_LIST | length)) {$env.DIRS_POSITION = 0} } - _fetch -1 --forget_current # step to previous slot + # step to previous slot + _fetch -1 --forget_current --always_cd } @@ -69,8 +83,8 @@ export def-env show [] { for $p in ($env.DIRS_LIST | enumerate) { let is_act_slot = $p.index == $env.DIRS_POSITION $out = ($out | append [ - [active, path]; - [($is_act_slot), + [active, path]; + [($is_act_slot), (if $is_act_slot {$env.PWD} else {$p.item}) # show current PWD in lieu of active slot ] ]) @@ -105,9 +119,10 @@ export def-env goto [shell?: int] { export alias g = goto # fetch item helper -def-env _fetch [ +def-env _fetch [ offset: int, # signed change to position --forget_current # true to skip saving PWD + --always_cd # true to always cd ] { if not ($forget_current) { # first record current working dir in current slot of ring, to track what CD may have done. @@ -116,13 +131,13 @@ def-env _fetch [ # figure out which entry to move to # nushell 'mod' operator is really 'remainder', can return negative values. - # see: https://stackoverflow.com/questions/13683563/whats-the-difference-between-mod-and-remainder + # see: https://stackoverflow.com/questions/13683563/whats-the-difference-between-mod-and-remainder let len = ($env.DIRS_LIST | length) mut pos = ($env.DIRS_POSITION + $offset) mod $len if ($pos < 0) { $pos += $len} # if using a different position in ring, CD there. - if ($pos != $env.DIRS_POSITION) { + if ($always_cd or $pos != $env.DIRS_POSITION) { $env.DIRS_POSITION = $pos cd ($env.DIRS_LIST | get $pos ) } diff --git a/crates/nu-std/tests/test_dirs.nu b/crates/nu-std/tests/test_dirs.nu index 895417062..a2c847b59 100644 --- a/crates/nu-std/tests/test_dirs.nu +++ b/crates/nu-std/tests/test_dirs.nu @@ -14,7 +14,7 @@ def before-each [] { mkdir $base_path $path_a $path_b - {base_path: $base_path, path_a:$path_a, path_b: $path_b} + {base_path: $base_path, path_a: $path_a, path_b: $path_b} } def after-each [] { @@ -46,6 +46,7 @@ def test_dirs_command [] { # the def-env gets messed up use std dirs + # Stack: [BASE] assert equal [$c.base_path] $env.DIRS_LIST "list is just pwd after initialization" dirs next @@ -54,24 +55,33 @@ def test_dirs_command [] { dirs prev assert equal $c.base_path $env.DIRS_LIST.0 "prev wraps at top of list" + # Stack becomes: [base PATH_B path_a] dirs add $c.path_b $c.path_a assert equal $c.path_b $env.PWD "add changes PWD to first added dir" assert length $env.DIRS_LIST 3 "add in fact adds to list" assert equal $c.path_a $env.DIRS_LIST.2 "add in fact adds to list" + # Stack becomes: [BASE path_b path_a] dirs next 2 # assert (not) equal requires span.start of first arg < span.end of 2nd assert equal $env.PWD $c.base_path "next wraps at end of list" + # Stack becomes: [base path_b PATH_A] dirs prev 1 assert equal $c.path_a $env.PWD "prev wraps at start of list" cur_dir_check $c.path_a "prev wraps to end from start of list" + # Stack becomes: [base PATH_B] dirs drop assert length $env.DIRS_LIST 2 "drop removes from list" assert equal $env.PWD $c.path_b "drop changes PWD to previous in list (before dropped element)" assert equal (dirs show) [[active path]; [false $c.base_path] [true $c.path_b]] "show table contains expected information" + + # Stack becomes: [BASE] + dirs drop + assert length $env.DIRS_LIST 1 "drop removes from list" + assert equal $env.PWD $c.base_path "drop changes PWD (regression test for #9449)" } def test_dirs_next [] {