diff mbox series

[v2] git-prompt: make colourization consistent

Message ID 20220602145935.10512-1-joak-pet@online.no (mailing list archive)
State Superseded
Headers show
Series [v2] git-prompt: make colourization consistent | expand

Commit Message

Joakim Petersen June 2, 2022, 2:59 p.m. UTC
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(+)

Comments

Joakim Petersen June 2, 2022, 9:56 p.m. UTC | #1
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
Junio C Hamano June 2, 2022, 10:49 p.m. UTC | #2
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.
Joakim Petersen June 3, 2022, 1:55 p.m. UTC | #3
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.
diff mbox series

Patch

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"
 }