Ensure a pre-existing DEBUG trap is preserved as a preexec function. #50
Conversation
|
@rcaloras just checking if you've had a chance to look at this PR? |
bash-preexec.sh
Outdated
| eval '__bp_original_debug_trap() { | ||
| '"$prior_trap"' | ||
| }' | ||
| preexec_functions+=("$__bp_original_debug_trap") |
There was a problem hiding this comment.
Looks like the way you're adding to preexec functions is different than precmd. Shouldn't this be like:
preexec_functions+=(__bp_original_debug_trap)There was a problem hiding this comment.
Oof, good catch. I realize my test is also missing the point, since the DEBUG trap is invoked immediately. I've got a better test pending.
This would also have been caught if we didn't silently skip missing functions - I'd been thinking about suggesting changing that, I'll file a bug unless you'd prefer to leave it be.
There was a problem hiding this comment.
I think skipping missing functions is more pragmatic (although you can get silent failures you might want to know) and is the actual behavior by preexec/precmd in zsh. I think we should be trying to emulate that experience.
bash-preexec.sh
Outdated
| @@ -47,7 +47,17 @@ __bp_last_argument_prev_command="$_" | |||
|
|
|||
| # Command to set our preexec trap. It's invoked once via | |||
| # PROMPT_COMMAND and then removed. | |||
| __bp_trap_install_string="trap '__bp_preexec_invoke_exec \"\$_\"' DEBUG;" | |||
| __bp_trap_install() { | |||
| local prior_trap=$(trap -p DEBUG | sed "s/[^']*'\(.*\)'[^']*/\1/") | |||
There was a problem hiding this comment.
Modified your change via my comment below then found this issue.
I think there's an edge case here where a trap contains output. If I set a simple debug trap with an echo it's pulling in the output that is produced from the debug trap here. and setting it in the original debug trap function. e.g:
$ trap
trap -- '' SIGTSTP
trap -- '' SIGTTIN
trap -- '' SIGTTOU
$ trap 'echo "from trap"' DEBUG
$ source bash-preexec.sh
from trap
from trap
from trap
from trap
from trap
from trap
from trap
from trap
from trap
$ type __bp_original_debug_trap
from trap
__bp_original_debug_trap is a function
__bp_original_debug_trap ()
{
from trap;
from trap;
echo "from trap"
}This causes __bp_original_debug_trap to error with the output 'from trap' in the function.
There was a problem hiding this comment.
That's really interesting. I think this is due to set -T, I'll experiment some more but I may have to go back to something like your __bp_trap_install_string solution if set -T won't work. I'll get back to you.
There was a problem hiding this comment.
Ok, this is fixed. It seems set -T is a non-starter, since it causes the DEBUG trap to be run from within command substitutions:
$ trap 'echo "from trap"' DEBUG
$ trap_cmd=$(trap -p DEBUG)
from trap
$ set -T
from trap
$ trap_t_cmd=$(trap -p DEBUG)
from trap
$ trap DEBUG
from trap
$ :
$ echo "$trap_cmd"
trap -- 'echo "from trap"' DEBUG
$ echo "$trap_t_cmd"
from trap
trap -- 'echo "from trap"' DEBUG
But the pattern I'm using now (capture the trap and clear it in PROMPT_COMMAND, then install the new trap in a function) appears to work.
* Capture any pre-existing PROMPT_COMMAND hook in the same way, adding it as a precmd function. * No longer need to do string manipulations of the PROMPT_COMMAND's contents. * Added a unit test of the new trapping semantics. * Tidied up some other tests to test the public API and avoid tweaking internal state (e.g. the old $_ test would fail to catch a bug in this change). Resolves issue #39.
|
👍 🍔 🍟 @dimo414 💯 💯 👍 Thanks so much for your contributions. This is great! |
|
Happy to help :) |
(e.g. the old $_ test would fail to catch a bug in this change).
Resolves issue #39.