romkatv / gitstatus

Git status for Bash and Zsh prompt
GNU General Public License v3.0
1.65k stars 101 forks source link

Append to PROMPT_COMMAND #403

Closed samiam closed 10 months ago

samiam commented 11 months ago

Issue

The problem is when using other BASH prompting packages, gitstatus clobbers the PROMPT_COMMAND envar.

This results in an ordering issue; gitstatus has to be the first package to be sourced before other prompt packages, otherwise gitstatus will clobber the existing PROMPT_COMMAND.

This patch simply appends gitstatus to the PROMPT_COMMAND array so that source order becomes irrelevant and gitstatus plays nicely with other prompt packages.

From Bash manual:

 PROMPT_COMMAND
    If this variable is set, and is an array, the value of each set
    element is executed as a command prior to issuing each primary
    prompt.  If this is set but not an array variable, its value is
    used as a command to execute instead.

Testing current behavior

# gitstatus last - broken
~$ PS1='\w\$ ' bash --norc
~$ echo ${PROMPT_COMMAND[@]}

~$ source ${HOMEBREW_PREFIX}/opt/kube-ps1/share/kube-ps1.sh
~$ echo ${PROMPT_COMMAND[@]}
_kube_ps1_prompt_update;:
~$ source ${HOMEBREW_PREFIX}/opt/gitstatus/gitstatus.prompt.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
gitstatus_prompt_update
samiam@mac ~
$ exit
exit

# gitstatus first - ok
~$ PS1='\w\$ ' bash --norc
~$ echo ${PROMPT_COMMAND[@]}

~$ source ${HOMEBREW_PREFIX}/opt/gitstatus/gitstatus.prompt.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
gitstatus_prompt_update
samiam@mac ~
$ source ${HOMEBREW_PREFIX}/opt/kube-ps1/share/kube-ps1.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
_kube_ps1_prompt_update;gitstatus_prompt_update
samiam@mac ~
$ exit
exit

Testing patched behavior

# gitstatus last - ok
~$ PS1='\w\$ ' bash --norc
~$ echo ${PROMPT_COMMAND[@]}

~$ source ${HOMEBREW_PREFIX}/opt/kube-ps1/share/kube-ps1.sh
~$ echo ${PROMPT_COMMAND[@]}
_kube_ps1_prompt_update;:
~$ source ${HOMEBREW_PREFIX}/opt/gitstatus/gitstatus.prompt.p1.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
_kube_ps1_prompt_update;: gitstatus_prompt_update
samiam@mac ~
$ exit
exit

# gitstatus first - ok
~$ PS1='\w\$ ' bash --norc
~$ echo ${PROMPT_COMMAND[@]}

~$ source ${HOMEBREW_PREFIX}/opt/gitstatus/gitstatus.prompt.p1.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
gitstatus_prompt_update
samiam@mac ~
$ source ${HOMEBREW_PREFIX}/opt/kube-ps1/share/kube-ps1.sh
samiam@mac ~
$ echo ${PROMPT_COMMAND[@]}
_kube_ps1_prompt_update;gitstatus_prompt_update
samiam@mac ~
$ exit
exit
romkatv commented 11 months ago

PROMPT_COMMAND as an array is supported only in bash starting with 5.1. What happens when this code is used on older bash?

Some people source their startup files more than once. Would this cause performance degradation?

samiam commented 11 months ago

I've addressed your concerns by using variable assignment as done in kube_ps1.

romkatv commented 10 months ago

How would this work?

source gitstatus.prompt.sh

# This line is executed by another plugin that also wants to hook PROMPT_COMMAND.
PROMPT_COMMAND=('something_else' "${PROMPT_COMMAND[@]}")

source gitstatus.prompt.sh

Would this result in two gitstatus_prompt_update calls per prompt?

samiam commented 10 months ago

It would result in two gitstatus_prompt_update calls per prompt.

This could be solved by testing both the variable and the array type.

[[ $PROMPT_COMMAND =~ gitstatus_prompt_update || ${PROMPT_COMMAND[@]} =~ gitstatus_prompt_update ]] ||
    PROMPT_COMMAND="gitstatus_prompt_update;${PROMPT_COMMAND:-:}"

Of course, this is leading down the rabbit hole of which type to assign.

If the var is a string, append/prepend to string.
If the var is an array, add an element to the array. If the var is empty, which type do you use? Do you look at the bash version? And how do you determine the var type?

Thoughts?

samiam commented 10 months ago

I'm not exactly proud of this code, but it does work. Let me know if you want me to push this to the PR.

# Assign PROMPT_COMMAND based on whether it's a scalar or array
[[ $PROMPT_COMMAND =~ gitstatus_prompt_update ||
   ${PROMPT_COMMAND[@]} =~ gitstatus_prompt_update ]] || {
  if [[ -n "${PROMPT_COMMAND}" &&
        "$(declare -p PROMPT_COMMAND)" =~ ^"declare -a" ]]; then
    PROMPT_COMMAND+=(gitstatus_prompt_update)
  else
    PROMPT_COMMAND="gitstatus_prompt_update;${PROMPT_COMMAND:-:}"
  fi
}
samiam commented 10 months ago

prompt_test.txt

I've attached a test file for the prompt change. Results:

$ docker run --rm -it -v ~/gitstatus:/gitstatus ubuntu:12.04 bash
root@2a675799447a:/# /gitstatus/prompt_test
4.2.25(1)-release

# On mac
$ ./prompt_test
5.2.15(1)-release
romkatv commented 10 months ago

An additional fork is too great a price for this feature. We are talking about saving the user one line of code at best.

samiam commented 10 months ago

I understand the additional fork is a performance hit.

But I'm confused "about saving the user one line of code". Can you elaborate?

Is the current PR code or the modification below sufficient to fix the issue or did you have something else in mind?

[[ $PROMPT_COMMAND =~ gitstatus_prompt_update || ${PROMPT_COMMAND[@]} =~ gitstatus_prompt_update ]] ||
    PROMPT_COMMAND="gitstatus_prompt_update;${PROMPT_COMMAND:-:}"
romkatv commented 10 months ago

Merged with some changes: https://github.com/romkatv/gitstatus/commit/7e7b5e807df193f678718813ff88a4a97717f121

Thanks!