Repository: reubeno/brush
Pull request #458 (2025-05-04 04:27:16)

feat: implement {cd,pwd} -{L,P}
This integrates the compat tests, clap annotation updates, and some of the core builtin updates from #219.

There are 2 notable differences with this PR's approach:

* In place of a custom implementation of path normalization, this PR instead opts for adopting the [`normalize-path`](https://crates.io/crates/normalize-path) crate with the acknowledged trade-off that this latter crate likely does *not* necessarily handle Windows paths.

* This version does not persistently maintain a logical and physical representation of the current working dir. Rather, it maintains the existing single representation, and only computes physical paths on-demand when required. We still may very well need to go with maintaining both, but for now this keeps the change more minimal and seems to be more in line with handling the edge case of how, for example, `pwd -P` should behave on a directory that has been renamed/moved/altered since being `cd`'d into.

We will leave the older PR open for now, and re-evaluate how best to preserve/use its Windows-specific logic in subsequent changes.

_(Note: @39555 -- the commit tags you as a contributor because you had done the heavy lifting of identifying the work needed and also because this PR directly incorporates some of your changes from the above-linked PR.)_

Resolves: #202.

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.10 μs` | `   18.81 μs` | `    0.71 μs` | `🟠 +3.94%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.87 μs` | `    1.83 μs` | `   -0.05 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   22.97 μs` | `   22.04 μs` | `   -0.93 μs` | `🟢 -4.04%    ` |
| `function_call                       ` | `    2.47 μs` | `    2.40 μs` | `   -0.08 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   57.10 μs` | `   56.84 μs` | `   -0.25 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `24232.32 μs` | `23975.26 μs` | ` -257.06 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1665.57 μs` | ` 1667.42 μs` | `    1.85 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.76 μs` | `    1.73 μs` | `   -0.02 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   15.32 μs` | `   15.19 μs` | `   -0.12 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2338.50 μs` | ` 2397.61 μs` | `   59.11 μs` | `⚪  Unchanged` |
| `tokenize_sample_script              ` | `    2.89 μs` | `    2.84 μs` | `   -0.05 μs` | `🟢 -1.76%    ` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/cd.rs | 🟢 81.82% | 🟢 82.35% | 🟢 0.53% |
| brush-core/src/builtins/pwd.rs | 🟢 80.95% | 🟢 88.89% | 🟢 7.94% |
| brush-core/src/shell.rs | 🟢 82.99% | 🟢 83.01% | 🟢 0.02% |
| **Overall Coverage** | **🟢 76.39%** | **🟢 76.41%** | **🟢 0.02%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `76.41%`

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1289            | <span style='color:green'>61.12</span> |
| ❗️ Error           | 226           | <span style='color:red'>10.72</span> |
| ❌ Fail            | 200            | <span style='color:red'>9.48</span> |
| ⏩ Skip            | 379            | 17.97 |
| ❎ Expected Fail   | 14   | 0.66 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2109           | 100.00 |

<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 59671b4..aa2701c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -279,6 +279,7 @@ dependencies = [
  "itertools 0.14.0",
  "lazy_static",
  "nix 0.30.0",
+ "normalize-path",
  "os_pipe",
  "pprof",
  "procfs",
@@ -1580,6 +1581,12 @@ dependencies = [
  "libc",
 ]
 
+[[package]]
+name = "normalize-path"
+version = "0.2.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "f5438dd2b2ff4c6df6e1ce22d825ed2fa93ee2922235cc45186991717f0a892d"
+
 [[package]]
 name = "nu-ansi-term"
 version = "0.46.0"
diff --git a/brush-core/Cargo.toml b/brush-core/Cargo.toml
index c7dd8b8..56ef62f 100644
--- a/brush-core/Cargo.toml
+++ b/brush-core/Cargo.toml
@@ -29,6 +29,7 @@ futures = "0.3.31"
 indexmap = "2.9.0"
 itertools = "0.14.0"
 lazy_static = "1.5.0"
+normalize-path = "0.2.1"
 rand = "0.9.1"
 thiserror = "2.0.12"
 tracing = "0.1.41"
diff --git a/brush-core/src/builtins/cd.rs b/brush-core/src/builtins/cd.rs
index ed932e5..3226402 100644
--- a/brush-core/src/builtins/cd.rs
+++ b/brush-core/src/builtins/cd.rs
@@ -3,17 +3,17 @@ use std::path::PathBuf;
 
 use clap::Parser;
 
-use crate::{builtins, commands};
+use crate::{builtins, commands, error};
 
 /// Change the current shell working directory.
 #[derive(Parser)]
 pub(crate) struct CdCommand {
     /// Force following symlinks.
-    #[arg(short = 'L')]
+    #[arg(short = 'L', overrides_with = "use_physical_dir")]
     force_follow_symlinks: bool,
 
     /// Use physical dir structure without following symlinks.
-    #[arg(short = 'P')]
+    #[arg(short = 'P', overrides_with = "force_follow_symlinks")]
     use_physical_dir: bool,
 
     /// Exit with non zero exit status if current working directory resolution fails.
@@ -34,18 +34,14 @@ impl builtins::Command for CdCommand {
     async fn execute(
         &self,
         context: commands::ExecutionContext<'_>,
-    ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
-        // TODO: implement options
-        if self.force_follow_symlinks
-            || self.use_physical_dir
-            || self.exit_on_failed_cwd_resolution
-            || self.file_with_xattr_as_dir
-        {
-            return crate::error::unimp("options to cd");
+    ) -> Result<builtins::ExitCode, error::Error> {
+        // TODO: implement 'cd -@'
+        if self.file_with_xattr_as_dir {
+            return error::unimp("cd -@");
         }
 
         let mut should_print = false;
-        let target_dir = if let Some(target_dir) = &self.target_dir {
+        let mut target_dir = if let Some(target_dir) = &self.target_dir {
             // `cd -', equivalent to `cd $OLDPWD'
             if target_dir.as_os_str() == "-" {
                 should_print = true;
@@ -69,6 +65,20 @@ impl builtins::Command for CdCommand {
             }
         };
 
+        if self.use_physical_dir
+            || context
+                .shell
+                .options
+                .do_not_resolve_symlinks_when_changing_dir
+        {
+            // -e is only relevant in physical mode.
+            if self.exit_on_failed_cwd_resolution {
+                return error::unimp("cd -e");
+            }
+
+            target_dir = context.shell.get_absolute_path(target_dir).canonicalize()?;
+        }
+
         if let Err(e) = context.shell.set_working_dir(&target_dir) {
             writeln!(context.stderr(), "cd: {e}")?;
             return Ok(builtins::ExitCode::Custom(1));
diff --git a/brush-core/src/builtins/pwd.rs b/brush-core/src/builtins/pwd.rs
index 8ad06db..2466398 100644
--- a/brush-core/src/builtins/pwd.rs
+++ b/brush-core/src/builtins/pwd.rs
@@ -1,16 +1,16 @@
 use crate::{builtins, commands};
 use clap::Parser;
-use std::io::Write;
+use std::{borrow::Cow, io::Write, path::Path};
 
 /// Display the current working directory.
 #[derive(Parser)]
 pub(crate) struct PwdCommand {
     /// Print the physical directory without any symlinks.
-    #[arg(short = 'P')]
+    #[arg(short = 'P', overrides_with = "allow_symlinks")]
     physical: bool,
 
     /// Print $PWD if it names the current working directory.
-    #[arg(short = 'L')]
+    #[arg(short = 'L', overrides_with = "physical")]
     allow_symlinks: bool,
 }
 
@@ -19,19 +19,19 @@ impl builtins::Command for PwdCommand {
         &self,
         context: commands::ExecutionContext<'_>,
     ) -> Result<crate::builtins::ExitCode, crate::error::Error> {
-        //
-        // TODO: implement flags
-        // TODO: look for 'physical' option in execution context
-        //
+        let mut cwd: Cow<'_, Path> = context.shell.working_dir.as_path().into();
 
-        if self.physical || self.allow_symlinks {
-            writeln!(context.stderr(), "UNIMPLEMENTED: pwd with -P or -L")?;
-            return Ok(builtins::ExitCode::Unimplemented);
-        }
+        let should_canonicalize = self.physical
+            || context
+                .shell
+                .options
+                .do_not_resolve_symlinks_when_changing_dir;
 
-        let cwd = context.shell.working_dir.to_string_lossy().into_owned();
+        if should_canonicalize {
+            cwd = cwd.canonicalize()?.into();
+        }
 
-        writeln!(context.stdout(), "{cwd}")?;
+        writeln!(context.stdout(), "{}", cwd.to_string_lossy())?;
 
         Ok(builtins::ExitCode::Success)
     }
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
index a74975c..1b6b13b 100644
--- a/brush-core/src/shell.rs
+++ b/brush-core/src/shell.rs
@@ -5,6 +5,7 @@ use std::io::{Read, Write};
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
+use normalize_path::NormalizePath;
 use rand::Rng;
 
 use crate::arithmetic::Evaluatable;
@@ -1492,8 +1493,8 @@ impl Shell {
             }
         }
 
-        // TODO: Don't canonicalize, just normalize.
-        let cleaned_path = abs_path.canonicalize()?;
+        // Normalize the path (but don't canonicalize it).
+        let cleaned_path = abs_path.normalize();
 
         let pwd = cleaned_path.to_string_lossy().to_string();
 


Test patch:
diff --git a/brush-shell/tests/cases/builtins/cd.yaml b/brush-shell/tests/cases/builtins/cd.yaml
index 67b860f..d517a77 100644
--- a/brush-shell/tests/cases/builtins/cd.yaml
+++ b/brush-shell/tests/cases/builtins/cd.yaml
@@ -41,3 +41,100 @@ cases:
       echo $?
       echo "pwd: $PWD"
 
+
+  - name: "cd ~"
+    stdin: |
+      mkdir ./my_home
+      export HOME="$(realpath ./my_home)"
+      (
+        cd ~
+        echo "pwd: $(basename $PWD)"
+      )
+      (
+        cd -L ~
+        echo "pwd: $(basename $PWD)"
+      )
+      (
+        cd -P ~
+        echo "pwd: $(basename $PWD)"
+      )
+
+
+  - name: "cd with symlink"
+    stdin: |
+      mkdir -p ./level1/level2/level3
+      cd level1
+      ln -s ./level2/level3 ./symlink
+
+      # -L by default
+      (
+        cd ./symlink
+        echo "$(basename $PWD)"
+        cd ..
+        echo "$(basename $PWD)"
+      )
+
+  - name: "cd -L"
+    stdin: |
+      mkdir -p ./level1/level2/level3
+      cd level1
+      ln -s ./level2/level3 ./symlink
+
+      echo "[Case 1]"
+      (
+        cd ./symlink
+        echo "$(basename $PWD)"
+        cd -L ..
+        echo "$(basename $PWD)"
+      )
+
+      echo "[Case 2]"
+      (
+        cd -L ./symlink
+        echo "$(basename $PWD)"
+        cd ..
+        echo "$(basename $PWD)"
+      )
+
+      # without pwd
+      echo "[Case 3]"
+      (
+        cd -L ./symlink
+        echo "$(basename $PWD)"
+        export PWD=
+        cd -L ..
+        echo "$(basename $PWD)"
+      )
+
+      echo "[Case 4]"
+      (
+        cd -L ./symlink
+        export PWD=
+        # start a shell without $PWD
+        (
+          cd .
+          echo "$(basename $PWD)"
+        )
+      )
+
+  - name: "cd -P"
+    stdin: |
+      mkdir -p ./level1/level2/level3
+      cd level1
+      ln -s ./level2/level3 ./symlink
+
+      echo "[Case 1]"
+      (
+        cd ./symlink
+        echo "cd ./symlink => $(basename $PWD)"
+        cd -P ..
+        echo "cd -P .. => $(basename $PWD)"
+      )
+
+      echo "[Case 2]"
+      (
+        cd -P ./symlink
+        echo "cd -P ./symlink => $(basename $PWD)"
+        cd ..
+        echo "cd .. => $(basename $PWD)"
+      )
diff --git a/brush-shell/tests/cases/builtins/pwd.yaml b/brush-shell/tests/cases/builtins/pwd.yaml
index c216e0b..7a517b8 100644
--- a/brush-shell/tests/cases/builtins/pwd.yaml
+++ b/brush-shell/tests/cases/builtins/pwd.yaml
@@ -9,3 +9,62 @@ cases:
       cd usr
       pwd
       echo "Result: $?"
+
+  - name: "pwd -LP"
+    stdin: |
+      mkdir -p ./level1/level2/level3
+      cd level1
+      ln -s ./level2/level3 ./symlink
+
+      (
+        cd ./symlink
+        basename $(pwd)
+        basename $(pwd -L)
+        basename $(pwd -P)
+      )
+      (
+        cd ./symlink
+        export PWD=
+        basename $(pwd)
+        basename $(pwd -L)
+        basename $(pwd -P)
+      )
+      (
+        cd ./symlink
+        export PWD=
+        # start a shell without $PWD
+        (
+          basename $(pwd)
+          basename $(pwd -L)
+          basename $(pwd -P)
+        )
+      )
+
+      cd ~
+      pwd
+      pwd -L
+      pwd -P
+
+  - name: "pwd with moved dir"
+    known_failure: true # Needs investigation
+    stdin: |
+      root=$(pwd)
+
+      mkdir -p ${root}/subdir
+      cd ${root}/subdir
+      mv ${root}/subdir ${root}/renamed
+
+      echo "pwd -L: $(basename $(pwd -L))"
+      echo "pwd -P: $(basename $(pwd -P))"
+
+  - name: "pwd with removed dir"
+    known_failure: true # Needs investigation
+    stdin: |
+      root=$(pwd)
+
+      mkdir -p ${root}/subdir
+      cd ${root}/subdir
+      rmdir ${root}/subdir
+
+      echo "pwd -L: $(basename $(pwd -L))"
+      echo "pwd -P: $(basename $(pwd -P))"