Open ilikepi opened 8 years ago
It's been so long that a lot of this has seeped out of my head by now, but I was just re-reading my comment at https://github.com/postmodern/chruby/issues/227#issue-23588607 and thinking about the difference between calling
chruby_auto
and
[[ "$BASH_COMMAND" != "$PROMPT_COMMAND" ]] && chruby_auto
As I understand it, this change would repeatedly call chruby_auto
without the initial "$BASH_COMMAND" != "$PROMPT_COMMAND"
check. Does that have any side-effect when using bash?
@aprescott In my proposed change, the check of $PROMPT_COMMAND
is still present within its original location in the DEBUG trap (line 36). The check prevents the trap from executing when $PROMPT_COMMAND
itself executes. If the check weren't there and the user had $PROMPT_COMMAND
set, the trap code would run twice:
$ export PROMPT_COMMAND='echo prompt'
prompt
$ pwd
/Users/ilikepi
prompt
$ function saytrap() {
> echo trap
> }
prompt
$ trap 'saytrap' DEBUG
trap
prompt
$ pwd
trap
/Users/ilikepi
trap
prompt
I believe the check is most appropriate within the trap rather than in the wrapper function. If the check were moved to within the wrapper, that could easily lead to issues when a user tried to override the wrapper.
+1 for this PR.
I set my own DEBUG
trap (to time each shell command), and it gets ugly if not all contained in one (well named) function.
Are there any objections to this? (I note it's over a year old)
How about providing a common chruby_hook
function that provides an entry-point for any/all hooks that run after every command, which auto.sh
adds to.
How about providing a common
chruby_hook
function that provides an entry-point for any/all hooks that run after every command, whichauto.sh
adds to.
Isn’t that exactly preexec
?
Isn’t that exactly
preexec
?
Unfortunately Bash (in particular Bash 3 which macOS still ships) does not have preexec
.
Then I’m once again advertising for https://github.com/rcaloras/bash-preexec, which saves us from manually fiddling with DEBUG
trap (which is difficult to get right, anyway). It also fixes #367 for free: https://github.com/rcaloras/bash-preexec/blob/master/test/bash-preexec.bats#L198-L212
If users are still using trap
they would come across other issues anyway, possibly not just #227 and #367. I think the right way to use trap
is that there is only one thing in a Bash instance that owns the trap
.
For example, what if we have chpython
, chnode
, and chperl
? Compare the hook solution
function chruby_hook() {
chpython_auto
}
function chpython_hook() {
chnode_auto
}
function chnode_hook() {
chperl_auto
}
with the bash-preexec solution
preexec_functions+=(chruby_auto chpython_auto chnode_auto chperl_auto)
In addition, what about #399? Hence, my proposal: if preexec exist, use it (Bash or Zsh); fall back to trap
(so that a fresh clean Bash “just works”).
Then I’m once again advertising for https://github.com/rcaloras/bash-preexec,
How did I not see this sooner? bash-preexec looks like it would solve our problems and allow for better interoperability with other tools that wish to hook DEBUG
. I am a little hesitant about adding a dependency that's not readily available via package managers. However, we could vendor bash-preexec and only load it when under bash?
Presumably you'd also check to see if bash-preexec
was already loaded before attempting to load the vendored version?
Personally I'm not sure the added cost of vendoring a dependency is a net gain over the proposal in this PR, or the alternate proposal in https://github.com/postmodern/chruby/pull/355#issuecomment-770474556. Incorporating bash-preexec
seems counter to the minimalist nature of chruby. It could always be documented as a recommended addon for people who didn't want to mess with the DEBUG trap explicitly.
I’m recommending detecting bash-preexec
. I’m not suggesting vendoring it.
I’m recommending detecting bash-preexec. I’m not suggesting vendoring it.
Then to me, that change seems like it would be independent of this PR. Personally I'd prefer not to have to install a separate add-on in order to be able to more easily control when chruby_auto
is called.
This change implements the proposal in #227 and likely addresses #346 as well. All tests pass on OS X 10.10.
It occurs to me that this is somewhat of a breaking change for zsh users who expect chruby_auto to be added directly to preexec_functions (for example, the code in this comment from issue #191).