search

rcaloras / bash-preexec

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

Return original value from `__bp_precmd_invoke_cmd` #131

Closed jordemort closed 2 years ago

jordemort commented 2 years ago

WIthout this change, __bp_precmd_invoke_cmd may not restore $? to its original value after being run.

Since __bp_precmd_invoke_cmd get installed as the first thing in PROMPT_COMMAND, this will cause problems if there is an existing PROMPT_COMMAND (which is perhaps not aware of bash-preexec) that wants to do something with the exit status of the command that was just run.

jordemort commented 2 years ago

Found while using iTerm2's shell integration, filed an issue over there as well: https://gitlab.com/gnachman/iterm2/-/issues/10334

jordemort commented 2 years ago

Ah I see this breaks one of the precmd tests. Is there a good way to only restore the value of $? if it's inside a prompt command?

rcaloras commented 2 years ago

@jordemort thanks for opening and submitting the PR! Doing some small manual tests and I'm able to recreate. I think your fix may be good, it's just the test expects a zero return code from that function and this changes it to be the prior $? value.

Could you update https://github.com/rcaloras/bash-preexec/blob/fd2ffa8876d3940c97ffdc3cc807e43277cf72da/test/bash-preexec.bats#L178 to account for it? Should be 251 I think in this case.

rcaloras commented 2 years ago

FWIW looking at another project that uses PROMPT_COMMAND e.g. https://github.com/nojhan/liquidprompt not sure this preserves the return value either for other functions in PROMPT_COMMAND. My guess is this varies from project to project if it's being maintained.

It's worth noting, that bash-preexec does preserve this for functions which use the preexec and precmd arrays like zsh.

rcaloras commented 2 years ago

cc @dimo414 as well for any thoughts. I think with the test update passing should be good to merge.

dimo414 commented 2 years ago

Change LGTM; I played around with it manually and it doesn't break anything on my end. I'm a little concerned there was some motivating reason this wasn't done earlier, but the behavior makes sense to me.

rcaloras commented 2 years ago

I'm a little concerned there was some motivating reason this wasn't done earlier, but the behavior makes sense to me.

Thanks for reviewing! I feel the same. I can't recall prior motivation/reasoning though.

My impression is there isn't actually a standard here and that one interpretation (the simplest) is that PROMPT_COMMAND functions should simply return their values as normal and should not be responsible for maintaining original $?, unlike how it is maintained in zsh for precmd functions (which we successfully emulate). The main difference being bash-preexec should ideally have less side effects as a library leveraging/manipulating PROMPT_COMMAND, rather than a user's own functions.

rcaloras commented 2 years ago

I'll merge and cut a version in the coming week pending no other feedback.

jordemort commented 2 years ago

Sorry, I started chasing another squirrel and forgot about this. I'll have another look tomorrow.

jordemort commented 2 years ago

Thanks for updating the tests for me! This looks good, and it sounds like it won't break other people's stuff.

rcaloras commented 2 years ago

Now available in latest 0.5.0 release. https://github.com/rcaloras/bash-preexec/releases/tag/0.5.0