You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
A coworker pointed out that that the fix to #39 doesn't properly preserve the behavior of the DEBUG trap, because preexec only triggers on interactive prompts, whereas before installation the command was triggered on every DEBUG event.
I think this can be fixed by simply moving the old DEBUG trap out of preexec_functions and instead eval-ing it at the beginning of __bp_preexec_invoke_exec - before the short-circuiting checks are made.
The only problem with this is it makes it difficult for someone to clean up the old trap - it's no longer in the trap itself nor in the preexec / preexec_functions, but some other variable. It's simple enough to document that (e.g. "The old DEBUG trap will be preserved in a bp_prior_debug_trap variable") but that widens the API surface and might be abused.
I'll prepare a patch, but I wanted to get some opinions on the approach first.
The text was updated successfully, but these errors were encountered:
@dimo414 interesting observation. Since the original implementation simply overwrote the existing DEBUG trap I think anything is really an improvement here :)
I'm in favor of having a variable preserving the contents of the prior debug trap and invoked how you spec'd. Although it widens the API it's definitely a better experience. It does make things a little more complicated, but if we want to support legit preservation it's worth the compromise 👍
A coworker pointed out that that the fix to #39 doesn't properly preserve the behavior of the DEBUG trap, because preexec only triggers on interactive prompts, whereas before installation the command was triggered on every DEBUG event.
I think this can be fixed by simply moving the old DEBUG trap out of
preexec_functions
and insteadeval
-ing it at the beginning of__bp_preexec_invoke_exec
- before the short-circuiting checks are made.The only problem with this is it makes it difficult for someone to clean up the old trap - it's no longer in the trap itself nor in the
preexec
/preexec_functions
, but some other variable. It's simple enough to document that (e.g. "The old DEBUG trap will be preserved in abp_prior_debug_trap
variable") but that widens the API surface and might be abused.I'll prepare a patch, but I wanted to get some opinions on the approach first.
The text was updated successfully, but these errors were encountered: