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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2017, 17:29 UTC
Message-ID: <xmqqh958uoot.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170109103258.25341-1-pclouds@gmail.com>

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

Show 13 quoted lines

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

Show 22 quoted lines

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