| Message ID | 20220601134414.66825-1-joak-pet@online.no (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] git-prompt: make colourization consistent | expand |
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!
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.
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"?
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.
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.
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" }
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(+) base-commit: e54793a95afeea1e10de1e5ad7eab914e7416250