Re: [PATCH 07/10] color: delay auto-color decision until point of use
- From
Jeff King <peff@peff.net>
- Date
- Aug 18, 2011, 22:28 UTC
- Message-ID
- <20110818222817.GA8668@sigill.intra.peff.net>
- In-Reply-To
- <7vvctu7402.fsf@alter.siamese.dyndns.org>
On Thu, Aug 18, 2011 at 02:59:37PM -0700, Junio C Hamano wrote:
Show 29 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > diff --git a/color.h b/color.h
> > index a190a25..d715fd5 100644
> > --- a/color.h
> > +++ b/color.h
> > @@ -49,6 +49,16 @@ struct strbuf;
> > #define GIT_COLOR_NIL "NIL"
> >
> > /*
> > + * The first three are chosen to match common usage in the code, and what is
> > + * returned from git_config_colorbool. The "auto" value can be returned from
> > + * config_colorbool, and will be converted by want_color() into either 0 or 1.
> > + */
> > +#define GIT_COLOR_UNKNOWN -1
> > +#define GIT_COLOR_ALWAYS 0
> > +#define GIT_COLOR_NEVER 1
> > +#define GIT_COLOR_AUTO 2
>
> The ALWAYS/NEVER somehow go against my intuition. Let me trace one
> codepath starting from git_branch_config().
>
> branch_use_color is set from git_config_colorbool("color.branch");
> -> given "never", git_config_colorbool() returns 0;
> branch_get_color() asks want_color(branch_use_color);
> -> want_color() returns if the given value is positive.
>
> Because git_config_colorbool() does not use the above symbolic constants,
> everything goes well, but aren't these two swapped?Oooops. Yes, they are completely swapped and I'm an idiot. But as you noticed, we don't actually _use_ them anywhere. I started on replacing every "0" with NEVER, every "1" with ALWAYS, and every "-1" with UNKNOWN. But it really bloated the patch, and didn't actually make the code any more readable.
The only symbolic constant that is really necessary is the AUTO one. I just felt odd randomly defining "2" as GIT_COLOR_AUTO, but not defining the other possible values of the enumeration. So definitely they should be swapped. I'm also fine with just dropping all of them except AUTO.
-Peff