From: Dan McGee <dpmcgee@gmail.com>
Date: Tue, 05 Apr 2011 00:32:05 GMT
Subject: Re: [PATCH] Share color list between graph and show-branch
Message-ID: <BANLkTint1+c0h9DExydWeeafdgawEJPuMw@mail.gmail.com>
In-Reply-To: <7v7hbbcfoj.fsf@alter.siamese.dyndns.org>

On Sun, Apr 3, 2011 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> diff --git a/color.h b/color.h
>> index c0528cf..a7da793 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -53,6 +53,10 @@ struct strbuf;
>>   */
>>  extern int git_use_color_default;
>>
>> +extern const char *column_colors_ansi[13];
>> +
>> +/* Ignore the RESET at the end when giving the size */
>> +#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)
>
> Sneaky.
>
> I first went "Huh? -- this array-size macro cannot work", expecting that
> the array is not decleared with a fixed size in the header.
>
> It may make sense to unify these two palettes whose slot assignment does
> not have any meaning, but it feels that the above change totally goes
> against the spirit of using ARRAY_SIZE() macro, the point of which is to
> liberate programmers from having to count and adjust the size when adding
> the contents to the array.
>
> Wouldn't it make more sense to do something like
>
>    >>> in the header <<<
>    extern const char *custom_colors_ansi[];
>    extern const int CUSTOM_COLORS_ANSI_MAX;
>
>    >>> in the code <<<
>    const char *custom_colors_ansi[] = {
>            ... as before ...
>    };
>    /* Does not count the last element "RESET" */
>    const int CUSTOM_COLORS_ANSI_MAX = ARRAY_SIZE(custom_colors_ansi) - 1;
>
> to avoid mistakes?

Duh. This makes way more sense, I'll resend the patch with the
necessary changes; I couldn't think of an elegant way to do it at the
time.

On another note, we also have this whole crazy "- 1" bit and the RESET
element at the end, and yet I see nowhere that slot is actually used.
It looks like this was introduced by commit 1e3d4119d21df28.

-Dan