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

[Junio C Hamano <gitster@pobox.com>]

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.