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

[Junio C Hamano]

Duy Nguyen  writes:

> On Mon, Jan 9, 2017 at 12:30 PM, Jeff King  wrote:
>> I also wonder if it is worth just using argv_array. We do not care about
>> ...
>> It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.
>
> Indeed. My only complaint is it's "argv_array_" and not
> "string_array_" but we could think about renaming it later when we see
> another use like this.

Yup, when Peff said "argv-array", it took me a few breaths, until I
realized that argv-array was merely an array of strings, to convince
myself that the data structure can be used here.  If we are going to
use it in more places, we may want to rename it somehow.

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

[Duy Nguyen]

On Mon, Jan 9, 2017 at 12:30 PM, Jeff King  wrote:
> 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();
>   ...
>   if (!color_parse_mem(..., color))
>   argv_array_push(, color);
>   ...
>   argv_array_push(, 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.

Indeed. My only complaint is it's "argv_array_" and not
"string_array_" but we could think about renaming it later when we see
another use like this.
-- 
Duy

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

[Duy Nguyen]

On Mon, Jan 9, 2017 at 12:34 PM, Jeff King  wrote:
>> +test_expect_success 'log --graph with merge with log.graphColors' '
>> + test_config log.graphColors " blue , cyan , red " &&
>
> This funny syntax isn't required, right? It should work with the more
> natural:
>
> test_config log.graphColors "blue, cyan, red"

Yeah spaces are optional. I just wanted to put extra spaces there
because I hit a bug with them.
-- 
Duy

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

[Jeff King]

On Sun, Jan 08, 2017 at 05:13:33PM +0700, Nguyễn Thái Ngọc Duy wrote:

> If you have a 256 colors terminal (or one with true color support), then
> the predefined 12 colors seem limited. On the other hand, you don't want
> to draw graph lines with every single color in this mode because the two
> colors could look extremely similar. This option allows you to hand pick
> the colors you want.
> 
> Even with standard terminal, if your background color is neither black
> or white, then the graph line may match your background and become
> hidden. You can exclude your background color (or simply the colors you
> hate) with this.

I like this approach much more than the 256-color option.

>  * I'm not going with the cumulative behavior because I think that's
>just harder to manage colors, and we would need a way to remove
>colors from the config too.

Yeah, figuring out the list semantics would be a pain. This makes it
hard to exclude a single color, but I think it's more likely somebody
would want to replace the whole set with something that works well
against their background.

> +test_expect_success 'log --graph with merge with log.graphColors' '
> + test_config log.graphColors " blue , cyan , red " &&

This funny syntax isn't required, right? It should work with the more
natural:

test_config log.graphColors "blue, cyan, red"

-Peff

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

[Jeff King]

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

> > +{
> > +   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();
  ...
  if (!color_parse_mem(..., color))
  argv_array_push(, color);
  ...
  argv_array_push(, 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

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

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

> +{
> + 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;
}