From 146a1ade6d2cc8e82058c5e743bbf1b0ba420bfe Mon Sep 17 00:00:00 2001
From: Jon Parise <jon@indelible.org>
Date: Mon, 13 Jan 2025 11:17:21 -0500
Subject: [PATCH] Improve prior_trap processing

We use `trap` to bootstrap our installation function (__bp_install). We
remove our code upon first execution but need to restore any preexisting
trap calls. We previously used `sed` to process the trap string, but
that had two downsides:

1. `sed` is an external command dependency. It needs to exist on the
   system, and we need to invoke it in a subshell (which has some
   runtime cost).
2. The regular expression pattern was imperfect and didn't handle
   trickier cases like `'` characters in the trap string:

        $ (trap "echo 'hello'" DEBUG; trap -p DEBUG)
        hello
        trap -- 'echo '\''hello'\''' DEBUG

This change removes the dependency on `sed` by locally evaluating the
trap string and extracting any prior trap. This works reliably because
we control the format our trap string, which looks like this (with
newlines expanded):

    __bp_trap_string="$(trap -p DEBUG)"
    trap - DEBUG
    __bp_install
---
 bash-preexec.sh        |  6 ++----
 test/bash-preexec.bats | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/bash-preexec.sh b/bash-preexec.sh
index b998944..f6eae79 100644
--- a/bash-preexec.sh
+++ b/bash-preexec.sh
@@ -297,10 +297,8 @@ __bp_install() {
     trap '__bp_preexec_invoke_exec "$_"' DEBUG
 
     # Preserve any prior DEBUG trap as a preexec function
-    local prior_trap
-    # we can't easily do this with variable expansion. Leaving as sed command.
-    # shellcheck disable=SC2001
-    prior_trap=$(sed "s/[^']*'\(.*\)'[^']*/\1/" <<<"${__bp_trap_string:-}")
+    eval "local trap_argv=(${__bp_trap_string:-})"
+    local prior_trap=${trap_argv[2]:-}
     unset __bp_trap_string
     if [[ -n "$prior_trap" ]]; then
         eval '__bp_original_debug_trap() {
diff --git a/test/bash-preexec.bats b/test/bash-preexec.bats
index 152d5a6..7f2ed8b 100644
--- a/test/bash-preexec.bats
+++ b/test/bash-preexec.bats
@@ -103,6 +103,26 @@ set_exit_code_and_run_precmd() {
   (( trap_count_snapshot < trap_invoked_count ))
 }
 
+@test "__bp_install should preserve an existing DEBUG trap containing quotes" {
+  trap_invoked_count=0
+  foo() { (( trap_invoked_count += 1 )); }
+
+  # note setting this causes BATS to mis-report the failure line when this test fails
+  trap "foo && echo 'hello' >/dev/null" debug
+  [ "$(trap -p DEBUG | cut -d' ' -f3-7)" == "'foo && echo '\''hello'\'' >/dev/null'" ]
+
+  bp_install
+  trap_count_snapshot=$trap_invoked_count
+
+  [ "$(trap -p DEBUG | cut -d' ' -f3)" == "'__bp_preexec_invoke_exec" ]
+  [[ "${preexec_functions[*]}" == *"__bp_original_debug_trap"* ]] || return 1
+
+  __bp_interactive_mode # triggers the DEBUG trap
+
+  # ensure the trap count is still being incremented after the trap's been overwritten
+  (( trap_count_snapshot < trap_invoked_count ))
+}
+
 @test "__bp_sanitize_string should remove semicolons and trim space" {
 
     __bp_sanitize_string output "   true1;  "$'\n'