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

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

[Nguyễn Thái Ngọc Duy]

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.

 BTW anyone interested in bringing this type [1] of --graph to git? I
 tried the unicode box characters, but the vertical lines do not join
 with diagonal ones, making the graph a bit ugly (even though it's
 still better than ascii version)

 [1] https://github.com/magit/magit/issues/495#issuecomment-17480757

 Documentation/rev-list-options.txt |  5 ++++-
 graph.c                            | 31 ++++++++++++++++++++++++++++++-
 graph.h                            |  8 +++++++-
 revision.c                         |  4 ++++
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 5da7cf5..0a0c2f3 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -825,7 +825,7 @@ you would get an output like this:
 	-xxxxxxx... 1st on a
 -----------------------------------------------------------------------
 
---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
@@ -836,6 +836,9 @@ This enables parent rewriting, see 'History Simplification' below.
 +
 This implies the `--topo-order` option by default, but the
 `--date-order` option may also be specified.
++
+The only supported option is `256colors` which allows more than 16
+colors for drawing the commit history.
 
 --show-linear-break[=<barrier>]::
 	When --graph is not used, all history branches are flattened
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;
+	}
 	if (!column_colors)
 		graph_set_column_colors(column_colors_ansi,
 					column_colors_ansi_max);
@@ -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;
 }
diff --git a/graph.h b/graph.h
index af62339..8069aa4 100644
--- a/graph.h
+++ b/graph.h
@@ -40,7 +40,13 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);
 /*
  * Create a new struct git_graph.
  */
