Re: [PATCH] log: support 256 colors with --graph=256colors

[Duy Nguyen <pclouds@gmail.com>]

On Wed, Dec 21, 2016 at 12:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> diff --git a/graph.c b/graph.c
>> index d4e8519..75375a1 100644
>> --- a/graph.c
>> +++ b/graph.c
>> @@ -78,6 +78,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)
>>
>>  static const char **column_colors;
>>  static unsigned short column_colors_max;
>> +static int column_colors_step;
>>
>>  void graph_set_column_colors(const char **colors, unsigned short colors_max)
>>  {
>> @@ -234,10 +235,24 @@ void graph_setup_line_prefix(struct diff_options *diffopt)
>>  }
>>
>>
>> -struct git_graph *graph_init(struct rev_info *opt)
>> +struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg)
>>  {
>>       struct git_graph *graph = xmalloc(sizeof(struct git_graph));
>>
>> +     if (arg && !strcmp(arg, "256colors")) {
>> +             int i, start = 17, stop = 232;
>> +             column_colors_max = stop - start;
>> +             column_colors =
>> +                     xmalloc((column_colors_max + 1) * sizeof(*column_colors));
>> +             for (i = start; i < stop; i++) {
>> +                     struct strbuf sb = STRBUF_INIT;
>> +                     strbuf_addf(&sb, "\033[38;5;%dm", i);
>> +                     column_colors[i - start] = strbuf_detach(&sb, NULL);
>> +             }
>> +             column_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);
>> +             /* ignore the closet 16 colors on either side for the next line */
>> +             column_colors_step = 16;
>> +     }
>
> So you pre-fill a table of colors with 232-17=215 slots.  Is the
> idea that it is a co-prime with column_colors_step which is set to
> 16 so that going over the table with wraparound will cover all its
> elements?

Originally yes (because the next color would be more or less the same,
maybe brighter or darker a bit), then I went fancy with the rand()
thing...

>
>> @@ -382,6 +397,20 @@ static unsigned short graph_get_current_column_color(const struct git_graph *gra
>>   */
>>  static void graph_increment_column_color(struct git_graph *graph)
>>  {
>> +     if (column_colors_step) {
>> +             static int random_initialized;
>> +             int v;
>> +
>> +             if (!random_initialized) {
>> +                     srand((unsigned int)getpid());
>> +                     random_initialized = 1;
>> +             }
>> +             v = rand() % (column_colors_max - column_colors_step * 2);
>> +             graph->default_column_color += column_colors_step + v;
>> +             graph->default_column_color %= column_colors_max;
>> +             return;
>> +     }
>> +
>>       graph->default_column_color = (graph->default_column_color + 1) %
>>               column_colors_max;
>>  }
>
> This is too ugly to live as-is for two reasons.
>
>  - Do you really need rand()?  Doesn't this frustrate somebody who
>    runs the same "git log" in two terminals in order to view an
>    overly tall graph, expecting both commands that were started with
>    the same set of arguments to paint the same line in the same
>    color?

No we probably don't need rand(). The thinking was.. now that we have
a lot more colors to choose from, let's add some randomness, maybe
it'll reduce the chance of showing the same colors in the same line.

There was another concern with a fixed number of steps too, that we
could get into a stable jump sequence and never use all the colors
(e.g. step 3 with 6 colors total, to simplify). But I verify that
we'll use all the colors (at least until we allow people to customize
step and the number colors)
-- 
Duy