log: support 256 colors with --graph=256colors

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
I got mad after tracing two consecutive red history lines in git log --graph --oneline back to their merge points, far far away. Yeah probably should fire up tig, or gitk or something.

This may sound like a good thing to add, but I don't know how good it is compared to the good old 16 color palette, yet as I haven't tried it for long since it's just written.

graph.c +4 -0

+	if (arg && !strcmp(arg, "256colors")) {
+		int i, start = 17, stop = 232;
+		column_colors_max = stop - start;
+		column_colors =

Quoted 9 lines · Nguyễn Thái Ngọc Duy

Signed-off-by Nguyễn Thái Ngọc Duy <[email protected]>

---
I got mad after tracing two consecutive red history lines in git log --graph --oneline back to their merge points, far far away. Yeah probably should fire up tig, or gitk or something.

This may sound like a good thing to add, but I don't know how good it is compared to the good old 16 color palette, yet as I haven't tried it for long since it's just written.

Hmm. At some point the colors become too close together to be easily distinguishable. In your code you have:

Quoted 4 lines

graph.c +4 -0

+     if (arg && !strcmp(arg, "256colors")) {
+             int i, start = 17, stop = 232;
+             column_colors_max = stop - start;
+             column_colors =

graph.c void graph_setup_line_prefix(struct diff_options *diffopt) · line 255

Quoted 10 lines

graph.c +10 -0

+                     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 step by 16, over a set of 215 colors. That seems to give only 13 colors, versus the original 16. :)

I know that is a simplification. If you wrap around, then you get your 13 colors, and then another 13 colors that aren't _quite_ the same, and so on, until you've used all 256. I'm just not sure if the 1st and 14th color would be visually different enough for it to matter (I admit I didn't do any experiments, though).

Quoted 5 lines

---graph::
+--graph[=<options>]::
Draw a text-based graphical representation of the commit history on the left hand side of the output. This may cause extra lines to be printed in between commits, in order for the graph history

I wonder if we would ever want another use for "--graph=foo"

I guess any such thing could fall under the name of "graph options", and we'd end up with "--graph=256colors,unicode" or something like that.

I do suspect people would want a config option for this, though. I.e., you'd want to enable it all the time if you have a terminal which can handle 256 colors, not just for a particular invocation.

.... But I think I could approach it a different way:
collect colors that have names. That reduces the number of colors so we can go back to "step 1 at a time" and still don't run into two similar colors often.

If I were doing this, I'd just prepare a table with 32 color slots or so [*1*], start at a random spot (say 017:00005f) of

https://en.wikipedia.org/wiki/File:Xterm_256color_chart.svg,

and pick spots by jumping southeast like a chess knight (i.e. 017->030->043->086->...) until the table is filled, wrapping around at the edge of that color chart as necessary.

graph.c +6 -1

index d4e8519..75375a1 100644
@@ -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 =

graph.c +7 -0

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

Quoted 32 lines · Nguyễn Thái Ngọc Duy

graph.c +13 -1

index d4e8519..75375a1 100644
@@ -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);

graph.c void graph_setup_line_prefix(struct diff_options *diffopt) · line 255

Quoted 3 lines

graph.c +3 -0

+             /* 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?

Quoted 12 lines

graph.c +7 -0

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

graph.c static unsigned short graph_get_current_column_color(const struct git_graph *gra · line 416

Quoted 10 lines

graph.c +7 -0

+             }
+             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?

Sounds like the good first step should be something like this instead of jumping straight to generating a new color palette automatically.

graph.c +5 -0

+		return;
+	}
+
+	start = string;
+	end = string + strlen(string);

graph.c +8 -0

+		start = comma + 1;
+	}
+	free(string);
+	ALLOC_GROW(colors, colors_max + 1, colors_alloc);
+	colors[colors_max] = xstrdup(GIT_COLOR_RESET);
+	graph_set_column_colors((const char **)colors, colors_max);
+}
+
 void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
 	column_colors = colors;

graph.c enum graph_state { · line 77

Quoted 12 lines

graph.c +12 -0

+{
+	static char **colors;
+	static int colors_max, colors_alloc;
+	char *string = NULL;
+	const char *end, *start;
+	int i;
+
+	for (i = 0; i < colors_max; i++)
+		free(colors[i]);
+	if (colors)
+		free(colors[colors_max]);
+	colors_max = 0;

The correctness of the first loop relies on the fact that colors is non-null when colors_max is not zero, and then the freeing of the colors relies on something else. It is not wrong per-se, but it will reduce the "Huh?" factor if you wrote it like so:

	if (colors) {
        	/* 
                 * Reinitialize, but keep the colors[] array.
		 * Note that the last entry is allocated for
		 * reset but colors_max does not count it, hence
		 * "i <= colors_max", not "i < colors_max".
		 */
		int i;
		for (i = 0; i <= colors_max; i++)
			free(colors[i]);
		colors_max = 0;
	}

