From: Jeff King <peff@peff.net>
Date: Thu, 05 Apr 2018 22:53:20 GMT
Subject: Re: [PATCH v4 3/3] builtin/config: introduce `color` type specifier
Message-ID: <20180405225320.GA10913@sigill.intra.peff.net>
In-Reply-To: <CAPig+cQoTQ47n13q3t5MMS+UZQR6jbFDpVSP8Z=L4qckjrsxeQ@mail.gmail.com>

On Thu, Apr 05, 2018 at 06:52:29PM -0400, Eric Sunshine wrote:

> On Thu, Apr 5, 2018 at 6:36 PM, Jeff King <peff@peff.net> wrote:
> > 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);

It may be subjective, but I happen to agree with you.

-Peff