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

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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.

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

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

> On Tue, Dec 20, 2016 at 07:39:29PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  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:
>
>> +     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 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).

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

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

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

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

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

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

> diff --git a/graph.c b/graph.c
> index d4e8519..9c58fd1 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -79,6 +79,39 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)
>  static const char **column_colors;
>  static unsigned short column_colors_max;
>  
> +static void set_column_colors_by_config(void)
> +{
> +	static char **colors;
> +	static int colors_max, colors_alloc;

> +	char *string = NULL;
> +	const char *end, *start;
> +
> +	if (git_config_get_string("log.graphcolors", &string)) {
> +		graph_set_column_colors(column_colors_ansi,
> +					column_colors_ansi_max);

> +		return;
> +	}
> +
> +	start = string;
> +	end = string + strlen(string);
> +	while (start < end) {
> +		const char *comma = strchrnul(start, ',');
> +		char color[COLOR_MAXLEN];
> +
> +		if (!color_parse_mem(start, comma - start, color)) {
> +			ALLOC_GROW(colors, colors_max + 1, colors_alloc);
> +			colors[colors_max++] = xstrdup(color);
> +		} else
> +			warning(_("ignore invalid color '%.*s'"),
> +				(int)(comma - start), start);

> +		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;
> @@ -239,8 +272,7 @@ 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);
> +		set_column_colors_by_config();

> diff --git a/graph.c b/graph.c
> index dd17201..048f5cb 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -62,6 +62,49 @@ enum graph_state {
>  static const char **column_colors;
>  static unsigned short column_colors_max;
>  
> +static void set_column_colors(void)

> +{
> +	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;

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

> On Mon, Jan 9, 2017 at 12:30 PM, Jeff King <peff@peff.net> wrote:
>> 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.

>> +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"

> +	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))

> +			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;
> @@ -208,8 +248,7 @@ 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);
> +		read_graph_colors_config();

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +	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

> On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 
> > 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.

> On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> 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

> +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];
> +
> +		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);
> +}

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

> +		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);
> +	}

> On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> +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];
>> +
>> +		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.
>
>> @@ -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:
>
>> +		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);
>   }

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