Skip to content

Request for public/stable APIs to enable/disable the hooks and to manually invoke registered functions #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Request for public/stable APIs to enable/disable the hooks and to manually invoke registered functions #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
befc702
Separate "__bp_invoke_pre{cmd,exec}_functions"
akinomyoga Jan 22, 2022
cbd3390
Add APIs for external precmd/preexec integrations
akinomyoga Jan 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 96 additions & 20 deletions bash-preexec.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,11 @@ __bp_inside_precmd=0
__bp_inside_preexec=0

# Initial PROMPT_COMMAND string that is removed from PROMPT_COMMAND post __bp_install
__bp_install_string=$'__bp_trap_string="$(trap -p DEBUG)"\ntrap - DEBUG\n__bp_install'
bash_preexec_install_string=$'__bp_trap_string="$(trap -p DEBUG)"\ntrap - DEBUG\n__bp_install'

# The command string that is registered to the DEBUG trap.
# shellcheck disable=SC2016
bash_preexec_trapdebug_string='__bp_preexec_invoke_exec "$_"'

# Fails if any of the given variables are readonly
# Reference https://stackoverflow.com/a/4441178
Expand Down Expand Up @@ -157,21 +161,38 @@ __bp_precmd_invoke_cmd() {
return
fi
local __bp_inside_precmd=1
bash_preexec_invoke_precmd_functions "$__bp_last_ret_value" "$__bp_last_argument_prev_command"

__bp_set_ret_value "$__bp_last_ret_value" "$__bp_last_argument_prev_command"
}

# This function invokes every function defined in our function array
# "precmd_function". This function receives the arguments $1 and $2 for $? and
# $_, respectively, which will be set for each precmd function. This function
# returns the last non-zero exit status of the hook functions. If there is no
# error, this function returns 0.
bash_preexec_invoke_precmd_functions() {
local lastexit=$1 lastarg=$2
# Invoke every function defined in our function array.
local precmd_function
local precmd_function_ret_value
local precmd_ret_value=0
for precmd_function in "${precmd_functions[@]}"; do

# Only execute this function if it actually exists.
# Test existence of functions with: declare -[Ff]
if type -t "$precmd_function" 1>/dev/null; then
__bp_set_ret_value "$__bp_last_ret_value" "$__bp_last_argument_prev_command"
__bp_set_ret_value "$lastexit" "$lastarg"
# Quote our function invocation to prevent issues with IFS
"$precmd_function"
precmd_function_ret_value=$?
if [[ "$precmd_function_ret_value" != 0 ]]; then
precmd_ret_value="$precmd_function_ret_value"
fi
fi
done

__bp_set_ret_value "$__bp_last_ret_value"
__bp_set_ret_value "$precmd_ret_value"
}

# Sets a return value in $?. We may want to get access to the $? variable in our
Expand Down Expand Up @@ -260,7 +281,27 @@ __bp_preexec_invoke_exec() {
return
fi

# Invoke every function defined in our function array.
bash_preexec_invoke_preexec_functions "${__bp_last_ret_value:-}" "$__bp_last_argument_prev_command" "$this_command"
local preexec_ret_value=$?

# Restore the last argument of the last executed command, and set the return
# value of the DEBUG trap to be the return code of the last preexec function
# to return an error.
# If `extdebug` is enabled a non-zero return value from any preexec function
# will cause the user's command not to execute.
# Run `shopt -s extdebug` to enable
__bp_set_ret_value "$preexec_ret_value" "$__bp_last_argument_prev_command"
}

# This function invokes every function defined in our function array
# "preexec_function". This function receives the arguments $1 and $2 for $?
# and $_, respectively, which will be set for each preexec function. The third
# argument $3 specifies the user command that is going to be executed
# (corresponding to BASH_COMMAND in the DEBUG trap). This function returns the
# last non-zero exit status from the preexec functions. If there is no error,
# this function returns `0`.
bash_preexec_invoke_preexec_functions() {
local lastexit=$1 lastarg=$2 this_command=$3
local preexec_function
local preexec_function_ret_value
local preexec_ret_value=0
Expand All @@ -269,7 +310,7 @@ __bp_preexec_invoke_exec() {
# Only execute each function if it actually exists.
# Test existence of function with: declare -[fF]
if type -t "$preexec_function" 1>/dev/null; then
__bp_set_ret_value "${__bp_last_ret_value:-}"
__bp_set_ret_value "$lastexit" "$lastarg"
# Quote our function invocation to prevent issues with IFS
"$preexec_function" "$this_command"
preexec_function_ret_value="$?"
Expand All @@ -278,14 +319,7 @@ __bp_preexec_invoke_exec() {
fi
fi
done

# Restore the last argument of the last executed command, and set the return
# value of the DEBUG trap to be the return code of the last preexec function
# to return an error.
# If `extdebug` is enabled a non-zero return value from any preexec function
# will cause the user's command not to execute.
# Run `shopt -s extdebug` to enable
__bp_set_ret_value "$preexec_ret_value" "$__bp_last_argument_prev_command"
__bp_set_ret_value "$preexec_ret_value"
}

__bp_install() {
Expand All @@ -294,7 +328,8 @@ __bp_install() {
return 1
fi

trap '__bp_preexec_invoke_exec "$_"' DEBUG
# shellcheck disable=SC2064
trap "$bash_preexec_trapdebug_string" DEBUG

# Preserve any prior DEBUG trap as a preexec function
local prior_trap
Expand Down Expand Up @@ -327,7 +362,7 @@ __bp_install() {
# Remove setting our trap install string and sanitize the existing prompt command string
existing_prompt_command="${PROMPT_COMMAND:-}"
# Edge case of appending to PROMPT_COMMAND
existing_prompt_command="${existing_prompt_command//$__bp_install_string/:}" # no-op
existing_prompt_command="${existing_prompt_command//$bash_preexec_install_string/:}" # no-op
existing_prompt_command="${existing_prompt_command//$'\n':$'\n'/$'\n'}" # remove known-token only
existing_prompt_command="${existing_prompt_command//$'\n':;/$'\n'}" # remove known-token only
__bp_sanitize_string existing_prompt_command "$existing_prompt_command"
Expand All @@ -346,10 +381,13 @@ __bp_install() {
PROMPT_COMMAND+=$'\n__bp_interactive_mode'
fi

# Add two functions to our arrays for convenience
# of definition.
precmd_functions+=(precmd)
preexec_functions+=(preexec)
# Add two functions to our arrays for convenience of definition only when
# the functions have not yet added.
if [[ ! ${__bp_installed_convenience_functions-} ]]; then
__bp_installed_convenience_functions=1
precmd_functions+=(precmd)
preexec_functions+=(preexec)
fi

# Invoke our two functions manually that were added to $PROMPT_COMMAND
__bp_precmd_invoke_cmd
Expand All @@ -371,8 +409,46 @@ __bp_install_after_session_init() {
PROMPT_COMMAND=${sanitized_prompt_command}$'\n'
fi
# shellcheck disable=SC2179 # PROMPT_COMMAND is not an array in bash <= 5.0
PROMPT_COMMAND+=${__bp_install_string}
PROMPT_COMMAND+=${bash_preexec_install_string}
}

# Remove hooks installed in the DEBUG trap and PROMPT_COMMAND.
bash_preexec_uninstall() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced that bash-preexec should be responsible for maintaining any over head of uninstalling itself from both trap and prompt_command. Any libraries using these shared variables have more or less had to clean them up and maintain them on their own, as does bash-preexec with its installation. With the defined global variables what's stopping from just implementing this in your own library?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the defined global variables what's stopping from just implementing this in your own library?

If the setup of bash-preexec.sh would never change in the future forever, I could just assume the current setup of bash-preexec.sh and write a code to clean up the things that are installed by the current bash-preexec.sh.

However, if the __bp_install configuration can change in the future (e.g. as suggested in #128), the cleanup code also needs to be updated. I don't think it is a good idea to update the cleanup code at the users' side every time the __bp_install configuration of bash-preexec.sh is changed. In particular, if the method of uninstalling the hooks would not be provided by bash-preexec.sh, the user/library needs to maintain the separate cleanup code for each of all the past versions of bash-preexec.sh and to detect the precise version of bash-preexec.sh.

This is actually the reason that I would like to request the "stable" public API rather than relying on the details of the internal implementation that can be frequently changed.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your reasoning and desire for this as a way to help with your library and not just implementing it yourself. I just think there's no real standard here I've seen other libraries that have clashed with bash-preexec as well since they also make the assumption they'll have access to these parts of the system configuration. (Liquid Prompt is one example) My assumption is, these too can cause issues with ble.sh and we're trying to chase individual fires here.

I am concerned that this is an esoteric use case and will likely just add operational overhead to maintaining it. With that said, if any other contributors see value in this I'm supportive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Maybe I miss your exact point (What does "the system configuration" mean?), but I think I'll later need to look at Liquid Prompt.

# Remove __bp_install hook from PROMPT_COMMAND
# shellcheck disable=SC2178 # PROMPT_COMMAND is not an array in bash <= 5.0
if [[ ${PROMPT_COMMAND-} == *"$bash_preexec_install_string"* ]]; then
PROMPT_COMMAND="${PROMPT_COMMAND//${bash_preexec_install_string}[;$'\n']}" # Edge case of appending to PROMPT_COMMAND
PROMPT_COMMAND="${PROMPT_COMMAND//$bash_preexec_install_string}"
fi

# Remove precmd hook from PROMPT_COMMAND
local i prompt_command
for i in "${!PROMPT_COMMAND[@]}"; do
prompt_command=${PROMPT_COMMAND[i]}
case $prompt_command in
__bp_precmd_invoke_cmd | __bp_interactive_mode)
prompt_command= ;;
*)
prompt_command=${prompt_command/#$'__bp_precmd_invoke_cmd\n'/$'\n'}
prompt_command=${prompt_command%$'\n__bp_interactive_mode'}
prompt_command=${prompt_command#$'\n'}
esac
PROMPT_COMMAND[i]=$prompt_command
done

# Remove preexec hook in the DEBUG trap
local q="'" Q="'\''"
if [[ $(trap -p DEBUG) == "trap -- '${bash_preexec_trapdebug_string//$q/$Q}' DEBUG" ]]; then
if [[ ${__bp_trap_string-} ]]; then
eval -- "$__bp_trap_string"
else
trap - DEBUG
fi
fi
}
# Note: We need to add "trace" attribute to the function so that "trap - DEBUG"
# inside the function takes an effect.
declare -ft bash_preexec_uninstall

# Run our install so long as we're not delaying it.
if [[ -z "${__bp_delay_install:-}" ]]; then
Expand Down
124 changes: 122 additions & 2 deletions test/bash-preexec.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,11 @@ set_exit_code_and_run_precmd() {

# Assert that before running, the command contains the install string, and
# afterwards it does not
[[ "$PROMPT_COMMAND" == *"$__bp_install_string"* ]] || return 1
[[ "$PROMPT_COMMAND" == *"$bash_preexec_install_string"* ]] || return 1

eval_PROMPT_COMMAND

[[ "$PROMPT_COMMAND" != *"$__bp_install_string"* ]] || return 1
[[ "$PROMPT_COMMAND" != *"$bash_preexec_install_string"* ]] || return 1
}

@test "__bp_install should preserve an existing DEBUG trap" {
Expand All @@ -103,6 +103,56 @@ set_exit_code_and_run_precmd() {
(( trap_count_snapshot < trap_invoked_count ))
}

@test "__bp_install should register convenience functions \"preexec\" and \"precmd\" only once" {
precmd_functions=()
preexec_functions=()
__bp_install
bash_preexec_uninstall
__bp_install

count=0
for hook in "${precmd_functions[@]}"; do
if [[ "$hook" == precmd ]] ; then
count=$((count+1))
fi
done
[ "$count" == 1 ]

count=0
for hook in "${preexec_functions[@]}"; do
if [[ "$hook" == preexec ]] ; then
count=$((count+1))
fi
done
[ "$count" == 1 ]
}

@test "bash_preexec_uninstall should remove the hooks in DEBUG and PROMPT_COMMAND" {
__bp_install

q="'" Q="'\''"
[[ "$(join_PROMPT_COMMAND)" == *"__bp_precmd_invoke_cmd"* ]] || return 1
[[ "$(join_PROMPT_COMMAND)" == *"__bp_interactive_mode"* ]] || return 1
[ "$(trap -p DEBUG)" == "trap -- '${bash_preexec_trapdebug_string//$q/$Q}' DEBUG" ]

bash_preexec_uninstall

q="'" Q="'\''"
[[ "$(join_PROMPT_COMMAND)" != *"__bp_precmd_invoke_cmd"* ]] || return 1
[[ "$(join_PROMPT_COMMAND)" != *"__bp_interactive_mode"* ]] || return 1
[ "$(trap -p DEBUG)" != "trap -- '${bash_preexec_trapdebug_string//$q/$Q}' DEBUG" ]
}

@test "bash_preexec_uninstall should remove the unprocessed __bp_install hook in PROMPT_COMMAND" {
__bp_install_after_session_init

[[ "$PROMPT_COMMAND" == *"$bash_preexec_install_string"* ]]

bash_preexec_uninstall

[[ "$PROMPT_COMMAND" != *"$bash_preexec_install_string"* ]]
}

@test "__bp_sanitize_string should remove semicolons and trim space" {

__bp_sanitize_string output " true1; "$'\n'
Expand Down Expand Up @@ -308,6 +358,76 @@ set_exit_code_and_run_precmd() {
[ $status -eq 1 ]
}

@test "bash_preexec_invoke_precmd_functions should be transparent for \$? and \$_" {
tester1() { test1_lastexit=$? test1_lastarg=$_; }
tester2() { test2_lastexit=$? test2_lastarg=$_; }
precmd_functions=(tester1 tester2)
trap - DEBUG # remove the Bats stack-trace trap so $_ doesn't get overwritten
bash_preexec_invoke_precmd_functions 111 'vxxJlwNx9VPJDA' || true

[ "$test1_lastexit" == 111 ]
[ "$test1_lastarg" == 'vxxJlwNx9VPJDA' ]
[ "$test2_lastexit" == 111 ]
[ "$test2_lastarg" == 'vxxJlwNx9VPJDA' ]
}

@test "bash_preexec_invoke_precmd_functions returns the last non-zero exit status" {
tester1() { return 91; }
tester2() { return 38; }
tester3() { return 0; }
precmd_functions=(tester1 tester2 tester3)
status=0
bash_preexec_invoke_precmd_functions 1 'lastarg' || status=$?

[ "$status" == 38 ]

precmd_functions=(tester3)
status=0
bash_preexec_invoke_precmd_functions 1 'lastarg' || status=$?

[ "$status" == 0 ]
}

@test "bash_preexec_invoke_preexec_functions should be transparent for \$? and \$_" {
tester1() { test1_lastexit=$? test1_lastarg=$_; }
tester2() { test2_lastexit=$? test2_lastarg=$_; }
preexec_functions=(tester1 tester2)
trap - DEBUG # remove the Bats stack-trace trap so $_ doesn't get overwritten
bash_preexec_invoke_preexec_functions 87 'ehQrzHTHtE2E7Q' 'command' || true

[ "$test1_lastexit" == 87 ]
[ "$test1_lastarg" == 'ehQrzHTHtE2E7Q' ]
[ "$test2_lastexit" == 87 ]
[ "$test2_lastarg" == 'ehQrzHTHtE2E7Q' ]
}

@test "bash_preexec_invoke_preexec_functions returns the last non-zero exit status" {
tester1() { return 52; }
tester2() { return 112; }
tester3() { return 0; }
preexec_functions=(tester1 tester2 tester3)
status=0
bash_preexec_invoke_preexec_functions 1 'lastarg' 'command' || status=$?

[ "$status" == 112 ]

preexec_functions=(tester3)
status=0
bash_preexec_invoke_preexec_functions 1 'lastarg' 'command' || status=$?

[ "$status" == 0 ]
}

@test "bash_preexec_invoke_preexec_functions should supply a current command in the first argument" {
tester1() { test1_bash_command=$1; }
tester2() { test2_bash_command=$1; }
preexec_functions=(tester1 tester2)
bash_preexec_invoke_preexec_functions 1 'lastarg' 'UEVkErELArSwjA' || true

[ "$test1_bash_command" == 'UEVkErELArSwjA' ]
[ "$test2_bash_command" == 'UEVkErELArSwjA' ]
}

@test "in_prompt_command should detect if a command is part of PROMPT_COMMAND" {

PROMPT_COMMAND=$'precmd_invoke_cmd\n something; echo yo\n __bp_interactive_mode'
Expand Down