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

On Wed, Dec 21, 2016 at 12:26 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

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...

quoted

@@ -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