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