Skip to contentSkip to conversation

git-prompt: cleaning and improvement

PATCH Superseded Eduardo R. D'Avila 4 participants 15 messages Jun 15, 2016

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

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
#1 lore raw

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
index 15521cc..6a88778 100755
@@ -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
#6 lore raw

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
index 86a4f3f..b02b7b2 100644
@@ -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
#11 lore raw

PATCH 3/4 git-prompt.sh: do not print duplicate clean color code

In reply to 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.

---
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
index b02b7b2..70515cc 100644
@@ -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
index 6a88778..1101adf 100755
@@ -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
#13 lore raw

PATCH 4/4 git-prompt.sh: add missing information in comments

In reply to Eduardo R. D'Avila

Mention 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
index 70515cc..3ab2a69 100644
@@ -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
#2 lore raw

Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode

In reply to Eduardo R. D'Avila

Hi,

Quoted 11 lines · Eduardo R. D'Avila

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.

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

#7 lore raw

Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code

In reply to Eduardo R. D'Avila

contrib/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

#12 lore raw

Re: [PATCH 3/4] git-prompt.sh: do not print duplicate clean color code

In reply to Eduardo R. D'Avila
Quoted 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.

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 ;)

#14 lore raw

Re: [PATCH 4/4] git-prompt.sh: add missing information in comments

In reply to Eduardo R. D'Avila
Quoted 18 lines · Eduardo R. D'Avila

Mention 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 +0 -0
index 70515cc..3ab2a69 100644

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 repo
Signature · 1 line
1.8.3.1.487.g28387b2
#3 lore raw

Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode

In reply to SZEDER Gábor

2013/6/22 SZEDER Gábor <[email protected]>:

Quoted 11 lines · Eduardo R. D'Avila
Quoted 10 lines

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.

Quoted 7 lines
Quoted 1 line

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

#4 lore raw

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.

#8 lore raw

Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code

In reply to Eduardo R. 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 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

#9 lore raw

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

#5 lore raw

Re: [PATCH 1/4] t9903: add tests for git-prompt pcmode

In reply to Eduardo D'Avila
Quoted 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

#10 lore raw

Re: [PATCH 2/4] git-prompt.sh: refactor colored prompt code

In reply to SZEDER Gábor

2013/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.