From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 16 Oct 2012 15:58:07 GMT
Subject: Re: [PATCH 3/3] Change colors to be based on git status -sb in color mode
Message-ID: <7vvceatnn4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20121016053257.GA8366@simaj.xs4all.nl>

Simon Oosthoek <s.oosthoek@xs4all.nl> writes:

> this patch is an additional patch to the previous series of two.


> @@ -325,35 +323,45 @@ __git_ps1 ()
>  
>  		local f="$w$i$s$u"
>  		if [ $pcmode = yes ]; then
> +			if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
>  				local c_red='\e[31m'
>  				local c_green='\e[32m'
>  				local c_lblue='\e[1;34m'
>  				local c_clear='\e[0m'
>  				local branchstring="$c${b##refs/heads/}"
> +				local branch_color="$c_clear"
> +				local flags_color="$c_lblue"
>  
> +				case "$b" in 
> +					\(*\))	branch_color="$c_red"
> +					;;
> +					*)	local branch_color="$c_green"
> +					;;
> +				esac

Why "local" only on one side and not the other?

Also switching on "$b" and relying on how it happens to be formatted
inside parentheses when detached (which can very well change in the
future) looks like a fragile bug waiting to be triggered.

>  				# Setting PS1 directly with \[ and \] around colors
>  				# is necessary to prevent wrapping issues!
> +				PS1="$ps1pc_start (\[$branch_color\]$branchstring\[$c_clear\]"
> +
> +				if [ -n "$w$i$s$u$r$p" ]; then
> +					PS1="$PS1 "
> +				fi
> +				if [ "$w" = "*" ]; then
> +					PS1="$PS1\[$c_red\]$w"
> +				fi
> +				if [ -n "$i" ]; then
> +					PS1="$PS1\[$c_green\]$i"
> +				fi

You gave nice symbolic names to branch_color and flags_color, but
these two use hardcoded colors?