Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> 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.