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.
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.
LGTM. This preserves the "blocking" behavior of preexec returning non-zero even if another preexec function returns 0, which is ideal.
| # 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 |
|
@brandonweeks Any thoughts on this change? You were the original contributor of preserving the return status of the last function :) |
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.
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.
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.
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.
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.
Hi - decided to go ahead and make the change.
There was a problem hiding this comment.
@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.