-struct git_graph *graph_init(struct rev_info *opt);
+struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg);
+
+static inline struct git_graph *graph_init(struct rev_info *opt)
+{
+	return graph_init_with_options(opt, NULL);
+}
+
 
 /*
  * Update a git_graph with a new commit.
diff --git a/revision.c b/revision.c
index b37dbec..07bea54 100644
--- a/revision.c
+++ b/revision.c
@@ -1933,6 +1933,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->topo_order = 1;
 		revs->rewrite_parents = 1;
 		revs->graph = graph_init(revs);
+	} else if (skip_prefix(arg, "--graph=", &arg)) {
+		revs->topo_order = 1;
+		revs->rewrite_parents = 1;
+		revs->graph = graph_init_with_options(revs, arg);
 	} else if (!strcmp(arg, "--root")) {
 		revs->show_root_diff = 1;
 	} else if (!strcmp(arg, "--no-commit-id")) {
-- 
2.8.2.524.g6ff3d78

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

[Jeff King]

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

-Peff

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

[Duy Nguyen]

On Tue, Dec 20, 2016 at 11:57 PM, Jeff King <peff@peff.net> wrote:
> 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).

Yep. If the jump sequence is a random one, we're less likely to run
into this. But I think Junio's "run git-log in 2 terminals with the
same coloring" convinces me randomization here is not the best thing.

The best solution would be select colors per text line, so we can pick
different colors. But I think that's a lot of computation (and
probably an NP problem too). The second best option is have a good,
predefined color palette. We don't need a red of all shades, we need
something that look distinct enough from the rest. I googled for this
first and failed. 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.

>> ---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 do. See the screenshot in [1] from the original mail. I have to
stare at --graph so often lately that it might get my attention before
other things.

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

Exactly.

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

Yeah. That also means we need the ability to override/negate config
options, perhaps something like --graph=-256colors.
-- 
Duy

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

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

I suspect that there is a "cultural" bias that makes the idea
unworkable.  I haven't found a definitive source, but I think there
are a lot more hues and shades of red that have names than hues and
shades of blue, for example.

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.


[Footnote]

*1* ...because I do not think the thin graph lines painted in too
many colours on the screen would be easily distinguishable from each
other anyway.

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

[Duy Nguyen]

On Fri, Dec 23, 2016 at 2:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

If you want to play with that, [1] may help, which sorts colors in hsv
space, and you can select a few colors to see how they look, either
manually or by calculation. Yeah we probably get maybe 32 (or 48 if
you stretch it a bit) distinct colors. Not sure if it's any better
with true color terminals, probably not.

[1] https://gist.github.com/pclouds/616b67f0b5a9b20d74373286574cd0ac
-- 
Duy

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

[Junio C Hamano]

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?  

 - Even if you needed rand(), you should be able to factor out
   computation of V so that the function does not need an early
   return that hints totally different processing for two codepaths.

        static void graph_increment_column_color(struct git_graph *g)
        {
                int next;

                if (! 256-color in use) {
                        next = 1;
                } else {
                        do whatever to compute your v;
                        next = v + column_colors_step;
                }
                g->default_column_color =
                (g->default_column_color + v) / column_colors_max;
        }

   Or something like that, perhaps?

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

[Duy Nguyen]

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

[PATCH v2] log --graph: customize the graph lines with config log.graphColors

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Sounds like the good first step should be something like this instead
 of jumping straight to generating a new color palette automatically.

 It's not hard to create a script that generate this config value
 based on some jump calculation, if you don't want to manually picking
 colors.

 Documentation/config.txt |  4 ++++
 graph.c                  | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index d51182a..4f26c2a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2033,6 +2033,10 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graphColors::
+	A list of colors, separated by commas, that can be used to draw
+	history lines in `git log --graph`.
+
 log.showRoot::
 	If true, the initial commit will be shown as a big creation event.
 	This is equivalent to a diff against an empty tree.
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();
 
 	graph->commit = NULL;
 	graph->revs = opt;
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v2] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

I like this not merely as a good first step but potentially a good
endgame.

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

I doubt you want these to be static (and allow multiple instances of
the same configuration variable to accumulate).  As you defined the
value to be comma-separated colors, users would want the usual "last
one wins" rule to override previous settings, no?

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

On the other hand, if you do want cumulative semantics, then you'd
need to free the previous set you held in the statics here, I would
think.

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

"ignore invalid color"?  Who is giving the command to ignore to whom?

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

"by" in the function name somehow sounds funny, at least to me.

[PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Compared to v2:

 * set_column_colors_by_config() is renamed to set_column_colors()
 * no memory leak if the function is called the second time
 * proper space trimming since color_parse_mem expects so
 * fixed the warning message, giving some context
 * at least one test to exercise the code
 * 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.

 Documentation/config.txt |  4 ++++
 graph.c                  | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 t/t4202-log.sh           | 22 ++++++++++++++++++++++
 3 files changed, 70 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..33a007b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2003,6 +2003,10 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graphColors::
+	A list of colors, separated by commas, that can be used to draw
+	history lines in `git log --graph`.
+
 log.showRoot::
 	If true, the initial commit will be shown as a big creation event.
 	This is equivalent to a diff against an empty tree.
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;
+
+	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];
+
+		while (start < comma && isspace(*start))
+			start++;
+
+		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' in log.graphColors"),
+				(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;
@@ -208,8 +251,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();
 
 	graph->commit = NULL;
 	graph->revs = opt;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e2db47c..afe0715 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+cat > expect.colors <<\EOF
+*   Merge branch 'side'
+<BLUE>|<RESET><CYAN>\<RESET>
+<BLUE>|<RESET> * side-2
+<BLUE>|<RESET> * side-1
+* <CYAN>|<RESET> Second
+* <CYAN>|<RESET> sixth
+* <CYAN>|<RESET> fifth
+* <CYAN>|<RESET> fourth
+<CYAN>|<RESET><CYAN>/<RESET>
+* third
+* second
+* initial
+EOF
+
+test_expect_success 'log --graph with merge with log.graphColors' '
+	test_config log.graphColors " blue , cyan , red " &&
+	git log --color=always --graph --date-order --pretty=tformat:%s |
+		test_decode_color | sed "s/ *\$//" >actual &&
+	test_cmp expect.colors actual
+'
+
 test_expect_success 'log --raw --graph -m with merge' '
 	git log --raw --graph --oneline -m master | head -n 500 >actual &&
 	grep "initial" actual
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

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

When I said "'by config' sounds funny", I meant "'from config' may
be more natural".  Perhaps name this read_graph_colors_config(), as
that (i.e. reading "log.graphColors") is what it does.

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

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Jeff King]

On Sun, Jan 08, 2017 at 07:05:12PM -0800, Junio C Hamano wrote:

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

Yeah, I agree that what you've written here is less confusing. Less
confusing still would be to keep colors_nr, and deal separately with the
"max" interface to graph_set_column_colors().

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.

-Peff

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Duy Nguyen]

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

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

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

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

Yup, when Peff said "argv-array", it took me a few breaths, until I
realized that argv-array was merely an array of strings, to convince
myself that the data structure can be used here.  If we are going to
use it in more places, we may want to rename it somehow.

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Jeff King]

On Sun, Jan 08, 2017 at 05:13:33PM +0700, Nguyễn Thái Ngọc Duy wrote:

> 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 like this approach much more than the 256-color option.

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

Yeah, figuring out the list semantics would be a pain. This makes it
hard to exclude a single color, but I think it's more likely somebody
would want to replace the whole set with something that works well
against their background.

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

-Peff

Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors

[Duy Nguyen]

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

Yeah spaces are optional. I just wanted to put extra spaces there
because I hit a bug with them.
-- 
Duy

[PATCH v4] log --graph: customize the graph lines with config log.graphColors

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v4:

 * rename the function again (and it reads better)
 * use argv_array
 * fix a bug with two consecutive commas (after spaces are trimmed)

 Documentation/config.txt |  4 ++++
 graph.c                  | 43 +++++++++++++++++++++++++++++++++++++++++--
 t/t4202-log.sh           | 22 ++++++++++++++++++++++
 3 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..33a007b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2003,6 +2003,10 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graphColors::
+	A list of colors, separated by commas, that can be used to draw
+	history lines in `git log --graph`.
+
 log.showRoot::
 	If true, the initial commit will be shown as a big creation event.
 	This is equivalent to a diff against an empty tree.
diff --git a/graph.c b/graph.c
index dd17201..83b1d42 100644
--- a/graph.c
+++ b/graph.c
@@ -4,6 +4,7 @@
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"
+#include "argv-array.h"
 
 /* Internal API */
 
