Repository: reubeno/brush
Pull request #402 (2025-03-11 02:37:54)

feat: implement `command -p`
Rebases #200 against latest changes in tree + adapts to move platform-specific code under `sys`. Also adds a couple of basic compat tests to cover the functionality.

_Huge credit to @39555 for sorting out the problem and getting this working! I've created this under a separate PR to avoid stomping on the previous branch._

github-actions[bot]: ## Test Results
    2 files      9 suites   1m 53s ⏱️
  694 tests   694 ✅ 0 💤 0 ❌
1 374 runs  1 374 ✅ 0 💤 0 ❌

Results for commit 62bd2e7e.

[test-results]:data:application/gzip;base64,H4sIANqkz2cC/13MyQ7CIBSF4VdpWLuQQQZfxgAXEmJbDMOq8d2lpNXW5fef5CzIh9FldB/IZUC5htKhGqAmXUKcGzGmLbSprCNXbNcjV2v/0zO8Wrp+g9dhPAWXUkxbSXVePzEVbNP+eUq/z+7DZffx0cZpCqUBcWKAOOEAK240UGDE36g2oJgUkhFjvPPSSvT+AFwlGaoHAQAA

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   18.18 μs` | `   18.01 μs` | `   -0.16 μs` | `🟢 -0.89%    ` |
| `eval_arithmetic                     ` | `    0.17 μs` | `    0.17 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `expand_one_string                   ` | `    1.80 μs` | `    1.80 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   21.68 μs` | `   22.86 μs` | `    1.19 μs` | `🟠 +5.48%    ` |
| `function_call                       ` | `    2.28 μs` | `    2.26 μs` | `   -0.02 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   56.92 μs` | `   56.84 μs` | `   -0.08 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `23286.44 μs` | `23645.17 μs` | `  358.73 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 1642.07 μs` | ` 1651.85 μs` | `    9.78 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.74 μs` | `    1.75 μs` | `    0.01 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   14.85 μs` | `   14.96 μs` | `    0.12 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2322.64 μs` | ` 3840.97 μs` | ` 1518.33 μs` | `🟠 +65.37%   ` |
| `tokenize_sample_script              ` | `    2.86 μs` | `    2.80 μs` | `   -0.06 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/builtins/command.rs | 🟢 91.01% | 🟢 94.12% | 🟢 3.11% |
| brush-core/src/commands.rs | 🟢 86.97% | 🟢 87.24% | 🟢 0.27% |
| brush-core/src/jobs.rs | 🔴 37.07% | 🔴 43.1% | 🟢 6.03% |
| brush-core/src/shell.rs | 🟢 85.87% | 🟢 86.33% | 🟢 0.46% |
| brush-core/src/sys/unix/fs.rs | 🟠 64.1% | 🟠 69.15% | 🟢 5.05% |
| brush-core/src/sys/unix/signal.rs | 🟠 61.02% | 🟠 64.41% | 🟢 3.39% |
| **Overall Coverage** | **🟢 78.16%** | **🟢 78.31%** | **🟢 0.15%** |

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

# Test Summary: bash-completion test suite
| Outcome            | Count                   | Percentage |
| ------------------ | ----------------------: | ---------: |
| ✅ Pass            | 1340            | <span style='color:green'>63.81</span> |
| ❗️ Error           | 222           | <span style='color:red'>10.57</span> |
| ❌ Fail            | 160            | <span style='color:red'>7.62</span> |
| ⏩ Skip            | 364            | 17.33 |
| ❎ Expected Fail   | 13   | 0.62 |
| ✔️ Unexpected Pass | 1 | <span style='color:red'>0.05</span> |
| 📊 Total           | 2100           | 100.00 |

<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/builtins/command.rs b/brush-core/src/builtins/command.rs
index 7f7c906..95f6e35 100644
--- a/brush-core/src/builtins/command.rs
+++ b/brush-core/src/builtins/command.rs
@@ -1,7 +1,7 @@
 use clap::Parser;
 use std::{fmt::Display, io::Write, path::Path};
 
-use crate::{builtins, commands, error, shell, sys::fs::PathExt, ExecutionResult};
+use crate::{builtins, commands, error, shell, sys, sys::fs::PathExt, ExecutionResult};
 
 /// Directly invokes an external command, without going through typical search order.
 #[derive(Parser)]
