Re: [PATCH v2] log --graph: customize the graph lines with config log.graphColors
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 25, 2016, 23:02 UTC
- Message-ID
- <xmqqzijjd34j.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161224113817.18407-1-pclouds@gmail.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.
Show 12 quoted lines
> 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?
Show 6 quoted lines
> + 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.
Show 15 quoted lines
> + 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?
Show 18 quoted lines
> + 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.