Re: [PATCH 1/2] handle color.ui at a central place

[Jeff King <peff@peff.net>]

On Sun, Jan 18, 2009 at 09:37:15PM +0100, Markus Heidelberg wrote:

> Not sure, if you it has something to do with the following, but I had
> this in my tree for some days now, waiting for the 2 commits mentioned
> in the log message to graduate to master, which happend just an hour or
> so ago.

I think this is probably an improvement, but I had in mind something a
little more drastic. Right now we keep munging one variable that is our
current idea of "should we do color" based on multiple config values.
Then you end up with (best case) this "finalize color config", which is
a bit ugly, or (worst case) bugs where the value hasn't always been
properly initialized (or finalized).

So I think it makes more sense to record each config value, and then
check a _function_ that does the right thing. I.e., you end up with
something like:

  if (use_color(COLOR_DIFF)) /* or COLOR_BRANCH, etc */
    ...

  int use_color(enum color_type t)
  {
    enum color_preference preference;
    preference = color_config[t];
    if (preference == COLOR_UNKNOWN)
      preference = color_config[COLOR_UI];
    if (preference == COLOR_UNKNOWN)
      preference = /* some sane default, possibly command-dependent */
    if (preference == COLOR_AUTO)
      return pager_in_use() || isatty(1);
    if (preference == COLOR_ALWAYS)
      return 1;
    if (preference == COLOR_NEVER)
      return 0;
  }

which very clearly expresses the policy being used (and you probably
want to memo-ize either that whole thing, or at least the isatty check
to avoid making repeated system calls).

-Peff