[PATCH 0/4] git-prompt: cleaning and improvement

[PATCH 0/4] git-prompt: cleaning and improvement

[Eduardo R. D'Avila]

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

-- 
1.8.3.1.487.g28387b2

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

[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.

Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>
---
254	0	t/t9903-bash-prompt.sh
 t/t9903-bash-prompt.sh | 254 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 254 insertions(+)

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 15521cc..6a88778 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -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
-- 
1.8.3.1.487.g28387b2

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

[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.

Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>
---
26	59	contrib/completion/git-prompt.sh
 contrib/completion/git-prompt.sh | 85 ++++++++++++----------------------------
 1 file changed, 26 insertions(+), 59 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 86a4f3f..b02b7b2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -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
 }
-- 
1.8.3.1.487.g28387b2

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

[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 <erdavila@gmail.com>
---
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(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index b02b7b2..70515cc 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -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
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 6a88778..1101adf 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -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" &&
 	(
-- 
1.8.3.1.487.g28387b2

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

[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.

Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>
---
5	4	contrib/completion/git-prompt.sh
 contrib/completion/git-prompt.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 70515cc..3ab2a69 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -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
-- 
1.8.3.1.487.g28387b2

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

[SZEDER Gábor]

Hi,

On Thu, Jun 20, 2013 at 11:25:26PM -0300, Eduardo R. D'Avila wrote:
> 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.
> 
> Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>

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 3/4] git-prompt.sh: do not print duplicate clean color code

[SZEDER Gábor]

On Thu, Jun 20, 2013 at 11:25:28PM -0300, Eduardo R. D'Avila wrote:
> 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 <erdavila@gmail.com>

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

[SZEDER Gábor]

On Thu, Jun 20, 2013 at 11:25:29PM -0300, Eduardo R. D'Avila wrote:
> 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.
> 
> Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>
> ---
> 5	4	contrib/completion/git-prompt.sh
>  contrib/completion/git-prompt.sh | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 70515cc..3ab2a69 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -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


>  #        <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.

>  
>  # __gitdir accepts 0 or 1 arguments (i.e., location)
>  # returns location of .git repo
> -- 
> 1.8.3.1.487.g28387b2
> 

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

[Øystein Walle]

Eduardo R. D'Avila <erdavila <at> gmail.com> writes:

> +		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 1/4] t9903: add tests for git-prompt pcmode

[Eduardo D'Avila]

2013/6/22 SZEDER Gábor <szeder@ira.uka.de>:
> On Thu, Jun 20, 2013 at 11:25:26PM -0300, Eduardo R. D'Avila wrote:
>> 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.
>>
>> Signed-off-by: Eduardo R. D'Avila <erdavila@gmail.com>
>
> 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 2/4] git-prompt.sh: refactor colored prompt code

[Eduardo D'Avila]

2013/6/22 Øystein Walle <oystwa@gmail.com>:
> 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.

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 1/4] t9903: add tests for git-prompt pcmode

[Junio C Hamano]

"Eduardo D'Avila" <erdavila@gmail.com> writes:

> 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

[SZEDER Gábor]

On Sat, Jun 22, 2013 at 01:45:38PM -0300, Eduardo D'Avila wrote:
> 2013/6/22 Øystein Walle <oystwa@gmail.com>:
> > 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.
> 
> 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".

> 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

[SZEDER Gábor]

On Sat, Jun 22, 2013 at 01:32:38PM -0300, Eduardo D'Avila wrote:
> 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

[Eduardo R. D'Avila]

2013/6/23 SZEDER Gábor <szeder@ira.uka.de>:
> 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...

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