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

From: Jeff King <peff@peff.net>
Date: Jan 20, 2009, 04:04 UTC
Message-ID: <20090120040448.GA30714@sigill.intra.peff.net>
In-Reply-To: <200901182137.16562.markus.heidelberg@web.de>

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