git-prompt: cleaning and improvement
cover git-prompt: cleaning and improvement
The previously proposed patch set to enable color
prompt in non-PROMPT_COMMAND mode
(http://article.gmane.org/gmane.comp.version-control.git/228017)
introduce some problems with command line editing/browsing/completion, as explained by Simon Oosthoek, in
http://article.gmane.org/gmane.comp.version-control.git/228308 .
Some of the patches could still be used, so I'm
reposting them with an additional one to add some
missing information in usage comments.
Eduardo R. D'Avila (4):
t9903: add tests for git-prompt pcmode git-prompt.sh: refactor colored prompt code git-prompt.sh: do not print duplicate clean color code git-prompt.sh: add missing information in comments
contrib/completion/git-prompt.sh | 94 +++++---------- t/t9903-bash-prompt.sh | 254 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 285 insertions(+), 63 deletions(-)
Signature · 1 line
1.8.3.1.487.g28387b2
PATCH 1/4 t9903: add tests for git-prompt pcmode
git-prompt.sh lacks tests for PROMPT_COMMAND mode.
Add tests for:
* pcmode prompt without colors
* pcmode prompt with colors for bash
* pcmode prompt with colors for zsh
Having these tests enables an upcoming refactor in a safe way.
---
254 0 t/t9903-bash-prompt.sh
t/t9903-bash-prompt.sh | 254 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 254 insertions(+)
t/t9903-bash-prompt.sh +254 -0
@@ -10,6 +10,10 @@ test_description='test git-specific bash prompt functions' . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh" actual="$TRASH_DIRECTORY/actual" +c_red='\\[\\e[31m\\]' +c_green='\\[\\e[32m\\]' +c_lblue='\\[\\e[1;34m\\]' +c_clear='\\[\\e[0m\\]' test_expect_success 'setup for prompt tests' ' mkdir -p subdir/subsubdir &&
@@ -535,4 +539,254 @@ test_expect_success 'prompt - format string starting with dash' ' test_cmp expected "$actual" ' +test_expect_success 'prompt - pc mode' ' + printf "BEFORE: (master):AFTER" >expected && + printf "" >expected_output && + ( + __git_ps1 "BEFORE:" ":AFTER" >"$actual" && + test_cmp expected_output "$actual" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - branch name' ' + printf "BEFORE: (${c_green}master${c_clear}${c_clear}):AFTER" >expected && + ( + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" >"$actual" + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - detached head' ' + printf "BEFORE: (${c_red}(%s...)${c_clear}${c_clear}):AFTER" $(git log -1 --format="%h" b1^) >expected && + git checkout b1^ && + test_when_finished "git checkout master" && + ( + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_red}*${c_clear}):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + ( + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_green}+${c_clear}):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + git add -u && + ( + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER" >expected && + echo "dirty index" >file && + test_when_finished "git reset --hard" && + git add -u && + echo "dirty worktree" >file && + ( + GIT_PS1_SHOWCOLORHINTS=y && + GIT_PS1_SHOWDIRTYSTATE=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_green}#${c_clear}):AFTER" >expected && + ( + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + cd otherrepo && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - inside .git directory' ' + printf "BEFORE: (${c_green}GIT_DIR!${c_clear}${c_clear}):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + ( + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + cd .git && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - stash status indicator' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_lblue}\$${c_clear}):AFTER" >expected && + echo 2 >file && + git stash && + test_when_finished "git stash drop" && + ( + GIT_PS1_SHOWSTASHSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - bash color pc mode - untracked files status indicator' ' + printf "BEFORE: (${c_green}master${c_clear} ${c_red}%%${c_clear}):AFTER" >expected && + ( + GIT_PS1_SHOWUNTRACKEDFILES=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - branch name' ' + printf "BEFORE: (%%F{green}master%%f%%f):AFTER" >expected && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" >"$actual" + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - detached head' ' + printf "BEFORE: (%%F{red}(%s...)%%f%%f):AFTER" $(git log -1 --format="%h" b1^) >expected && + git checkout b1^ && + test_when_finished "git checkout master" && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - dirty status indicator - dirty worktree' ' + printf "BEFORE: (%%F{green}master%%f %%F{red}*%%f):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - dirty status indicator - dirty index' ' + printf "BEFORE: (%%F{green}master%%f %%F{green}+%%f):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + git add -u && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - dirty status indicator - dirty index and worktree' ' + printf "BEFORE: (%%F{green}master%%f %%F{red}*%%F{green}+%%f):AFTER" >expected && + echo "dirty index" >file && + test_when_finished "git reset --hard" && + git add -u && + echo "dirty worktree" >file && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWCOLORHINTS=y && + GIT_PS1_SHOWDIRTYSTATE=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - dirty status indicator - before root commit' ' + printf "BEFORE: (%%F{green}master%%f %%F{green}#%%f):AFTER" >expected && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + cd otherrepo && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - inside .git directory' ' + printf "BEFORE: (%%F{green}GIT_DIR!%%f%%f):AFTER" >expected && + echo "dirty" >file && + test_when_finished "git reset --hard" && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWDIRTYSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + cd .git && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - stash status indicator' ' + printf "BEFORE: (%%F{green}master%%f %%F{blue}$%%f):AFTER" >expected && + echo 2 >file && + git stash && + test_when_finished "git stash drop" && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWSTASHSTATE=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + +test_expect_success 'prompt - zsh color pc mode - untracked files status indicator' ' + printf "BEFORE: (%%F{green}master%%f %%F{red}%%%%%%f):AFTER" >expected && + ( + ZSH_VERSION=5.0.0 && + GIT_PS1_SHOWUNTRACKEDFILES=y && + GIT_PS1_SHOWCOLORHINTS=y && + __git_ps1 "BEFORE:" ":AFTER" && + printf "%s" "$PS1" >"$actual" + ) && + test_cmp expected "$actual" +' + test_done
Signature · 1 line
1.8.3.1.487.g28387b2
PATCH 2/4 git-prompt.sh: refactor colored prompt code
In reply to Eduardo R. D'Avila__git_ps1_colorize_gitstring() sets color codes and builds the prompt gitstring. It has duplicated code to handle color codes for bash and zsh shells. __git_ps1() also has duplicated logic to build the prompt gitstring.
Remove duplication of logic to build gitstring in__git_ps1_colorize_gitstring() and __git_ps1().
Leave in __git_ps1_colorize_gitstring() only logic to set color codes.
---
26 59 contrib/completion/git-prompt.sh
contrib/completion/git-prompt.sh | 85 ++++++++++++---------------------------- 1 file changed, 26 insertions(+), 59 deletions(-)
contrib/completion/git-prompt.sh +26 -59
@@ -225,8 +225,8 @@ __git_ps1_show_upstream () } # Helper function that is meant to be called from __git_ps1. It -# builds up a gitstring injecting color codes into the appropriate -# places. +# injects color codes into the appropriate gitstring variables used +# to build a gitstring. __git_ps1_colorize_gitstring () { if [[ -n ${ZSH_VERSION-} ]]; then
@@ -234,74 +234,40 @@ __git_ps1_colorize_gitstring () local c_green='%F{green}' local c_lblue='%F{blue}' local c_clear='%f' - local bad_color=$c_red - local ok_color=$c_green - local branch_color="$c_clear" - local flags_color="$c_lblue" - local branchstring="$c${b##refs/heads/}" - - if [ $detached = no ]; then - branch_color="$ok_color" - else - branch_color="$bad_color" - fi - - gitstring="$branch_color$branchstring$c_clear" - - if [ -n "$w$i$s$u$r$p" ]; then - gitstring="$gitstring$z" - fi - if [ "$w" = "*" ]; then - gitstring="$gitstring$bad_color$w" - fi - if [ -n "$i" ]; then - gitstring="$gitstring$ok_color$i" - fi - if [ -n "$s" ]; then - gitstring="$gitstring$flags_color$s" - fi - if [ -n "$u" ]; then - gitstring="$gitstring$bad_color$u" - fi - gitstring="$gitstring$c_clear$r$p" - return + else + # Using \[ and \] around colors + # is necessary to prevent wrapping issues! + local c_red='\[\e[31m\]' + local c_green='\[\e[32m\]' + local c_lblue='\[\e[1;34m\]' + local c_clear='\[\e[0m\]' fi - local c_red='\e[31m' - local c_green='\e[32m' - local c_lblue='\e[1;34m' - local c_clear='\e[0m' local bad_color=$c_red local ok_color=$c_green - local branch_color="$c_clear" local flags_color="$c_lblue" - local branchstring="$c${b##refs/heads/}" + local branch_color="" if [ $detached = no ]; then branch_color="$ok_color" else branch_color="$bad_color" fi + c="$branch_color$c" + b="$b$c_clear" - # Setting gitstring directly with \[ and \] around colors - # is necessary to prevent wrapping issues! - gitstring="\[$branch_color\]$branchstring\[$c_clear\]" - - if [ -n "$w$i$s$u$r$p" ]; then - gitstring="$gitstring$z" - fi if [ "$w" = "*" ]; then - gitstring="$gitstring\[$bad_color\]$w" + w="$bad_color$w" fi if [ -n "$i" ]; then - gitstring="$gitstring\[$ok_color\]$i" + i="$ok_color$i" fi if [ -n "$s" ]; then - gitstring="$gitstring\[$flags_color\]$s" + s="$flags_color$s" fi if [ -n "$u" ]; then - gitstring="$gitstring\[$bad_color\]$u" + u="$bad_color$u" fi - gitstring="$gitstring\[$c_clear\]$r$p" + r="$c_clear$r" } # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
@@ -443,19 +409,20 @@ __git_ps1 () fi local z="${GIT_PS1_STATESEPARATOR-" "}" + + # NO color option unless in PROMPT_COMMAND mode + if [ $pcmode = yes ] && [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then + __git_ps1_colorize_gitstring + fi + local f="$w$i$s$u" + local gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p" + if [ $pcmode = yes ]; then - local gitstring= - if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then - __git_ps1_colorize_gitstring - else - gitstring="$c${b##refs/heads/}${f:+$z$f}$r$p" - fi gitstring=$(printf -- "$printf_format" "$gitstring") PS1="$ps1pc_start$gitstring$ps1pc_end" else - # NO color option unless in PROMPT_COMMAND mode - printf -- "$printf_format" "$c${b##refs/heads/}${f:+$z$f}$r$p" + printf -- "$printf_format" "$gitstring" fi fi }
Signature · 1 line
1.8.3.1.487.g28387b2
PATCH 3/4 git-prompt.sh: do not print duplicate clean color code
In reply to Eduardo R. D'AvilaDo not print a duplicate clean color code when there is no other indicators other than the current branch in colored prompt.
---
1 1 contrib/completion/git-prompt.sh
6 6 t/t9903-bash-prompt.sh
contrib/completion/git-prompt.sh | 2 +- t/t9903-bash-prompt.sh | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-)
contrib/completion/git-prompt.sh +1 -1
@@ -253,8 +253,8 @@ __git_ps1_colorize_gitstring () branch_color="$bad_color" fi c="$branch_color$c" - b="$b$c_clear" + z="$c_clear$z" if [ "$w" = "*" ]; then w="$bad_color$w" fi
t/t9903-bash-prompt.sh +6 -6
@@ -551,7 +551,7 @@ test_expect_success 'prompt - pc mode' ' ' test_expect_success 'prompt - bash color pc mode - branch name' ' - printf "BEFORE: (${c_green}master${c_clear}${c_clear}):AFTER" >expected && + printf "BEFORE: (${c_green}master${c_clear}):AFTER" >expected && ( GIT_PS1_SHOWCOLORHINTS=y && __git_ps1 "BEFORE:" ":AFTER" >"$actual"
@@ -561,7 +561,7 @@ test_expect_success 'prompt - bash color pc mode - branch name' ' ' test_expect_success 'prompt - bash color pc mode - detached head' ' - printf "BEFORE: (${c_red}(%s...)${c_clear}${c_clear}):AFTER" $(git log -1 --format="%h" b1^) >expected && + printf "BEFORE: (${c_red}(%s...)${c_clear}):AFTER" $(git log -1 --format="%h" b1^) >expected && git checkout b1^ && test_when_finished "git checkout master" && (
@@ -627,7 +627,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - befo ' test_expect_success 'prompt - bash color pc mode - inside .git directory' ' - printf "BEFORE: (${c_green}GIT_DIR!${c_clear}${c_clear}):AFTER" >expected && + printf "BEFORE: (${c_green}GIT_DIR!${c_clear}):AFTER" >expected && echo "dirty" >file && test_when_finished "git reset --hard" && (
@@ -666,7 +666,7 @@ test_expect_success 'prompt - bash color pc mode - untracked files status indica ' test_expect_success 'prompt - zsh color pc mode - branch name' ' - printf "BEFORE: (%%F{green}master%%f%%f):AFTER" >expected && + printf "BEFORE: (%%F{green}master%%f):AFTER" >expected && ( ZSH_VERSION=5.0.0 && GIT_PS1_SHOWCOLORHINTS=y &&
@@ -677,7 +677,7 @@ test_expect_success 'prompt - zsh color pc mode - branch name' ' ' test_expect_success 'prompt - zsh color pc mode - detached head' ' - printf "BEFORE: (%%F{red}(%s...)%%f%%f):AFTER" $(git log -1 --format="%h" b1^) >expected && + printf "BEFORE: (%%F{red}(%s...)%%f):AFTER" $(git log -1 --format="%h" b1^) >expected && git checkout b1^ && test_when_finished "git checkout master" && (
@@ -748,7 +748,7 @@ test_expect_success 'prompt - zsh color pc mode - dirty status indicator - befor ' test_expect_success 'prompt - zsh color pc mode - inside .git directory' ' - printf "BEFORE: (%%F{green}GIT_DIR!%%f%%f):AFTER" >expected && + printf "BEFORE: (%%F{green}GIT_DIR!%%f):AFTER" >expected && echo "dirty" >file && test_when_finished "git reset --hard" && (
Signature · 1 line
1.8.3.1.487.g28387b2
PATCH 4/4 git-prompt.sh: add missing information in comments
In reply to Eduardo R. D'AvilaMention that the command below is needed for prompt in ZSH with PS1:
setopt PROMPT_SUBST
Make it clear that colored prompt is only available in PROMPT_COMMAND mode.
---
5 4 contrib/completion/git-prompt.sh
contrib/completion/git-prompt.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
contrib/completion/git-prompt.sh +5 -4
@@ -13,10 +13,10 @@ # 3a) Change your PS1 to call __git_ps1 as # command-substitution: # Bash: PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ ' -# ZSH: PS1='[%n@%m %c$(__git_ps1 " (%s)")]\$ ' +# ZSH: setopt PROMPT_SUBST ; PS1='[%n@%m %c$(__git_ps1 " (%s)")]\$ ' # the optional argument will be used as format string. -# 3b) Alternatively, if you are using bash, __git_ps1 can be -# used for PROMPT_COMMAND with two parameters, <pre> and +# 3b) Alternatively, if you are using Bash or ZSH, __git_ps1 can +# be used for PROMPT_COMMAND with two parameters, <pre> and # <post>, which are strings you would put in $PS1 before # and after the status string generated by the git-prompt # machinery. e.g.
@@ -78,7 +78,8 @@ # # If you would like a colored hint about the current dirty state, set # GIT_PS1_SHOWCOLORHINTS to a nonempty value. The colors are based on -# the colored output of "git status -sb". +# the colored output of "git status -sb" and are available only when +# using __git_ps1 for PROMPT_COMMAND. # __gitdir accepts 0 or 1 arguments (i.e., location) # returns location of .git repo
Signature · 1 line
1.8.3.1.487.g28387b2
Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode
In reply to Eduardo R. D'AvilaHi,
Quoted 11 lines · Eduardo R. D'Avila
git-prompt.shlacks tests forPROMPT_COMMANDmode.Add tests for:
* pcmode prompt without colors
* pcmode prompt with colors for bash
* pcmode prompt with colors for zshHaving these tests enables an upcoming refactor in a safe way.
Signed-off-by Eduardo R. D'Avila <[email protected]>
I doubt the value of separate tests for zsh. They might make sense as long as there are different code paths for doing coloring for the two shells, but after your refactorization in 2/4 there is only one common block of if statements, which is already thoroughly excercised by your tests for bash, making the separate tests for zsh redundant.
Gábor
Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code
In reply to Eduardo R. D'Avilacontrib/completion/git-prompt.sh __git_ps1_colorize_gitstring () · line 272
Quoted 9 lines · Eduardo R. D'Avila <erdavila <at> gmail.com> writes:
contrib/completion/git-prompt.sh +4
-4+ local c_red='\[\e[31m\]' + local c_green='\[\e[32m\]' + local c_lblue='\[\e[1;34m\]' + local c_clear='\[\e[0m\]' fi - local c_red='\e[31m' - local c_green='\e[32m' - local c_lblue='\e[1;34m' - local c_clear='\e[0m'
I've gotten the impression it's better to use tput to generate the escape sequences instead of hardcoding them. So something like:
local c_red='\['"$(tput setaf 1)"'\]' local c_green='\['"$(tput setaf 2)"'\]' local c_green='\['"$(tput setaf 4)"'\]' local c_clear='\['"$(tput sgr0)"'\]'
which is technically cleaner, if not visually.
The problem with that approach is that tput will be run several times for each prompt, so it would be best if the color variables were global. Another thing is that you rely on tput being available.
Øsse
Re: [PATCH 3/4] git-prompt.sh: do not print duplicate clean color code
In reply to Eduardo R. D'AvilaQuoted 5 lines · Eduardo R. D'Avila
Do not print a duplicate clean color code when there is no other indicators other than the current branch in colored prompt.
Signed-off-by Eduardo R. D'Avila <[email protected]>
Great. I wanted to point out in the previous versions of this series that the patch claiming to refactor coloring actually touches the test script because it silently fixes the clean color code duplication. Thanks for splitting that patch before I even managed to find the time to mention it ;)
Re: [PATCH 4/4] git-prompt.sh: add missing information in comments
In reply to Eduardo R. D'AvilaQuoted 18 lines · Eduardo R. D'Avila
Mention that the command below is needed for prompt in ZSH with PS1:
setoptPROMPT_SUBSTMake it clear that colored prompt is only available in
PROMPT_COMMANDmode.Signed-off-by Eduardo R. D'Avila <[email protected]>---
5 4contrib/completion/git-prompt.shcontrib/completion/git-prompt.sh | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)contrib/completion/git-prompt.sh +0
-0
contrib/completion/git-prompt.sh line 19
Quoted 10 lines
contrib/completion/git-prompt.sh +3
-3@@ -13,10 +13,10 @@ # 3a) Change your PS1 to call __git_ps1 as # command-substitution: # Bash: PS1='[\u@\h \W$(__git_ps1 " (%s)")]\$ ' -# ZSH: PS1='[%n@%m %c$(__git_ps1 " (%s)")]\$ ' +# ZSH: setopt PROMPT_SUBST ; PS1='[%n@%m %c$(__git_ps1 " (%s)")]\$ ' # the optional argument will be used as format string. -# 3b) Alternatively, if you are using bash, __git_ps1 can be -# used for PROMPT_COMMAND with two parameters, <pre> and +# 3b) Alternatively, if you are using Bash or ZSH, __git_ps1 can +# be used for PROMPT_COMMAND with two parameters, <pre> and
The git-prompt script only supports bash and zsh, so that "if you are using Bash or ZSH" part doesn't say much, does it? Furthermore, zsh doesn't have PROMPT_COMMAND but a similar facility. So how about something like this instead?
# 3b) Alternatively, __git_ps1 can be used for PROMPT_COMMAND in
# Bash or for precmd in ZSH with two parameters, <pre> and
contrib/completion/git-prompt.sh line 82
Quoted 10 lines
contrib/completion/git-prompt.sh +2
-1# <post>, which are strings you would put in $PS1 before # and after the status string generated by the git-prompt # machinery. e.g.@@ -78,7 +78,8 @@ # # If you would like a colored hint about the current dirty state, set # GIT_PS1_SHOWCOLORHINTS to a nonempty value. The colors are based on -# the colored output of "git status -sb". +# the colored output of "git status -sb" and are available only when +# using __git_ps1 for PROMPT_COMMAND.
Likewise:
# using __git_ps1 for PROMPT_COMMAND or precmd.
Quoted 6 lines
contrib/completion/git-prompt.sh +0
-0# __gitdir accepts 0 or 1 arguments (i.e., location) # returns location of .git repoSignature · 1 line
1.8.3.1.487.g28387b2
Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode
In reply to SZEDER Gábor2013/6/22 SZEDER Gábor <[email protected]>:
Quoted 11 lines · Eduardo R. D'Avila
Quoted 10 lines
git-prompt.shlacks tests forPROMPT_COMMANDmode.Add tests for:
* pcmode prompt without colors
* pcmode prompt with colors for bash
* pcmode prompt with colors for zshHaving these tests enables an upcoming refactor in a safe way.
Quoted 7 lines
Quoted 1 line
Signed-off-by Eduardo R. D'Avila <[email protected]>I doubt the value of separate tests for zsh. They might make sense as long as there are different code paths for doing coloring for the two shells, but after your refactorization in 2/4 there is only one common block of if statements, which is already thoroughly excercised by your tests for bash, making the separate tests for zsh redundant.
These tests where important to make sure that I wouldn't break anything during the refactorization. Having them pass before *and* after refactorization guarantees that nothing was broken (except for some subtle case that might have not be included in the tests).
However, I agree that they became redundant.
Would it make sense to include a patch that only removes the zsh tests, after the
refactorization? I'm considering that *1* simple zsh test must be kept during this removal, to make sure the code path for zsh is run.
Eduardo
Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode
Quoted 4 lines · Eduardo D'Avila
However, I agree that they became redundant.
Would it make sense to include a patch that only removes the zsh tests, after the refactorization?
I think that would be a sensible thing to do.
In any case, Szeder seems to be giving a good guidance on patches in the completion area (not just this series), and completion is not something I am deeply interested in, so I'll watch from the sidelines and wait until you two to figure out the best solution ;-)
Thanks for working on this topic.
Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code
In reply to Eduardo R. D'Avila2013/6/22 Øystein Walle <[email protected]>:
Quoted 8 lines
I've gotten the impression it's better to use tput to generate the escape sequences instead of hardcoding them. So something like:
local c_red='\['"$(tput setaf 1)"'\]'
local c_green='\['"$(tput setaf 2)"'\]'
local c_green='\['"$(tput setaf 4)"'\]'
local c_clear='\['"$(tput sgr0)"'\]'
Quoted 5 lines
which is technically cleaner, if not visually.
The problem with that approach is that tput will be run several times for each prompt, so it would be best if the color variables were global. Another thing is that you rely on tput being available.
Are there any guidelines regarding global shell script variables?
I'm considering doing this:
__git_c_red="\[$(tput setaf 1 || echo -e '\e[31m')\]"
which handles the case where tput is not available.
Eduardo
Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code
Quoted 9 lines · Eduardo D'Avila
2013/6/22 Øystein Walle <[email protected]>:
Quoted 8 lines
I've gotten the impression it's better to use tput to generate the escape sequences instead of hardcoding them. So something like:
local c_red='\['"$(tput setaf 1)"'\]'
local c_green='\['"$(tput setaf 2)"'\]'
local c_green='\['"$(tput setaf 4)"'\]'
local c_clear='\['"$(tput sgr0)"'\]'
Quoted 7 lines
Quoted 5 lines
which is technically cleaner, if not visually.
The problem with that approach is that tput will be run several times for each prompt, so it would be best if the color variables were global. Another thing is that you rely on tput being available.
Are there any guidelines regarding global shell script variables?
There are a couple of global variables in the completion script, they are all prefixed with "__git_", e.g. $__git_porcelain_commands. In the prompt script we don't have any global variables at the moment, but if we were to create some, then they should be prefixed similarly. Or perhaps with "__git_ps1_". Anyway, in case of a global variable I'd spell out "color" completely instead of abbreviating it as "c".
Quoted 3 lines
I'm considering doing this:
__git_c_red="\[$(tput setaf 1 || echo -e '\e[31m')\]"which handles the case where tput is not available.
To me 'tput setaf 1' is just a little bit less greek than '\e[31m'.
I'm wary of relying on tput's availability. It's part of ncurses, which is an essential package in many (most? all?) linux distros, but I don't know how it is with other supported platforms. So I think we'd have to stick to the hard-coded escape sequences as a fallback anyway. And if we can't get rid of these escape sequences completely, then I don't really see the point of using tput, especially considering the additional delay that would be caused by fork()ing four subshells and fork()+exec()ing four external commands on Windows.
However, I don't know much about the caveats of terminals, so I can't judge the benefits of using tput instead of the escape sequences.
Gábor
Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode
In reply to Eduardo D'AvilaQuoted 10 lines · Eduardo D'Avila
These tests where important to make sure that I wouldn't break anything during the refactorization. Having them pass before *and* after refactorization guarantees that nothing was broken (except for some subtle case that might have not be included in the tests).
However, I agree that they became redundant.
Would it make sense to include a patch that only removes the zsh tests, after the
refactorization?
That would be fine with me.
Thanks,
Gábor
Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code
In reply to SZEDER Gábor2013/6/23 SZEDER Gábor <[email protected]>:
Quoted 8 lines
I'm wary of relying on tput's availability. It's part of ncurses, which is an essential package in many (most? all?) linux distros, but I don't know how it is with other supported platforms. So I think we'd have to stick to the hard-coded escape sequences as a fallback anyway. (...)
However, I don't know much about the caveats of terminals, so I can't judge the benefits of using tput instead of the escape sequences.
I'm exactly in the same situation...
Quoted 2 lines
considering the additional delay that would be caused by fork()ing four subshells and fork()+exec()ing four external commands on Windows.
Well... That would be only once, during script loading.
Given the concerns raised by Gábor (edited and quoted above) and that there is no known issue (afaik) with the current implementation, I'm tending to revert to the escape sequences.