I also wonder if it is worth just using argv_array. We do not care about NULL-terminating the list here, but it actually works pretty well as a generic string-array class (and keeping a NULL at the end of any array-of-pointers is a reasonable defensive measure). Then the function becomes:

  argv_array_clear(&colors);
  ...
  if (!color_parse_mem(..., color))
          argv_array_push(&colors, color);
  ...
  argv_array_push(&colors, GIT_COLOR_RESET);
  /* graph_set_column_colors takes a max-index, not a count */
  graph_set_column_colors(colors.argv, colors.argc - 1);

It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.

Quoted 3 lines · Jeff King

I also wonder if it is worth just using argv_array. We do not care about
...
It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.

Indeed. My only complaint is it's "argv_array_" and not "string_array_" but we could think about renaming it later when we see another use like this.

If you have a 256 colors terminal (or one with true color support), then the predefined 12 colors seem limited. On the other hand, you don't want to draw graph lines with every single color in this mode because the two colors could look extremely similar. This option allows you to hand pick the colors you want.

Even with standard terminal, if your background color is neither black or white, then the graph line may match your background and become hidden. You can exclude your background color (or simply the colors you hate) with this.

* I'm not going with the cumulative behavior because I think that's just harder to manage colors, and we would need a way to remove colors from the config too.

t/t4202-log.sh test_expect_success 'log --graph with merge' ' · line 332

Quoted 2 lines

t/t4202-log.sh +2 -0

+test_expect_success 'log --graph with merge with log.graphColors' '
+     test_config log.graphColors " blue , cyan , red " &&

This funny syntax isn't required, right? It should work with the more natural:

test_config log.graphColors "blue, cyan, red"

graph.c +12 -0

+			argv_array_push(&colors, color);
+		else
+			warning(_("ignore invalid color '%.*s' in log.graphColors"),
+				(int)(comma - start), start);
+		start = comma + 1;
+	}
+	free(string);
+	argv_array_push(&colors, GIT_COLOR_RESET);
+	/* graph_set_column_colors takes a max-index, not a count */
+	graph_set_column_colors(colors.argv, colors.argc - 1);
+}
+
 void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
 	column_colors = colors;

