rcaloras / bash-preexec

⚡ preexec and precmd functions for Bash just like Zsh.
MIT License
877 stars 93 forks source link

Question on HISTCONTROL #48

Closed gnachman closed 7 years ago

gnachman commented 7 years ago

After adopting bash-preexec for a script I maintain, I received a bug report that it changes HISTCONTROL: https://gitlab.com/gnachman/iterm2/issues/5787#note_34771330

Based on the comment in the code, I think I'd rather preserve HISTCONTROL than get leading and trailing spaces correctly reported (I strip them anyway):

# Remove ignorespace and or replace ignoreboth from HISTCONTROL
# so we can accurately invoke preexec with a command from our
# history even if it starts with a space.

Is the comment accurate? Are there any traps if I remove the call to __bp_adjust_histcontrol? Will anything worse happen than losing some whitespace at the beginning of a command?

I tested it and it seems safe, but I thought I'd check before unleashing this change in prod.

rcaloras commented 7 years ago

@gnachman Thanks for opening. I added bp_adjust_histcontrol because bash-preexec currently depends on history for getting the entered command to pass to preexec. For reference: https://github.com/rcaloras/bash-preexec/blob/master/bash-preexec.sh#L181 If you don't use $1 in preexec, removing it is perfectly safe. Are you using $1 in preexec?

This is the most accurate way to get the command, but unfortunately doesn't work in the scenario where someone has ignoreboth or ignorespace. It won't record the command, but preexec will still be invoked grabbing the last line in history. Alternatively you can get the entered command from $BASH_COMMAND but this isn't exactly as the user has typed it (e.g. expanding variables and aliases I believe).

rcaloras commented 7 years ago

btw, excited to hear you're using bash-preexec for item2. Love your project. Keep up the great work :+1:

gnachman commented 7 years ago

That explains it. I am not using $1 in preexec. You can see the combined code here:

https://iterm2.com/shell_integration/bash

I think I'll remove the call to bp_adjust_histcontrol. If someone has two scripts that use bash-preexec they'll need to do some hacking.

rcaloras commented 7 years ago

Awesome, sounds like it should be all good. :+1: