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

[akinomyoga]

bash-preexec relies on the DEBUG trap (for preexec) and PROMPT_COMMAND (for precmd). In particular, the hook using the DEBUG trap is a kind of hack and fragile. The DEBUG trap is called for every single shell command (of the top-level when set -T or shopt -s extdebug is not set), which makes the execution slower. The DEBUG trap can interfere with other Bash configurations that use the DEBUG trap. The implementation of preexec by the DEBUG trap also causes problems like #115, #104, #116, #100, #25, and #6. The hook using PROMPT_COMMAND (for precmd) is not so stable, in particular when other configurations add settings such as PROMPT_COMMAND="${PROMPT_COMMAND+$PROMPT_COMMAND;}__new_settings__" (see #128 for example).

Therefore, when the other framework provides proper support for the preexec and precmd hooks, it would be better to just use them instead of relying on fragile DEBUG and PROMPT_COMMAND. To allow users or other Bash configuration frameworks to choose how precmd and preexec of bash-preexec will be invoked, I would like to request public/stable APIs to install/uninstall DEBUG and PROMPT hooks in arbitrary timing and also the public/stable APIs to invoke {precmd,preexec}_functions from external mechanisms of preexec and precmd.

The public APIs that I would like to request in this PR are:

• A function to invoke preexec_functions
  • See the shell function bash_preexec_invoke_preexec_functions in this PR.
• A function to invoke precmd_functions
  • See the shell function bash_preexec_invoke_precmd_functions in this PR.
• A string that can be evaluated to install the hooks in DEBUG and PROMPT_COMMAND
  • See the shell variable bash_preexec_install_string in this PR, which was originally named __bp_install_string.
• A trap string that bash-preexec registers to the DEBUG trap
  • See the shell variable bash_preexec_trapdebug_string in this PR.
• A function to remove the hooks in DEBUG and PROMPT_COMMAND
  • See the shell function bash_preexec_uninstall. This is a new feature that did not exist previously.

About the API names

I have initially created commit fe46a97 with the existing naming convention of __bp_*. However, now we have merged #123 where bash_preexec_* has been adopted as the name for the public interfaces, so I have followed the new convention in commit 01bfc4e. I can adjust these naming if there are any requests.

Background

In my project of Bash configuration (ble.sh), I have so far received issues caused by the interference with bash-preexec several times. I have been trying to make the Bash configuration work with bash-preexec, but there is still a limitation, and it is hard to make it work perfectly (including the other issues that are reported in this upstream repository). ble.sh actually provides proper support for the PRECMD and PREEXEC hooks, so I would like to recommend users to use them instead of bash-preexec, but there are many existing settings and configurations that rely on bash-preexec. Also, I don't want to unnecessarily require users to create dependency on ble.sh. So I decided to rewrite the hooks by bash-preexec when ble.sh detects it. Currently, ble.sh assumes many things about the internals of bash-preexec in rewriting the hooks by bash-preexec, which is not ideal. What I actually need is just several public/stable APIs just I described above.

Related discussions:

• [WSL2] input delay akinomyoga/ble.sh#96 (comment)
• plugins: Add ble.sh plugin Bash-it/bash-it#1884
• [Bash 3.2 in macOS] keymap 'emacs' is not defined. akinomyoga/ble.sh#33
• [Jake McCrary prompt timer] bash: 61 - : syntax error: operand expected (error token is "- ") akinomyoga/ble.sh#176 (comment)

[dimo414]

ble.sh actually provides proper support for the PRECMD and PREEXEC hooks

Can you provide some pointers for this behavior? What is more "proper" about your approach?

[akinomyoga]

What is more "proper" about your approach?

Maybe the word choice proper was not the appropriate one. I'm not a native speaker so don't know what would be the best-fit word for this case, but ble.sh is an alternative implementation of a line editor. Everything including the prompt calculation and command execution is under the control of ble.sh. In particular, because the prompt calculation and the user commands are directly executed by ble.sh, PRECMD and PREEXEC can just be directly inserted in the execution sequence of ble.sh. I used proper to mean that they are designed for those purpose from the beginning unlike the DEBUG trap and PROMPT_COMMAND that are originally designed for other purposes, i.e., for bashdb and the prompt settings.

Can you provide some pointers for this behavior?

It depends on what kind of information you need, but if you need the information on the interface of these hooks, it is explained in §1.2.2 of the manual. If you need the implementation details, it is hard to point one place because ble.sh implements an entire line editor and requires reading larger code sections to understand the whole execution flow, but PRECMD is executed from here and PREEXEC is executed from here. ble-edit/exec:gexec/invoke-hook-with-setexit sets $? and $_ to the ones by the previous user command and $BASH_COMMAND to the one by the previous (current) user command for PRECMD (PREEXEC). blehook/invoke is implemented here. When a PREEXEC handler is a function name or a command name, the current user command that we are trying to run now is specified to the first argument.

Edit (2022-02-20): If you are interested in how these APIs can be used from the framework that provides precmd and preexec mechanisms, you can read ble.sh/contrib/bash-preexec.bash, where we are using assumed APIs: __bp_precmd_invoke_functions (corresponding to bash_precmd_invoke_preexec_functions in this PR), __bp_preexec_invoke_functions (bash_preexec_invoke_preexec_functions), __bp_uninstall (bash_preexec_uninstall), __bp_install_string (bash_preexec_install_string), and __bp_trapdebug_string (bash_preexec_trapdebug_string).

[akinomyoga]

I have added code comments to explain the new functions and force-pushed the changes.

The overall diff of the forced push is here.

