Repository: reubeno/brush
Pull request #205 (2024-10-15 02:33:30)

fix: slight compat improvements to set -x
Does a better job of single-quoting words that need it in simple commands (and some cases of extended test expressions.

github-actions[bot]: ## Test Results
  2 files    9 suites   59s :stopwatch:
454 tests 454 :white_check_mark: 0 :zzz: 0 :x:
899 runs  899 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 48307415.

[test-results]:data:application/gzip;base64,H4sIAFfWDWcC/13MSw7CIBSF4a00jB2A5bbgZgyBS0Jsi+Exaty7QFpFh99/krMT6xaM5DZcLwOJ2aUGWWByUMn5rRCqy5LqxoGfuses9X96uGdJ9BOscstPwBB8OErIW/0UUh44L/vyfWzuDpv7P+3X1aUCwsVIZ85AWW5GisgEAtcTk5Raw2YDE4BFM5LXG3aaGFwEAQAA

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `expand_one_string                   ` | `    3.34 μs` | `    3.34 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   59.39 μs` | `   59.98 μs` | `    0.59 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `29538.83 μs` | `30428.40 μs` | `  889.57 μs` | `🟠 +3.01%    ` |
| `parse_bash_completion               ` | ` 2698.74 μs` | ` 2711.37 μs` | `   12.63 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    3.93 μs` | `    3.96 μs` | `    0.03 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   90.24 μs` | `   91.64 μs` | `    1.40 μs` | `⚪  Unchanged` |
| `run_one_builtin_command             ` | `  107.96 μs` | `  108.65 μs` | `    0.70 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 1916.21 μs` | ` 1915.01 μs` | `   -1.20 μs` | `⚪  Unchanged` |
| `run_one_external_command_directly   ` | ` 1006.93 μs` | ` 1006.30 μs` | `   -0.64 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/escape.rs | 🟢 85.95% | 🟢 86.91% | 🟢 0.96% |
| brush-core/src/extendedtests.rs | 🟠 69.9% | 🟠 70.55% | 🟢 0.65% |
| brush-core/src/shell.rs | 🟢 81.95% | 🟢 81.8% | 🔴 -0.15% |
| brush-core/src/variables.rs | 🟢 83.22% | 🟢 83.12% | 🔴 -0.1% |
| brush-parser/src/ast.rs | 🔴 24.96% | 🔴 25.66% | 🟢 0.7% |
| **Overall Coverage** | **🟢 75.31%** | **🟢 75.36%** | **🟢 0.05%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `75.36%`
<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-core/src/escape.rs b/brush-core/src/escape.rs
index bfd68a1..8d6d09e 100644
--- a/brush-core/src/escape.rs
+++ b/brush-core/src/escape.rs
@@ -171,10 +171,10 @@ pub(crate) enum QuoteMode {
     Quote,
 }
 
-pub(crate) fn quote_if_needed(s: &str, mode: QuoteMode) -> String {
+pub(crate) fn quote_if_needed<S: AsRef<str>>(s: S, mode: QuoteMode) -> String {
     match mode {
-        QuoteMode::BackslashEscape => escape_with_backslash(s),
-        QuoteMode::Quote => escape_with_quoting(s),
+        QuoteMode::BackslashEscape => escape_with_backslash(s.as_ref()),
+        QuoteMode::Quote => escape_with_quoting(s.as_ref()),
     }
 }
 
@@ -197,7 +197,7 @@ fn escape_with_backslash(s: &str) -> String {
 
 fn escape_with_quoting(s: &str) -> String {
     // TODO: Handle single-quote!
-    if s.chars().any(needs_escaping) {
+    if s.is_empty() || s.chars().any(needs_escaping) {
         std::format!("'{s}'")
     } else {
         s.to_owned()
@@ -235,9 +235,17 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_escape() {
+    fn test_backslash_escape() {
         assert_eq!(quote_if_needed("a", QuoteMode::BackslashEscape), "a");
         assert_eq!(quote_if_needed("a b", QuoteMode::BackslashEscape), r"a\ b");
+        assert_eq!(quote_if_needed("", QuoteMode::BackslashEscape), "");
+    }
+
+    #[test]
+    fn test_quote_escape() {
+        assert_eq!(quote_if_needed("a", QuoteMode::Quote), "a");
+        assert_eq!(quote_if_needed("a b", QuoteMode::Quote), "'a b'");
+        assert_eq!(quote_if_needed("", QuoteMode::Quote), "''");
     }
 
     fn assert_echo_expands_to(unexpanded: &str, expected: &str) {
diff --git a/brush-core/src/variables.rs b/brush-core/src/variables.rs
index 1d5470c..d7478fa 100644
--- a/brush-core/src/variables.rs
+++ b/brush-core/src/variables.rs
@@ -3,7 +3,7 @@ use std::borrow::Cow;
 use std::collections::BTreeMap;
 use std::fmt::{Display, Write};
 
-use crate::error;
+use crate::{error, escape};
 
 /// A shell variable.
 #[derive(Clone, Debug)]
@@ -510,11 +510,10 @@ pub enum ShellValueLiteral {
     Array(ArrayLiteral),
 }
 
-impl Display for ShellValueLiteral {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+impl ShellValueLiteral {
+    pub(crate) fn fmt_for_tracing(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
-            // TODO: Decide if it needs to be quoted.
-            ShellValueLiteral::Scalar(s) => write!(f, "{s}"),
+            ShellValueLiteral::Scalar(s) => Self::fmt_scalar_for_tracing(s.as_str(), f),
             ShellValueLiteral::Array(elements) => {
                 write!(f, "(")?;
                 for (i, (key, value)) in elements.0.iter().enumerate() {
@@ -522,14 +521,27 @@ impl Display for ShellValueLiteral {
                         write!(f, " ")?;
                     }
                     if let Some(key) = key {
-                        write!(f, "[{key}]=")?;
+                        write!(f, "[")?;
+                        Self::fmt_scalar_for_tracing(key.as_str(), f)?;
+                        write!(f, "]=")?;
                     }
-                    write!(f, "{value}")?;
+                    Self::fmt_scalar_for_tracing(value.as_str(), f)?;
                 }
                 write!(f, ")")
             }
         }
     }
+
+    fn fmt_scalar_for_tracing(s: &str, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let processed = escape::quote_if_needed(s, escape::QuoteMode::Quote);
+        write!(f, "{processed}")
+    }
+}
+
+impl Display for ShellValueLiteral {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.fmt_for_tracing(f)
+    }
 }
 
 impl From<&str> for ShellValueLiteral {


Test patch:
diff --git a/brush-core/src/extendedtests.rs b/brush-core/src/extendedtests.rs
index 4fab511..ea3588d 100644
--- a/brush-core/src/extendedtests.rs
+++ b/brush-core/src/extendedtests.rs
@@ -2,8 +2,11 @@ use brush_parser::ast;
 use std::path::Path;
 
 use crate::{
-    env, error, expansion, namedoptions, patterns,
-    sys::{fs::MetadataExt, fs::PathExt, users},
+    env, error, escape, expansion, namedoptions, patterns,
+    sys::{
+        fs::{MetadataExt, PathExt},
+        users,
+    },
     variables::{self, ArrayLiteral},
     Shell,
 };
@@ -47,7 +50,10 @@ async fn apply_unary_predicate(
     let expanded_operand = expansion::basic_expand_word(shell, operand).await?;
 
     if shell.options.print_commands_and_arguments {
-        shell.trace_command(std::format!("[[ {op} {expanded_operand} ]]"))?;
+        shell.trace_command(std::format!(
+            "[[ {op} {} ]]",
+            escape::quote_if_needed(&expanded_operand, escape::QuoteMode::Quote)
+        ))?;
     }
 
     apply_unary_predicate_to_str(op, expanded_operand.as_str(), shell)
diff --git a/brush-shell/tests/cases/options.yaml b/brush-shell/tests/cases/options.yaml
index 9495c98..86129c7 100644
--- a/brush-shell/tests/cases/options.yaml
+++ b/brush-shell/tests/cases/options.yaml
@@ -39,9 +39,8 @@ cases:
         echo "Math checks"
       fi
 
-      # TODO: Re-enable these
-      # var=" "
-      # [[ ${var} && ! ${var//[[:space:]]/} ]]
+      var="x"
+      [[ ${var} && ${var//[[:space:]]/} ]]
 
       for ((i = 0; i < 3; i++)); do
         echo $i