Open akinomyoga opened 2 years ago
ble.sh
actually provides proper support for thePRECMD
andPREEXEC
hooks
Can you provide some pointers for this behavior? What is more "proper" about your approach?
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
).
I have added code comments to explain the new functions and force-pushed the changes.
P.S. If we need to describe the new APIs in README, I can also add the description there.
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'}
@rcaloras @dimo414 Can you also review this PR? I also think #128 and #129 should be considered. Thanks.
@akinomyoga Thanks for the PR and support of this project. I'll take a look and chime in before end of week!
@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.
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
).
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.
@dimo414 or any other contributors have thoughts on the necessity of an uninstall function i.e. the proposedbash_preexec_uninstall
?
I have noticed that an existing test for __bp_install
is actually broken:
Here, line 58 should be trap - DEBUG
instead of trap DEBUG
because the install string has been updated in
e36de177c081fdfdf3f448716364cfbcafc26a2e 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).
@akinomyoga thanks for updating with tests. Looks good. Will hold for any other feedback/input from other contributors.
There was a conflict, so I rebased and squashed commits.
bash-preexec
relies on theDEBUG
trap (forpreexec
) andPROMPT_COMMAND
(forprecmd
). In particular, the hook using theDEBUG
trap is a kind of hack and fragile. TheDEBUG
trap is called for every single shell command (of the top-level whenset -T
orshopt -s extdebug
is not set), which makes the execution slower. TheDEBUG
trap can interfere with other Bash configurations that use theDEBUG
trap. The implementation ofpreexec
by theDEBUG
trap also causes problems like #115, #104, #116, #100, #25, and #6. The hook usingPROMPT_COMMAND
(forprecmd
) is not so stable, in particular when other configurations add settings such asPROMPT_COMMAND="${PROMPT_COMMAND+$PROMPT_COMMAND;}__new_settings__"
(see #128 for example).Therefore, when the other framework provides proper support for the
preexec
andprecmd
hooks, it would be better to just use them instead of relying on fragileDEBUG
andPROMPT_COMMAND
. To allow users or other Bash configuration frameworks to choose howprecmd
andpreexec
ofbash-preexec
will be invoked, I would like to request public/stable APIs to install/uninstallDEBUG
andPROMPT
hooks in arbitrary timing and also the public/stable APIs to invoke{precmd,preexec}_functions
from external mechanisms ofpreexec
andprecmd
.The public APIs that I would like to request in this PR are:
preexec_functions
bash_preexec_invoke_preexec_functions
in this PR.precmd_functions
bash_preexec_invoke_precmd_functions
in this PR.DEBUG
andPROMPT_COMMAND
bash_preexec_install_string
in this PR, which was originally named__bp_install_string
.bash-preexec
registers to theDEBUG
trapbash_preexec_trapdebug_string
in this PR.DEBUG
andPROMPT_COMMAND
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 wherebash_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 withbash-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 thePRECMD
andPREEXEC
hooks, so I would like to recommend users to use them instead ofbash-preexec
, but there are many existing settings and configurations that rely onbash-preexec
. Also, I don't want to unnecessarily require users to create dependency on ble.sh. So I decided to rewrite the hooks bybash-preexec
whenble.sh
detects it. Currently,ble.sh
assumes many things about the internals ofbash-preexec
in rewriting the hooks bybash-preexec
, which is not ideal. What I actually need is just several public/stable APIs just I described above.Related discussions: