[RFC PATCH] git-prompt: make colourization consistent

[RFC PATCH] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by clearing any colour before printing the
short upstream state indicator, as this immediately follows the last
coloured indicator.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
As of 0ec7c23cdc6bde5af3039c59e21507adf7579a99, colourization of the
output of __git_ps1 has changed such that the short upstream state
indicator inherits the colour of the last short state indicator before
it (if there is one), while before this change it was white/the default
text colour. Some examples of what I mean are (assuming all indicators
are enabled):
 * If the local tree is clean and there is something in the stash, both
   the '$' and the short upstream state indicator following it will be
   blue.
 * If the local tree has new, untracked files, both the '%' and the
   short upstream state indicator will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change is that previously, the short upstream state indicator appeared
immediately after the rebase/revert/bisect/merge state indicator, which
is prepended with the clear colour code, while it now follows the
sequence of colourized indicators, without any clearing of colour.
However, adding a clearing of colour before the short upstream state
indicator will change how the sparsity state indicator is colourized,
as it currently inherits (and before the change referenced also
inherited) the colour of the last short state indicator before it.
Reading the commit message of the change that introduced the sparsity
state indicator, it appears this colourization also was unintended.

 contrib/completion/git-prompt.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..dfd6cef35f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
 	if [ -n "$u" ]; then
 		u="$bad_color$u"
 	fi
+	p="$c_clear$p"
 	r="$c_clear$r"
 }
 

base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250
-- 
2.36.1

Re: [RFC PATCH] git-prompt: make colourization consistent

[Ævar Arnfjörð Bjarmason]

On Wed, Jun 01 2022, Joakim Petersen wrote:

> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. Make the colourization of these
> state indicators consistent by clearing any colour before printing the
> short upstream state indicator, as this immediately follows the last
> coloured indicator.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
> As of 0ec7c23cdc6bde5af3039c59e21507adf7579a99, colourization of the
> output of __git_ps1 has changed such that the short upstream state
> indicator inherits the colour of the last short state indicator before
> it (if there is one), while before this change it was white/the default
> text colour. Some examples of what I mean are (assuming all indicators
> are enabled):
>  * If the local tree is clean and there is something in the stash, both
>    the '$' and the short upstream state indicator following it will be
>    blue.
>  * If the local tree has new, untracked files, both the '%' and the
>    short upstream state indicator will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
>
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change is that previously, the short upstream state indicator appeared
> immediately after the rebase/revert/bisect/merge state indicator, which
> is prepended with the clear colour code, while it now follows the
> sequence of colourized indicators, without any clearing of colour.
> However, adding a clearing of colour before the short upstream state
> indicator will change how the sparsity state indicator is colourized,
> as it currently inherits (and before the change referenced also
> inherited) the colour of the last short state indicator before it.
> Reading the commit message of the change that introduced the sparsity
> state indicator, it appears this colourization also was unintended.
>
>  contrib/completion/git-prompt.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 87b2b916c0..dfd6cef35f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
>  	if [ -n "$u" ]; then
>  		u="$bad_color$u"
>  	fi
> +	p="$c_clear$p"
>  	r="$c_clear$r"
>  }
>  
>
> base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250

This seems to make sense to me, but I haven't looked deeply into it. But
let's CC the author of 0ec7c23cdc6 (git-prompt: make upstream state
indicator location consistent, 2022-02-27) (which I've done here).

For a non-RFC patch I think a rephrasing of most of what yo uhave below
"--" should be part of the message. Note how I referred to the
0ec... commit above, you should reference the commit like that (see
SubmittingPatches).

Thanks for working on this fix!
 

Re: [RFC PATCH] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. Make the colourization of these
> state indicators consistent by clearing any colour before printing the
> short upstream state indicator, as this immediately follows the last
> coloured indicator.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---

> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 87b2b916c0..dfd6cef35f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
>  	if [ -n "$u" ]; then
>  		u="$bad_color$u"
>  	fi
> +	p="$c_clear$p"
>  	r="$c_clear$r"
>  }

Hmph, am I correct to understand that the general flow of __git_ps1 is 

 (1) various pieces of information like $h, $w, $i, $s, $r, $b, $p,
     etc.  are declared "local" and values computed for them,
     either inside __git_ps1() itself, or by various helper
     functions it calls;

 (2) When GIT_PS1_SHOWCOLORHINTS is in effect, we may call the
     __git_ps1_colorize_gitstring helper (which is touched by the
     above hunk), that modifies these variables with color codes.
     Upon entry to this helper function, these variables prepared in
     (1) have no color effects.  Upon leaving, they do.

 (3) Finally, the PS1 is asseembled by concatenating these
     variables, whose text was prepared in (1) and then prefixed by
     color codes in (2), one of the earliest steps begins like so:

     local f="$h$w$i$s$u"
     local gitstring="$c$b${f:+$z$f}${sparse}$r$p"

In the final step of formulation, $p immediately follows $r in the
resulting $PS1, and the existing code at the end of the (2) prefixes
$c_clear before $r, and $r before such prefixing is free of coloring,
so it is curious how this patch makes difference (other than emitting
$c_clear one more time).  Unless there is a use of $p that does not
immediately follow $r, that is.

Thanks.

Re: [RFC PATCH] git-prompt: make colourization consistent

[Joakim Petersen]

On 01/06/2022 16:47, Ævar Arnfjörð Bjarmason wrote:
> This seems to make sense to me, but I haven't looked deeply into it. But
> let's CC the author of 0ec7c23cdc6 (git-prompt: make upstream state
> indicator location consistent, 2022-02-27) (which I've done here).
> 
> For a non-RFC patch I think a rephrasing of most of what yo uhave below
> "--" should be part of the message. Note how I referred to the
> 0ec... commit above, you should reference the commit like that (see
> SubmittingPatches).
> 
> Thanks for working on this fix!
>  >

Thanks for the pointers, I'll keep that in mind for the follow-up! I do
have one question regarding the procedure for the follow-up, though:
If there are no code changes, should it still be submitted as a "v2"?

Re: [RFC PATCH] git-prompt: make colourization consistent

[Joakim Petersen]

On 01/06/2022 20:07, Junio C Hamano wrote:
> Hmph, am I correct to understand that the general flow of __git_ps1 is
> 
>   (1) various pieces of information like $h, $w, $i, $s, $r, $b, $p,
>       etc.  are declared "local" and values computed for them,
>       either inside __git_ps1() itself, or by various helper
>       functions it calls;
> 
>   (2) When GIT_PS1_SHOWCOLORHINTS is in effect, we may call the
>       __git_ps1_colorize_gitstring helper (which is touched by the
>       above hunk), that modifies these variables with color codes.
>       Upon entry to this helper function, these variables prepared in
>       (1) have no color effects.  Upon leaving, they do.
> 
>   (3) Finally, the PS1 is asseembled by concatenating these
>       variables, whose text was prepared in (1) and then prefixed by
>       color codes in (2), one of the earliest steps begins like so:
> 
>       local f="$h$w$i$s$u"
>       local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
> 
> In the final step of formulation, $p immediately follows $r in the
> resulting $PS1, and the existing code at the end of the (2) prefixes
> $c_clear before $r, and $r before such prefixing is free of coloring,
> so it is curious how this patch makes difference (other than emitting
> $c_clear one more time).  Unless there is a use of $p that does not
> immediately follow $r, that is.
> 
> Thanks.
> 

Your understanding is correct for the flow before the change I
referenced (0ec7c23cdc6 (git-prompt: make upstream state
indicator location consistent, 2022-02-27)), however, that commit
changed the definition of $f and $gitstring to

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

This makes it so $p is no longer immediately preceded by $r, but rather
$u, which, like all the preceding variables, except $h, will be
colourized if enabled.

Re: [RFC PATCH] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> Your understanding is correct for the flow before the change I
> referenced (0ec7c23cdc6 (git-prompt: make upstream state
> indicator location consistent, 2022-02-27)), however, that commit
> changed the definition of $f and $gitstring to
>
> 	local f="$h$w$i$s$u$p"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>
> This makes it so $p is no longer immediately preceded by $r, but rather
> $u, which, like all the preceding variables, except $h, will be
> colourized if enabled.

Ah, OK.  With the above explanation, the change does make sense.
The mention of that commit does need to be in the proposed log
message, not under the three-dash line, as it is essential to
understand why the patch is not a no-op change.

Thanks.

[PATCH v2] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by clearing any colour before printing the
short upstream state indicator, as this immediately follows the last
coloured indicator.

As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), colourization in the output of __git_ps1 has
changed such that the short upstream state indicator inherits the colour
of the last short state indicator before it (if there is one), while
before this change it was white/the default text colour. Some examples
to illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If the local tree is clean and there is something in the stash, both
   the '$' and the short upstream state indicator following it will be
   blue.
 * If the local tree has new, untracked files, both the '%' and the
   short upstream state indicator will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change is that previously, the short upstream state indicator appeared
immediately after the rebase/revert/bisect/merge state indicator (note
the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

However, adding a clearing of colour before the short upstream state
indicator will change how the sparsity state indicator is colourized,
as it currently inherits (and before the change referenced also
inherited) the colour of the last short state indicator before it.
Reading the commit message of the change that introduced the sparsity
state indicator, afda36dbf3b (git-prompt: include sparsity state as
well, 2020-06-21), it appears this colourization also was unintended,
so clearing the colour for said indicator further increases consistency.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---

Range-diff against v1:
1:  e235caa7a8 = 1:  e235caa7a8 git-prompt: make colourization consistent

 contrib/completion/git-prompt.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..dfd6cef35f 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
 	if [ -n "$u" ]; then
 		u="$bad_color$u"
 	fi
+	p="$c_clear$p"
 	r="$c_clear$r"
 }
 
-- 
2.36.1

Re: [PATCH v2] git-prompt: make colourization consistent

[joak-pet]

On 02/06/2022 16:59, Joakim Petersen wrote:
> The short upstream state indicator inherits the colour of the last 
> short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. Make the colourization of these
> state indicators consistent by clearing any colour before printing the
> short upstream state indicator, as this immediately follows the last
> coloured indicator.
> 
> As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
> consistent, 2022-02-27), colourization in the output of __git_ps1 has
> changed such that the short upstream state indicator inherits the 
> colour
> of the last short state indicator before it (if there is one), while
> before this change it was white/the default text colour. Some examples
> to illustrate this behaviour (assuming all indicators are enabled and
> colourization is on):
>  * If the local tree is clean and there is something in the stash, both
>    the '$' and the short upstream state indicator following it will be
>    blue.
>  * If the local tree has new, untracked files, both the '%' and the
>    short upstream state indicator will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it 
> will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
> 
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change is that previously, the short upstream state indicator appeared
> immediately after the rebase/revert/bisect/merge state indicator (note
> the position of $p in $gitstring):
> 
> 	local f="$h$w$i$s$u"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
> 
> Said indicator is prepended with the clear colour code, and the short
> upstream state indicator is thus also uncoloured. Now, the short
> upstream state indicator follows the sequence of colourized indicators,
> without any clearing of colour (again note the position of $p, now in
> $f):
> 
> 	local f="$h$w$i$s$u$p"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
> 
> However, adding a clearing of colour before the short upstream state
> indicator will change how the sparsity state indicator is colourized,
> as it currently inherits (and before the change referenced also
> inherited) the colour of the last short state indicator before it.
> Reading the commit message of the change that introduced the sparsity
> state indicator, afda36dbf3b (git-prompt: include sparsity state as
> well, 2020-06-21), it appears this colourization also was unintended,
> so clearing the colour for said indicator further increases 
> consistency.
> 
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
> 
> Range-diff against v1:
> 1:  e235caa7a8 = 1:  e235caa7a8 git-prompt: make colourization 
> consistent
> 
>  contrib/completion/git-prompt.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/contrib/completion/git-prompt.sh 
> b/contrib/completion/git-prompt.sh
> index 87b2b916c0..dfd6cef35f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
>  	if [ -n "$u" ]; then
>  		u="$bad_color$u"
>  	fi
> +	p="$c_clear$p"
>  	r="$c_clear$r"
>  }

I just realized I forgot to write what changed between the RFC patch
and v2:

  * Clarify the reason why 0ec7c23cdc6 (git-prompt: make upstream state
    indicator location consistent, 2022-02-27) changed the colourization
    of the short upstream state indicator.
  * Explain the rationale for changing the sparsity state colourization.
  * Include examples of how the short upstream state indicator is
    currently colourized

Re: [PATCH v2] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> Range-diff against v1:
> 1:  e235caa7a8 = 1:  e235caa7a8 git-prompt: make colourization consistent
>
>  contrib/completion/git-prompt.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 87b2b916c0..dfd6cef35f 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
>  	if [ -n "$u" ]; then
>  		u="$bad_color$u"
>  	fi
> +	p="$c_clear$p"
>  	r="$c_clear$r"
>  }

Has this been tested?

It seems to break a handful of tests in t9903 for me.

Re: [PATCH v2] git-prompt: make colourization consistent

[Joakim Petersen]

On 03/06/2022 00:49, Junio C Hamano wrote:
> Joakim Petersen <joak-pet@online.no> writes:
> 
>> Range-diff against v1:
>> 1:  e235caa7a8 = 1:  e235caa7a8 git-prompt: make colourization consistent
>>
>>   contrib/completion/git-prompt.sh | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>> index 87b2b916c0..dfd6cef35f 100644
>> --- a/contrib/completion/git-prompt.sh
>> +++ b/contrib/completion/git-prompt.sh
>> @@ -286,6 +286,7 @@ __git_ps1_colorize_gitstring ()
>>   	if [ -n "$u" ]; then
>>   		u="$bad_color$u"
>>   	fi
>> +	p="$c_clear$p"
>>   	r="$c_clear$r"
>>   }
> 
> Has this been tested?
> 
> It seems to break a handful of tests in t9903 for me.
> 

Oh, no I hadn't run the test suite, sorry. I must've gotten too caught
up in other parts of the submitting process and simply forgot to run
them. After looking into it, the reason why the tests fail is that $p is
no longer empty when it is not set, so $f is no longer empty, leading to 
both $z and $p being inserted into $gitstring. This causes there to be
three clear-colour sequences in the final output instead of the expected
one. Wrapping the clearing of $p's colour in a check for emptiness like
the other short state indicators fixes the tests. I'll submit a v3
shortly.

[PATCH v3] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by clearing any colour before printing the
short upstream state indicator, as this immediately follows the last
coloured indicator.

As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), colourization in the output of __git_ps1 has
changed such that the short upstream state indicator inherits the colour
of the last short state indicator before it (if there is one), while
before this change it was white/the default text colour. Some examples
to illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If the local tree is clean and there is something in the stash, both
   the '$' and the short upstream state indicator following it will be
   blue.
 * If the local tree has new, untracked files, both the '%' and the
   short upstream state indicator will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change is that previously, the short upstream state indicator appeared
immediately after the rebase/revert/bisect/merge state indicator (note
the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.However,
adding a clearing of colour before the short upstream state indicator
will change how the sparsity state indicator is colourized only when the
short upstream state indicator is present, as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colourization also was unintended, so clearing the colour for said
indicator further increases consistency.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v2:
 * Wrapped clearing of $p's colour in a check for emptiness to avoid
   multiple colour clears in the final gitstring.
 * Added clearing of colour for $sparse, as it wouldn't acutally be
   cleared consistently with only the change from the previous bullet
    - Having the short upstream state indicator disabled would leave the
      sparse state indicator as it is without this patch.

Range-diff against v2:
1:  e235caa7a8 < -:  ---------- git-prompt: make colourization consistent
-:  ---------- > 1:  0e107d0496 git-prompt: make colourization consistent

 contrib/completion/git-prompt.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..4997545ee5 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -286,6 +286,12 @@ __git_ps1_colorize_gitstring ()
 	if [ -n "$u" ]; then
 		u="$bad_color$u"
 	fi
+	if [ -n "$p" ]; then
+		p="$c_clear$p"
+	fi
+	if [ -n "$sparse" ]; then
+		sparse="$c_clear$sparse"
+	fi
 	r="$c_clear$r"
 }
 
-- 
2.36.1

Re: [PATCH v3] git-prompt: make colourization consistent

[Junio C Hamano]

This is not a new issue, but seeing this:

 	if [ $detached = no ]; then
 		branch_color="$ok_color"
 	else
 		branch_color="$bad_color"
 	fi
 	c="$branch_color$c"
 
 	z="$c_clear$z"
 	if [ "$w" = "*" ]; then
 		w="$bad_color$w"
 	fi
 	if [ -n "$i" ]; then
 		i="$ok_color$i"
 	fi
 	if [ -n "$s" ]; then
 		s="$flags_color$s"
 	fi
 	if [ -n "$u" ]; then
 		u="$bad_color$u"
 	fi
+	if [ -n "$p" ]; then
+		p="$c_clear$p"
+	fi
+	if [ -n "$sparse" ]; then
+		sparse="$c_clear$sparse"
+	fi
 	r="$c_clear$r"
 }

it makes me wonder if the more forward looking and future-proof way
that is resistant to any future and random reshuffling like what
0ec7c23c (git-prompt: make upstream state indicator location
consistent, 2022-02-27) did would be to make it a rule to maintain
that there is no coloring by default, and when any of these tokens
like w, i, s, ... are not empty, enclose them inside "color-on" and
"color-off" sequence.

For example, 

 	if [ "$w" = "*" ]; then
 		w="$bad_color$w"
 	fi

would mean $w, when it is "*", would cause gitstring to contain an
asterisk that is painted in $bad_color, but ALSO causes whatever
that happens to come AFTER $w in gitstring to be painted in the same
color UNLESS it tries to protect itself.  Right now, $w may be
immediately followed by $i, and $i does protect itself by prefixing
with $ok_color, but if $i is empty, $w's coloring will extend to $s.

So, if we did this instead:

- 	z="$c_clear$z"
 	if [ "$w" = "*" ]; then
- 		w="$bad_color$w"
+ 		w="$bad_color$w$c_clear"
 	fi

and make similar changes to everything else we see above, we
probably can lose the ones that prefix with $c_clear, because each
token that paints itself in unusual color is now responsible for
returning the terminal state to normal with the $c_clear sequence
after it is done with it.  We do not have to special case sparse, p,
or r in this helper function at all if we go that route, no?

If the helper were written that way, then reshuffling the order of
the tokens done in 0ec7c23c (git-prompt: make upstream state
indicator location consistent, 2022-02-27) wouldn't have made the
patch under discussion necessary at all, which is what I see is
valuable from the "maintainability" point of view.

Re: [PATCH v3] git-prompt: make colourization consistent

[Joakim Petersen]

On 03/06/2022 18:38, Junio C Hamano wrote:
> This is not a new issue, but seeing this:
> 
>   	if [ $detached = no ]; then
>   		branch_color="$ok_color"
>   	else
>   		branch_color="$bad_color"
>   	fi
>   	c="$branch_color$c"
>   
>   	z="$c_clear$z"
>   	if [ "$w" = "*" ]; then
>   		w="$bad_color$w"
>   	fi
>   	if [ -n "$i" ]; then
>   		i="$ok_color$i"
>   	fi
>   	if [ -n "$s" ]; then
>   		s="$flags_color$s"
>   	fi
>   	if [ -n "$u" ]; then
>   		u="$bad_color$u"
>   	fi
> +	if [ -n "$p" ]; then
> +		p="$c_clear$p"
> +	fi
> +	if [ -n "$sparse" ]; then
> +		sparse="$c_clear$sparse"
> +	fi
>   	r="$c_clear$r"
>   }
> 
> it makes me wonder if the more forward looking and future-proof way
> that is resistant to any future and random reshuffling like what
> 0ec7c23c (git-prompt: make upstream state indicator location
> consistent, 2022-02-27) did would be to make it a rule to maintain
> that there is no coloring by default, and when any of these tokens
> like w, i, s, ... are not empty, enclose them inside "color-on" and
> "color-off" sequence.
> 
> For example,
> 
>   	if [ "$w" = "*" ]; then
>   		w="$bad_color$w"
>   	fi
> 
> would mean $w, when it is "*", would cause gitstring to contain an
> asterisk that is painted in $bad_color, but ALSO causes whatever
> that happens to come AFTER $w in gitstring to be painted in the same
> color UNLESS it tries to protect itself.  Right now, $w may be
> immediately followed by $i, and $i does protect itself by prefixing
> with $ok_color, but if $i is empty, $w's coloring will extend to $s.
> 
> So, if we did this instead:
> 
> - 	z="$c_clear$z"
>   	if [ "$w" = "*" ]; then
> - 		w="$bad_color$w"
> + 		w="$bad_color$w$c_clear"
>   	fi
> 
> and make similar changes to everything else we see above, we
> probably can lose the ones that prefix with $c_clear, because each
> token that paints itself in unusual color is now responsible for
> returning the terminal state to normal with the $c_clear sequence
> after it is done with it.  We do not have to special case sparse, p,
> or r in this helper function at all if we go that route, no?
> 
> If the helper were written that way, then reshuffling the order of
> the tokens done in 0ec7c23c (git-prompt: make upstream state
> indicator location consistent, 2022-02-27) wouldn't have made the
> patch under discussion necessary at all, which is what I see is
> valuable from the "maintainability" point of view.
> 

That does seem like a much better idea for maintainability, I can
change the patch to do this instead. I have one question, though: the
sequence $c$b (bare state and branch name) is a special case, where
they're intended to have the same colour, should I wrap both in colour
set, colour clear, or only clear after $b? The former requires rewriting
the tests or changing $gitstring to not include $c when $c is empty,
while the latter keeps the tests unchanged, but may pose a problem if
"BARE:" should at any point not appear immediately before the branch
name.

Re: [PATCH v3] git-prompt: make colourization consistent

[Joakim Petersen]

On 03/06/2022 19:23, Joakim Petersen wrote:
> That does seem like a much better idea for maintainability, I can
> change the patch to do this instead. I have one question, though: the
> sequence $c$b (bare state and branch name) is a special case, where
> they're intended to have the same colour, should I wrap both in colour
> set, colour clear, or only clear after $b? The former requires rewriting
> the tests or changing $gitstring to not include $c when $c is empty,
> while the latter keeps the tests unchanged, but may pose a problem if
> "BARE:" should at any point not appear immediately before the branch
> name.

Sorry, the former (colourizing and clearing $c and $b individually)
requires rewriting tests no matter what.

Re: [PATCH v3] git-prompt: make colourization consistent

[Justin Donnelly]

Hi all. I'm the author of 0ec7c23c (git-prompt: make upstream state
indicator location consistent, 2022-02-27). Sorry I'm a little late
jumping in. I was also going to propose something more comprehensive
and future-proof than what's there (adding the applicable color
(including clear) to all the indicators), but I like Junio's idea
better. The only other thing I have to add is that it's probably a
good idea to include a comment in the function
`__git_ps1_colorize_gitstring` explaining the design so future
developers/reviewers know.

Thanks,
Justin


On Fri, Jun 3, 2022 at 2:52 PM Joakim Petersen <joak-pet@online.no> wrote:
>
> On 03/06/2022 19:23, Joakim Petersen wrote:
> > That does seem like a much better idea for maintainability, I can
> > change the patch to do this instead. I have one question, though: the
> > sequence $c$b (bare state and branch name) is a special case, where
> > they're intended to have the same colour, should I wrap both in colour
> > set, colour clear, or only clear after $b? The former requires rewriting
> > the tests or changing $gitstring to not include $c when $c is empty,
> > while the latter keeps the tests unchanged, but may pose a problem if
> > "BARE:" should at any point not appear immediately before the branch
> > name.
>
> Sorry, the former (colourizing and clearing $c and $b individually)
> requires rewriting tests no matter what.

Re: [PATCH v3] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> That does seem like a much better idea for maintainability, I can
> change the patch to do this instead. I have one question, though: the
> sequence $c$b (bare state and branch name) is a special case, where
> they're intended to have the same colour, should I wrap both in colour
> set, colour clear, or only clear after $b?

If we want to allow $c and $b appear in different places (which I
have no opinion on), I would say we should just color them
independently and fix the test that expects the close linkage
between the two.  I offhand see no reason that they _must_ stay
together myself, though.

Thanks.

Re: [PATCH v3] git-prompt: make colourization consistent

[Junio C Hamano]

Justin Donnelly <justinrdonnelly@gmail.com> writes:

> Hi all. I'm the author of 0ec7c23c (git-prompt: make upstream state
> indicator location consistent, 2022-02-27). Sorry I'm a little late
> jumping in. I was also going to propose something more comprehensive
> and future-proof than what's there (adding the applicable color
> (including clear) to all the indicators), but I like Junio's idea
> better. The only other thing I have to add is that it's probably a
> good idea to include a comment in the function
> `__git_ps1_colorize_gitstring` explaining the design so future
> developers/reviewers know.

After thinking it again, I actually am OK with the original coloring
code structure.  The rule is "you always counter whatever color
settings left behind by somebody who came before you".

As long as the color effect you use is not additive (e.g. if the
final product is $a$b, and $a is prefixed with $c_red and $b is
prefixed with $c_blue, an additive coloring scheme may end up
painting b in purple), we'll save number of $c_clear we would need
to emit.  Plain colors are probably not additive, but some
attributes are, so this is more brittle than "always reset to the
base state" rule, but it may be more desirable in practice.

I have no strong preference either way.  But if we are to go that
route, we definitely need to make sure that the last element added
to gitstring is followed by $c_reset, by doing something like the
attached patch.  Currently, $r has unconditional $c_clear in front
of it, and $upstream is never colored, and that is the only thing
that is saving us from leftover color bleeding into whatever comes
after the prompt.

 contrib/completion/git-prompt.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git c/contrib/completion/git-prompt.sh w/contrib/completion/git-prompt.sh
index 87b2b916c0..c803b9fae5 100644
--- c/contrib/completion/git-prompt.sh
+++ w/contrib/completion/git-prompt.sh
@@ -287,6 +287,7 @@ __git_ps1_colorize_gitstring ()
 		u="$bad_color$u"
 	fi
 	r="$c_clear$r"
+	end_of_gitstring=$c_clear
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -556,6 +557,7 @@ __git_ps1 ()
 
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
+	local end_of_gitstring=
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
 	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
 		if [ $pcmode = yes ] || [ -n "${ZSH_VERSION-}" ]; then
@@ -570,7 +572,7 @@ __git_ps1 ()
 	fi
 
 	local f="$h$w$i$s$u$p"
-	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
+	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}${end_of_gitstring}"
 
 	if [ $pcmode = yes ]; then
 		if [ "${__git_printf_supports_v-}" != yes ]; then

Re: [PATCH v3] git-prompt: make colourization consistent

[Joakim Petersen]

On 03/06/2022 23:16, Junio C Hamano wrote:
> After thinking it again, I actually am OK with the original coloring
> code structure.  The rule is "you always counter whatever color
> settings left behind by somebody who came before you".
> 
> As long as the color effect you use is not additive (e.g. if the
> final product is $a$b, and $a is prefixed with $c_red and $b is
> prefixed with $c_blue, an additive coloring scheme may end up
> painting b in purple), we'll save number of $c_clear we would need
> to emit.  Plain colors are probably not additive, but some
> attributes are, so this is more brittle than "always reset to the
> base state" rule, but it may be more desirable in practice.
> 
> I have no strong preference either way.  But if we are to go that
> route, we definitely need to make sure that the last element added
> to gitstring is followed by $c_reset, by doing something like the
> attached patch.  Currently, $r has unconditional $c_clear in front
> of it, and $upstream is never colored, and that is the only thing
> that is saving us from leftover color bleeding into whatever comes
> after the prompt.

There might be something I'm not seeing, but having it so each element
counters whatever colour left by the preceding element seems less
intuitive when adding or moving elements in the final $gitstring. Adding
an element will then require going into __git_ps1_colorize_gitstring,
even when it is not intended to be colourized. All existing uncoloured
elements will also need to be prefixed to protect against colour bleed
from being moved around. I'm partial to the idea of each coloured
element clearing its own colour.

[PATCH v4] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by making all colourized indicators clear
their own colour.

As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), colourization in the output of __git_ps1 has
changed such that the short upstream state indicator inherits the colour
of the last short state indicator before it (if there is one), while
before this change it was white/the default text colour. Some examples
to illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If there is something in the stash, both the '$' and the short
   upstream state indicator following it will be blue.
 * If the local tree has new, untracked files and there is nothing in
   the stash, both the '%' and the    short upstream state indicator
   will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change is that previously, the short upstream state indicator appeared
immediately after the rebase/revert/bisect/merge state indicator (note
the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.
However, clearing colour of the colourized indicators changes how the
sparsity state indicator is colourized , as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colourization also was unintended, so clearing the colour for said
indicator further increases consistency.

Colouring of $c was made dependent on it not being empty, as it is no
longer being used to colour the branch name. Removal of $b's prefix was
moved to before the colourization so it gets cleared properly now that
colour codes are inserted into it.

Due to colour clearing being moved into the variables for each coloured
indicator, the tests for the coloured Bash prompt had to be changed:
 * All colour tests now have the colour codes around the expected
   content of the expanded $__git_ps1_branch_name variable instead of
   the unexpanded variable in the string.
 * The test with two indicators had a clear-colour code inserted after
   the symbol for the first indicator, since all indicators clear their
   own colours now.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v3:
 * All colourized variables now also clear their own colour.
 * Variables are only coloured if they are not empty, except $b (branch
   name), which is not an optional indicator.
 * Updated tests to reflect the new colourization behaviour.
 * Fixed a mistake in two of the examples; the stash indicator is the
   last of the short state indicators preceding the short upstream state
   indicator.

Range-diff against v3:
1:  0e107d0496 < -:  ---------- git-prompt: make colourization consistent
-:  ---------- > 1:  98ce78ddc5 git-prompt: make colourization consistent

 contrib/completion/git-prompt.sh | 20 +++++++++++---------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..32bb98bb8d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
 
 # Helper function that is meant to be called from __git_ps1.  It
 # injects color codes into the appropriate gitstring variables used
-# to build a gitstring.
+# to build a gitstring. Colored variables are responsible for clearing
+# their own color.
 __git_ps1_colorize_gitstring ()
 {
 	if [[ -n ${ZSH_VERSION-} ]]; then
@@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
 	else
 		branch_color="$bad_color"
 	fi
-	c="$branch_color$c"
+	if [ -n "$c" ]; then
+		c="$branch_color$c$c_clear"
+	fi
+	b="$branch_color$b$c_clear"
 
-	z="$c_clear$z"
 	if [ "$w" = "*" ]; then
-		w="$bad_color$w"
+		w="$bad_color$w$c_clear"
 	fi
 	if [ -n "$i" ]; then
-		i="$ok_color$i"
+		i="$ok_color$i$c_clear"
 	fi
 	if [ -n "$s" ]; then
-		s="$flags_color$s"
+		s="$flags_color$s$c_clear"
 	fi
 	if [ -n "$u" ]; then
-		u="$bad_color$u"
+		u="$bad_color$u$c_clear"
 	fi
-	r="$c_clear$r"
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -554,6 +556,7 @@ __git_ps1 ()
 		fi
 	fi
 
+	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
@@ -563,7 +566,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
 		__git_ps1_branch_name=$b
 		b="\${__git_ps1_branch_name}"
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..abd82eec35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v4] git-prompt: make colourization consistent

[Justin Donnelly]

On Sat, Jun 4, 2022 at 12:13 PM Joakim Petersen <joak-pet@online.no> wrote:
>
> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. Make the colourization of these
> state indicators consistent by making all colourized indicators clear
> their own colour.
>
> As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
> consistent, 2022-02-27), colourization in the output of __git_ps1 has
> changed such that the short upstream state indicator inherits the colour
> of the last short state indicator before it (if there is one), while
> before this change it was white/the default text colour. Some examples
> to illustrate this behaviour (assuming all indicators are enabled and
> colourization is on):
>  * If there is something in the stash, both the '$' and the short
>    upstream state indicator following it will be blue.
>  * If the local tree has new, untracked files and there is nothing in
>    the stash, both the '%' and the    short upstream state indicator
>    will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
>
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change is that previously, the short upstream state indicator appeared
> immediately after the rebase/revert/bisect/merge state indicator (note
> the position of $p in $gitstring):
>
>         local f="$h$w$i$s$u"
>         local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
>
> Said indicator is prepended with the clear colour code, and the short
> upstream state indicator is thus also uncoloured. Now, the short
> upstream state indicator follows the sequence of colourized indicators,
> without any clearing of colour (again note the position of $p, now in
> $f):
>
>         local f="$h$w$i$s$u$p"
>         local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>
> If the user is in a sparse checkout, the sparsity state indicator
> follows a similar pattern to the short upstream state indicator.
> However, clearing colour of the colourized indicators changes how the
> sparsity state indicator is colourized , as it currently inherits (and
> before the change referenced also inherited) the colour of the last
> short state indicator before it. Reading the commit message of the
> change that introduced the sparsity state indicator, afda36dbf3b
> (git-prompt: include sparsity state as well, 2020-06-21), it appears
> this colourization also was unintended, so clearing the colour for said
> indicator further increases consistency.
>
> Colouring of $c was made dependent on it not being empty, as it is no
> longer being used to colour the branch name. Removal of $b's prefix was
> moved to before the colourization so it gets cleared properly now that
> colour codes are inserted into it.
>
> Due to colour clearing being moved into the variables for each coloured
> indicator, the tests for the coloured Bash prompt had to be changed:
>  * All colour tests now have the colour codes around the expected
>    content of the expanded $__git_ps1_branch_name variable instead of
>    the unexpanded variable in the string.
>  * The test with two indicators had a clear-colour code inserted after
>    the symbol for the first indicator, since all indicators clear their
>    own colours now.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
> Changes since v3:
>  * All colourized variables now also clear their own colour.
>  * Variables are only coloured if they are not empty, except $b (branch
>    name), which is not an optional indicator.
>  * Updated tests to reflect the new colourization behaviour.
>  * Fixed a mistake in two of the examples; the stash indicator is the
>    last of the short state indicators preceding the short upstream state
>    indicator.
>
> Range-diff against v3:
> 1:  0e107d0496 < -:  ---------- git-prompt: make colourization consistent
> -:  ---------- > 1:  98ce78ddc5 git-prompt: make colourization consistent
>
>  contrib/completion/git-prompt.sh | 20 +++++++++++---------
>  t/t9903-bash-prompt.sh           | 18 +++++++++---------
>  2 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 87b2b916c0..32bb98bb8d 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
>
>  # Helper function that is meant to be called from __git_ps1.  It
>  # injects color codes into the appropriate gitstring variables used
> -# to build a gitstring.
> +# to build a gitstring. Colored variables are responsible for clearing
> +# their own color.
>  __git_ps1_colorize_gitstring ()
>  {
>         if [[ -n ${ZSH_VERSION-} ]]; then
> @@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
>         else
>                 branch_color="$bad_color"
>         fi
> -       c="$branch_color$c"
> +       if [ -n "$c" ]; then
> +               c="$branch_color$c$c_clear"
> +       fi
> +       b="$branch_color$b$c_clear"
>
> -       z="$c_clear$z"
>         if [ "$w" = "*" ]; then
> -               w="$bad_color$w"
> +               w="$bad_color$w$c_clear"
>         fi
>         if [ -n "$i" ]; then
> -               i="$ok_color$i"
> +               i="$ok_color$i$c_clear"
>         fi
>         if [ -n "$s" ]; then
> -               s="$flags_color$s"
> +               s="$flags_color$s$c_clear"
>         fi
>         if [ -n "$u" ]; then
> -               u="$bad_color$u"
> +               u="$bad_color$u$c_clear"
>         fi
> -       r="$c_clear$r"
>  }
>
>  # Helper function to read the first line of a file into a variable.
> @@ -554,6 +556,7 @@ __git_ps1 ()
>                 fi
>         fi
>
> +       b=${b##refs/heads/}
>         local z="${GIT_PS1_STATESEPARATOR-" "}"
>
>         # NO color option unless in PROMPT_COMMAND mode or it's Zsh
> @@ -563,7 +566,6 @@ __git_ps1 ()
>                 fi
>         fi
>
> -       b=${b##refs/heads/}
>         if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
>                 __git_ps1_branch_name=$b
>                 b="\${__git_ps1_branch_name}"
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index bbd513bab0..abd82eec35 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
>  '
>
>  test_expect_success 'prompt - bash color pc mode - branch name' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         (
>                 GIT_PS1_SHOWCOLORHINTS=y &&
>                 __git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
> @@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
>         git checkout b1^ &&
>         test_when_finished "git checkout main" &&
>         (
> @@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
>  '
>
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         echo "dirty" >file &&
>         test_when_finished "git reset --hard" &&
>         (
> @@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         echo "dirty" >file &&
>         test_when_finished "git reset --hard" &&
>         git add -u &&
> @@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         echo "dirty index" >file &&
>         test_when_finished "git reset --hard" &&
>         git add -u &&
> @@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         (
>                 GIT_PS1_SHOWDIRTYSTATE=y &&
>                 GIT_PS1_SHOWCOLORHINTS=y &&
> @@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
>         echo "dirty" >file &&
>         test_when_finished "git reset --hard" &&
>         (
> @@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
>  '
>
>  test_expect_success 'prompt - bash color pc mode - stash status indicator' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         echo 2 >file &&
>         git stash &&
>         test_when_finished "git stash drop" &&
> @@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
>  '
>
>  test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
> -       printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
> +       printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>         (
>                 GIT_PS1_SHOWUNTRACKEDFILES=y &&
>                 GIT_PS1_SHOWCOLORHINTS=y &&
> --
> 2.36.1
>

I like this solution. This isn't new, but does anybody know if there
is a reason why `$w` is compared for equality to "*" as opposed to
just checking whether it's a nonempty value (`-n`)? I think I'd
generally prefer it to be consistent with the others, which has the
added benefit of continuing to work if the asterisk is ever changed to
something else.

Re: [PATCH v4] git-prompt: make colourization consistent

[Joakim Petersen]

On 04/06/2022 19:30, Justin Donnelly wrote:
> I like this solution. This isn't new, but does anybody know if there
> is a reason why `$w` is compared for equality to "*" as opposed to
> just checking whether it's a nonempty value (`-n`)? I think I'd
> generally prefer it to be consistent with the others, which has the
> added benefit of continuing to work if the asterisk is ever changed to
> something else.
> 

Looking at the commit that introduced colourization, 9b7e776c0a5 (show 
color hints based on state of the git tree, 2012-10-10), it looks like
the author wanted the be as specific as possible in the check, with $w
only being empty or holding a '*', while $i could hold multiple
different indicators. Since the layout of the script has changed
significantly since then, I'll submit a v5 shortly with the $w check
altered.

[PATCH v5] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by making all colourized indicators clear
their own colour.

As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), colourization in the output of __git_ps1 has
changed such that the short upstream state indicator inherits the colour
of the last short state indicator before it (if there is one), while
before this change it was white/the default text colour. Some examples
to illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If there is something in the stash, both the '$' and the short
   upstream state indicator following it will be blue.
 * If the local tree has new, untracked files and there is nothing in
   the stash, both the '%' and the    short upstream state indicator
   will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change is that previously, the short upstream state indicator appeared
immediately after the rebase/revert/bisect/merge state indicator (note
the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.
However, clearing colour of the colourized indicators changes how the
sparsity state indicator is colourized , as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colourization also was unintended, so clearing the colour for said
indicator further increases consistency.

Colouring of $c was made dependent on it not being empty, as it is no
longer being used to colour the branch name. Removal of $b's prefix was
moved to before the colourization so it gets cleared properly now that
colour codes are inserted into it.

Due to colour clearing being moved into the variables for each coloured
indicator, the tests for the coloured Bash prompt had to be changed:
 * All colour tests now have the colour codes around the expected
   content of the expanded $__git_ps1_branch_name variable instead of
   the unexpanded variable in the string.
 * The test with two indicators had a clear-colour code inserted after
   the symbol for the first indicator, since all indicators clear their
   own colours now.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v4:
 * The check for whether to colourize $w has been altered to match the
   checks for the other indicators.

Range-diff against v4:
1:  98ce78ddc5 ! 1:  fffef5f73d git-prompt: make colourization consistent
    @@ contrib/completion/git-prompt.sh: __git_ps1_colorize_gitstring ()
     +	b="$branch_color$b$c_clear"
      
     -	z="$c_clear$z"
    - 	if [ "$w" = "*" ]; then
    +-	if [ "$w" = "*" ]; then
     -		w="$bad_color$w"
    ++	if [ -n "$w" ]; then
     +		w="$bad_color$w$c_clear"
      	fi
      	if [ -n "$i" ]; then

 contrib/completion/git-prompt.sh | 22 ++++++++++++----------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..cb01c2fd5d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
 
 # Helper function that is meant to be called from __git_ps1.  It
 # injects color codes into the appropriate gitstring variables used
-# to build a gitstring.
+# to build a gitstring. Colored variables are responsible for clearing
+# their own color.
 __git_ps1_colorize_gitstring ()
 {
 	if [[ -n ${ZSH_VERSION-} ]]; then
@@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
 	else
 		branch_color="$bad_color"
 	fi
-	c="$branch_color$c"
+	if [ -n "$c" ]; then
+		c="$branch_color$c$c_clear"
+	fi
+	b="$branch_color$b$c_clear"
 
-	z="$c_clear$z"
-	if [ "$w" = "*" ]; then
-		w="$bad_color$w"
+	if [ -n "$w" ]; then
+		w="$bad_color$w$c_clear"
 	fi
 	if [ -n "$i" ]; then
-		i="$ok_color$i"
+		i="$ok_color$i$c_clear"
 	fi
 	if [ -n "$s" ]; then
-		s="$flags_color$s"
+		s="$flags_color$s$c_clear"
 	fi
 	if [ -n "$u" ]; then
-		u="$bad_color$u"
+		u="$bad_color$u$c_clear"
 	fi
-	r="$c_clear$r"
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -554,6 +556,7 @@ __git_ps1 ()
 		fi
 	fi
 
+	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
@@ -563,7 +566,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
 		__git_ps1_branch_name=$b
 		b="\${__git_ps1_branch_name}"
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..abd82eec35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v5] git-prompt: make colourization consistent

[Bagas Sanjaya]

On 6/5/22 02:26, Joakim Petersen wrote:
> If the user is in a sparse checkout, the sparsity state indicator
> follows a similar pattern to the short upstream state indicator.
> However, clearing colour of the colourized indicators changes how the
> sparsity state indicator is colourized , as it currently inherits (and
> before the change referenced also inherited) the colour of the last
> short state indicator before it. Reading the commit message of the
> change that introduced the sparsity state indicator, afda36dbf3b
> (git-prompt: include sparsity state as well, 2020-06-21), it appears
> this colourization also was unintended, so clearing the colour for said
> indicator further increases consistency.
> 

colourization? I have never heared that. Did you mean "colorization" (en-US)
or "colourisation" (en-UK)? I assumed the former.

-- 
An old man doll... just what I always wanted! - Clara

Re: [PATCH v3] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> There might be something I'm not seeing, but having it so each element
> counters whatever colour left by the preceding element seems less
> intuitive when adding or moving elements in the final $gitstring. Adding
> an element will then require going into __git_ps1_colorize_gitstring,
> even when it is not intended to be colourized. All existing uncoloured
> elements will also need to be prefixed to protect against colour bleed
> from being moved around. I'm partial to the idea of each coloured
> element clearing its own colour.

I think that each makes sense in its own way.  Depending on what
assumption we can make on the use of terminal attributes, one can
produce shorter output than the other.

For example, if you have 3 things, A, B, and C, that are shown in
this order, the "clear after yourselves" scheme would give

	gitstring=<red>A<clear><blue>B<clear><green>C<clear>

while "clear the slate for yourself before you draw, the framework
will clear the effect of the last one" scheme can give

	gitstring=<red>A<blue>B<green>C<clear>

if we know that no additive terminal attributes are used, and the
latter gives a shorter output.

If we need to support some additive ones (like "reverse"), on the
other hand, and if each element is independent (i.e. "clear the
slate for me" cannot use the knowledge of what the previous one
did), then we have to write

	gitstring=<red>A<clear><blue>B<clear><green>C<clear>

for the latter, which becomes more verbose (but is the same as the
"each is on its own, clear after yourselves" version).

I have no strong preference either way myself.  "each on its own"
might be conceptually simpler and easier to understand and explain
what is going on.

Thanks.

Re: [PATCH v5] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. Make the colourization of these
> state indicators consistent by making all colourized indicators clear
> their own colour.

> As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
> consistent, 2022-02-27), colourization in the output of __git_ps1 has
> changed such that the short upstream state indicator inherits the colour
> of the last short state indicator before it (if there is one), while
> before this change it was white/the default text colour. Some examples
> to illustrate this behaviour (assuming all indicators are enabled and
> colourization is on):
>  * If there is something in the stash, both the '$' and the short
>    upstream state indicator following it will be blue.
>  * If the local tree has new, untracked files and there is nothing in
>    the stash, both the '%' and the    short upstream state indicator

looong space?

>    will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
>
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change is that previously, the short upstream state indicator appeared
> immediately after the rebase/revert/bisect/merge state indicator (note
> the position of $p in $gitstring):
>
> 	local f="$h$w$i$s$u"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
> 	
> Said indicator is prepended with the clear colour code, and the short
> upstream state indicator is thus also uncoloured. Now, the short
> upstream state indicator follows the sequence of colourized indicators,
> without any clearing of colour (again note the position of $p, now in
> $f):
>
> 	local f="$h$w$i$s$u$p"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>
> If the user is in a sparse checkout, the sparsity state indicator
> follows a similar pattern to the short upstream state indicator.
> However, clearing colour of the colourized indicators changes how the
> sparsity state indicator is colourized , as it currently inherits (and
> before the change referenced also inherited) the colour of the last
> short state indicator before it. Reading the commit message of the
> change that introduced the sparsity state indicator, afda36dbf3b
> (git-prompt: include sparsity state as well, 2020-06-21), it appears
> this colourization also was unintended, so clearing the colour for said
> indicator further increases consistency.

Here, after explaining how bad the current situation is, like the
above, is a good place to say what we do, i.e. "teach indicators to
clear after themselves".

> Colouring of $c was made dependent on it not being empty, as it is no
> longer being used to colour the branch name. Removal of $b's prefix was
> moved to before the colourization so it gets cleared properly now that
> colour codes are inserted into it.
>
> Due to colour clearing being moved into the variables for each coloured
> indicator, the tests for the coloured Bash prompt had to be changed:
>  * All colour tests now have the colour codes around the expected
>    content of the expanded $__git_ps1_branch_name variable instead of
>    the unexpanded variable in the string.
>  * The test with two indicators had a clear-colour code inserted after
>    the symbol for the first indicator, since all indicators clear their
>    own colours now.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---

Nicely written.

Will queue.

Thanks.

Re: [PATCH v5] git-prompt: make colourization consistent

[Joakim Petersen]

On 06/06/2022 18:29, Junio C Hamano wrote:
> Joakim Petersen <joak-pet@online.no> writes:
>>   * If the local tree has new, untracked files and there is nothing in
>>     the stash, both the '%' and the    short upstream state indicator
> 
> looong space?
> 

That is quite the long space indeed, I'll get that fixed.

>> If the user is in a sparse checkout, the sparsity state indicator
>> follows a similar pattern to the short upstream state indicator.
>> However, clearing colour of the colourized indicators changes how the
>> sparsity state indicator is colourized , as it currently inherits (and
>> before the change referenced also inherited) the colour of the last
>> short state indicator before it. Reading the commit message of the
>> change that introduced the sparsity state indicator, afda36dbf3b
>> (git-prompt: include sparsity state as well, 2020-06-21), it appears
>> this colourization also was unintended, so clearing the colour for said
>> indicator further increases consistency.
> 
> Here, after explaining how bad the current situation is, like the
> above, is a good place to say what we do, i.e. "teach indicators to
> clear after themselves".

I'll add a clear statement of what this patch does as well.

> 
> Nicely written.
> 
> Will queue.
> 
> Thanks.
> 
Alright, great!

Re: [PATCH v5] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> On 06/06/2022 18:29, Junio C Hamano wrote:
>> Joakim Petersen <joak-pet@online.no> writes:
>>>   * If the local tree has new, untracked files and there is nothing in
>>>     the stash, both the '%' and the    short upstream state indicator
>> looong space?
>> 
>
> That is quite the long space indeed, I'll get that fixed.
>
>>> If the user is in a sparse checkout, the sparsity state indicator
>>> follows a similar pattern to the short upstream state indicator.
>>> However, clearing colour of the colourized indicators changes how the
>>> sparsity state indicator is colourized , as it currently inherits (and
>>> before the change referenced also inherited) the colour of the last
>>> short state indicator before it. Reading the commit message of the
>>> change that introduced the sparsity state indicator, afda36dbf3b
>>> (git-prompt: include sparsity state as well, 2020-06-21), it appears
>>> this colourization also was unintended, so clearing the colour for said
>>> indicator further increases consistency.
>> Here, after explaining how bad the current situation is, like the
>> above, is a good place to say what we do, i.e. "teach indicators to
>> clear after themselves".
>
> I'll add a clear statement of what this patch does as well.

I think you already have "Make the coloring of these ... consistent
by making ..." much earlier, and moving it here would be sufficient.

>> Nicely written.
>> Will queue.
>> Thanks.
>> 
> Alright, great!

[PATCH v6] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. Make the colourization of these
state indicators consistent by making all colourized indicators clear
their own colour.

As of 0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), colourization in the output of __git_ps1 has
changed such that the short upstream state indicator inherits the colour
of the last short state indicator before it (if there is one), while
before this change it was white/the default text colour. Some examples
to illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If there is something in the stash, both the '$' and the short
   upstream state indicator following it will be blue.
 * If the local tree has new, untracked files and there is nothing in
   the stash, both the '%' and the short upstream state indicator
   will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change in colourization is that previously, the short upstream state
indicator appeared immediately after the rebase/revert/bisect/merge
state indicator (note the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.
However, clearing colour of the colourized indicators changes how the
sparsity state indicator is colourized, as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colourization also was unintended, so clearing the colour for said
indicator further increases consistency.

Teach indicators to clear their own colours. Make colouring of $c
dependent on it not being empty, as it is no longer being used to colour
the branch name. Move clearing of $b's prefix to before colourization so
it gets cleared properly when colour codes are inserted into it.

Change coloured Bash prompt tests to reflect the colourization changes:
 * Move the colour codes to wrap the expected content of the expanded
   $__git_ps1_branch_name in all tests.
 * Insert a clear-colour code after the symbol for the first indicator
   in "prompt - bash color pc mode - dirty status indicator - dirty
   index and worktree", to reflect that all indicators should clear
   their own colour.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v5:
 * Rephrase the explanation of what this patch does to be in the
   imperative mood and add a clear summarizing statement of what the
   patch does.
 * Fixed the long space and removed another stray space.

Range-diff against v5:
1:  fffef5f73d = 1:  50765eeb95 git-prompt: make colourization consistent

 contrib/completion/git-prompt.sh | 22 ++++++++++++----------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..cb01c2fd5d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
 
 # Helper function that is meant to be called from __git_ps1.  It
 # injects color codes into the appropriate gitstring variables used
-# to build a gitstring.
+# to build a gitstring. Colored variables are responsible for clearing
+# their own color.
 __git_ps1_colorize_gitstring ()
 {
 	if [[ -n ${ZSH_VERSION-} ]]; then
@@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
 	else
 		branch_color="$bad_color"
 	fi
-	c="$branch_color$c"
+	if [ -n "$c" ]; then
+		c="$branch_color$c$c_clear"
+	fi
+	b="$branch_color$b$c_clear"
 
-	z="$c_clear$z"
-	if [ "$w" = "*" ]; then
-		w="$bad_color$w"
+	if [ -n "$w" ]; then
+		w="$bad_color$w$c_clear"
 	fi
 	if [ -n "$i" ]; then
-		i="$ok_color$i"
+		i="$ok_color$i$c_clear"
 	fi
 	if [ -n "$s" ]; then
-		s="$flags_color$s"
+		s="$flags_color$s$c_clear"
 	fi
 	if [ -n "$u" ]; then
-		u="$bad_color$u"
+		u="$bad_color$u$c_clear"
 	fi
-	r="$c_clear$r"
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -554,6 +556,7 @@ __git_ps1 ()
 		fi
 	fi
 
+	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
@@ -563,7 +566,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
 		__git_ps1_branch_name=$b
 		b="\${__git_ps1_branch_name}"
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..abd82eec35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v5] git-prompt: make colourization consistent

[Joakim Petersen]

On 06/06/2022 19:41, Junio C Hamano wrote:
> Joakim Petersen <joak-pet@online.no> writes:
>> I'll add a clear statement of what this patch does as well.
> 
> I think you already have "Make the coloring of these ... consistent
> by making ..." much earlier, and moving it here would be sufficient.
> 

This comment made me realize there is some additional redundancy in the
message; I'll submit a v7 with a slightly clearer one, incorporating
your suggestion as well.

[PATCH v7] git-prompt: make colourization consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. This behaviour was introduced by
0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), while before this change the aforementioned
indicators were white/the default text colour. Some examples to
illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If there is something in the stash, both the '$' and the short
   upstream state indicator following it will be blue.
 * If the local tree has new, untracked files and there is nothing in
   the stash, both the '%' and the short upstream state indicator
   will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change in colourization is that previously, the short upstream state
indicator appeared immediately after the rebase/revert/bisect/merge
state indicator (note the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of colourized indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.
However, clearing colour of the colourized indicators changes how the
sparsity state indicator is colourized, as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colourization also was unintended, so clearing the colour for said
indicator further increases consistency.

Make the colourization of these state indicators consistent by making
all colourized indicators clear their own colour. Make colouring of $c
dependent on it not being empty, as it is no longer being used to colour
the branch name. Move clearing of $b's prefix to before colourization so
it gets cleared properly when colour codes are inserted into it. These
changes make changing the layout of the prompt less prone to unintended
colour changes in the future.

Change coloured Bash prompt tests to reflect the colourization changes:
 * Move the colour codes to wrap the expected content of the expanded
   $__git_ps1_branch_name in all tests.
 * Insert a clear-colour code after the symbol for the first indicator
   in "prompt - bash color pc mode - dirty status indicator - dirty
   index and worktree", to reflect that all indicators should clear
   their own colour.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v6:
 * Remove repeated statements and move all explanation of what the patch
   does to the latter part of the message.
 * Add a short statement about other benefits of the behavioural change.

Range-diff against v6:
1:  50765eeb95 = 1:  e25738c667 git-prompt: make colourization consistent

 contrib/completion/git-prompt.sh | 22 ++++++++++++----------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..cb01c2fd5d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
 
 # Helper function that is meant to be called from __git_ps1.  It
 # injects color codes into the appropriate gitstring variables used
-# to build a gitstring.
+# to build a gitstring. Colored variables are responsible for clearing
+# their own color.
 __git_ps1_colorize_gitstring ()
 {
 	if [[ -n ${ZSH_VERSION-} ]]; then
@@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
 	else
 		branch_color="$bad_color"
 	fi
-	c="$branch_color$c"
+	if [ -n "$c" ]; then
+		c="$branch_color$c$c_clear"
+	fi
+	b="$branch_color$b$c_clear"
 
-	z="$c_clear$z"
-	if [ "$w" = "*" ]; then
-		w="$bad_color$w"
+	if [ -n "$w" ]; then
+		w="$bad_color$w$c_clear"
 	fi
 	if [ -n "$i" ]; then
-		i="$ok_color$i"
+		i="$ok_color$i$c_clear"
 	fi
 	if [ -n "$s" ]; then
-		s="$flags_color$s"
+		s="$flags_color$s$c_clear"
 	fi
 	if [ -n "$u" ]; then
-		u="$bad_color$u"
+		u="$bad_color$u$c_clear"
 	fi
-	r="$c_clear$r"
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -554,6 +556,7 @@ __git_ps1 ()
 		fi
 	fi
 
+	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
@@ -563,7 +566,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
 		__git_ps1_branch_name=$b
 		b="\${__git_ps1_branch_name}"
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..abd82eec35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v5] git-prompt: make colourization consistent

[Junio C Hamano]

Bagas Sanjaya <bagasdotme@gmail.com> writes:

> On 6/5/22 02:26, Joakim Petersen wrote:
>> If the user is in a sparse checkout, the sparsity state indicator
>> follows a similar pattern to the short upstream state indicator.
>> However, clearing colour of the colourized indicators changes how the
>> sparsity state indicator is colourized , as it currently inherits (and
>> before the change referenced also inherited) the colour of the last
>> short state indicator before it. Reading the commit message of the
>> change that introduced the sparsity state indicator, afda36dbf3b
>> (git-prompt: include sparsity state as well, 2020-06-21), it appears
>> this colourization also was unintended, so clearing the colour for said
>> indicator further increases consistency.
>> 
>
> colourization? I have never heared that. Did you mean "colorization" (en-US)
> or "colourisation" (en-UK)? I assumed the former.

;-)  

Either way, that word is a mouthful.  Using verb "to color" and
"coloring" might be easier to read, perhaps?

Re: [PATCH v7] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. This behaviour was introduced by
> 0ec7c23cdc6 (git-prompt: make upstream state indicator location
> consistent, 2022-02-27), while before this change the aforementioned
> indicators were white/the default text colour. Some examples to
> illustrate this behaviour (assuming all indicators are enabled and
> colourization is on):
>  * If there is something in the stash, both the '$' and the short
>    upstream state indicator following it will be blue.
>  * If the local tree has new, untracked files and there is nothing in
>    the stash, both the '%' and the short upstream state indicator
>    will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
>
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change in colourization is that previously, the short upstream state
> indicator appeared immediately after the rebase/revert/bisect/merge
> state indicator (note the position of $p in $gitstring):
>
> 	local f="$h$w$i$s$u"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
> 	
> Said indicator is prepended with the clear colour code, and the short
> upstream state indicator is thus also uncoloured. Now, the short
> upstream state indicator follows the sequence of colourized indicators,
> without any clearing of colour (again note the position of $p, now in
> $f):
>
> 	local f="$h$w$i$s$u$p"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>
> If the user is in a sparse checkout, the sparsity state indicator
> follows a similar pattern to the short upstream state indicator.
> However, clearing colour of the colourized indicators changes how the
> sparsity state indicator is colourized, as it currently inherits (and
> before the change referenced also inherited) the colour of the last
> short state indicator before it. Reading the commit message of the
> change that introduced the sparsity state indicator, afda36dbf3b
> (git-prompt: include sparsity state as well, 2020-06-21), it appears
> this colourization also was unintended, so clearing the colour for said
> indicator further increases consistency.
>
> Make the colourization of these state indicators consistent by making
> all colourized indicators clear their own colour. Make colouring of $c
> dependent on it not being empty, as it is no longer being used to colour
> the branch name. Move clearing of $b's prefix to before colourization so
> it gets cleared properly when colour codes are inserted into it. These
> changes make changing the layout of the prompt less prone to unintended
> colour changes in the future.
>
> Change coloured Bash prompt tests to reflect the colourization changes:
>  * Move the colour codes to wrap the expected content of the expanded
>    $__git_ps1_branch_name in all tests.
>  * Insert a clear-colour code after the symbol for the first indicator
>    in "prompt - bash color pc mode - dirty status indicator - dirty
>    index and worktree", to reflect that all indicators should clear
>    their own colour.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
> Changes since v6:
>  * Remove repeated statements and move all explanation of what the patch
>    does to the latter part of the message.
>  * Add a short statement about other benefits of the behavioural change.

The handling of $w is different from the original (it used to be
that only '*' was painted in red, now any non-empty strings do), but
'*' is the only value that can be assigned to $w, so there is no
material difference.

Looking good.  Will queue.  Thanks.

Re: [PATCH v7] git-prompt: make colourization consistent

[SZEDER Gábor]

On Tue, Jun 07, 2022 at 01:50:24PM +0200, Joakim Petersen wrote:
> The short upstream state indicator inherits the colour of the last short
> state indicator before it (if there is one), and the sparsity state
> indicator inherits this colour as well. This behaviour was introduced by
> 0ec7c23cdc6 (git-prompt: make upstream state indicator location
> consistent, 2022-02-27), while before this change the aforementioned
> indicators were white/the default text colour. Some examples to
> illustrate this behaviour (assuming all indicators are enabled and
> colourization is on):
>  * If there is something in the stash, both the '$' and the short
>    upstream state indicator following it will be blue.
>  * If the local tree has new, untracked files and there is nothing in
>    the stash, both the '%' and the short upstream state indicator
>    will be red.
>  * If all local changes are added to the index and the stash is empty,
>    both the '+' and the short upstream state indicator following it will
>    be green.
>  * If the local tree is clean and there is nothing in the stash, the
>    short upstream state indicator will be white/${default text colour}.
> 
> This appears to be an unintended side-effect of the change, and makes
> little sense semantically (e.g. why is it bad to be in sync with
> upstream when you have uncommitted local changes?). The cause of the
> change in colourization is that previously, the short upstream state
> indicator appeared immediately after the rebase/revert/bisect/merge
> state indicator (note the position of $p in $gitstring):
> 
> 	local f="$h$w$i$s$u"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
> 	
> Said indicator is prepended with the clear colour code, and the short
> upstream state indicator is thus also uncoloured. Now, the short
> upstream state indicator follows the sequence of colourized indicators,
> without any clearing of colour (again note the position of $p, now in
> $f):
> 
> 	local f="$h$w$i$s$u$p"
> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
> 
> If the user is in a sparse checkout, the sparsity state indicator
> follows a similar pattern to the short upstream state indicator.
> However, clearing colour of the colourized indicators changes how the
> sparsity state indicator is colourized, as it currently inherits (and
> before the change referenced also inherited) the colour of the last
> short state indicator before it. Reading the commit message of the
> change that introduced the sparsity state indicator, afda36dbf3b
> (git-prompt: include sparsity state as well, 2020-06-21), it appears
> this colourization also was unintended, so clearing the colour for said
> indicator further increases consistency.
> 
> Make the colourization of these state indicators consistent by making
> all colourized indicators clear their own colour. Make colouring of $c
> dependent on it not being empty, as it is no longer being used to colour
> the branch name. Move clearing of $b's prefix to before colourization so
> it gets cleared properly when colour codes are inserted into it. These
> changes make changing the layout of the prompt less prone to unintended
> colour changes in the future.
> 
> Change coloured Bash prompt tests to reflect the colourization changes:
>  * Move the colour codes to wrap the expected content of the expanded
>    $__git_ps1_branch_name in all tests.
>  * Insert a clear-colour code after the symbol for the first indicator
>    in "prompt - bash color pc mode - dirty status indicator - dirty
>    index and worktree", to reflect that all indicators should clear
>    their own colour.

This patch seems to break colorization when __git_ps1() is invoked
from $PROMPT_COMMAND:

  ~/src/git (master)$ echo $PROMPT_COMMAND 
__git_ps1 "\[\e]0;\w - Terminal\a\e[01;32m\]\h\[\e[01;34m\] \w" "\[\e[01;34m\]\$\[\e[00m\] " " \[\e[01;34m\](%s\[\e[01;34m\])"
  ~/src/git (master)$ git checkout 9470605a1b
  HEAD is now at 9470605a1b git-prompt: make colourization consistent
  ~/src/git ((9470605a1b...))$ source contrib/completion/git-prompt.sh 
  ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ # uh-oh
  ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ git checkout 9470605a1b^
  Previous HEAD position was 9470605a1b git-prompt: make colourization consistent
  HEAD is now at 2668e3608e Sixth batch
  ~/src/git (\[\e[31m\](2668e3608e...)\[\e[0m\])$ source contrib/completion/git-prompt.sh 
  ~/src/git ((2668e3608e...))$ # Looks good.


> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
> Changes since v6:
>  * Remove repeated statements and move all explanation of what the patch
>    does to the latter part of the message.
>  * Add a short statement about other benefits of the behavioural change.
> 
> Range-diff against v6:
> 1:  50765eeb95 = 1:  e25738c667 git-prompt: make colourization consistent
> 
>  contrib/completion/git-prompt.sh | 22 ++++++++++++----------
>  t/t9903-bash-prompt.sh           | 18 +++++++++---------
>  2 files changed, 21 insertions(+), 19 deletions(-)
> 
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 87b2b916c0..cb01c2fd5d 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
>  
>  # Helper function that is meant to be called from __git_ps1.  It
>  # injects color codes into the appropriate gitstring variables used
> -# to build a gitstring.
> +# to build a gitstring. Colored variables are responsible for clearing
> +# their own color.
>  __git_ps1_colorize_gitstring ()
>  {
>  	if [[ -n ${ZSH_VERSION-} ]]; then
> @@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
>  	else
>  		branch_color="$bad_color"
>  	fi
> -	c="$branch_color$c"
> +	if [ -n "$c" ]; then
> +		c="$branch_color$c$c_clear"
> +	fi
> +	b="$branch_color$b$c_clear"
>  
> -	z="$c_clear$z"
> -	if [ "$w" = "*" ]; then
> -		w="$bad_color$w"
> +	if [ -n "$w" ]; then
> +		w="$bad_color$w$c_clear"
>  	fi
>  	if [ -n "$i" ]; then
> -		i="$ok_color$i"
> +		i="$ok_color$i$c_clear"
>  	fi
>  	if [ -n "$s" ]; then
> -		s="$flags_color$s"
> +		s="$flags_color$s$c_clear"
>  	fi
>  	if [ -n "$u" ]; then
> -		u="$bad_color$u"
> +		u="$bad_color$u$c_clear"
>  	fi
> -	r="$c_clear$r"
>  }
>  
>  # Helper function to read the first line of a file into a variable.
> @@ -554,6 +556,7 @@ __git_ps1 ()
>  		fi
>  	fi
>  
> +	b=${b##refs/heads/}
>  	local z="${GIT_PS1_STATESEPARATOR-" "}"
>  
>  	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
> @@ -563,7 +566,6 @@ __git_ps1 ()
>  		fi
>  	fi
>  
> -	b=${b##refs/heads/}
>  	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
>  		__git_ps1_branch_name=$b
>  		b="\${__git_ps1_branch_name}"
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index bbd513bab0..abd82eec35 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - branch name' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	(
>  		GIT_PS1_SHOWCOLORHINTS=y &&
>  		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
> @@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
>  	git checkout b1^ &&
>  	test_when_finished "git checkout main" &&
>  	(
> @@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	echo "dirty" >file &&
>  	test_when_finished "git reset --hard" &&
>  	(
> @@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	echo "dirty" >file &&
>  	test_when_finished "git reset --hard" &&
>  	git add -u &&
> @@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	echo "dirty index" >file &&
>  	test_when_finished "git reset --hard" &&
>  	git add -u &&
> @@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	(
>  		GIT_PS1_SHOWDIRTYSTATE=y &&
>  		GIT_PS1_SHOWCOLORHINTS=y &&
> @@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
>  	echo "dirty" >file &&
>  	test_when_finished "git reset --hard" &&
>  	(
> @@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - stash status indicator' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	echo 2 >file &&
>  	git stash &&
>  	test_when_finished "git stash drop" &&
> @@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
>  '
>  
>  test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
> -	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
> +	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
>  	(
>  		GIT_PS1_SHOWUNTRACKEDFILES=y &&
>  		GIT_PS1_SHOWCOLORHINTS=y &&
> -- 
> 2.36.1
> 

Re: [PATCH v7] git-prompt: make colourization consistent

[Joakim Petersen]

On 09/06/2022 11:03, SZEDER Gábor wrote:
> This patch seems to break colorization when __git_ps1() is invoked
> from $PROMPT_COMMAND:
> 
>    ~/src/git (master)$ echo $PROMPT_COMMAND
> __git_ps1 "\[\e]0;\w - Terminal\a\e[01;32m\]\h\[\e[01;34m\] \w" "\[\e[01;34m\]\$\[\e[00m\] " " \[\e[01;34m\](%s\[\e[01;34m\])"
>    ~/src/git (master)$ git checkout 9470605a1b
>    HEAD is now at 9470605a1b git-prompt: make colourization consistent
>    ~/src/git ((9470605a1b...))$ source contrib/completion/git-prompt.sh
>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ # uh-oh
>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ git checkout 9470605a1b^
>    Previous HEAD position was 9470605a1b git-prompt: make colourization consistent
>    HEAD is now at 2668e3608e Sixth batch
>    ~/src/git (\[\e[31m\](2668e3608e...)\[\e[0m\])$ source contrib/completion/git-prompt.sh
>    ~/src/git ((2668e3608e...))$ # Looks good.
> 

While I did test this on my own prompt for v6 (which is identical to v7
in terms of code) and not see any breakage, I have the same issue with
v7. Maybe I forgot to re-source the changed git-prompt.sh. Either way,
The issue stems from $b being wrapped in $__git_ps1_branch_name and then
back into itself after colouring. Moving this wrapping to before colour
is applied fixes this. I will submit a v8 shortly.

Re: [PATCH v7] git-prompt: make colourization consistent

[Joakim Petersen]

On 07/06/2022 18:22, Junio C Hamano wrote:
> Joakim Petersen <joak-pet@online.no> writes:
> 
>> The short upstream state indicator inherits the colour of the last short
>> state indicator before it (if there is one), and the sparsity state
>> indicator inherits this colour as well. This behaviour was introduced by
>> 0ec7c23cdc6 (git-prompt: make upstream state indicator location
>> consistent, 2022-02-27), while before this change the aforementioned
>> indicators were white/the default text colour. Some examples to
>> illustrate this behaviour (assuming all indicators are enabled and
>> colourization is on):
>>   * If there is something in the stash, both the '$' and the short
>>     upstream state indicator following it will be blue.
>>   * If the local tree has new, untracked files and there is nothing in
>>     the stash, both the '%' and the short upstream state indicator
>>     will be red.
>>   * If all local changes are added to the index and the stash is empty,
>>     both the '+' and the short upstream state indicator following it will
>>     be green.
>>   * If the local tree is clean and there is nothing in the stash, the
>>     short upstream state indicator will be white/${default text colour}.
>>
>> This appears to be an unintended side-effect of the change, and makes
>> little sense semantically (e.g. why is it bad to be in sync with
>> upstream when you have uncommitted local changes?). The cause of the
>> change in colourization is that previously, the short upstream state
>> indicator appeared immediately after the rebase/revert/bisect/merge
>> state indicator (note the position of $p in $gitstring):
>>
>> 	local f="$h$w$i$s$u"
>> 	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
>> 	
>> Said indicator is prepended with the clear colour code, and the short
>> upstream state indicator is thus also uncoloured. Now, the short
>> upstream state indicator follows the sequence of colourized indicators,
>> without any clearing of colour (again note the position of $p, now in
>> $f):
>>
>> 	local f="$h$w$i$s$u$p"
>> 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>>
>> If the user is in a sparse checkout, the sparsity state indicator
>> follows a similar pattern to the short upstream state indicator.
>> However, clearing colour of the colourized indicators changes how the
>> sparsity state indicator is colourized, as it currently inherits (and
>> before the change referenced also inherited) the colour of the last
>> short state indicator before it. Reading the commit message of the
>> change that introduced the sparsity state indicator, afda36dbf3b
>> (git-prompt: include sparsity state as well, 2020-06-21), it appears
>> this colourization also was unintended, so clearing the colour for said
>> indicator further increases consistency.
>>
>> Make the colourization of these state indicators consistent by making
>> all colourized indicators clear their own colour. Make colouring of $c
>> dependent on it not being empty, as it is no longer being used to colour
>> the branch name. Move clearing of $b's prefix to before colourization so
>> it gets cleared properly when colour codes are inserted into it. These
>> changes make changing the layout of the prompt less prone to unintended
>> colour changes in the future.
>>
>> Change coloured Bash prompt tests to reflect the colourization changes:
>>   * Move the colour codes to wrap the expected content of the expanded
>>     $__git_ps1_branch_name in all tests.
>>   * Insert a clear-colour code after the symbol for the first indicator
>>     in "prompt - bash color pc mode - dirty status indicator - dirty
>>     index and worktree", to reflect that all indicators should clear
>>     their own colour.
>>
>> Signed-off-by: Joakim Petersen <joak-pet@online.no>
>> ---
>> Changes since v6:
>>   * Remove repeated statements and move all explanation of what the patch
>>     does to the latter part of the message.
>>   * Add a short statement about other benefits of the behavioural change.
> 
> The handling of $w is different from the original (it used to be
> that only '*' was painted in red, now any non-empty strings do), but
> '*' is the only value that can be assigned to $w, so there is no
> material difference.
> 
> Looking good.  Will queue.  Thanks.
> 

The change regarding $w was mentioned below --- for v5:
 > Changes since v4:
 >  * The check for whether to colourize $w has been altered to match the
 >    checks for the other indicators.
I'll add a mention of this to the commit message as well.

Re: [PATCH v5] git-prompt: make colourization consistent

[Joakim Petersen]

On 07/06/2022 18:04, Junio C Hamano wrote:
> Bagas Sanjaya <bagasdotme@gmail.com> writes:
> 
>> On 6/5/22 02:26, Joakim Petersen wrote:
>>> If the user is in a sparse checkout, the sparsity state indicator
>>> follows a similar pattern to the short upstream state indicator.
>>> However, clearing colour of the colourized indicators changes how the
>>> sparsity state indicator is colourized , as it currently inherits (and
>>> before the change referenced also inherited) the colour of the last
>>> short state indicator before it. Reading the commit message of the
>>> change that introduced the sparsity state indicator, afda36dbf3b
>>> (git-prompt: include sparsity state as well, 2020-06-21), it appears
>>> this colourization also was unintended, so clearing the colour for said
>>> indicator further increases consistency.
>>>
>>
>> colourization? I have never heared that. Did you mean "colorization" (en-US)
>> or "colourisation" (en-UK)? I assumed the former.
> 
> ;-)
> 
> Either way, that word is a mouthful.  Using verb "to color" and
> "coloring" might be easier to read, perhaps?
> 

I went with "colourization" as that is what the process is referred to
in the code, however, I do agree it is a mouthful compared to "colour".
Since I'm already submitting a v8 for other reasons, I'll make this
change in the message as well.

Since the comment about the choice of suffix spelling got picked up,
I'll repeat what I told Bagas off list: "-ize" being American English
is a misconception; most uses of "z" instead of "s" are AE, but the verb
suffix is closer to its origin in Greek with "z", and is preferred by
Oxford for this reason.

[PATCH v8] git-prompt: make colouring consistent

[Joakim Petersen]

The short upstream state indicator inherits the colour of the last short
state indicator before it (if there is one), and the sparsity state
indicator inherits this colour as well. This behaviour was introduced by
0ec7c23cdc6 (git-prompt: make upstream state indicator location
consistent, 2022-02-27), while before this change the aforementioned
indicators were white/the default text colour. Some examples to
illustrate this behaviour (assuming all indicators are enabled and
colourization is on):
 * If there is something in the stash, both the '$' and the short
   upstream state indicator following it will be blue.
 * If the local tree has new, untracked files and there is nothing in
   the stash, both the '%' and the short upstream state indicator
   will be red.
 * If all local changes are added to the index and the stash is empty,
   both the '+' and the short upstream state indicator following it will
   be green.
 * If the local tree is clean and there is nothing in the stash, the
   short upstream state indicator will be white/${default text colour}.

This appears to be an unintended side-effect of the change, and makes
little sense semantically (e.g. why is it bad to be in sync with
upstream when you have uncommitted local changes?). The cause of the
change in colour is that previously, the short upstream state indicator
appeared immediately after the rebase/revert/bisect/merge state
indicator (note the position of $p in $gitstring):

	local f="$h$w$i$s$u"
	local gitstring="$c$b${f:+$z$f}${sparse}$r$p"
	
Said indicator is prepended with the clear colour code, and the short
upstream state indicator is thus also uncoloured. Now, the short
upstream state indicator follows the sequence of coloured indicators,
without any clearing of colour (again note the position of $p, now in
$f):

	local f="$h$w$i$s$u$p"
	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"

If the user is in a sparse checkout, the sparsity state indicator
follows a similar pattern to the short upstream state indicator.
However, clearing colour of the coloured indicators changes how the
sparsity state indicator is coloured, as it currently inherits (and
before the change referenced also inherited) the colour of the last
short state indicator before it. Reading the commit message of the
change that introduced the sparsity state indicator, afda36dbf3b
(git-prompt: include sparsity state as well, 2020-06-21), it appears
this colouring also was unintended, so clearing the colour for said
indicator further increases consistency.

Make the colouring of these state indicators consistent by making all
coloured indicators clear their own colour. Make colouring of $c
dependent on it not being empty, as it is no longer being used to colour
the branch name. Change $w to follow the same behaviour so changes to
its content don't change how it is coloured. Move clearing of $b's
prefix to before colouring so it gets cleared properly when colour codes
are inserted into it. These changes make changing the layout of the
prompt less prone to unintended colour changes in the future.

Change coloured Bash prompt tests to reflect the colourization changes:
 * Move the colour codes to wrap the expected content of the expanded
   $__git_ps1_branch_name in all tests.
 * Insert a clear-colour code after the symbol for the first indicator
   in "prompt - bash color pc mode - dirty status indicator - dirty
   index and worktree", to reflect that all indicators should clear
   their own colour.

Having each indicator clear its own colour lessens the cost of
refactoring the prompt layout, as the outcome of colouring becomes
independent on the position of indicators in the final prompt.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v7:
 * Fixed branch name ($b) not being coloured, but rather having its
   colour codes printed as text.
 * Added mention of the change to the check for whether to colour $w.
 * Replaced use of "colourize" and derivatives in the message with
   "colour".

Range-diff against v7:
1:  50765eeb95 ! 1:  1082f46b4c git-prompt: make colourization consistent
    @@ contrib/completion/git-prompt.sh: __git_ps1_colorize_gitstring ()
      
      # Helper function to read the first line of a file into a variable.
     @@ contrib/completion/git-prompt.sh: __git_ps1 ()
    - 		fi
      	fi
      
    -+	b=${b##refs/heads/}
      	local z="${GIT_PS1_STATESEPARATOR-" "}"
    ++	b=${b##refs/heads/}
    ++	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
    ++		__git_ps1_branch_name=$b
    ++		b="\${__git_ps1_branch_name}"
    ++	fi
    ++
      
      	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
    + 	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
     @@ contrib/completion/git-prompt.sh: __git_ps1 ()
      		fi
      	fi
      
     -	b=${b##refs/heads/}
    - 	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
    - 		__git_ps1_branch_name=$b
    - 		b="\${__git_ps1_branch_name}"
    +-	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
    +-		__git_ps1_branch_name=$b
    +-		b="\${__git_ps1_branch_name}"
    +-	fi
    +-
    + 	local f="$h$w$i$s$u$p"
    + 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
    + 
     
      ## t/t9903-bash-prompt.sh ##
     @@ t/t9903-bash-prompt.sh: test_expect_success 'prompt - pc mode' '

 contrib/completion/git-prompt.sh | 32 +++++++++++++++++---------------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 87b2b916c0..44bfc7972e 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -245,7 +245,8 @@ __git_ps1_show_upstream ()
 
 # Helper function that is meant to be called from __git_ps1.  It
 # injects color codes into the appropriate gitstring variables used
-# to build a gitstring.
+# to build a gitstring. Colored variables are responsible for clearing
+# their own color.
 __git_ps1_colorize_gitstring ()
 {
 	if [[ -n ${ZSH_VERSION-} ]]; then
@@ -271,22 +272,23 @@ __git_ps1_colorize_gitstring ()
 	else
 		branch_color="$bad_color"
 	fi
-	c="$branch_color$c"
+	if [ -n "$c" ]; then
+		c="$branch_color$c$c_clear"
+	fi
+	b="$branch_color$b$c_clear"
 
-	z="$c_clear$z"
-	if [ "$w" = "*" ]; then
-		w="$bad_color$w"
+	if [ -n "$w" ]; then
+		w="$bad_color$w$c_clear"
 	fi
 	if [ -n "$i" ]; then
-		i="$ok_color$i"
+		i="$ok_color$i$c_clear"
 	fi
 	if [ -n "$s" ]; then
-		s="$flags_color$s"
+		s="$flags_color$s$c_clear"
 	fi
 	if [ -n "$u" ]; then
-		u="$bad_color$u"
+		u="$bad_color$u$c_clear"
 	fi
-	r="$c_clear$r"
 }
 
 # Helper function to read the first line of a file into a variable.
@@ -555,6 +557,12 @@ __git_ps1 ()
 	fi
 
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
+	b=${b##refs/heads/}
+	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
+		__git_ps1_branch_name=$b
+		b="\${__git_ps1_branch_name}"
+	fi
+
 
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
 	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
@@ -563,12 +571,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
-	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
-		__git_ps1_branch_name=$b
-		b="\${__git_ps1_branch_name}"
-	fi
-
 	local f="$h$w$i$s$u$p"
 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
 
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index bbd513bab0..abd82eec35 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,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}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_green}+${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
+	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v7] git-prompt: make colourization consistent

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> On 09/06/2022 11:03, SZEDER Gábor wrote:
>> This patch seems to break colorization when __git_ps1() is invoked
>> from $PROMPT_COMMAND:
>>    ~/src/git (master)$ echo $PROMPT_COMMAND
>> __git_ps1 "\[\e]0;\w - Terminal\a\e[01;32m\]\h\[\e[01;34m\] \w" "\[\e[01;34m\]\$\[\e[00m\] " " \[\e[01;34m\](%s\[\e[01;34m\])"
>>    ~/src/git (master)$ git checkout 9470605a1b
>>    HEAD is now at 9470605a1b git-prompt: make colourization consistent
>>    ~/src/git ((9470605a1b...))$ source contrib/completion/git-prompt.sh
>>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ # uh-oh
>>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ git checkout 9470605a1b^
>>    Previous HEAD position was 9470605a1b git-prompt: make colourization consistent
>>    HEAD is now at 2668e3608e Sixth batch
>>    ~/src/git (\[\e[31m\](2668e3608e...)\[\e[0m\])$ source contrib/completion/git-prompt.sh
>>    ~/src/git ((2668e3608e...))$ # Looks good.
>> 
>
> While I did test this on my own prompt for v6 (which is identical to v7
> in terms of code) and not see any breakage, I have the same issue with
> v7. Maybe I forgot to re-source the changed git-prompt.sh. Either way,
> The issue stems from $b being wrapped in $__git_ps1_branch_name and then
> back into itself after colouring. Moving this wrapping to before colour
> is applied fixes this. I will submit a v8 shortly.

As the topic is already in 'next' (and presumably that is how SZEDER
noticed the breakage), please make it an incremental fix-up.

Thanks.

[PATCH] git-prompt: fix expansion of branch colour codes

[Joakim Petersen]

Because of the wrapping of the branch name variable $b, the colour codes
in the variable don't get applied, but are instead printed directly in
the output. Move the wrapping of $b to before colour codes are inserted
to correct this.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
 contrib/completion/git-prompt.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index cb01c2fd5d..1435548e00 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -556,9 +556,14 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
+	b=${b##refs/heads/}
+	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
+		__git_ps1_branch_name=$b
+		b="\${__git_ps1_branch_name}"
+	fi
+
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
 	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
 		if [ $pcmode = yes ] || [ -n "${ZSH_VERSION-}" ]; then
@@ -566,11 +571,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
-		__git_ps1_branch_name=$b
-		b="\${__git_ps1_branch_name}"
-	fi
-
 	local f="$h$w$i$s$u$p"
 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
 

base-commit: 9470605a1b03dac8fc4f801132e36964b4fbb8c3
-- 
2.36.1

Re: [PATCH] git-prompt: fix expansion of branch colour codes

[Junio C Hamano]

Joakim Petersen <joak-pet@online.no> writes:

> Because of the wrapping of the branch name variable $b, the colour codes
> in the variable don't get applied, but are instead printed directly in
> the output. Move the wrapping of $b to before colour codes are inserted
> to correct this.
>
> Signed-off-by: Joakim Petersen <joak-pet@online.no>
> ---
>  contrib/completion/git-prompt.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)

t9903 seems to fail with this, though...?

> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index cb01c2fd5d..1435548e00 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -556,9 +556,14 @@ __git_ps1 ()
>  		fi
>  	fi
>  
> -	b=${b##refs/heads/}
>  	local z="${GIT_PS1_STATESEPARATOR-" "}"
>  
> +	b=${b##refs/heads/}
> +	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
> +		__git_ps1_branch_name=$b
> +		b="\${__git_ps1_branch_name}"
> +	fi
> +
>  	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
>  	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
>  		if [ $pcmode = yes ] || [ -n "${ZSH_VERSION-}" ]; then
> @@ -566,11 +571,6 @@ __git_ps1 ()
>  		fi
>  	fi
>  
> -	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
> -		__git_ps1_branch_name=$b
> -		b="\${__git_ps1_branch_name}"
> -	fi
> -
>  	local f="$h$w$i$s$u$p"
>  	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
>  
>
> base-commit: 9470605a1b03dac8fc4f801132e36964b4fbb8c3

Re: [PATCH] git-prompt: fix expansion of branch colour codes

[Joakim Petersen]

On 10/06/2022 02:05, Junio C Hamano wrote:
> t9903 seems to fail with this, though...?
> 

Ah yes, sorry about that, I seem to have gotten a bit ahead of myself.
Since the wrapping of $b was entirely moved to before colouring happens,
the tests need to be reverted back to their state from before the
previous commit, with the exception of the extra $c_clear in the test
with two active indicators. I'll submit a v2 with this change shortly.

[PATCH v2] git-prompt: fix expansion of branch colour codes

[Joakim Petersen]

Because of the wrapping of the branch name variable $b, the colour codes
in the variable don't get applied, but are instead printed directly in
the output. Move the wrapping of $b to before colour codes are inserted
to correct this. Revert move of branch name colour codes in tests, as
the branch name is now coloured after the wrapping instead of before.

Signed-off-by: Joakim Petersen <joak-pet@online.no>
---
Changes since v1:
 * Tests have been reverted to how they were before the initial
   colouring change in 9470605a1b0 (git-prompt: make colourization
   consistent, 2022-06-07), with the exception of the added $c_clear for
   the test with two active indicators.

Range-diff against v1:
1:  f4e46d37a5 < -:  ---------- git-prompt: fix expansion of branch colour codes
-:  ---------- > 1:  2520d60f0f git-prompt: fix expansion of branch colour codes

 contrib/completion/git-prompt.sh | 12 ++++++------
 t/t9903-bash-prompt.sh           | 18 +++++++++---------
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index cb01c2fd5d..1435548e00 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -556,9 +556,14 @@ __git_ps1 ()
 		fi
 	fi
 
-	b=${b##refs/heads/}
 	local z="${GIT_PS1_STATESEPARATOR-" "}"
 
+	b=${b##refs/heads/}
+	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
+		__git_ps1_branch_name=$b
+		b="\${__git_ps1_branch_name}"
+	fi
+
 	# NO color option unless in PROMPT_COMMAND mode or it's Zsh
 	if [ -n "${GIT_PS1_SHOWCOLORHINTS-}" ]; then
 		if [ $pcmode = yes ] || [ -n "${ZSH_VERSION-}" ]; then
@@ -566,11 +571,6 @@ __git_ps1 ()
 		fi
 	fi
 
-	if [ $pcmode = yes ] && [ $ps1_expanded = yes ]; then
-		__git_ps1_branch_name=$b
-		b="\${__git_ps1_branch_name}"
-	fi
-
 	local f="$h$w$i$s$u$p"
 	local gitstring="$c$b${f:+$z$f}${sparse}$r${upstream}"
 
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index abd82eec35..6a30f5719c 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -541,7 +541,7 @@ test_expect_success 'prompt - pc mode' '
 '
 
 test_expect_success 'prompt - bash color pc mode - branch name' '
-	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nmain" >expected &&
 	(
 		GIT_PS1_SHOWCOLORHINTS=y &&
 		__git_ps1 "BEFORE:" ":AFTER" >"$actual" &&
@@ -551,7 +551,7 @@ test_expect_success 'prompt - bash color pc mode - branch name' '
 '
 
 test_expect_success 'prompt - bash color pc mode - detached head' '
-	printf "BEFORE: (\${__git_ps1_branch_name}):AFTER\\n${c_red}(%s...)"${c_clear} $(git log -1 --format="%h" b1^) >expected &&
+	printf "BEFORE: (${c_red}\${__git_ps1_branch_name}${c_clear}):AFTER\\n(%s...)" $(git log -1 --format="%h" b1^) >expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout main" &&
 	(
@@ -563,7 +563,7 @@ test_expect_success 'prompt - bash color pc mode - detached head' '
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty worktree' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}):AFTER\\nmain" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -576,7 +576,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}+${c_clear}):AFTER\\nmain" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -590,7 +590,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirty index and worktree' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}*${c_clear}${c_green}+${c_clear}):AFTER\\nmain" >expected &&
 	echo "dirty index" >file &&
 	test_when_finished "git reset --hard" &&
 	git add -u &&
@@ -605,7 +605,7 @@ test_expect_success 'prompt - bash color pc mode - dirty status indicator - dirt
 '
 
 test_expect_success 'prompt - bash color pc mode - dirty status indicator - before root commit' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_green}#${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_green}#${c_clear}):AFTER\\nmain" >expected &&
 	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
@@ -617,7 +617,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: (\${__git_ps1_branch_name}):AFTER\\n${c_green}GIT_DIR!${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear}):AFTER\\nGIT_DIR!" >expected &&
 	echo "dirty" >file &&
 	test_when_finished "git reset --hard" &&
 	(
@@ -631,7 +631,7 @@ test_expect_success 'prompt - bash color pc mode - inside .git directory' '
 '
 
 test_expect_success 'prompt - bash color pc mode - stash status indicator' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_lblue}\$${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_lblue}\$${c_clear}):AFTER\\nmain" >expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
@@ -645,7 +645,7 @@ test_expect_success 'prompt - bash color pc mode - stash status indicator' '
 '
 
 test_expect_success 'prompt - bash color pc mode - untracked files status indicator' '
-	printf "BEFORE: (\${__git_ps1_branch_name} ${c_red}%%${c_clear}):AFTER\\n${c_green}main${c_clear}" >expected &&
+	printf "BEFORE: (${c_green}\${__git_ps1_branch_name}${c_clear} ${c_red}%%${c_clear}):AFTER\\nmain" >expected &&
 	(
 		GIT_PS1_SHOWUNTRACKEDFILES=y &&
 		GIT_PS1_SHOWCOLORHINTS=y &&
-- 
2.36.1

Re: [PATCH v7] git-prompt: make colourization consistent

[SZEDER Gábor]

On Thu, Jun 09, 2022 at 11:29:25AM -0700, Junio C Hamano wrote:
> Joakim Petersen <joak-pet@online.no> writes:
> 
> > On 09/06/2022 11:03, SZEDER Gábor wrote:
> >> This patch seems to break colorization when __git_ps1() is invoked
> >> from $PROMPT_COMMAND:
> >>    ~/src/git (master)$ echo $PROMPT_COMMAND
> >> __git_ps1 "\[\e]0;\w - Terminal\a\e[01;32m\]\h\[\e[01;34m\] \w" "\[\e[01;34m\]\$\[\e[00m\] " " \[\e[01;34m\](%s\[\e[01;34m\])"
> >>    ~/src/git (master)$ git checkout 9470605a1b
> >>    HEAD is now at 9470605a1b git-prompt: make colourization consistent
> >>    ~/src/git ((9470605a1b...))$ source contrib/completion/git-prompt.sh
> >>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ # uh-oh
> >>    ~/src/git (\[\e[31m\](9470605a1b...)\[\e[0m\])$ git checkout 9470605a1b^
> >>    Previous HEAD position was 9470605a1b git-prompt: make colourization consistent
> >>    HEAD is now at 2668e3608e Sixth batch
> >>    ~/src/git (\[\e[31m\](2668e3608e...)\[\e[0m\])$ source contrib/completion/git-prompt.sh
> >>    ~/src/git ((2668e3608e...))$ # Looks good.
> >> 
> >
> > While I did test this on my own prompt for v6 (which is identical to v7
> > in terms of code) and not see any breakage, I have the same issue with
> > v7. Maybe I forgot to re-source the changed git-prompt.sh. Either way,
> > The issue stems from $b being wrapped in $__git_ps1_branch_name and then
> > back into itself after colouring. Moving this wrapping to before colour
> > is applied fixes this. I will submit a v8 shortly.
> 
> As the topic is already in 'next' (and presumably that is how SZEDER
> noticed the breakage),

Indeed.  I usually use a custom git built from 'next' with a couple of
my forever-WIP topics merged on top, and I just happened to build and
deploy a version with this patch already merged the other day, with
the additional stroke of luck that I opened a new terminal window
(what I normally rarely do) whose shell sourced the buggy prompt
script.

I did notice this patch being discussed on the ML, and found the
amount of changes to the expected output in the tests somewhat
suspicious, but, alas, haven't managed to take a closer look before
the patch went into 'next'.  Still hasn't, actually, but FWIW Joakim's
fix (as 0e5d9ef395 in 'seen') does work for me.


Thanks,
Gábor