Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 5, 2018, 22:52 UTC
Message-ID: <CAPig+cQoTQ47n13q3t5MMS+UZQR6jbFDpVSP8Z=L4qckjrsxeQ@mail.gmail.com>
In-Reply-To: <20180405223620.GD29117@sigill.intra.peff.net>

On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:

Show 16 quoted lines

> On Wed, Apr 04, 2018 at 07:59:17PM -0700, Taylor Blau wrote:
>> +     if (type == TYPE_COLOR) {
>> +             char v[COLOR_MAXLEN];
>> +             if (!git_config_color(v, key, value))
>> +                     /*
>> +                      * The contents of `v` now contain an ANSI escape
>> +                      * sequence, not suitable for including within a
>> +                      * configuration file. Treat the above as a
>> +                      * "sanity-check", and return the given value, which we
>> +                      * know is representable as valid color code.
>> +                      */
>> +                     return xstrdup(value);
>> +             die("cannot parse color '%s'", value);
>> +     }
>
> And this returns the original. Good.

It's entirely subjective, but I find this easier to read and understand when written like this:

    char v[COLOR_MAXLEN];
    if (git_config_color(v, key, value))
        die("cannot parse color '%s'", value);

    /*
     * The contents of `v` now contain an ANSI escape
     * sequence, not suitable for including within a
     * configuration file. Treat the above as a
     * "sanity-check", and return the given value, which we
     * know is representable as valid color code.
    */
    return xstrdup(value);