rcaloras / bash-preexec

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

Bash 5.1 supports making PROMPT_COMMAND an array #130

Open dimo414 opened 2 years ago

dimo414 commented 2 years ago

This isn't strictly a bug/FR; if you feel like enabling Discussions for this repo I can repost this as a discussion

I noticed Bash 5.1 allows PROMPT_COMMAND to be an array:

ee. PROMPT_COMMAND: can now be an array variable, each element of which can contain a command to be executed like a string PROMPT_COMMAND variable.

This obviously won't help older releases that bash-preexec continues to support, but it's an interesting new development we might want to utilize. It would help situations like #129 for instance.

akinomyoga commented 2 years ago

FYI, in the bug-bash mailing list, there has been a discussion on how to handle the situation when there are both types of configurations that assumes the scalar PROMPT_COMMAND and the array PROMPT_COMMAND. [ Note that there have been two distinct variables PROMPT_COMMAND and PROMPT_COMMANDS in 5.1-alpha, but PROMPT_COMMANDS are renamed to PROMPT_COMMAND before the release after the suggestion by Martijn. ] If it would use bash-5.1 PROMPT_COMMAND, I recommend adopting Martijn's suggestion (see the original thread for the backgrounds):

if ((BASH_VERSINFO[0] > 5 || BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] >= 1)); then
  PROMPT_COMMAND=${PROMPT_COMMAND-}
  PROMPT_COMMAND+=("some command here")
else
  # ....
fi
akinomyoga commented 1 year ago

I realized that Bash 5.1 PROMPT_COMMAND breaks bash-preexec if there is an element in PROMPT_COMMAND with its index greater than 0.

The following ~/.bashrc would reproduce the issue:

# ~/.bashrc

PROMPT_COMMAND=
PROMPT_COMMAND[1]='((++prompt_count))'
preexec() { echo PREEXEC >/dev/tty; }
precmd() { echo PRECMD >/dev/tty; }
source ~/.mwg/git/rcaloras/bash-preexec/bash-preexec.sh

With this ~/.bashrc, the result becomes

$ echo hello
hello
PRECMD
PREEXEC
$

This is because PROMPT_COMMAND becomes

declare -a PROMPT_COMMAND=([0]=$'__bp_precmd_invoke_cmd\n__bp_interactive_mode' [1]="((++prompt_count))")

so that ${PROMPT_COMMAND[1]} (((++prompt_count))) is executed just after __bp_interactive_mode. The function call __bp_interactive_mode needs to be put after the last element of PROMPT_COMMAND in Bash 5.1+.

rcaloras commented 1 year ago

@akinomyoga thanks for reporting. Any idea how prevalent an issue this is? Not sure how many people are already using bash 5.1 and using it in this fashion.

Regardless, do you think this simple enough that we could just introduce a conditional in __bp_install to handle this accordingly?

akinomyoga commented 1 year ago

Not sure how many people are already using bash 5.1 and using it in this fashion.

Now the latest releases of Linux distributions already use Bash 5.1 or 5.2, so I think many people are already using Bash 5.1 & 5.2.

Nevertheless, I guess not so many Bash configurations have started to use PROMPT_COMMAND[1], PROMPT_COMMAND[2], …, yet I think I have seen some scripts using it (e.g. kitty's shell-integration).

Any idea how prevalent an issue this is?

Currently, a small number of configurations use the array PROMPT_COMMAND with a test on the Bash version, but I think people are going to start using the array PROMPT_COMMAND when Bash 5.0 and lower versions mostly disappear from the market.

Regardless, do you think this simple enough that we could just introduce a conditional in __bp_install to handle this accordingly?

Yes, I think so. I'll submit a PR for it (along with a suggested solution to #140).

zachbai commented 1 year ago

Was this issue indeed fixed by #141?