diff --git a/bash-preexec.sh b/bash-preexec.sh
index f1758d3..06db478 100644
--- a/bash-preexec.sh
+++ b/bash-preexec.sh
@@ -153,6 +153,9 @@ __bp_precmd_invoke_cmd() {
     bash_preexec_invoke_precmd_functions "$__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, that will be set for the precmd functions.
 bash_preexec_invoke_precmd_functions() {
     local __lastexit=$1 __lastarg=$2
     # Invoke every function defined in our function array.
@@ -267,8 +270,12 @@ __bp_preexec_invoke_exec() {
     __bp_set_ret_value "$preexec_ret_value" "$__bp_last_argument_prev_command"
 }

-# This function invokes every function defined in our function array.  This
-# function assigns the last error exit status to the variable
+# This function invokes every function defined in our function array
+# "preexec_function".  This function receives the arguments $1 and $2 for $?
+# and $_, respectively, that will be set for the preexec functions.  The third
+# argument $3 specifies the user command that is going to be executed
+# (corresponding to BASH_COMMAND in the DEBUG trap).  This function assigns the
+# last non-zero exit status from the preexec functions to the variable
 # `preexec_ret_value`.  If there is no error, preexec_ret_value is set to `0`.
 bash_preexec_invoke_preexec_functions() {
     local __lastexit=$1 __lastarg=$2 __this_command=$3
@@ -367,6 +374,7 @@ __bp_install_after_session_init() {
     PROMPT_COMMAND+=${bash_preexec_install_string}
 }

+# Remove hooks installed in the DEBUG trap and PROMPT_COMMAND.
 bash_preexec_uninstall() {
     # Remove __bp_install hook from PROMPT_COMMAND
     if [[ ${PROMPT_COMMAND-} == *"$bash_preexec_install_string"* ]]; then
@@ -389,6 +397,7 @@ bash_preexec_uninstall() {
         fi
     fi
 }
+declare -ft bash_preexec_uninstall

 # Run our install so long as we're not delaying it.
 if [[ -z "${__bp_delay_install:-}" ]]; then

P.S. If we need to describe the new APIs in README, I can also add the description there.

Supported Bash versions

By the way, what is the minimal Bash version that bash-preexec supports? It doesn't seem to be explicitly mentioned in the code and README. I can see a code comment mentioning Bash 3.1 while we are using PROMPT_COMMAND+= (Bash 3.1 feature) and printf -v var (Bash 3.1 feature), so is it Bash 3.1? It is actually related to the quoting in the following lines.

    PROMPT_COMMAND=${PROMPT_COMMAND/#$'__bp_precmd_invoke_cmd\n'/$'\n'}
    PROMPT_COMMAND=${PROMPT_COMMAND%$'\n__bp_interactive_mode'}
    PROMPT_COMMAND=${PROMPT_COMMAND#$'\n'}

[akinomyoga]

@rcaloras @dimo414 Can you also review this PR? I also think #128 and #129 should be considered. Thanks.

[rcaloras]

@akinomyoga Thanks for the PR and support of this project. I'll take a look and chime in before end of week!

[rcaloras]

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?

[akinomyoga]

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.

[rcaloras]

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.

[akinomyoga]

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.

[rcaloras]

@akinomyoga thanks again for the PR and trying to help make bash-preexec better. This small library has gotten more widely used across other projects/companies than ever originally anticipated. As such, it's definitely opinionated toward original use cases and has clashed withe other libraries over shared state like PROMPT_COMMAND and the debug trap. The biggest expectation is that bash-preexec is the last modifier of these things during bash configuration and that it will be responsible for managing them. I can see how this would be an issue with your library that could effectively supersede the need for bash-preexec in general.

I think I'm open to most of this change provided it's not breaking. Left comments and questions in line.

By the way, what is the minimal Bash version that bash-preexec supports?

I think 3.1 sounds right. Unless there's some other feature being used that would date it later. We should add this to the documentation at the top.

[akinomyoga]

Thank you for your careful review and valuable comments! I have addressed or commented on the points that you have raised in each review comment. edit: I have also rebased it on top of the current master.

By the way, what is the minimal Bash version that bash-preexec supports?

I think 3.1 sounds right. Unless there's some other feature being used that would date it later. We should add this to the documentation at the top.

Thank you for the clarification! Maybe we might also add the version check at the beginning of bash-preexec.sh (just after the existing check for the variable BASH_VERSION).

[rcaloras]

Thank you for the clarification! Maybe we might also add the version check at the beginning of bash-preexec.sh (just after the existing check for the variable BASH_VERSION).

Sure. Feel free to add in separate commit if you'd like. Not necessary in this stack.

[rcaloras]

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.

[rcaloras]

@dimo414 or any other contributors have thoughts on the necessity of an uninstall function i.e. the proposedbash_preexec_uninstall?

[akinomyoga]

I have noticed that an existing test for __bp_install is actually broken:

bash-preexec/test/bash-preexec.bats

Lines 53 to 59 in d308800

	[[ "$PROMPT_COMMAND" == *"trap - DEBUG"* ]] || return 1
	[[ "$PROMPT_COMMAND" == *"__bp_install"* ]] || return 1
	eval "$PROMPT_COMMAND"
	[[ "$PROMPT_COMMAND" != *"trap DEBUG"* ]] || return 1
	[[ "$PROMPT_COMMAND" != *"__bp_install"* ]] || return 1

Here, line 58 should be trap - DEBUG instead of trap DEBUG because the install string has been updated in
e36de17 for PR #106. It is an oversight in e36de17.

However, now I believe we can and should use bash_preexec_install_string / __bp_install_string. I have updated the test to use bash_preexec_install_string (4a37150).

[rcaloras]

@akinomyoga thanks for updating with tests. Looks good. Will hold for any other feedback/input from other contributors.

[akinomyoga]

There was a conflict, so I rebased and squashed commits.