Skip to content
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

Update DEBUG preserving behavior to fire on all DEBUG traps, not just preexec events #54

Closed
wants to merge 0 commits into from

Conversation

dimo414
Copy link
Collaborator

@dimo414 dimo414 commented Sep 16, 2017

Fixes issue #52.

@rcaloras
Copy link
Owner

@dimo414

Tried running this locally and looks like we'll invoke the original function more often than originally intended. Is this an issue? Wondering if this is simply a side effect of bash-preexec in general. What do you think?

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ trap
trap -- '' SIGTSTP
trap -- '' SIGTTIN
trap -- '' SIGTTOU

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ trap 'echo "From trap"' DEBUG

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ ls
From trap
bash-preexec.sh  LICENSE.md  README.md  test  test.sh

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ pwd
From trap
/home/elementz/git/bash-preexec

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ source bash-preexec.sh 
From trap
From trap
From trap
From trap
From trap
From trap
From trap
From trap
From trap
From trap
From trap

# Notice the additional invocations here
elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ pwd
From trap
/home/elementz/git/bash-preexec
From trap
From trap

elementz@Kashmir:~/git/bash-preexec (dimo414-master)$ echo
From trap

From trap
From trap

@dimo414
Copy link
Collaborator Author

dimo414 commented Sep 17, 2017

I think this is an expected side-effect - I can replicate the same thing without my change by manually tweaking the DEBUG trap:

bash-4.3$ source bash-preexec.sh
bash-4.3$ trap -p DEBUG
trap -- '__bp_preexec_invoke_exec "$_"' DEBUG
bash-4.3$ trap '__bp_preexec_invoke_exec "$_"; echo "From trap"' DEBUG
From trap
From trap
bash-4.3$ echo
From trap

From trap
From trap

One of the extra traps is triggered by the PROMPT_COMMAND invocation, so that's definitely expected. I've looked at the set -x output but I don't immediately see why the third trap event is triggered. Regardless, anyone defining a DEBUG trap would need to account for these potential other triggers (as bash-preexec is doing).

@rcaloras
Copy link
Owner

Thanks for following up @dimo414. I think this preserves the integrity of the trap, but I'm not sure this is the best experience. If we weren't manipulating the DEBUG trap, do you think this is more or less the expected behavior by comparison to simply invoking on preexec?

Put another way, do you think invoking DEBUG multiple times since we're piggy backing on the trap is better than invoking it on preexec? The experience invoking on preexec could be considered closer to the functionality you'd experience without bash-preexec.

@dimo414 Do you have any examples of DEBUG trap usage that we'd want to invoke multiple times in this case other than to simply preserve integrity of the trap? I'm slightly inclined to think that users who would still need to preserve true DEBUG functionality would simply augment the trap either by editing directly or editing bash-preexec locally to add it.

@dimo414
Copy link
Collaborator Author

dimo414 commented Sep 18, 2017

Good question - I agree that most people using the DEBUG trap in an interactive session probably want preexec semantics and have to jump through hoops to get it (since they're not using this library). However, we can't know that's what was intended. It's true that installing bash-preexec potentially changes the number of times the original DEBUG trap would be triggered, but anyone implementing their own DEBUG trap has to account for the possibility of the trap firing at other times than just preexec, or their implementation is simply wrong.

The experience invoking on preexec could be considered closer to the functionality you'd experience without bash-preexec.

I don't know if I agree with that - in particular the DEBUG trap fires twice for $ foo; bar but preexec only fires once:

bash-4.3$ trap 'From trap' DEBUG
bash-4.3$ echo foo; echo bar
From trap
foo
From trap
bar
bash-4.3$ source bash-preexec.sh
From trap
From trap
From trap
bash-4.3$ echo foo; echo bar
From trap
foo
bar

I don't have a concrete use-case for needing this distinction or otherwise needing to preserve the DEBUG semantics at the moment, but it does seem a bit odd to me not to do so. That said, maybe it's better to hold off on this fix for now?

@rhansen
Copy link

rhansen commented Sep 18, 2017

I have a DEBUG trap that sets the title of my terminal window to $BASH_COMMAND (among other things). For this usage, multiple invocations of the trap is important: If I run foo && bar, and both foo and bar are long-running commands, I want the title to display bar once foo has completed.

@dimo414 dimo414 closed this Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants