Re: [PATCH v4] 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:

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