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

From: Jeff King <peff@peff.net>
Date: Jan 9, 2017, 05:30 UTC
Message-ID: <20170109053047.sn75d6ynipgf4nur@sigill.intra.peff.net>
In-Reply-To: <xmqq37gtyluf.fsf@gitster.mtv.corp.google.com>

On Sun, Jan 08, 2017 at 07:05:12PM -0800, Junio C Hamano wrote:

Show 30 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;
> 	}

Yeah, I agree that what you've written here is less confusing. Less confusing still would be to keep colors_nr, and deal separately with the "max" interface to graph_set_column_colors().

I also wonder if it is worth just using argv_array. We do not care about NULL-terminating the list here, but it actually works pretty well as a generic string-array class (and keeping a NULL at the end of any array-of-pointers is a reasonable defensive measure). Then the function becomes:

  argv_array_clear(&colors);
  ...
  if (!color_parse_mem(..., color))
          argv_array_push(&colors, color);
  ...
  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);

It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.

-Peff