On Mon, Oct 10, 2016 at 09:59:17PM +0200, René Scharfe wrote:
> > > 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.
I don't think that will parse "%C(auto,foo)", as we hit the
!skip_prefix() of the conditional, and do not look for "auto," at all.
I think you'd have to move the check for "auto," inside the if block.
I'm leaning towards just writing it out the long way, though, as I did
in my reply to Junio.
> > > 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);
Yeah, that is a bit more clear (the final conditional just needs to
check that we actually found a color).
-Peff