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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 9, 2017, 03:05 UTC
Message-ID: <xmqq37gtyluf.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170108101333.26221-1-pclouds@gmail.com>

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

Show 9 quoted lines

> diff --git a/graph.c b/graph.c
> index dd17201..048f5cb 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -62,6 +62,49 @@ enum graph_state {
>  static const char **column_colors;
>  static unsigned short column_colors_max;
>  
> +static void set_column_colors(void)

When I said "'by config' sounds funny", I meant "'from config' may be more natural". Perhaps name this read_graph_colors_config(), as that (i.e. reading "log.graphColors") is what it does.

Show 12 quoted lines

> +{
> +	static char **colors;
> +	static int colors_max, colors_alloc;
> +	char *string = NULL;
> +	const char *end, *start;
> +	int i;
> +
> +	for (i = 0; i < colors_max; i++)
> +		free(colors[i]);
> +	if (colors)
> +		free(colors[colors_max]);
> +	colors_max = 0;

The correctness of the first loop relies on the fact that colors is non-null when colors_max is not zero, and then the freeing of the colors relies on something else. It is not wrong per-se, but it will reduce the "Huh?" factor if you wrote it like so:

	if (colors) {
        	/* 
                 * Reinitialize, but keep the colors[] array.
		 * Note that the last entry is allocated for
		 * reset but colors_max does not count it, hence
		 * "i <= colors_max", not "i < colors_max".
		 */
		int i;
		for (i = 0; i <= colors_max; i++)
			free(colors[i]);
		colors_max = 0;
	}