@@ -62,6 +63,45 @@ enum graph_state {
 static const char **column_colors;
 static unsigned short column_colors_max;
 
+static void read_graph_colors_config(void)
+{
+	static struct argv_array colors = ARGV_ARRAY_INIT;
+	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;
+	}
+
+	argv_array_clear(&colors);
+	start = string;
+	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();
 
 	graph->commit = NULL;
 	graph->revs = opt;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e2db47c..0aeabed 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+cat > expect.colors <<\EOF
+*   Merge branch 'side'
+<BLUE>|<RESET><CYAN>\<RESET>
+<BLUE>|<RESET> * side-2
+<BLUE>|<RESET> * side-1
+* <CYAN>|<RESET> Second
+* <CYAN>|<RESET> sixth
+* <CYAN>|<RESET> fifth
+* <CYAN>|<RESET> fourth
+<CYAN>|<RESET><CYAN>/<RESET>
+* third
+* second
+* initial
+EOF
+
+test_expect_success 'log --graph with merge with log.graphColors' '
+	test_config log.graphColors ",, blue,invalid-color, cyan, red  , " &&
+	git log --color=always --graph --date-order --pretty=tformat:%s |
+		test_decode_color | sed "s/ *\$//" >actual &&
+	test_cmp expect.colors actual
+'
+
 test_expect_success 'log --raw --graph -m with merge' '
 	git log --raw --graph --oneline -m master | head -n 500 >actual &&
 	grep "initial" actual
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v4] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

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 understood by parse_color() and is understood by
color_parse_mem() only when the length is given correctly?

I am undecided, but leaning towards declaring that it is a bug in
color_parse_mem() not in this caller.

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

