Am 10.10.2016 um 19:42 schrieb Jeff King:
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.
- 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