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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Apr 05, 2018 at 06:53:20PM -0400, Jeff King wrote:
> 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.

I agree, too :-). I have changed this in the forthcoming re-roll.

Thanks,
Taylor