Now that the helper is renamed to be about "reading the
configuration to figure out the graph colors", the division of labor
between the caller and the callee we see here is suboptimal for
readability, I would think.   

We would want to see the caller to either be

	if (!column_colors) {
		if (read_graph_colors_config())
			; /* ok */
		else                        
			graph_set_column_colors(ansi, ansi_max);
	}

or better yet, something like:

	if (!column_colors) {
		const char **colors = column_colors_ansi;
		int colors_max = column_colors_ansi_max;

		read_graph_colors_config(&colors, &colors_max);
		graph_set_collumn_colors(colors, colors_max);
	}

The last one would make it clear that by default we use ansi but
override that default by calling that function whose purpose is to
read the values that override the default from the configuration.

I haven't thought things thru regarding memory leakages etc., so
there may be valid reasons why the last one is infeasible.

Re: [PATCH v4] log --graph: customize the graph lines with config log.graphColors

[Duy Nguyen]

Just FYI. The broken internet cables in Vietnam seem to hit my ISP
really hard. It's nearly impossible to make a TCP connection. So I'm
basically off the grid, hopefully not longer than two weeks.

On 1/10/17, Junio C Hamano <gitster@pobox.com> wrote:
> 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

[PATCH v5 0/3] nd/log-graph-configurable-colors

[Nguyễn Thái Ngọc Duy]

v5 moves space trimming to color_parse_mem() from read_graph_colors_config,
which is renamed to parse_graph... because the config reading is moved
back to graph_init.

I think it looks better, but we may be pushing the limits of
argv_array's abuse.

Nguyễn Thái Ngọc Duy (3):
  color.c: fix color_parse_mem() with value_len == 0
  color.c: trim leading spaces in color_parse_mem()
  log --graph: customize the graph lines with config log.graphColors

 Documentation/config.txt |  4 ++++
 color.c                  | 10 +++++++++-
 graph.c                  | 42 +++++++++++++++++++++++++++++++++++++++---
 t/t4202-log.sh           | 22 ++++++++++++++++++++++
 4 files changed, 74 insertions(+), 4 deletions(-)

-- 
2.8.2.524.g6ff3d78

[PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 color.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/color.c b/color.c
index 81c2676..a9eadd1 100644
--- a/color.c
+++ b/color.c
@@ -207,6 +207,9 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 	struct color fg = { COLOR_UNSPECIFIED };
 	struct color bg = { COLOR_UNSPECIFIED };
 
+	if (!len)
+		return -1;
+
 	if (!strncasecmp(value, "reset", len)) {
 		xsnprintf(dst, end - dst, GIT_COLOR_RESET);
 		return 0;
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

[Jeff King]

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.

-Peff

Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

[Jeff King]

On Thu, Jan 19, 2017 at 11:38:41AM -0500, Jeff King wrote:

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

Hrm. This seems to break the add--interactive script if you do not have
color.diff.plain set:

  $ GIT_TRACE=1 git add -p
  ...
  22:58:12.568990 [pid=11401] git.c:387  trace: built-in: git 'config' '--get-color' 'color.diff.plain' ''
  fatal: unable to parse default color value
  config --get-color color.diff.plain : command returned error: 128

As you can see, the default value the empty string, which is now an
error.

The default in the C code for that value is GIT_COLOR_NORMAL, which
really is the empty string. So I think the old code was buggy to choose
"reset", but the new one is worse because it fails entirely. :)

We probably want something like this instead:

diff --git a/color.c b/color.c
index 190b2da96..dee61557e 100644
--- a/color.c
+++ b/color.c
@@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 		len--;
 	}
 
