Re: [PATCH v3] git-prompt: make colourization consistent
- From
Joakim Petersen <joak-pet@online.no>
- Date
- Jun 3, 2022, 17:23 UTC
- Message-ID
- <7d391d82-b15e-4a31-5207-c4037fec0bf9@online.no>
- In-Reply-To
- <xmqqy1ydhfcc.fsf@gitster.g>
On 03/06/2022 18:38, Junio C Hamano wrote:
Show 73 quoted lines
> 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.