@@ -36,14 +36,12 @@ impl builtins::Command for CommandCommand {
     ) -> Result<builtins::ExitCode, error::Error> {
         // Silently exit if no command was provided.
         if let Some(command_name) = self.command() {
-            if self.use_default_path {
-                return error::unimp("command -p");
-            }
-
             if self.print_description || self.print_verbose_description {
-                if let Some(found_cmd) =
-                    Self::try_find_command(context.shell, command_name.as_str())
-                {
+                if let Some(found_cmd) = Self::try_find_command(
+                    context.shell,
+                    command_name.as_str(),
+                    self.use_default_path,
+                ) {
                     if self.print_description {
                         writeln!(context.stdout(), "{found_cmd}")?;
                     } else {
@@ -64,7 +62,8 @@ impl builtins::Command for CommandCommand {
                     Ok(builtins::ExitCode::Custom(1))
                 }
             } else {
-                self.execute_command(context, command_name).await
+                self.execute_command(context, command_name, self.use_default_path)
+                    .await
             }
         } else {
             Ok(builtins::ExitCode::Success)
@@ -88,7 +87,11 @@ impl Display for FoundCommand {
 
 impl CommandCommand {
     #[allow(clippy::unwrap_in_result)]
-    fn try_find_command(shell: &mut shell::Shell, command_name: &str) -> Option<FoundCommand> {
+    fn try_find_command(
+        shell: &mut shell::Shell,
+        command_name: &str,
+        use_default_path: bool,
+    ) -> Option<FoundCommand> {
         // Look in path.
         if command_name.contains(std::path::MAIN_SEPARATOR) {
             let candidate_path = shell.get_absolute_path(Path::new(command_name));
@@ -110,9 +113,17 @@ impl CommandCommand {
                 }
             }
 
-            shell
-                .find_first_executable_in_path_using_cache(command_name)
-                .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
+            if use_default_path {
+                let dirs = sys::fs::get_default_standard_utils_paths();
+                shell
+                    .find_executables_in(dirs.iter(), command_name)
+                    .first()
+                    .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
+            } else {
+                shell
+                    .find_first_executable_in_path_using_cache(command_name)
+                    .map(|path| FoundCommand::External(path.to_string_lossy().to_string()))
+            }
         }
     }
 
@@ -120,10 +131,17 @@ impl CommandCommand {
         &self,
         mut context: commands::ExecutionContext<'_>,
         command_name: &str,
+        use_default_path: bool,
     ) -> Result<builtins::ExitCode, error::Error> {
         command_name.clone_into(&mut context.command_name);
         let command_and_args = self.command_and_args.iter().map(|arg| arg.into()).collect();
 
+        let path_dirs = if use_default_path {
+            Some(sys::fs::get_default_standard_utils_paths())
+        } else {
+            None
+        };
+
         // We do not have an existing process group to place this into.
         let mut pgid = None;
 
@@ -134,6 +152,7 @@ impl CommandCommand {
             &mut pgid,
             command_and_args,
             false, /* use functions? */
+            path_dirs,
         )
         .await?
         {
diff --git a/brush-core/src/commands.rs b/brush-core/src/commands.rs
index ee3edbd..c75c4b3 100644
--- a/brush-core/src/commands.rs
+++ b/brush-core/src/commands.rs
@@ -273,6 +273,7 @@ pub(crate) async fn execute(
     process_group_id: &mut Option<i32>,
     args: Vec<CommandArg>,
     use_functions: bool,
+    path_dirs: Option<Vec<String>>,
 ) -> Result<CommandSpawnResult, error::Error> {
     if !cmd_context.command_name.contains(std::path::MAIN_SEPARATOR) {
         let builtin = cmd_context
@@ -307,10 +308,19 @@ pub(crate) async fn execute(
             }
         }
 
-        if let Some(path) = cmd_context
-            .shell
-            .find_first_executable_in_path_using_cache(&cmd_context.command_name)
-        {
+        let path = if let Some(path_dirs) = path_dirs {
+            cmd_context
+                .shell
+                .find_executables_in(path_dirs.iter(), &cmd_context.command_name)
+                .first()
+                .cloned()
+        } else {
+            cmd_context
+                .shell
+                .find_first_executable_in_path_using_cache(&cmd_context.command_name)
+        };
+
+        if let Some(path) = path {
             let resolved_path = path.to_string_lossy();
             execute_external_command(
                 cmd_context,
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index f64f33d..9014ed9 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -1116,6 +1116,7 @@ impl ExecuteInPipeline for ast::SimpleCommand {
                 &mut context.process_group_id,
                 args,
                 true, /* use functions? */
+                None,
             )
             .await;
 
diff --git a/brush-core/src/shell.rs b/brush-core/src/shell.rs
index bb6eaca..a0cd271 100644
--- a/brush-core/src/shell.rs
+++ b/brush-core/src/shell.rs
@@ -540,12 +540,9 @@ impl Shell {
         // PATH (if not already set)
         #[cfg(unix)]
         if !self.env.is_set("PATH") {
-            self.env.set_global(
-                "PATH",
-                ShellVariable::new(
-                    "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin".into(),
-                ),
-            )?;
+            let default_path_str = sys::fs::get_default_executable_search_paths().join(":");
+            self.env
+                .set_global("PATH", ShellVariable::new(default_path_str.into()))?;
         }
 
         // PIPESTATUS
@@ -1291,12 +1288,33 @@ impl Shell {
     /// # Arguments
     ///
     /// * `required_glob_pattern` - The glob pattern to match against.
-    #[allow(clippy::manual_flatten)]
     pub fn find_executables_in_path(&self, required_glob_pattern: &str) -> Vec<PathBuf> {
+        self.find_executables_in(
+            self.env
+                .get_str("PATH", self)
+                .unwrap_or_default()
+                .split(':'),
+            required_glob_pattern,
+        )
+    }
+
+    /// Finds executables in the given paths, matching the given glob pattern.
+    ///
+    /// # Arguments
+    ///
+    /// * `paths` - The paths to search in
+    /// * `required_glob_pattern` - The glob pattern to match against.
+    #[allow(clippy::manual_flatten)]
+    pub fn find_executables_in<T: AsRef<str>>(
+        &self,
+        paths: impl Iterator<Item = T>,
+        required_glob_pattern: &str,
+    ) -> Vec<PathBuf> {
         let is_executable = |path: &Path| path.is_file() && path.executable();
 
         let mut executables = vec![];
-        for dir_str in self.get_env_str("PATH").unwrap_or_default().split(':') {
+        for dir_str in paths {
+            let dir_str = dir_str.as_ref();
             let pattern =
                 patterns::Pattern::from(std::format!("{dir_str}/{required_glob_pattern}"))
                     .set_extended_globbing(self.options.extended_globbing)
diff --git a/brush-core/src/sys/stubs/fs.rs b/brush-core/src/sys/stubs/fs.rs
index 22221cd..2ed31d6 100644
--- a/brush-core/src/sys/stubs/fs.rs
+++ b/brush-core/src/sys/stubs/fs.rs
@@ -52,3 +52,11 @@ pub(crate) trait StubMetadataExt {
 }
 
 impl StubMetadataExt for std::fs::Metadata {}
+
+pub(crate) fn get_default_executable_search_paths() -> Vec<String> {
+    vec![]
+}
+
+pub(crate) fn get_default_standard_utils_paths() -> Vec<String> {
+    vec![]
+}
diff --git a/brush-core/src/sys/unix/fs.rs b/brush-core/src/sys/unix/fs.rs
index c3710c4..e70259f 100644
--- a/brush-core/src/sys/unix/fs.rs
+++ b/brush-core/src/sys/unix/fs.rs
@@ -1,6 +1,19 @@
+use std::os::unix::ffi::OsStringExt;
 use std::os::unix::fs::{FileTypeExt, MetadataExt};
 use std::path::Path;
 
+const DEFAULT_EXECUTABLE_SEARCH_PATHS: &[&str] = &[
+    "/usr/local/sbin",
+    "/usr/local/bin",
+    "/usr/sbin",
+    "/usr/bin",
+    "/sbin",
+    "/bin",
+];
+
+const DEFAULT_STANDARD_UTILS_PATHS: &[&str] =
+    &["/bin", "/usr/bin", "/sbin", "/usr/sbin", "/etc", "/usr/etc"];
+
 impl crate::sys::fs::PathExt for Path {
     fn readable(&self) -> bool {
         nix::unistd::access(self, nix::unistd::AccessFlags::R_OK).is_ok()
@@ -56,3 +69,78 @@ fn try_get_file_type(path: &Path) -> Option<std::fs::FileType> {
 fn try_get_file_mode(path: &Path) -> Option<u32> {
     path.metadata().map(|metadata| metadata.mode()).ok()
 }
+
+pub(crate) fn get_default_executable_search_paths() -> Vec<String> {
+    DEFAULT_EXECUTABLE_SEARCH_PATHS
+        .iter()
+        .map(|s| (*s).to_owned())
+        .collect()
+}
+
+/// Retrieves the platform-specific set of paths that should contain standard system
+/// utilities. Used by `command -p`, for example.
+pub(crate) fn get_default_standard_utils_paths() -> Vec<String> {
+    //
+    // Try to call confstr(_CS_PATH). If that fails, can't find a string value, or
+    // finds an empty string, then we'll fall back to hard-coded defaults.
+    //
+
+    if let Ok(Some(cs_path)) = confstr_cs_path() {
+        if !cs_path.is_empty() {
+            return cs_path.split(':').map(|s| s.to_string()).collect();
+        }
+    }
+
+    DEFAULT_STANDARD_UTILS_PATHS
+        .iter()
+        .map(|s| (*s).to_owned())
+        .collect()
+}
+
+fn confstr_cs_path() -> Result<Option<String>, std::io::Error> {
+    let value = confstr(nix::libc::_CS_PATH)?;
+
+    if let Some(value) = value {
+        let value_str = value
+            .into_string()
+            .map_err(|_err| std::io::Error::new(std::io::ErrorKind::InvalidData, "Invalid data"))?;
+        Ok(Some(value_str))
+    } else {
+        Ok(None)
+    }
+}
+
+/// A wrapper for [`nix::libc::confstr`]. Returns a value for the default PATH variable which
+/// indicates where all the POSIX.2 standard utilities can be found.
+///
+/// N.B. We would strongly prefer to use a safe API exposed (in an idiomatic way) by nix
+/// or similar. Until that exists, we accept the need to make the unsafe call directly.
+fn confstr(name: nix::libc::c_int) -> Result<Option<std::ffi::OsString>, std::io::Error> {
+    let required_size = unsafe { nix::libc::confstr(name, std::ptr::null_mut(), 0) };
+
+    // When confstr returns 0, it either means there's no value associated with _CS_PATH, or
+    // _CS_PATH is considered invalid (and not present) on this platform. In both cases, we
+    // treat it as a non-existent value and return None.
+    if required_size == 0 {
+        return Ok(None);
+    }
+
+    let mut buffer = Vec::<u8>::with_capacity(required_size);
+
+    // NOTE: Writing `c_char` (i8 or u8 depending on the platform) into `Vec<u8>` is fine,
+    // as i8 and u8 have compatible representations,
+    // and Rust does not support platforms where `c_char` is not 8-bit wide.
+    let final_size =
+        unsafe { nix::libc::confstr(name, buffer.as_mut_ptr().cast(), buffer.capacity()) };
+
+    if final_size == 0 {
+        return Err(std::io::Error::last_os_error());
+    }
+
+    unsafe { buffer.set_len(final_size) };
+
+    // The last byte is a null terminator.
+    buffer.pop();
+
+    Ok(Some(std::ffi::OsString::from_vec(buffer)))
+}


Test patch:
diff --git a/brush-shell/tests/cases/builtins/command.yaml b/brush-shell/tests/cases/builtins/command.yaml
index 0823c17..7d3efe0 100644
--- a/brush-shell/tests/cases/builtins/command.yaml
+++ b/brush-shell/tests/cases/builtins/command.yaml
@@ -31,6 +31,16 @@ cases:
       echo "[/usr/bin/non-existent]"
       command -v /usr/bin/non-existent || echo "2. Not found"
 
+  - name: "command -v -p"
+    stdin: |
+      unset PATH
+
+      echo "[no -p]"
+      command -v cat
+
+      echo "[-p]"
+      command -v -p cat
+
   - name: "command -v with full paths"
     skip: true # TODO: investigate why this fails on arch linux
     stdin: |
@@ -57,3 +67,9 @@ cases:
   - name: "command with --help"
     stdin: |
       command ls --help
+
+  - name: "command with -p"
+    stdin: |
+      unset PATH
+
+      command -p -- ls