-	if (!len)
-		return -1;
+	if (!len) {
+		dst[0] = '\0';
+		return 0;
+	}
 
 	if (!strncasecmp(ptr, "reset", len)) {
 		xsnprintf(dst, end - dst, GIT_COLOR_RESET);

The breakage is in 'next' (it looks like it went out a few days ago; I'm
surprised I didn't notice until now).

-Peff

[PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 color.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/color.c b/color.c
index a9eadd1..7bb4a96 100644
--- a/color.c
+++ b/color.c
@@ -207,10 +207,15 @@ int color_parse_mem(const char *value, int value_len, char *dst)
 	struct color fg = { COLOR_UNSPECIFIED };
 	struct color bg = { COLOR_UNSPECIFIED };
 
+	while (len > 0 && isspace(*ptr)) {
+		ptr++;
+		len--;
+	}
+
 	if (!len)
 		return -1;
 
-	if (!strncasecmp(value, "reset", len)) {
+	if (!strncasecmp(ptr, "reset", len)) {
 		xsnprintf(dst, end - dst, GIT_COLOR_RESET);
 		return 0;
 	}
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

[Jeff King]

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

Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Yeah, my reading stuttered at the same place in the original, and
your rewrite looks a lot more sensible.

[PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/config.txt |  4 ++++
 graph.c                  | 42 +++++++++++++++++++++++++++++++++++++++---
 t/t4202-log.sh           | 22 ++++++++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..33a007b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2003,6 +2003,10 @@ log.follow::
 	i.e. it cannot be used to follow multiple files and does not work well
 	on non-linear history.
 
+log.graphColors::
+	A list of colors, separated by commas, that can be used to draw
+	history lines in `git log --graph`.
+
 log.showRoot::
 	If true, the initial commit will be shown as a big creation event.
 	This is equivalent to a diff against an empty tree.
diff --git a/graph.c b/graph.c
index dd17201..822d40a 100644
--- a/graph.c
+++ b/graph.c
@@ -4,6 +4,7 @@
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"
+#include "argv-array.h"
 
 /* Internal API */
 
@@ -62,6 +63,26 @@ enum graph_state {
 static const char **column_colors;
 static unsigned short column_colors_max;
 
+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);
+}
+
 void graph_set_column_colors(const char **colors, unsigned short colors_max)
 {
 	column_colors = colors;
@@ -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);
+	}
 
 	graph->commit = NULL;
 	graph->revs = opt;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index e2db47c..0aeabed 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '
 	test_cmp expect actual
 '
 
+cat > expect.colors <<\EOF
+*   Merge branch 'side'
+<BLUE>|<RESET><CYAN>\<RESET>
+<BLUE>|<RESET> * side-2
+<BLUE>|<RESET> * side-1
+* <CYAN>|<RESET> Second
+* <CYAN>|<RESET> sixth
+* <CYAN>|<RESET> fifth
+* <CYAN>|<RESET> fourth
+<CYAN>|<RESET><CYAN>/<RESET>
+* third
+* second
+* initial
+EOF
+
+test_expect_success 'log --graph with merge with log.graphColors' '
+	test_config log.graphColors ",, blue,invalid-color, cyan, red  , " &&
+	git log --color=always --graph --date-order --pretty=tformat:%s |
+		test_decode_color | sed "s/ *\$//" >actual &&
+	test_cmp expect.colors actual
+'
+
 test_expect_success 'log --raw --graph -m with merge' '
 	git log --raw --graph --oneline -m master | head -n 500 >actual &&
 	grep "initial" actual
-- 
2.8.2.524.g6ff3d78

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Jeff King]

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

-Peff

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

Will queue with a squashable change.

Thanks.

 graph.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/graph.c b/graph.c
index 822d40ae20..00aeee36d8 100644
--- a/graph.c
+++ b/graph.c
@@ -229,22 +229,20 @@ struct git_graph *graph_init(struct rev_info *opt)
 	struct git_graph *graph = xmalloc(sizeof(struct git_graph));
 
 	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)) {
+		if (git_config_get_string("log.graphcolors", &string)) {
+			/* not configured -- use default */
+			graph_set_column_colors(column_colors_ansi,
+						column_colors_ansi_max);
+		} else {
 			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(custom_colors.argv,
+						custom_colors.argc - 1);
 		}
-		/* graph_set_column_colors takes a max-index, not a count */
-		graph_set_column_colors(colors->argv, colors->argc - 1);
 	}
 
 	graph->commit = NULL;