Return original value from __bp_precmd_invoke_cmd

[jordemort]

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]

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

[jordemort]

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]

@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

bash-preexec/test/bash-preexec.bats

Line 178 in fd2ffa8

[ $status -eq 0 ]

to account for it? Should be 251 I think in this case.

[rcaloras]

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]

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

[dimo414]

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]

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]

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

[jordemort]

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

[jordemort]

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

[rcaloras]

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