| Message ID | 20220607115024.64724-1-joak-pet@online.no (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9470605a1b03dac8fc4f801132e36964b4fbb8c3 |
| Headers | show |
| Series | [v7] git-prompt: make colourization consistent | expand |
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.
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 >
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.
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.
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.
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
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 &&
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(-)