graph.c enum graph_state { · line 92

Quoted 13 lines · Nguyễn Thái Ngọc Duy

graph.c +13 -0

+	end = string + strlen(string);
+	while (start < end) {
+		const char *comma = strchrnul(start, ',');
+		char color[COLOR_MAXLEN];
+
+		while (start < comma && isspace(*start))
+			start++;
+		if (start == comma) {
+			start = comma + 1;
+			continue;
+		}
+
+		if (!color_parse_mem(start, comma - start, color))

So you skip the leading blanks but let color_parse_mem() trim the trailing blanks? It would work once the control reaches the loop, but wouldn't that miss

	git -c log.graphColors=' reset , blue, red' log --graph

as "reset" is not

In this code we want to match the word "reset". If len is zero, strncasecmp() will return zero and we incorrectly assume it's "reset" as a result.

Quoted 3 lines · Nguyễn Thái Ngọc Duy

In this code we want to match the word "reset". If len is zero, strncasecmp() will return zero and we incorrectly assume it's "reset" as a result.

This is probably a good idea. This _is_ user-visible, so it's possible somebody was using empty config as a synonym for "reset". But since it was never documented, I feel like relying on that is somewhat crazy.

Normally color_parse_mem() is called from config parser which trims the leading spaces already. The new caller in the next patch won't. Let's be tidy and trim leading spaces too (we already trim trailing spaces before comma).

Quoted 4 lines · Nguyễn Thái Ngọc Duy

Normally color_parse_mem() is called from config parser which trims the leading spaces already. The new caller in the next patch won't. Let's be tidy and trim leading spaces too (we already trim trailing spaces before comma).

What comma? I don't think that exists until the next patch. :)

I think just trimming from the front is OK, though, because color_parse_mem() trims trailing whitespace after a word. So either you have a word and we will trim after it, or you do not (in which case this will trim everything and hit the !len case you added).

So maybe a better commit message is just:

Normally color_parse_mem() is called from config parser which trims the leading spaces already. The new caller in the next patch won't. Let's be tidy and trim leading spaces too (we already trim trailing spaces after a word).

-Peff

graph.c +9 -0

+static void parse_graph_colors_config(struct argv_array *colors, const char *string)
+{
+	const char *end, *start;
+
+	start = string;
+	end = string + strlen(string);
+	while (start < end) {
+		const char *comma = strchrnul(start, ',');
+		char color[COLOR_MAXLEN];

Quoted 9 lines · Nguyễn Thái Ngọc Duy

graph.c +9 -0

+static void parse_graph_colors_config(struct argv_array *colors, const char *string)
+{
+	const char *end, *start;
+
+	start = string;
+	end = string + strlen(string);
+	while (start < end) {
+		const char *comma = strchrnul(start, ',');
+		char color[COLOR_MAXLEN];

graph.c enum graph_state { · line 84

Quoted 10 lines

graph.c +10 -0

+
+		if (!color_parse_mem(start, comma - start, color))
+			argv_array_push(colors, color);
+		else
+			warning(_("ignore invalid color '%.*s' in log.graphColors"),
+				(int)(comma - start), start);
+		start = comma + 1;
+	}
+	argv_array_push(colors, GIT_COLOR_RESET);
+}

This looks good.

graph.c struct git_graph *graph_init(struct rev_info *opt) · line 235

Quoted 12 lines

graph.c +5 -3

@@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)
 {
 	struct git_graph *graph = xmalloc(sizeof(struct git_graph));
 
-	if (!column_colors)
-		graph_set_column_colors(column_colors_ansi,
-					column_colors_ansi_max);
+	if (!column_colors) {
+		struct argv_array ansi_colors = {
+			column_colors_ansi,
+			column_colors_ansi_max + 1
+		};

Hrm. At first I thought this would cause memory corrution, because your argv_array_clear() would try to free() the non-heap array you've stuffed inside. But you only clear the custom_colors array which actually is dynamically allocated. This outer one is just here to give uniform access:

graph.c struct git_graph *graph_init(struct rev_info *opt) · line 248

Quoted 13 lines

graph.c +13 -0

+		struct argv_array *colors = &ansi_colors;
+		char *string;
+
+		if (!git_config_get_string("log.graphcolors", &string)) {
+			static struct argv_array custom_colors = ARGV_ARRAY_INIT;
+			argv_array_clear(&custom_colors);
+			parse_graph_colors_config(&custom_colors, string);
+			free(string);
+			colors = &custom_colors;
+		}
+		/* graph_set_column_colors takes a max-index, not a count */
+		graph_set_column_colors(colors->argv, colors->argc - 1);
+	}

Since there's only one line that cares about the result of "colors", maybe it would be less confusing to do:

  if (!git_config_get-string("log.graphcolors", &string)) {
	... parse, etc ...
	graph_set_column_colors(colors.argv, colors.argc - 1);
  } else {
	graph_set_column_colors(column_colors_ansi,
	                        column_colors_ansi_max);

}

Quoted 10 lines

Since there's only one line that cares about the result of "colors", maybe it would be less confusing to do:

  if (!git_config_get-string("log.graphcolors", &string)) {
	... parse, etc ...
	graph_set_column_colors(colors.argv, colors.argc - 1);
  } else {
	graph_set_column_colors(column_colors_ansi,
	                        column_colors_ansi_max);

}

Yes, that would be much much less confusing. By doing so, the cover letter can lose "pushing the limits of abuse", too ;-).