-
Notifications
You must be signed in to change notification settings - Fork 94
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.
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
Change to preexec function error code #46
Conversation
bash-preexec.sh
Outdated
@@ -199,14 +199,16 @@ __bp_preexec_invoke_exec() { | |||
if type -t "$preexec_function" 1>/dev/null; then | |||
__bp_set_ret_value $__bp_last_ret_value | |||
$preexec_function "$this_command" | |||
preexec_ret_value="$?" | |||
if (( $? != 0 )); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small style comment. Also why don't we set the value to $?
?
if [[ "$?" != 0 ]]; then
preexec_ret_value="$?"
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This preserves the "blocking" behavior of preexec returning non-zero even if another preexec function returns 0, which is ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to $?.
# Also preserves the return value of the last function executed in preexec | ||
# If `extdebug` is enabled a non-zero return value from the last function | ||
# in prexec causes the command not to execute | ||
# Restore the last argument of the last executed command, and set the return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating comments 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np! :)
@brandonweeks Any thoughts on this change? You were the original contributor of preserving the return status of the last function :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commits should be squashed. :)
bash-preexec.sh
Outdated
@@ -199,14 +199,16 @@ __bp_preexec_invoke_exec() { | |||
if type -t "$preexec_function" 1>/dev/null; then | |||
__bp_set_ret_value $__bp_last_ret_value | |||
$preexec_function "$this_command" | |||
preexec_ret_value="$?" | |||
if (( $? != 0 )); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This preserves the "blocking" behavior of preexec returning non-zero even if another preexec function returns 0, which is ideal.
@jombooth I'll stamp and merge once comments are addressed. Thanks for submitting! |
0711550
to
cafcc6a
Compare
SGTM - of course! |
bash-preexec.sh
Outdated
@@ -199,14 +200,18 @@ __bp_preexec_invoke_exec() { | |||
if type -t "$preexec_function" 1>/dev/null; then | |||
__bp_set_ret_value $__bp_last_ret_value | |||
$preexec_function "$this_command" | |||
preexec_ret_value="$?" | |||
preexec_function_ret_value="$?" | |||
if (( $preexec_function_ret_value != 0 )); then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also change this to be double square brackets? That's the style for this file.
if [[ $preexec_function_ret_value != 0 ]]; then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The round brackets are actually a different construct (arithmetic expansions - http://tldp.org/LDP/abs/html/arithexp.html#ARITHEXPREF) that seem pretty reasonable here, since the variable used is guaranteed to be an integer. Doing it the other way would be a string comparison, which works, but I think this approach is preferable. Happy to be overruled on this, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also - the reason I didn't quote the variable is that they're not needed inside arithmetic expansions as word splitting, etc, don't occur. The $ could be dropped as well, but I find code that does that a little confusing to read since I'm used to $-dereferencing of variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi - decided to go ahead and make the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jombooth I think your point is valid! I would have merged :) This way is more consistent I guess.
cafcc6a
to
b1bc3d1
Compare
b1bc3d1
to
b6a9b9a
Compare
I made the change requested earlier, from (( to [[. Please let me know if anything else is needed! |
@jombooth Sorry for the delayed response. Thanks for addressing changes and your contributions! Much obliged! |
👍 🍔 🍟 |
Some friends and I are using bash-preexec in an application wherein our users sometimes load multiple projects that depend on bash-preexec in their shells. One of these uses extdebug, along with the useful feature that the command the user tries to run is skipped if the last function in preexec_functions returns an error - but we run into problems where the last function in preexec ends up being, eg, one that always returns 0, because the user also sources another script which tacks an extra function onto the end of preexec_functions.
This script changes the behavior of bash-preexec so that an error from any function in the array causes the overall debug trap call to return 1, so that extdebug can be used reliably in conjunction with multiple projects that use bash-preexec.