rcaloras / bash-preexec

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

Library Authors documentation can still lead to issues #146

Closed pblesi closed 7 months ago

pblesi commented 1 year ago

The Library Authors section in the README indicates the following.

if [[ -n "${bash_preexec_imported:-}" ]]; then
    echo "Bash-preexec is loaded."
fi

However, older versions of bash-preexec (e.g. 0.4.1) used the following value for indicating if bash-preexec was loaded:

"${__bp_imported}"

Updating the README example to

if [[ -n "${bash_preexec_imported:-}" || [[ -n "${__bp_imported:-}" ]]; then
    echo "Bash-preexec is loaded."
fi

Would properly detect if previous versions have been loaded.

Additionally, I believe bash-preexec has some logic to short circuit if it detects that it has already been loaded. I experienced issues where preexec commands no longer ran (the trap was removed) when version 0.5.0 was attempted to be loaded after version 0.4.1

akinomyoga commented 1 year ago

However, older versions of bash-preexec (e.g. 0.4.1) used the following value for indicating if bash-preexec was loaded:

The name change has been introduced by discussions in #122 and #123. Before that, __bp_imported was not a part of the public API of bash-preexec; __bp_imported was not supposed to be referenced by libraries. It is also described in the code comments.

Additionally, [...]. I experienced issues where preexec commands no longer ran (the trap was removed) when version 0.5.0 was attempted to be loaded after version 0.4.1

I think this should be fixed in bash-preexec. There are two options to fix it. One is to avoid loading the new version of bash-preexec when an older version of bash-preexec is already loaded. The other is to make the new version properly overwrite the older version that is already loaded.

jimwins commented 7 months ago

This issue would be good to address. I just ran into the situation where one package (WezTerm) had loaded a 0.4.1 version of bash-preexec and then another (Atuin) loaded 0.5.0 and things broke in mysterious ways. Adding a check against __bp_imported to later versions of bash-preexec would make sense at least as a minimal step.