Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors

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

Jeff King <peff@peff.net> writes:

> On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> +static void parse_graph_colors_config(struct argv_array *colors, const char *string)
>> +{
>> +	const char *end, *start;
>> +
>> +	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))
>> +			argv_array_push(colors, color);
>> +		else
>> +			warning(_("ignore invalid color '%.*s' in log.graphColors"),
>> +				(int)(comma - start), start);
>> +		start = comma + 1;
>> +	}
>> +	argv_array_push(colors, GIT_COLOR_RESET);
>> +}
>
> This looks good.
>
>> @@ -207,9 +228,24 @@ 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);
>> +	if (!column_colors) {
>> +		struct argv_array ansi_colors = {
>> +			column_colors_ansi,
>> +			column_colors_ansi_max + 1
>> +		};
>
> Hrm. At first I thought this would cause memory corrution, because your
> argv_array_clear() would try to free() the non-heap array you've stuffed
> inside. But you only clear the custom_colors array which actually is
> dynamically allocated. This outer one is just here to give uniform
> access:
>
>> +		struct argv_array *colors = &ansi_colors;
>> +		char *string;
>> +
>> +		if (!git_config_get_string("log.graphcolors", &string)) {
>> +			static struct argv_array custom_colors = ARGV_ARRAY_INIT;
>> +			argv_array_clear(&custom_colors);
>> +			parse_graph_colors_config(&custom_colors, string);
>> +			free(string);
>> +			colors = &custom_colors;
>> +		}
>> +		/* graph_set_column_colors takes a max-index, not a count */
>> +		graph_set_column_colors(colors->argv, colors->argc - 1);
>> +	}
>
> Since there's only one line that cares about the result of "colors",
> maybe it would be less confusing to do:
>
>   if (!git_config_get-string("log.graphcolors", &string)) {
> 	... parse, etc ...
> 	graph_set_column_colors(colors.argv, colors.argc - 1);
>   } else {
> 	graph_set_column_colors(column_colors_ansi,
> 	                        column_colors_ansi_max);
>   }

Yes, that would be much much less confusing.  By doing so, the cover
letter can lose "pushing the limits of abuse", too ;-).