-
Notifications
You must be signed in to change notification settings - Fork 95
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
preexec
doesn't work for function declarations
#6
Comments
Hey thanks for the feedback! This is a known limitation along with subshell parsing e.g. doing something like (ls) I have a branch in progress for this fix, but it's not too easy. https://github.com/rcaloras/bash-preexec/tree/SubShellParsing. It actually takes a decent amount of time for bash to handle this case and I believe it creates some lag. Would love your contribution if you can help figure it out. I'll give it another look tonight as well. |
@rcaloras , thanks. I'll read https://github.com/rcaloras/bash-preexec/tree/SubShellParsing this evening:) |
Reminder for me:
|
That should address the space issue :) Not sure about functions yet though. |
@rcaloras , thanks:)
Overwriting |
Yeah it should also replace Why is overwriting histcontrol a privacy issue? This plugin overwrites both bash's |
that's the joke:) Reminder for me:
|
-Replace ignoreboth with simpley ignoredups
Added a commit to handle ignoreboth. |
Could probably add a few more checks to make sure history is properly enabled. Something like history_check=$(set -o | grep history)
if [[ "$history_check" == "history off" ]]; then
echo "Enable history to use bash preexec"
return 1
fi; |
I think it might be easier to get functions by checking changes in the bash history then investigating things like sub shell parsing. |
@rcaloras , thanks.
Too many checks: I vote for auto enabling:) I've collected some useful settings: # enable access to the command history
set -o history
# save each line of a multi-line command in the same history entry
shopt -s cmdhist
# save the command with embedded newlines instead of semicolons
shopt -s lithist
# save lines which begin with a space character in the history list
# preserve dups!
HISTCONTROL= # already done
# save all lines in the history list
HISTIGNORE=
# every command being saved on the history list (there is no limit)
HISTSIZE=-1
bind 'set history-size -1' |
Another $ source .bash-preexec.sh
$ preexec() { echo "just typed $1"; }
$ HISTTIMEFORMAT='%F %T '
just typed HISTTIMEFORMAT='%F %T '
$ echo WAT
just typed 2015-06-22 04:56:43 echo WAT Possible solution: local this_command="$(HISTTIMEFORMAT= history 1 | sed -e "s/^[ ]*[0-9]*[ ]*//g")"; |
Want to submit a pull request for that one? Would love to get you some contributor credit :) Feel free to open it as a separate issue if you'd like so we can close it specifically. |
Fixed:) |
preexec
doesn't work for functionspreexec
doesn't work for function declarations and (list) compound commands
- Watches for history changes to properly invoke preexec function. Handles duplicates and shell startup edge cases. - Still a bit of a hack since it's not truly prexection (i.e. will come after the function or subshell is executed) but the only way I found in bash to get prexec to invoke for both functions and subshells.
@evverx I created a viable work around that's based on history changes to detect sub shell and function executed commands. Give it a look. I'm testing it out now. I'll merge to master and cut a release if I find it's working fine. |
Also moved your history related suggestions to #10. Thanks! |
Wow! |
@rcaloras , sorry. git clone https://github.com/rcaloras/bash-preexec
cd bash-preexec/
git checkout --track origin/FunctionAndSubshells
source bash-preexec.sh
preexec() { echo "just typed $1"; }
function wat { echo wat; } # outputs just typed 'function wat { echo wat; }'. It's OK!
function wat { echo wat; } # nothing here! It's not OK
(date) # outputs just typed '(date)'. It's OK.
(date) # nothing here. It's not OK.
date # outputs just typed 'date'
date # outputs just typed 'date' |
So here's the problem. Function declarations and subshell commands don't invoke the Although I removed the ignoring for white spaces by default, I'm not sure I'm comfortable with removing ignore duplicates :/ |
I have a question that seems to be related to some of this discussion. While using this script, the history does not store duplicate commands. Is there any particular reason for that? Forgive me if this is self-explanatory, I possibly haven't looked at it enough to figure it out on my own. |
Hi, @isp5 . |
Thanks! Any particular reason the history was set like that? |
|
I'm using a weird series of hacks on top of the old twistedmatrix code but it behaves itself with a PS1 like this (i.e., preexec and precmd run at the right time):
Is that what you meant about prompts that include environment information? |
@gnachman Yeah exactly. I'm looking through your code now. I'll definitely try to recreate it. Looks like you're doing some interesting stuff with PS1 to work around it e.g. https://github.com/gnachman/iterm2-website/blob/master/source/misc/bash_startup.in#L62-L83. I actually just had some users of Bashhub try to use Iterm2 + Shell Integration and it failed because we're both trying to install our own versions of preexec :( I'd love to fix up this project to make it compatible with the awesome iTerm2 :) I wrote bash-preexec to include function arrays and to avoid multiple inclusion which allows multiple shell integrations to add preexec functions triggered through one debug trap hook. Any chance you'd be willing to switch to this library if I make it compatible? |
@rcaloras I would love to not own the preexec code. It hurts my head. The main difficulty is ensuring it works on the ancient version of bash that ships with Mac OS, and the weird hack for CentOS 7.2. |
@gnachman Awesome, would love to take it off your hands. I'm traveling over the next few weeks, but will take a look at aligning mine with some of your changes when I get back. Thanks! 👍 |
- Addresses part of #6 - Still doesn't support functions. - Also expose the correct exit code to preexec.
@evverx Addressed the issue for subshells by looking at some of the code from @gnachman. Not positive there's a solution for functions :/ I'm going to release this and another change. Give it a try if you get a chance. If everything looks good I'll rename this issue to just address function declarations. |
@rcaloras , works fine! Thanks!
But I don't really understand how:) |
preexec
doesn't work for function declarations and (list) compound commandspreexec
doesn't work for function declarations
In 3458480 Remove ignorespace from $HISTCONTROL and in 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
In 3458480 Remove ignorespace from $HISTCONTROL and in 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
In 3458480 Remove ignorespace from $HISTCONTROL and in 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
In 3458480 Remove ignorespace from $HISTCONTROL and in 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
After 3458480 Remove ignorespace from $HISTCONTROL and after 7e55ac1 Follow up commit for issue rcaloras#6 -Replace ignoreboth with simpley ignoredups this script would remove 'ignorespace' and would replace 'ignoreboth' with 'ignoredups'. This effectively disables the functionality of not adding space prefixed commands into history. It used to happen siliently and could be quite confusing to users who use this feature. This script relies on the command to be in the history, but we can mostly fix the issue by "manual" removing a whitespace prefixed command from the history after reading it from there.
The text was updated successfully, but these errors were encountered: