Re: [PATCH] pretty: respect color settings for %C placeholders
- From
René Scharfe <l.s.r@web.de>
- Date
- Oct 10, 2016, 19:59 UTC
- Message-ID
- <19e59db7-f3dd-35ec-8cf1-b070b1c05abe@web.de>
- In-Reply-To
- <20161010174257.b4uxplavefjyr6rl@sigill.intra.peff.net>
Am 10.10.2016 um 19:42 schrieb Jeff King:
Show 20 quoted lines
> On Mon, Oct 10, 2016 at 07:09:12PM +0200, René Scharfe wrote:
>
>>> diff --git a/pretty.c b/pretty.c
>>> index 25efbca..73e58b5 100644
>>> --- a/pretty.c
>>> +++ b/pretty.c
>>> @@ -965,22 +965,31 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */
>>>
>>> if (!end)
>>> return 0;
>>> - if (skip_prefix(begin, "auto,", &begin)) {
>>> +
>>> + if (!skip_prefix(begin, "always,", &begin)) {
>>> if (!want_color(c->pretty_ctx->color))
>>> return end - placeholder + 1;
>>> }
>>
>> Shouldn't we have an "else" here?
>
> I'm not sure what you mean; can you write it out? > - if (skip_prefix(begin, "auto,", &begin)) {
> +
> + if (!skip_prefix(begin, "always,", &begin)) {
> if (!want_color(c->pretty_ctx->color))
> return end - placeholder + 1;
> } else { /* here */> + /* this is a historical noop */ > + skip_prefix(begin, "auto,", &begin);
}
Otherwise "always,auto," would be allowed and mean the same as "always,", which just seems wrong. Not a biggie.
Show 27 quoted lines
>>> - if (skip_prefix(placeholder + 1, "red", &rest)) >>> + if (skip_prefix(placeholder + 1, "red", &rest) && >>> + want_color(c->pretty_ctx->color)) >>> strbuf_addstr(sb, GIT_COLOR_RED); >>> - else if (skip_prefix(placeholder + 1, "green", &rest)) >>> + else if (skip_prefix(placeholder + 1, "green", &rest) && >>> + want_color(c->pretty_ctx->color)) >>> strbuf_addstr(sb, GIT_COLOR_GREEN); >>> - else if (skip_prefix(placeholder + 1, "blue", &rest)) >>> + else if (skip_prefix(placeholder + 1, "blue", &rest) && >>> + want_color(c->pretty_ctx->color)) >>> strbuf_addstr(sb, GIT_COLOR_BLUE); >>> - else if (skip_prefix(placeholder + 1, "reset", &rest)) >>> + else if (skip_prefix(placeholder + 1, "reset", &rest) && >>> + want_color(c->pretty_ctx->color)) >>> strbuf_addstr(sb, GIT_COLOR_RESET); >>> return rest - placeholder; >>> } >> >> Perhaps it's a funtion like add_color(sb, ctx, color) or similar would be >> nice? > > I actually wrote it that way first (I called it "maybe_add_color()"), > but it felt a little funny to have a separate function that people might > be tempted to reuse (the right solution is generally to check > want_color() early as above, but we can't do that here because we have > to find the end of each placeholder).
OK. A variable then? Lazy pseudo-code:
if (RED) color = red; else if (GREEN) ...
if (want_color()) strbuf_addstr(sb, color);
> What I have here is a little funny, too, though, as it keeps trying > other color names if it finds "red" but want_color() returns 0.
Oh, missed that somehow.. O_o