{"thread":{"id":"44742","subject":"[PATCH] log: support 256 colors with --graph=256colors","startedAt":"2016-12-20T12:39:41Z","lastAt":"2017-01-28T04:07:17Z","messageCount":30,"participants":["Nguyễn Thái Ngọc Duy","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308140","messageId":"20161220123929.15329-1-pclouds@gmail.com","threadId":"44742","inReplyTo":null,"subject":"[PATCH] log: support 256 colors with --graph=256colors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-20T12:39:29Z","receivedAt":"2016-12-20T12:39:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n I got mad after tracing two consecutive red history lines in `git log\n --graph --oneline` back to their merge points, far far away. Yeah\n probably should fire up tig, or gitk or something.\n\n This may sound like a good thing to add, but I don't know how good it\n is compared to the good old 16 color palette, yet as I haven't tried it\n for long since it's just written.\n\n BTW anyone interested in bringing this type [1] of --graph to git? I\n tried the unicode box characters, but the vertical lines do not join\n with diagonal ones, making the graph a bit ugly (even though it's\n still better than ascii version)\n\n [1] https://github.com/magit/magit/issues/495#issuecomment-17480757\n\n Documentation/rev-list-options.txt | 5 ++++-\n graph.c | 31 ++++++++++++++++++++++++++++++-\n graph.h | 8 +++++++-\n revision.c | 4 ++++\n 4 files changed, 45 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 5da7cf5..0a0c2f3 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -825,7 +825,7 @@ you would get an output like this:\n \t-xxxxxxx... 1st on a\n -----------------------------------------------------------------------\n \n---graph::\n+--graph[=]::\n \tDraw a text-based graphical representation of the commit history\n \ton the left hand side of the output. This may cause extra lines\n \tto be printed in between commits, in order for the graph history\n@@ -836,6 +836,9 @@ This enables parent rewriting, see 'History Simplification' below.\n +\n This implies the `--topo-order` option by default, but the\n `--date-order` option may also be specified.\n++\n+The only supported option is `256colors` which allows more than 16\n+colors for drawing the commit history.\n \n --show-linear-break[=]::\n \tWhen --graph is not used, all history branches are flattened\ndiff --git a/graph.c b/graph.c\nindex d4e8519..75375a1 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -78,6 +78,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n \n static const char **column_colors;\n static unsigned short column_colors_max;\n+static int column_colors_step;\n \n void graph_set_column_colors(const char **colors, unsigned short colors_max)\n {\n@@ -234,10 +235,24 @@ void graph_setup_line_prefix(struct diff_options *diffopt)\n }\n \n \n-struct git_graph *graph_init(struct rev_info *opt)\n+struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg)\n {\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n+\tif (arg && !strcmp(arg, \"256colors\")) {\n+\t\tint i, start = 17, stop = 232;\n+\t\tcolumn_colors_max = stop - start;\n+\t\tcolumn_colors =\n+\t\t\txmalloc((column_colors_max + 1) * sizeof(*column_colors));\n+\t\tfor (i = start; i < stop; i++) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"\\033[38;5;%dm\", i);\n+\t\t\tcolumn_colors[i - start] = strbuf_detach(&sb, NULL);\n+\t\t}\n+\t\tcolumn_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);\n+\t\t/* ignore the closet 16 colors on either side for the next line */\n+\t\tcolumn_colors_step = 16;\n+\t}\n \tif (!column_colors)\n \t\tgraph_set_column_colors(column_colors_ansi,\n \t\t\t\t\tcolumn_colors_ansi_max);\n@@ -382,6 +397,20 @@ static unsigned short graph_get_current_column_color(const struct git_graph *gra\n */\n static void graph_increment_column_color(struct git_graph *graph)\n {\n+\tif (column_colors_step) {\n+\t\tstatic int random_initialized;\n+\t\tint v;\n+\n+\t\tif (!random_initialized) {\n+\t\t\tsrand((unsigned int)getpid());\n+\t\t\trandom_initialized = 1;\n+\t\t}\n+\t\tv = rand() % (column_colors_max - column_colors_step * 2);\n+\t\tgraph->default_column_color += column_colors_step + v;\n+\t\tgraph->default_column_color %= column_colors_max;\n+\t\treturn;\n+\t}\n+\n \tgraph->default_column_color = (graph->default_column_color + 1) %\n \t\tcolumn_colors_max;\n }\ndiff --git a/graph.h b/graph.h\nindex af62339..8069aa4 100644\n--- a/graph.h\n+++ b/graph.h\n@@ -40,7 +40,13 @@ void graph_set_column_colors(const char **colors, unsigned short colors_max);\n /*\n * Create a new struct git_graph.\n */\n-struct git_graph *graph_init(struct rev_info *opt);\n+struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg);\n+\n+static inline struct git_graph *graph_init(struct rev_info *opt)\n+{\n+\treturn graph_init_with_options(opt, NULL);\n+}\n+\n \n /*\n * Update a git_graph with a new commit.\ndiff --git a/revision.c b/revision.c\nindex b37dbec..07bea54 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1933,6 +1933,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->topo_order = 1;\n \t\trevs->rewrite_parents = 1;\n \t\trevs->graph = graph_init(revs);\n+\t} else if (skip_prefix(arg, \"--graph=\", &arg)) {\n+\t\trevs->topo_order = 1;\n+\t\trevs->rewrite_parents = 1;\n+\t\trevs->graph = graph_init_with_options(revs, arg);\n \t} else if (!strcmp(arg, \"--root\")) {\n \t\trevs->show_root_diff = 1;\n \t} else if (!strcmp(arg, \"--no-commit-id\")) {\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308150","messageId":"20161220165754.hkmnsxiwbcgn6uin@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20161220123929.15329-1-pclouds@gmail.com","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-20T16:57:55Z","receivedAt":"2016-12-20T16:58:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 20, 2016 at 07:39:29PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> I got mad after tracing two consecutive red history lines in `git log\n> --graph --oneline` back to their merge points, far far away. Yeah\n> probably should fire up tig, or gitk or something.\n> \n> This may sound like a good thing to add, but I don't know how good it\n> is compared to the good old 16 color palette, yet as I haven't tried it\n> for long since it's just written.\n\nHmm. At some point the colors become too close together to be easily\ndistinguishable. In your code you have:\n\n> +\tif (arg && !strcmp(arg, \"256colors\")) {\n> +\t\tint i, start = 17, stop = 232;\n> +\t\tcolumn_colors_max = stop - start;\n> +\t\tcolumn_colors =\n> +\t\t\txmalloc((column_colors_max + 1) * sizeof(*column_colors));\n> +\t\tfor (i = start; i < stop; i++) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&sb, \"\\033[38;5;%dm\", i);\n> +\t\t\tcolumn_colors[i - start] = strbuf_detach(&sb, NULL);\n> +\t\t}\n> +\t\tcolumn_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);\n> +\t\t/* ignore the closet 16 colors on either side for the next line */\n> +\t\tcolumn_colors_step = 16;\n> +\t}\n\nSo you step by 16, over a set of 215 colors. That seems to give only 13\ncolors, versus the original 16. :)\n\nI know that is a simplification. If you wrap around, then you get your\n13 colors, and then another 13 colors that aren't _quite_ the same, and\nso on, until you've used all 256. I'm just not sure if the 1st and 14th\ncolor would be visually different enough for it to matter (I admit I\ndidn't do any experiments, though).\n\n> ---graph::\n> +--graph[=]::\n> \tDraw a text-based graphical representation of the commit history\n> \ton the left hand side of the output. This may cause extra lines\n> \tto be printed in between commits, in order for the graph history\n\nI wonder if we would ever want another use for \"--graph=foo\". I guess\nany such thing could fall under the name of \"graph options\", and we'd\nend up with \"--graph=256colors,unicode\" or something like that.\n\nI do suspect people would want a config option for this, though. I.e.,\nyou'd want to enable it all the time if you have a terminal which can\nhandle 256 colors, not just for a particular invocation.\n\n-Peff\n"},{"id":"308153","messageId":"xmqqh95yldg4.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20161220123929.15329-1-pclouds@gmail.com","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T17:26:03Z","receivedAt":"2016-12-20T17:26:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/graph.c b/graph.c\n> index d4e8519..75375a1 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -78,6 +78,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n> \n> static const char **column_colors;\n> static unsigned short column_colors_max;\n> +static int column_colors_step;\n> \n> void graph_set_column_colors(const char **colors, unsigned short colors_max)\n> {\n> @@ -234,10 +235,24 @@ void graph_setup_line_prefix(struct diff_options *diffopt)\n> }\n> \n> \n> -struct git_graph *graph_init(struct rev_info *opt)\n> +struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg)\n> {\n> \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n> \n> +\tif (arg && !strcmp(arg, \"256colors\")) {\n> +\t\tint i, start = 17, stop = 232;\n> +\t\tcolumn_colors_max = stop - start;\n> +\t\tcolumn_colors =\n> +\t\t\txmalloc((column_colors_max + 1) * sizeof(*column_colors));\n> +\t\tfor (i = start; i < stop; i++) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&sb, \"\\033[38;5;%dm\", i);\n> +\t\t\tcolumn_colors[i - start] = strbuf_detach(&sb, NULL);\n> +\t\t}\n> +\t\tcolumn_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);\n> +\t\t/* ignore the closet 16 colors on either side for the next line */\n> +\t\tcolumn_colors_step = 16;\n> +\t}\n\nSo you pre-fill a table of colors with 232-17=215 slots. Is the\nidea that it is a co-prime with column_colors_step which is set to\n16 so that going over the table with wraparound will cover all its\nelements?\n\n> @@ -382,6 +397,20 @@ static unsigned short graph_get_current_column_color(const struct git_graph *gra\n> */\n> static void graph_increment_column_color(struct git_graph *graph)\n> {\n> +\tif (column_colors_step) {\n> +\t\tstatic int random_initialized;\n> +\t\tint v;\n> +\n> +\t\tif (!random_initialized) {\n> +\t\t\tsrand((unsigned int)getpid());\n> +\t\t\trandom_initialized = 1;\n> +\t\t}\n> +\t\tv = rand() % (column_colors_max - column_colors_step * 2);\n> +\t\tgraph->default_column_color += column_colors_step + v;\n> +\t\tgraph->default_column_color %= column_colors_max;\n> +\t\treturn;\n> +\t}\n> +\n> \tgraph->default_column_color = (graph->default_column_color + 1) %\n> \t\tcolumn_colors_max;\n> }\n\nThis is too ugly to live as-is for two reasons.\n\n - Do you really need rand()? Doesn't this frustrate somebody who\n runs the same \"git log\" in two terminals in order to view an\n overly tall graph, expecting both commands that were started with\n the same set of arguments to paint the same line in the same\n color? \n\n - Even if you needed rand(), you should be able to factor out\n computation of V so that the function does not need an early\n return that hints totally different processing for two codepaths.\n\n static void graph_increment_column_color(struct git_graph *g)\n {\n int next;\n\n if (! 256-color in use) {\n next = 1;\n } else {\n do whatever to compute your v;\n next = v + column_colors_step;\n }\n g->default_column_color =\n (g->default_column_color + v) / column_colors_max;\n }\n\n Or something like that, perhaps?\n"},{"id":"308243","messageId":"CACsJy8A_4N9jrV7JnuHPEX5uMWQ6fojoagzpQ5TPceB+9iAipw@mail.gmail.com","threadId":"44742","inReplyTo":"xmqqh95yldg4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-22T09:38:44Z","receivedAt":"2016-12-22T09:39:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Dec 21, 2016 at 12:26 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> diff --git a/graph.c b/graph.c\n>> index d4e8519..75375a1 100644\n>> --- a/graph.c\n>> +++ b/graph.c\n>> @@ -78,6 +78,7 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n>>\n>> static const char **column_colors;\n>> static unsigned short column_colors_max;\n>> +static int column_colors_step;\n>>\n>> void graph_set_column_colors(const char **colors, unsigned short colors_max)\n>> {\n>> @@ -234,10 +235,24 @@ void graph_setup_line_prefix(struct diff_options *diffopt)\n>> }\n>>\n>>\n>> -struct git_graph *graph_init(struct rev_info *opt)\n>> +struct git_graph *graph_init_with_options(struct rev_info *opt, const char *arg)\n>> {\n>> struct git_graph *graph = xmalloc(sizeof(struct git_graph));\n>>\n>> + if (arg && !strcmp(arg, \"256colors\")) {\n>> + int i, start = 17, stop = 232;\n>> + column_colors_max = stop - start;\n>> + column_colors =\n>> + xmalloc((column_colors_max + 1) * sizeof(*column_colors));\n>> + for (i = start; i < stop; i++) {\n>> + struct strbuf sb = STRBUF_INIT;\n>> + strbuf_addf(&sb, \"\\033[38;5;%dm\", i);\n>> + column_colors[i - start] = strbuf_detach(&sb, NULL);\n>> + }\n>> + column_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);\n>> + /* ignore the closet 16 colors on either side for the next line */\n>> + column_colors_step = 16;\n>> + }\n>\n> So you pre-fill a table of colors with 232-17=215 slots. Is the\n> idea that it is a co-prime with column_colors_step which is set to\n> 16 so that going over the table with wraparound will cover all its\n> elements?\n\nOriginally yes (because the next color would be more or less the same,\nmaybe brighter or darker a bit), then I went fancy with the rand()\nthing...\n\n>\n>> @@ -382,6 +397,20 @@ static unsigned short graph_get_current_column_color(const struct git_graph *gra\n>> */\n>> static void graph_increment_column_color(struct git_graph *graph)\n>> {\n>> + if (column_colors_step) {\n>> + static int random_initialized;\n>> + int v;\n>> +\n>> + if (!random_initialized) {\n>> + srand((unsigned int)getpid());\n>> + random_initialized = 1;\n>> + }\n>> + v = rand() % (column_colors_max - column_colors_step * 2);\n>> + graph->default_column_color += column_colors_step + v;\n>> + graph->default_column_color %= column_colors_max;\n>> + return;\n>> + }\n>> +\n>> graph->default_column_color = (graph->default_column_color + 1) %\n>> column_colors_max;\n>> }\n>\n> This is too ugly to live as-is for two reasons.\n>\n> - Do you really need rand()? Doesn't this frustrate somebody who\n> runs the same \"git log\" in two terminals in order to view an\n> overly tall graph, expecting both commands that were started with\n> the same set of arguments to paint the same line in the same\n> color?\n\nNo we probably don't need rand(). The thinking was.. now that we have\na lot more colors to choose from, let's add some randomness, maybe\nit'll reduce the chance of showing the same colors in the same line.\n\nThere was another concern with a fixed number of steps too, that we\ncould get into a stable jump sequence and never use all the colors\n(e.g. step 3 with 6 colors total, to simplify). But I verify that\nwe'll use all the colors (at least until we allow people to customize\nstep and the number colors)\n-- \nDuy\n"},{"id":"308244","messageId":"CACsJy8CnS1=_vA5xhbZ94Qyh7ySC5FvaALu1vhQwt_YJya4wHA@mail.gmail.com","threadId":"44742","inReplyTo":"20161220165754.hkmnsxiwbcgn6uin@sigill.intra.peff.net","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-22T09:48:48Z","receivedAt":"2016-12-22T09:49:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Dec 20, 2016 at 11:57 PM, Jeff King wrote:\n> On Tue, Dec 20, 2016 at 07:39:29PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> I got mad after tracing two consecutive red history lines in `git log\n>> --graph --oneline` back to their merge points, far far away. Yeah\n>> probably should fire up tig, or gitk or something.\n>>\n>> This may sound like a good thing to add, but I don't know how good it\n>> is compared to the good old 16 color palette, yet as I haven't tried it\n>> for long since it's just written.\n>\n> Hmm. At some point the colors become too close together to be easily\n> distinguishable. In your code you have:\n>\n>> + if (arg && !strcmp(arg, \"256colors\")) {\n>> + int i, start = 17, stop = 232;\n>> + column_colors_max = stop - start;\n>> + column_colors =\n>> + xmalloc((column_colors_max + 1) * sizeof(*column_colors));\n>> + for (i = start; i < stop; i++) {\n>> + struct strbuf sb = STRBUF_INIT;\n>> + strbuf_addf(&sb, \"\\033[38;5;%dm\", i);\n>> + column_colors[i - start] = strbuf_detach(&sb, NULL);\n>> + }\n>> + column_colors[column_colors_max] = xstrdup(GIT_COLOR_RESET);\n>> + /* ignore the closet 16 colors on either side for the next line */\n>> + column_colors_step = 16;\n>> + }\n>\n> So you step by 16, over a set of 215 colors. That seems to give only 13\n> colors, versus the original 16. :)\n>\n> I know that is a simplification. If you wrap around, then you get your\n> 13 colors, and then another 13 colors that aren't _quite_ the same, and\n> so on, until you've used all 256. I'm just not sure if the 1st and 14th\n> color would be visually different enough for it to matter (I admit I\n> didn't do any experiments, though).\n\nYep. If the jump sequence is a random one, we're less likely to run\ninto this. But I think Junio's \"run git-log in 2 terminals with the\nsame coloring\" convinces me randomization here is not the best thing.\n\nThe best solution would be select colors per text line, so we can pick\ndifferent colors. But I think that's a lot of computation (and\nprobably an NP problem too). The second best option is have a good,\npredefined color palette. We don't need a red of all shades, we need\nsomething that look distinct enough from the rest. I googled for this\nfirst and failed. But I think I could approach it a different way:\ncollect colors that have names. That reduces the number of colors so\nwe can go back to \"step 1 at a time\" and still don't run into two\nsimilar colors often.\n\n>> ---graph::\n>> +--graph[=]::\n>> Draw a text-based graphical representation of the commit history\n>> on the left hand side of the output. This may cause extra lines\n>> to be printed in between commits, in order for the graph history\n>\n> I wonder if we would ever want another use for \"--graph=foo\"\n\nI do. See the screenshot in [1] from the original mail. I have to\nstare at --graph so often lately that it might get my attention before\nother things.\n\n> I guess any such thing could fall under the name of \"graph options\", and we'd\n> end up with \"--graph=256colors,unicode\" or something like that.\n\nExactly.\n\n> I do suspect people would want a config option for this, though. I.e.,\n> you'd want to enable it all the time if you have a terminal which can\n> handle 256 colors, not just for a particular invocation.\n\nYeah. That also means we need the ability to override/negate config\noptions, perhaps something like --graph=-256colors.\n-- \nDuy\n"},{"id":"308260","messageId":"xmqqtw9vg4vh.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"CACsJy8CnS1=_vA5xhbZ94Qyh7ySC5FvaALu1vhQwt_YJya4wHA@mail.gmail.com","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-22T19:06:58Z","receivedAt":"2016-12-22T19:07:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> .... But I think I could approach it a different way:\n> collect colors that have names. That reduces the number of colors so\n> we can go back to \"step 1 at a time\" and still don't run into two\n> similar colors often.\n\nI suspect that there is a \"cultural\" bias that makes the idea\nunworkable. I haven't found a definitive source, but I think there\nare a lot more hues and shades of red that have names than hues and\nshades of blue, for example.\n\nIf I were doing this, I'd just prepare a table with 32 color slots\nor so [*1*], start at a random spot (say 017:00005f) of\n\n https://en.wikipedia.org/wiki/File:Xterm_256color_chart.svg, \n\nand pick spots by jumping southeast like a chess knight\n(i.e. 017->030->043->086->...) until the table is filled, wrapping\naround at the edge of that color chart as necessary.\n\n\n[Footnote]\n\n*1* ...because I do not think the thin graph lines painted in too\nmany colours on the screen would be easily distinguishable from each\nother anyway.\n"},{"id":"308320","messageId":"20161224113817.18407-1-pclouds@gmail.com","threadId":"44742","inReplyTo":"20161220123929.15329-1-pclouds@gmail.com","subject":"[PATCH v2] log --graph: customize the graph lines with config log.graphColors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-24T11:38:17Z","receivedAt":"2016-12-24T11:38:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If you have a 256 colors terminal (or one with true color support), then\nthe predefined 12 colors seem limited. On the other hand, you don't want\nto draw graph lines with every single color in this mode because the two\ncolors could look extremely similar. This option allows you to hand pick\nthe colors you want.\n\nEven with standard terminal, if your background color is neither black\nor white, then the graph line may match your background and become\nhidden. You can exclude your background color (or simply the colors you\nhate) with this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Sounds like the good first step should be something like this instead\n of jumping straight to generating a new color palette automatically.\n\n It's not hard to create a script that generate this config value\n based on some jump calculation, if you don't want to manually picking\n colors.\n\n Documentation/config.txt | 4 ++++\n graph.c | 36 ++++++++++++++++++++++++++++++++++--\n 2 files changed, 38 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d51182a..4f26c2a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2033,6 +2033,10 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graphColors::\n+\tA list of colors, separated by commas, that can be used to draw\n+\thistory lines in `git log --graph`.\n+\n log.showRoot::\n \tIf true, the initial commit will be shown as a big creation event.\n \tThis is equivalent to a diff against an empty tree.\ndiff --git a/graph.c b/graph.c\nindex d4e8519..9c58fd1 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -79,6 +79,39 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n static const char **column_colors;\n static unsigned short column_colors_max;\n \n+static void set_column_colors_by_config(void)\n+{\n+\tstatic char **colors;\n+\tstatic int colors_max, colors_alloc;\n+\tchar *string = NULL;\n+\tconst char *end, *start;\n+\n+\tif (git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\tgraph_set_column_colors(column_colors_ansi,\n+\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\treturn;\n+\t}\n+\n+\tstart = string;\n+\tend = string + strlen(string);\n+\twhile (start < end) {\n+\t\tconst char *comma = strchrnul(start, ',');\n+\t\tchar color[COLOR_MAXLEN];\n+\n+\t\tif (!color_parse_mem(start, comma - start, color)) {\n+\t\t\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n+\t\t\tcolors[colors_max++] = xstrdup(color);\n+\t\t} else\n+\t\t\twarning(_(\"ignore invalid color '%.*s'\"),\n+\t\t\t\t(int)(comma - start), start);\n+\t\tstart = comma + 1;\n+\t}\n+\tfree(string);\n+\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n+\tcolors[colors_max] = xstrdup(GIT_COLOR_RESET);\n+\tgraph_set_column_colors((const char **)colors, colors_max);\n+}\n+\n void graph_set_column_colors(const char **colors, unsigned short colors_max)\n {\n \tcolumn_colors = colors;\n@@ -239,8 +272,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n \tif (!column_colors)\n-\t\tgraph_set_column_colors(column_colors_ansi,\n-\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\tset_column_colors_by_config();\n \n \tgraph->commit = NULL;\n \tgraph->revs = opt;\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308323","messageId":"CACsJy8ANbXZWOV+zN-S8c8V0aTWZ4DHZ=GVmuuAqmY=oFh8T-Q@mail.gmail.com","threadId":"44742","inReplyTo":"xmqqtw9vg4vh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] log: support 256 colors with --graph=256colors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-25T02:36:36Z","receivedAt":"2016-12-25T02:37:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 23, 2016 at 2:06 AM, Junio C Hamano wrote:\n> If I were doing this, I'd just prepare a table with 32 color slots\n> or so [*1*], start at a random spot (say 017:00005f) of\n>\n> https://en.wikipedia.org/wiki/File:Xterm_256color_chart.svg,\n>\n> and pick spots by jumping southeast like a chess knight\n> (i.e. 017->030->043->086->...) until the table is filled, wrapping\n> around at the edge of that color chart as necessary.\n\nIf you want to play with that, [1] may help, which sorts colors in hsv\nspace, and you can select a few colors to see how they look, either\nmanually or by calculation. Yeah we probably get maybe 32 (or 48 if\nyou stretch it a bit) distinct colors. Not sure if it's any better\nwith true color terminals, probably not.\n\n[1] https://gist.github.com/pclouds/616b67f0b5a9b20d74373286574cd0ac\n-- \nDuy\n"},{"id":"308328","messageId":"xmqqzijjd34j.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20161224113817.18407-1-pclouds@gmail.com","subject":"Re: [PATCH v2] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-25T23:02:04Z","receivedAt":"2016-12-25T23:02:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Sounds like the good first step should be something like this instead\n> of jumping straight to generating a new color palette automatically.\n\nI like this not merely as a good first step but potentially a good\nendgame.\n\n> diff --git a/graph.c b/graph.c\n> index d4e8519..9c58fd1 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -79,6 +79,39 @@ static void graph_show_line_prefix(const struct diff_options *diffopt)\n> static const char **column_colors;\n> static unsigned short column_colors_max;\n> \n> +static void set_column_colors_by_config(void)\n> +{\n> +\tstatic char **colors;\n> +\tstatic int colors_max, colors_alloc;\n\nI doubt you want these to be static (and allow multiple instances of\nthe same configuration variable to accumulate). As you defined the\nvalue to be comma-separated colors, users would want the usual \"last\none wins\" rule to override previous settings, no?\n\n> +\tchar *string = NULL;\n> +\tconst char *end, *start;\n> +\n> +\tif (git_config_get_string(\"log.graphcolors\", &string)) {\n> +\t\tgraph_set_column_colors(column_colors_ansi,\n> +\t\t\t\t\tcolumn_colors_ansi_max);\n\nOn the other hand, if you do want cumulative semantics, then you'd\nneed to free the previous set you held in the statics here, I would\nthink.\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tstart = string;\n> +\tend = string + strlen(string);\n> +\twhile (start < end) {\n> +\t\tconst char *comma = strchrnul(start, ',');\n> +\t\tchar color[COLOR_MAXLEN];\n> +\n> +\t\tif (!color_parse_mem(start, comma - start, color)) {\n> +\t\t\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n> +\t\t\tcolors[colors_max++] = xstrdup(color);\n> +\t\t} else\n> +\t\t\twarning(_(\"ignore invalid color '%.*s'\"),\n> +\t\t\t\t(int)(comma - start), start);\n\n\"ignore invalid color\"? Who is giving the command to ignore to whom?\n\n> +\t\tstart = comma + 1;\n> +\t}\n> +\tfree(string);\n> +\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n> +\tcolors[colors_max] = xstrdup(GIT_COLOR_RESET);\n> +\tgraph_set_column_colors((const char **)colors, colors_max);\n> +}\n> +\n> void graph_set_column_colors(const char **colors, unsigned short colors_max)\n> {\n> \tcolumn_colors = colors;\n> @@ -239,8 +272,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n> \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n> \n> \tif (!column_colors)\n> -\t\tgraph_set_column_colors(column_colors_ansi,\n> -\t\t\t\t\tcolumn_colors_ansi_max);\n> +\t\tset_column_colors_by_config();\n\n\"by\" in the function name somehow sounds funny, at least to me.\n"},{"id":"308960","messageId":"20170108101333.26221-1-pclouds@gmail.com","threadId":"44742","inReplyTo":"xmqqzijjd34j.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-08T10:13:33Z","receivedAt":"2017-01-08T10:16:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If you have a 256 colors terminal (or one with true color support), then\nthe predefined 12 colors seem limited. On the other hand, you don't want\nto draw graph lines with every single color in this mode because the two\ncolors could look extremely similar. This option allows you to hand pick\nthe colors you want.\n\nEven with standard terminal, if your background color is neither black\nor white, then the graph line may match your background and become\nhidden. You can exclude your background color (or simply the colors you\nhate) with this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Compared to v2:\n\n * set_column_colors_by_config() is renamed to set_column_colors()\n * no memory leak if the function is called the second time\n * proper space trimming since color_parse_mem expects so\n * fixed the warning message, giving some context\n * at least one test to exercise the code\n * I'm not going with the cumulative behavior because I think that's\n just harder to manage colors, and we would need a way to remove\n colors from the config too.\n\n Documentation/config.txt | 4 ++++\n graph.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--\n t/t4202-log.sh | 22 ++++++++++++++++++++++\n 3 files changed, 70 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..33a007b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2003,6 +2003,10 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graphColors::\n+\tA list of colors, separated by commas, that can be used to draw\n+\thistory lines in `git log --graph`.\n+\n log.showRoot::\n \tIf true, the initial commit will be shown as a big creation event.\n \tThis is equivalent to a diff against an empty tree.\ndiff --git a/graph.c b/graph.c\nindex dd17201..048f5cb 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -62,6 +62,49 @@ enum graph_state {\n static const char **column_colors;\n static unsigned short column_colors_max;\n \n+static void set_column_colors(void)\n+{\n+\tstatic char **colors;\n+\tstatic int colors_max, colors_alloc;\n+\tchar *string = NULL;\n+\tconst char *end, *start;\n+\tint i;\n+\n+\tfor (i = 0; i < colors_max; i++)\n+\t\tfree(colors[i]);\n+\tif (colors)\n+\t\tfree(colors[colors_max]);\n+\tcolors_max = 0;\n+\n+\tif (git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\tgraph_set_column_colors(column_colors_ansi,\n+\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\treturn;\n+\t}\n+\n+\tstart = string;\n+\tend = string + strlen(string);\n+\twhile (start < end) {\n+\t\tconst char *comma = strchrnul(start, ',');\n+\t\tchar color[COLOR_MAXLEN];\n+\n+\t\twhile (start < comma && isspace(*start))\n+\t\t\tstart++;\n+\n+\t\tif (!color_parse_mem(start, comma - start, color)) {\n+\t\t\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n+\t\t\tcolors[colors_max++] = xstrdup(color);\n+\t\t} else\n+\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n+\t\t\t\t(int)(comma - start), start);\n+\t\tstart = comma + 1;\n+\t}\n+\tfree(string);\n+\tALLOC_GROW(colors, colors_max + 1, colors_alloc);\n+\tcolors[colors_max] = xstrdup(GIT_COLOR_RESET);\n+\tgraph_set_column_colors((const char **)colors, colors_max);\n+}\n+\n void graph_set_column_colors(const char **colors, unsigned short colors_max)\n {\n \tcolumn_colors = colors;\n@@ -208,8 +251,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n \tif (!column_colors)\n-\t\tgraph_set_column_colors(column_colors_ansi,\n-\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\tset_column_colors();\n \n \tgraph->commit = NULL;\n \tgraph->revs = opt;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e2db47c..afe0715 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '\n \ttest_cmp expect actual\n '\n \n+cat > expect.colors <<\\EOF\n+* Merge branch 'side'\n+|\\\n+| * side-2\n+| * side-1\n+* | Second\n+* | sixth\n+* | fifth\n+* | fourth\n+|/\n+* third\n+* second\n+* initial\n+EOF\n+\n+test_expect_success 'log --graph with merge with log.graphColors' '\n+\ttest_config log.graphColors \" blue , cyan , red \" &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s |\n+\t\ttest_decode_color | sed \"s/ *\\$//\" >actual &&\n+\ttest_cmp expect.colors actual\n+'\n+\n test_expect_success 'log --raw --graph -m with merge' '\n \tgit log --raw --graph --oneline -m master | head -n 500 >actual &&\n \tgrep \"initial\" actual\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"308978","messageId":"xmqq37gtyluf.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20170108101333.26221-1-pclouds@gmail.com","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T03:05:12Z","receivedAt":"2017-01-09T03:05:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/graph.c b/graph.c\n> index dd17201..048f5cb 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -62,6 +62,49 @@ enum graph_state {\n> static const char **column_colors;\n> static unsigned short column_colors_max;\n> \n> +static void set_column_colors(void)\n\nWhen I said \"'by config' sounds funny\", I meant \"'from config' may\nbe more natural\". Perhaps name this read_graph_colors_config(), as\nthat (i.e. reading \"log.graphColors\") is what it does.\n\n> +{\n> +\tstatic char **colors;\n> +\tstatic int colors_max, colors_alloc;\n> +\tchar *string = NULL;\n> +\tconst char *end, *start;\n> +\tint i;\n> +\n> +\tfor (i = 0; i < colors_max; i++)\n> +\t\tfree(colors[i]);\n> +\tif (colors)\n> +\t\tfree(colors[colors_max]);\n> +\tcolors_max = 0;\n\nThe correctness of the first loop relies on the fact that colors is\nnon-null when colors_max is not zero, and then the freeing of the\ncolors relies on something else. It is not wrong per-se, but it\nwill reduce the \"Huh?\" factor if you wrote it like so:\n\n\tif (colors) {\n \t/* \n * Reinitialize, but keep the colors[] array.\n\t\t * Note that the last entry is allocated for\n\t\t * reset but colors_max does not count it, hence\n\t\t * \"i <= colors_max\", not \"i < colors_max\".\n\t\t */\n\t\tint i;\n\t\tfor (i = 0; i <= colors_max; i++)\n\t\t\tfree(colors[i]);\n\t\tcolors_max = 0;\n\t}\n\n"},{"id":"308983","messageId":"20170109053047.sn75d6ynipgf4nur@sigill.intra.peff.net","threadId":"44742","inReplyTo":"xmqq37gtyluf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-09T05:30:47Z","receivedAt":"2017-01-09T05:30:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 08, 2017 at 07:05:12PM -0800, Junio C Hamano wrote:\n\n> > +{\n> > +\tstatic char **colors;\n> > +\tstatic int colors_max, colors_alloc;\n> > +\tchar *string = NULL;\n> > +\tconst char *end, *start;\n> > +\tint i;\n> > +\n> > +\tfor (i = 0; i < colors_max; i++)\n> > +\t\tfree(colors[i]);\n> > +\tif (colors)\n> > +\t\tfree(colors[colors_max]);\n> > +\tcolors_max = 0;\n> \n> The correctness of the first loop relies on the fact that colors is\n> non-null when colors_max is not zero, and then the freeing of the\n> colors relies on something else. It is not wrong per-se, but it\n> will reduce the \"Huh?\" factor if you wrote it like so:\n> \n> \tif (colors) {\n> \t/* \n> * Reinitialize, but keep the colors[] array.\n> \t\t * Note that the last entry is allocated for\n> \t\t * reset but colors_max does not count it, hence\n> \t\t * \"i <= colors_max\", not \"i < colors_max\".\n> \t\t */\n> \t\tint i;\n> \t\tfor (i = 0; i <= colors_max; i++)\n> \t\t\tfree(colors[i]);\n> \t\tcolors_max = 0;\n> \t}\n\nYeah, I agree that what you've written here is less confusing. Less\nconfusing still would be to keep colors_nr, and deal separately with the\n\"max\" interface to graph_set_column_colors().\n\nI also wonder if it is worth just using argv_array. We do not care about\nNULL-terminating the list here, but it actually works pretty well as a\ngeneric string-array class (and keeping a NULL at the end of any\narray-of-pointers is a reasonable defensive measure). Then the function\nbecomes:\n\n argv_array_clear(&colors);\n ...\n if (!color_parse_mem(..., color))\n argv_array_push(&colors, color);\n ...\n argv_array_push(&colors, GIT_COLOR_RESET);\n /* graph_set_column_colors takes a max-index, not a count */\n graph_set_column_colors(colors.argv, colors.argc - 1);\n\nIt is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.\n\n-Peff\n"},{"id":"308984","messageId":"20170109053446.2vg3y37ck5ahhyxg@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20170108101333.26221-1-pclouds@gmail.com","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-09T05:34:46Z","receivedAt":"2017-01-09T05:34:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 08, 2017 at 05:13:33PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> If you have a 256 colors terminal (or one with true color support), then\n> the predefined 12 colors seem limited. On the other hand, you don't want\n> to draw graph lines with every single color in this mode because the two\n> colors could look extremely similar. This option allows you to hand pick\n> the colors you want.\n> \n> Even with standard terminal, if your background color is neither black\n> or white, then the graph line may match your background and become\n> hidden. You can exclude your background color (or simply the colors you\n> hate) with this.\n\nI like this approach much more than the 256-color option.\n\n> * I'm not going with the cumulative behavior because I think that's\n> just harder to manage colors, and we would need a way to remove\n> colors from the config too.\n\nYeah, figuring out the list semantics would be a pain. This makes it\nhard to exclude a single color, but I think it's more likely somebody\nwould want to replace the whole set with something that works well\nagainst their background.\n\n> +test_expect_success 'log --graph with merge with log.graphColors' '\n> +\ttest_config log.graphColors \" blue , cyan , red \" &&\n\nThis funny syntax isn't required, right? It should work with the more\nnatural:\n\n test_config log.graphColors \"blue, cyan, red\"\n\n-Peff\n"},{"id":"309014","messageId":"CACsJy8BifECbq2K0W2B0-pAxCiWpVgzTdhVLKv01YDSLgcmi_A@mail.gmail.com","threadId":"44742","inReplyTo":"20170109053446.2vg3y37ck5ahhyxg@sigill.intra.peff.net","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-09T10:10:27Z","receivedAt":"2017-01-09T10:11:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 9, 2017 at 12:34 PM, Jeff King wrote:\n>> +test_expect_success 'log --graph with merge with log.graphColors' '\n>> + test_config log.graphColors \" blue , cyan , red \" &&\n>\n> This funny syntax isn't required, right? It should work with the more\n> natural:\n>\n> test_config log.graphColors \"blue, cyan, red\"\n\nYeah spaces are optional. I just wanted to put extra spaces there\nbecause I hit a bug with them.\n-- \nDuy\n"},{"id":"309015","messageId":"CACsJy8AjW1TrLO28mSUSTc6V+_0kuShf7V-=Pkw3Cw9t7ZRfyw@mail.gmail.com","threadId":"44742","inReplyTo":"20170109053047.sn75d6ynipgf4nur@sigill.intra.peff.net","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-09T10:30:23Z","receivedAt":"2017-01-09T10:31:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jan 9, 2017 at 12:30 PM, Jeff King wrote:\n> I also wonder if it is worth just using argv_array. We do not care about\n> NULL-terminating the list here, but it actually works pretty well as a\n> generic string-array class (and keeping a NULL at the end of any\n> array-of-pointers is a reasonable defensive measure). Then the function\n> becomes:\n>\n> argv_array_clear(&colors);\n> ...\n> if (!color_parse_mem(..., color))\n> argv_array_push(&colors, color);\n> ...\n> argv_array_push(&colors, GIT_COLOR_RESET);\n> /* graph_set_column_colors takes a max-index, not a count */\n> graph_set_column_colors(colors.argv, colors.argc - 1);\n>\n> It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.\n\nIndeed. My only complaint is it's \"argv_array_\" and not\n\"string_array_\" but we could think about renaming it later when we see\nanother use like this.\n-- \nDuy\n"},{"id":"309017","messageId":"20170109103258.25341-1-pclouds@gmail.com","threadId":"44742","inReplyTo":"20170108101333.26221-1-pclouds@gmail.com","subject":"[PATCH v4] log --graph: customize the graph lines with config log.graphColors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-09T10:32:58Z","receivedAt":"2017-01-09T10:33:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If you have a 256 colors terminal (or one with true color support), then\nthe predefined 12 colors seem limited. On the other hand, you don't want\nto draw graph lines with every single color in this mode because the two\ncolors could look extremely similar. This option allows you to hand pick\nthe colors you want.\n\nEven with standard terminal, if your background color is neither black\nor white, then the graph line may match your background and become\nhidden. You can exclude your background color (or simply the colors you\nhate) with this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v4:\n\n * rename the function again (and it reads better)\n * use argv_array\n * fix a bug with two consecutive commas (after spaces are trimmed)\n\n Documentation/config.txt | 4 ++++\n graph.c | 43 +++++++++++++++++++++++++++++++++++++++++--\n t/t4202-log.sh | 22 ++++++++++++++++++++++\n 3 files changed, 67 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..33a007b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2003,6 +2003,10 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graphColors::\n+\tA list of colors, separated by commas, that can be used to draw\n+\thistory lines in `git log --graph`.\n+\n log.showRoot::\n \tIf true, the initial commit will be shown as a big creation event.\n \tThis is equivalent to a diff against an empty tree.\ndiff --git a/graph.c b/graph.c\nindex dd17201..83b1d42 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -4,6 +4,7 @@\n #include \"graph.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n+#include \"argv-array.h\"\n \n /* Internal API */\n \n@@ -62,6 +63,45 @@ enum graph_state {\n static const char **column_colors;\n static unsigned short column_colors_max;\n \n+static void read_graph_colors_config(void)\n+{\n+\tstatic struct argv_array colors = ARGV_ARRAY_INIT;\n+\tchar *string = NULL;\n+\tconst char *end, *start;\n+\n+\tif (git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\tgraph_set_column_colors(column_colors_ansi,\n+\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\treturn;\n+\t}\n+\n+\targv_array_clear(&colors);\n+\tstart = string;\n+\tend = string + strlen(string);\n+\twhile (start < end) {\n+\t\tconst char *comma = strchrnul(start, ',');\n+\t\tchar color[COLOR_MAXLEN];\n+\n+\t\twhile (start < comma && isspace(*start))\n+\t\t\tstart++;\n+\t\tif (start == comma) {\n+\t\t\tstart = comma + 1;\n+\t\t\tcontinue;\n+\t\t}\n+\n+\t\tif (!color_parse_mem(start, comma - start, color))\n+\t\t\targv_array_push(&colors, color);\n+\t\telse\n+\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n+\t\t\t\t(int)(comma - start), start);\n+\t\tstart = comma + 1;\n+\t}\n+\tfree(string);\n+\targv_array_push(&colors, GIT_COLOR_RESET);\n+\t/* graph_set_column_colors takes a max-index, not a count */\n+\tgraph_set_column_colors(colors.argv, colors.argc - 1);\n+}\n+\n void graph_set_column_colors(const char **colors, unsigned short colors_max)\n {\n \tcolumn_colors = colors;\n@@ -208,8 +248,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n \tif (!column_colors)\n-\t\tgraph_set_column_colors(column_colors_ansi,\n-\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\tread_graph_colors_config();\n \n \tgraph->commit = NULL;\n \tgraph->revs = opt;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e2db47c..0aeabed 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '\n \ttest_cmp expect actual\n '\n \n+cat > expect.colors <<\\EOF\n+* Merge branch 'side'\n+|\\\n+| * side-2\n+| * side-1\n+* | Second\n+* | sixth\n+* | fifth\n+* | fourth\n+|/\n+* third\n+* second\n+* initial\n+EOF\n+\n+test_expect_success 'log --graph with merge with log.graphColors' '\n+\ttest_config log.graphColors \",, blue,invalid-color, cyan, red , \" &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s |\n+\t\ttest_decode_color | sed \"s/ *\\$//\" >actual &&\n+\ttest_cmp expect.colors actual\n+'\n+\n test_expect_success 'log --raw --graph -m with merge' '\n \tgit log --raw --graph --oneline -m master | head -n 500 >actual &&\n \tgrep \"initial\" actual\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309033","messageId":"xmqqfukswbvw.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"CACsJy8AjW1TrLO28mSUSTc6V+_0kuShf7V-=Pkw3Cw9t7ZRfyw@mail.gmail.com","subject":"Re: [PATCH v3] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T14:23:15Z","receivedAt":"2017-01-09T14:24:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, Jan 9, 2017 at 12:30 PM, Jeff King wrote:\n>> I also wonder if it is worth just using argv_array. We do not care about\n>> ...\n>> It is not much shorter than ALLOC_GROW(), but IMHO it is easier to read.\n>\n> Indeed. My only complaint is it's \"argv_array_\" and not\n> \"string_array_\" but we could think about renaming it later when we see\n> another use like this.\n\nYup, when Peff said \"argv-array\", it took me a few breaths, until I\nrealized that argv-array was merely an array of strings, to convince\nmyself that the data structure can be used here. If we are going to\nuse it in more places, we may want to rename it somehow.\n"},{"id":"309042","messageId":"xmqqh958uoot.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20170109103258.25341-1-pclouds@gmail.com","subject":"Re: [PATCH v4] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T17:29:38Z","receivedAt":"2017-01-09T17:29:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> +\tend = string + strlen(string);\n> +\twhile (start < end) {\n> +\t\tconst char *comma = strchrnul(start, ',');\n> +\t\tchar color[COLOR_MAXLEN];\n> +\n> +\t\twhile (start < comma && isspace(*start))\n> +\t\t\tstart++;\n> +\t\tif (start == comma) {\n> +\t\t\tstart = comma + 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tif (!color_parse_mem(start, comma - start, color))\n\nSo you skip the leading blanks but let color_parse_mem() trim the\ntrailing blanks? It would work once the control reaches the loop,\nbut wouldn't that miss\n\n\tgit -c log.graphColors=' reset , blue, red' log --graph \n\nas \"reset\" is not understood by parse_color() and is understood by\ncolor_parse_mem() only when the length is given correctly?\n\nI am undecided, but leaning towards declaring that it is a bug in\ncolor_parse_mem() not in this caller.\n\n> +\t\t\targv_array_push(&colors, color);\n> +\t\telse\n> +\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n> +\t\t\t\t(int)(comma - start), start);\n> +\t\tstart = comma + 1;\n> +\t}\n> +\tfree(string);\n> +\targv_array_push(&colors, GIT_COLOR_RESET);\n> +\t/* graph_set_column_colors takes a max-index, not a count */\n> +\tgraph_set_column_colors(colors.argv, colors.argc - 1);\n> +}\n> +\n> void graph_set_column_colors(const char **colors, unsigned short colors_max)\n> {\n> \tcolumn_colors = colors;\n> @@ -208,8 +248,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n> \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n> \n> \tif (!column_colors)\n> -\t\tgraph_set_column_colors(column_colors_ansi,\n> -\t\t\t\t\tcolumn_colors_ansi_max);\n> +\t\tread_graph_colors_config();\n\nNow that the helper is renamed to be about \"reading the\nconfiguration to figure out the graph colors\", the division of labor\nbetween the caller and the callee we see here is suboptimal for\nreadability, I would think. \n\nWe would want to see the caller to either be\n\n\tif (!column_colors) {\n\t\tif (read_graph_colors_config())\n\t\t\t; /* ok */\n\t\telse \n\t\t\tgraph_set_column_colors(ansi, ansi_max);\n\t}\n\nor better yet, something like:\n\n\tif (!column_colors) {\n\t\tconst char **colors = column_colors_ansi;\n\t\tint colors_max = column_colors_ansi_max;\n\n\t\tread_graph_colors_config(&colors, &colors_max);\n\t\tgraph_set_collumn_colors(colors, colors_max);\n\t}\n\nThe last one would make it clear that by default we use ansi but\noverride that default by calling that function whose purpose is to\nread the values that override the default from the configuration.\n\nI haven't thought things thru regarding memory leakages etc., so\nthere may be valid reasons why the last one is infeasible.\n\n"},{"id":"309270","messageId":"CACsJy8AbdDxsSUDSx7=kezjPOCQnhpNdwKyMvb-xQ+d7wd99Tw@mail.gmail.com","threadId":"44742","inReplyTo":"xmqqh958uoot.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] log --graph: customize the graph lines with config log.graphColors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-12T12:20:14Z","receivedAt":"2017-01-12T12:20:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Just FYI. The broken internet cables in Vietnam seem to hit my ISP\nreally hard. It's nearly impossible to make a TCP connection. So I'm\nbasically off the grid, hopefully not longer than two weeks.\n\nOn 1/10/17, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> +\tend = string + strlen(string);\n>> +\twhile (start < end) {\n>> +\t\tconst char *comma = strchrnul(start, ',');\n>> +\t\tchar color[COLOR_MAXLEN];\n>> +\n>> +\t\twhile (start < comma && isspace(*start))\n>> +\t\t\tstart++;\n>> +\t\tif (start == comma) {\n>> +\t\t\tstart = comma + 1;\n>> +\t\t\tcontinue;\n>> +\t\t}\n>> +\n>> +\t\tif (!color_parse_mem(start, comma - start, color))\n>\n> So you skip the leading blanks but let color_parse_mem() trim the\n> trailing blanks? It would work once the control reaches the loop,\n> but wouldn't that miss\n>\n> \tgit -c log.graphColors=' reset , blue, red' log --graph\n>\n> as \"reset\" is not\n"},{"id":"309716","messageId":"20170119114123.23784-1-pclouds@gmail.com","threadId":"44742","inReplyTo":"20170109103258.25341-1-pclouds@gmail.com","subject":"[PATCH v5 0/3] nd/log-graph-configurable-colors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-19T11:41:20Z","receivedAt":"2017-01-19T11:41:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v5 moves space trimming to color_parse_mem() from read_graph_colors_config,\nwhich is renamed to parse_graph... because the config reading is moved\nback to graph_init.\n\nI think it looks better, but we may be pushing the limits of\nargv_array's abuse.\n\nNguyễn Thái Ngọc Duy (3):\n color.c: fix color_parse_mem() with value_len == 0\n color.c: trim leading spaces in color_parse_mem()\n log --graph: customize the graph lines with config log.graphColors\n\n Documentation/config.txt | 4 ++++\n color.c | 10 +++++++++-\n graph.c | 42 +++++++++++++++++++++++++++++++++++++++---\n t/t4202-log.sh | 22 ++++++++++++++++++++++\n 4 files changed, 74 insertions(+), 4 deletions(-)\n\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309717","messageId":"20170119114123.23784-2-pclouds@gmail.com","threadId":"44742","inReplyTo":"20170119114123.23784-1-pclouds@gmail.com","subject":"[PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-19T11:41:21Z","receivedAt":"2017-01-19T11:41:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In this code we want to match the word \"reset\". If len is zero,\nstrncasecmp() will return zero and we incorrectly assume it's \"reset\" as\na result.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n color.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/color.c b/color.c\nindex 81c2676..a9eadd1 100644\n--- a/color.c\n+++ b/color.c\n@@ -207,6 +207,9 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \tstruct color fg = { COLOR_UNSPECIFIED };\n \tstruct color bg = { COLOR_UNSPECIFIED };\n \n+\tif (!len)\n+\t\treturn -1;\n+\n \tif (!strncasecmp(value, \"reset\", len)) {\n \t\txsnprintf(dst, end - dst, GIT_COLOR_RESET);\n \t\treturn 0;\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309718","messageId":"20170119114123.23784-3-pclouds@gmail.com","threadId":"44742","inReplyTo":"20170119114123.23784-1-pclouds@gmail.com","subject":"[PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-19T11:41:22Z","receivedAt":"2017-01-19T11:41:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Normally color_parse_mem() is called from config parser which trims the\nleading spaces already. The new caller in the next patch won't. Let's be\ntidy and trim leading spaces too (we already trim trailing spaces before\ncomma).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n color.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/color.c b/color.c\nindex a9eadd1..7bb4a96 100644\n--- a/color.c\n+++ b/color.c\n@@ -207,10 +207,15 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \tstruct color fg = { COLOR_UNSPECIFIED };\n \tstruct color bg = { COLOR_UNSPECIFIED };\n \n+\twhile (len > 0 && isspace(*ptr)) {\n+\t\tptr++;\n+\t\tlen--;\n+\t}\n+\n \tif (!len)\n \t\treturn -1;\n \n-\tif (!strncasecmp(value, \"reset\", len)) {\n+\tif (!strncasecmp(ptr, \"reset\", len)) {\n \t\txsnprintf(dst, end - dst, GIT_COLOR_RESET);\n \t\treturn 0;\n \t}\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309719","messageId":"20170119114123.23784-4-pclouds@gmail.com","threadId":"44742","inReplyTo":"20170119114123.23784-1-pclouds@gmail.com","subject":"[PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2017-01-19T11:41:23Z","receivedAt":"2017-01-19T11:41:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If you have a 256 colors terminal (or one with true color support), then\nthe predefined 12 colors seem limited. On the other hand, you don't want\nto draw graph lines with every single color in this mode because the two\ncolors could look extremely similar. This option allows you to hand pick\nthe colors you want.\n\nEven with standard terminal, if your background color is neither black\nor white, then the graph line may match your background and become\nhidden. You can exclude your background color (or simply the colors you\nhate) with this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/config.txt | 4 ++++\n graph.c | 42 +++++++++++++++++++++++++++++++++++++++---\n t/t4202-log.sh | 22 ++++++++++++++++++++++\n 3 files changed, 65 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..33a007b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2003,6 +2003,10 @@ log.follow::\n \ti.e. it cannot be used to follow multiple files and does not work well\n \ton non-linear history.\n \n+log.graphColors::\n+\tA list of colors, separated by commas, that can be used to draw\n+\thistory lines in `git log --graph`.\n+\n log.showRoot::\n \tIf true, the initial commit will be shown as a big creation event.\n \tThis is equivalent to a diff against an empty tree.\ndiff --git a/graph.c b/graph.c\nindex dd17201..822d40a 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -4,6 +4,7 @@\n #include \"graph.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n+#include \"argv-array.h\"\n \n /* Internal API */\n \n@@ -62,6 +63,26 @@ enum graph_state {\n static const char **column_colors;\n static unsigned short column_colors_max;\n \n+static void parse_graph_colors_config(struct argv_array *colors, const char *string)\n+{\n+\tconst char *end, *start;\n+\n+\tstart = string;\n+\tend = string + strlen(string);\n+\twhile (start < end) {\n+\t\tconst char *comma = strchrnul(start, ',');\n+\t\tchar color[COLOR_MAXLEN];\n+\n+\t\tif (!color_parse_mem(start, comma - start, color))\n+\t\t\targv_array_push(colors, color);\n+\t\telse\n+\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n+\t\t\t\t(int)(comma - start), start);\n+\t\tstart = comma + 1;\n+\t}\n+\targv_array_push(colors, GIT_COLOR_RESET);\n+}\n+\n void graph_set_column_colors(const char **colors, unsigned short colors_max)\n {\n \tcolumn_colors = colors;\n@@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)\n {\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n-\tif (!column_colors)\n-\t\tgraph_set_column_colors(column_colors_ansi,\n-\t\t\t\t\tcolumn_colors_ansi_max);\n+\tif (!column_colors) {\n+\t\tstruct argv_array ansi_colors = {\n+\t\t\tcolumn_colors_ansi,\n+\t\t\tcolumn_colors_ansi_max + 1\n+\t\t};\n+\t\tstruct argv_array *colors = &ansi_colors;\n+\t\tchar *string;\n+\n+\t\tif (!git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\t\tstatic struct argv_array custom_colors = ARGV_ARRAY_INIT;\n+\t\t\targv_array_clear(&custom_colors);\n+\t\t\tparse_graph_colors_config(&custom_colors, string);\n+\t\t\tfree(string);\n+\t\t\tcolors = &custom_colors;\n+\t\t}\n+\t\t/* graph_set_column_colors takes a max-index, not a count */\n+\t\tgraph_set_column_colors(colors->argv, colors->argc - 1);\n+\t}\n \n \tgraph->commit = NULL;\n \tgraph->revs = opt;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e2db47c..0aeabed 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' '\n \ttest_cmp expect actual\n '\n \n+cat > expect.colors <<\\EOF\n+* Merge branch 'side'\n+|\\\n+| * side-2\n+| * side-1\n+* | Second\n+* | sixth\n+* | fifth\n+* | fourth\n+|/\n+* third\n+* second\n+* initial\n+EOF\n+\n+test_expect_success 'log --graph with merge with log.graphColors' '\n+\ttest_config log.graphColors \",, blue,invalid-color, cyan, red , \" &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s |\n+\t\ttest_decode_color | sed \"s/ *\\$//\" >actual &&\n+\ttest_cmp expect.colors actual\n+'\n+\n test_expect_success 'log --raw --graph -m with merge' '\n \tgit log --raw --graph --oneline -m master | head -n 500 >actual &&\n \tgrep \"initial\" actual\n-- \n2.8.2.524.g6ff3d78\n\n"},{"id":"309735","messageId":"20170119163841.zhtpz6rxdieaywuy@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20170119114123.23784-2-pclouds@gmail.com","subject":"Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T16:38:41Z","receivedAt":"2017-01-19T16:39:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> In this code we want to match the word \"reset\". If len is zero,\n> strncasecmp() will return zero and we incorrectly assume it's \"reset\" as\n> a result.\n\nThis is probably a good idea. This _is_ user-visible, so it's possible\nsomebody was using empty config as a synonym for \"reset\". But since it\nwas never documented, I feel like relying on that is somewhat crazy.\n\n-Peff\n"},{"id":"309736","messageId":"20170119164157.mvoadhxxwwynedoz@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20170119114123.23784-3-pclouds@gmail.com","subject":"Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T16:41:57Z","receivedAt":"2017-01-19T16:43:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> Normally color_parse_mem() is called from config parser which trims the\n> leading spaces already. The new caller in the next patch won't. Let's be\n> tidy and trim leading spaces too (we already trim trailing spaces before\n> comma).\n\nWhat comma? I don't think that exists until the next patch. :)\n\nI think just trimming from the front is OK, though, because\ncolor_parse_mem() trims trailing whitespace after a word. So either you\nhave a word and we will trim after it, or you do not (in which case\nthis will trim everything and hit the !len case you added).\n\nSo maybe a better commit message is just:\n\n Normally color_parse_mem() is called from config parser which trims\n the leading spaces already. The new caller in the next patch won't.\n Let's be tidy and trim leading spaces too (we already trim trailing\n spaces after a word).\n\n-Peff\n"},{"id":"309737","messageId":"20170119165127.6cxw64fjz7aevkq2@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20170119114123.23784-4-pclouds@gmail.com","subject":"Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-19T16:51:27Z","receivedAt":"2017-01-19T16:51:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:\n\n> +static void parse_graph_colors_config(struct argv_array *colors, const char *string)\n> +{\n> +\tconst char *end, *start;\n> +\n> +\tstart = string;\n> +\tend = string + strlen(string);\n> +\twhile (start < end) {\n> +\t\tconst char *comma = strchrnul(start, ',');\n> +\t\tchar color[COLOR_MAXLEN];\n> +\n> +\t\tif (!color_parse_mem(start, comma - start, color))\n> +\t\t\targv_array_push(colors, color);\n> +\t\telse\n> +\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n> +\t\t\t\t(int)(comma - start), start);\n> +\t\tstart = comma + 1;\n> +\t}\n> +\targv_array_push(colors, GIT_COLOR_RESET);\n> +}\n\nThis looks good.\n\n> @@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)\n> {\n> \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n> \n> -\tif (!column_colors)\n> -\t\tgraph_set_column_colors(column_colors_ansi,\n> -\t\t\t\t\tcolumn_colors_ansi_max);\n> +\tif (!column_colors) {\n> +\t\tstruct argv_array ansi_colors = {\n> +\t\t\tcolumn_colors_ansi,\n> +\t\t\tcolumn_colors_ansi_max + 1\n> +\t\t};\n\nHrm. At first I thought this would cause memory corrution, because your\nargv_array_clear() would try to free() the non-heap array you've stuffed\ninside. But you only clear the custom_colors array which actually is\ndynamically allocated. This outer one is just here to give uniform\naccess:\n\n> +\t\tstruct argv_array *colors = &ansi_colors;\n> +\t\tchar *string;\n> +\n> +\t\tif (!git_config_get_string(\"log.graphcolors\", &string)) {\n> +\t\t\tstatic struct argv_array custom_colors = ARGV_ARRAY_INIT;\n> +\t\t\targv_array_clear(&custom_colors);\n> +\t\t\tparse_graph_colors_config(&custom_colors, string);\n> +\t\t\tfree(string);\n> +\t\t\tcolors = &custom_colors;\n> +\t\t}\n> +\t\t/* graph_set_column_colors takes a max-index, not a count */\n> +\t\tgraph_set_column_colors(colors->argv, colors->argc - 1);\n> +\t}\n\nSince there's only one line that cares about the result of \"colors\",\nmaybe it would be less confusing to do:\n\n if (!git_config_get-string(\"log.graphcolors\", &string)) {\n\t... parse, etc ...\n\tgraph_set_column_colors(colors.argv, colors.argc - 1);\n } else {\n\tgraph_set_column_colors(column_colors_ansi,\n\t column_colors_ansi_max);\n }\n\n-Peff\n"},{"id":"309746","messageId":"xmqq60la3o5e.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20170119164157.mvoadhxxwwynedoz@sigill.intra.peff.net","subject":"Re: [PATCH v5 2/3] color.c: trim leading spaces in color_parse_mem()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T18:22:53Z","receivedAt":"2017-01-19T18:27:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jan 19, 2017 at 06:41:22PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> Normally color_parse_mem() is called from config parser which trims the\n>> leading spaces already. The new caller in the next patch won't. Let's be\n>> tidy and trim leading spaces too (we already trim trailing spaces before\n>> comma).\n>\n> What comma? I don't think that exists until the next patch. :)\n>\n> I think just trimming from the front is OK, though, because\n> color_parse_mem() trims trailing whitespace after a word. So either you\n> have a word and we will trim after it, or you do not (in which case\n> this will trim everything and hit the !len case you added).\n>\n> So maybe a better commit message is just:\n>\n> Normally color_parse_mem() is called from config parser which trims\n> the leading spaces already. The new caller in the next patch won't.\n> Let's be tidy and trim leading spaces too (we already trim trailing\n> spaces after a word).\n>\n> -Peff\n\nYeah, my reading stuttered at the same place in the original, and\nyour rewrite looks a lot more sensible.\n"},{"id":"309748","messageId":"xmqq1svy3nxi.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"20170119165127.6cxw64fjz7aevkq2@sigill.intra.peff.net","subject":"Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T18:27:37Z","receivedAt":"2017-01-19T18:27:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jan 19, 2017 at 06:41:23PM +0700, Nguyễn Thái Ngọc Duy wrote:\n>\n>> +static void parse_graph_colors_config(struct argv_array *colors, const char *string)\n>> +{\n>> +\tconst char *end, *start;\n>> +\n>> +\tstart = string;\n>> +\tend = string + strlen(string);\n>> +\twhile (start < end) {\n>> +\t\tconst char *comma = strchrnul(start, ',');\n>> +\t\tchar color[COLOR_MAXLEN];\n>> +\n>> +\t\tif (!color_parse_mem(start, comma - start, color))\n>> +\t\t\targv_array_push(colors, color);\n>> +\t\telse\n>> +\t\t\twarning(_(\"ignore invalid color '%.*s' in log.graphColors\"),\n>> +\t\t\t\t(int)(comma - start), start);\n>> +\t\tstart = comma + 1;\n>> +\t}\n>> +\targv_array_push(colors, GIT_COLOR_RESET);\n>> +}\n>\n> This looks good.\n>\n>> @@ -207,9 +228,24 @@ struct git_graph *graph_init(struct rev_info *opt)\n>> {\n>> \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n>> \n>> -\tif (!column_colors)\n>> -\t\tgraph_set_column_colors(column_colors_ansi,\n>> -\t\t\t\t\tcolumn_colors_ansi_max);\n>> +\tif (!column_colors) {\n>> +\t\tstruct argv_array ansi_colors = {\n>> +\t\t\tcolumn_colors_ansi,\n>> +\t\t\tcolumn_colors_ansi_max + 1\n>> +\t\t};\n>\n> Hrm. At first I thought this would cause memory corrution, because your\n> argv_array_clear() would try to free() the non-heap array you've stuffed\n> inside. But you only clear the custom_colors array which actually is\n> dynamically allocated. This outer one is just here to give uniform\n> access:\n>\n>> +\t\tstruct argv_array *colors = &ansi_colors;\n>> +\t\tchar *string;\n>> +\n>> +\t\tif (!git_config_get_string(\"log.graphcolors\", &string)) {\n>> +\t\t\tstatic struct argv_array custom_colors = ARGV_ARRAY_INIT;\n>> +\t\t\targv_array_clear(&custom_colors);\n>> +\t\t\tparse_graph_colors_config(&custom_colors, string);\n>> +\t\t\tfree(string);\n>> +\t\t\tcolors = &custom_colors;\n>> +\t\t}\n>> +\t\t/* graph_set_column_colors takes a max-index, not a count */\n>> +\t\tgraph_set_column_colors(colors->argv, colors->argc - 1);\n>> +\t}\n>\n> Since there's only one line that cares about the result of \"colors\",\n> maybe it would be less confusing to do:\n>\n> if (!git_config_get-string(\"log.graphcolors\", &string)) {\n> \t... parse, etc ...\n> \tgraph_set_column_colors(colors.argv, colors.argc - 1);\n> } else {\n> \tgraph_set_column_colors(column_colors_ansi,\n> \t column_colors_ansi_max);\n> }\n\nYes, that would be much much less confusing. By doing so, the cover\nletter can lose \"pushing the limits of abuse\", too ;-).\n\n\n"},{"id":"309762","messageId":"xmqqd1fi269d.fsf@gitster.mtv.corp.google.com","threadId":"44742","inReplyTo":"xmqq1svy3nxi.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v5 3/3] log --graph: customize the graph lines with config log.graphColors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-19T19:34:38Z","receivedAt":"2017-01-19T19:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Since there's only one line that cares about the result of \"colors\",\n>> maybe it would be less confusing to do:\n>>\n>> if (!git_config_get-string(\"log.graphcolors\", &string)) {\n>> \t... parse, etc ...\n>> \tgraph_set_column_colors(colors.argv, colors.argc - 1);\n>> } else {\n>> \tgraph_set_column_colors(column_colors_ansi,\n>> \t column_colors_ansi_max);\n>> }\n>\n> Yes, that would be much much less confusing. By doing so, the cover\n> letter can lose \"pushing the limits of abuse\", too ;-).\n\nWill queue with a squashable change.\n\nThanks.\n\n graph.c | 18 ++++++++----------\n 1 file changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 822d40ae20..00aeee36d8 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -229,22 +229,20 @@ struct git_graph *graph_init(struct rev_info *opt)\n \tstruct git_graph *graph = xmalloc(sizeof(struct git_graph));\n \n \tif (!column_colors) {\n-\t\tstruct argv_array ansi_colors = {\n-\t\t\tcolumn_colors_ansi,\n-\t\t\tcolumn_colors_ansi_max + 1\n-\t\t};\n-\t\tstruct argv_array *colors = &ansi_colors;\n \t\tchar *string;\n-\n-\t\tif (!git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\tif (git_config_get_string(\"log.graphcolors\", &string)) {\n+\t\t\t/* not configured -- use default */\n+\t\t\tgraph_set_column_colors(column_colors_ansi,\n+\t\t\t\t\t\tcolumn_colors_ansi_max);\n+\t\t} else {\n \t\t\tstatic struct argv_array custom_colors = ARGV_ARRAY_INIT;\n \t\t\targv_array_clear(&custom_colors);\n \t\t\tparse_graph_colors_config(&custom_colors, string);\n \t\t\tfree(string);\n-\t\t\tcolors = &custom_colors;\n+\t\t\t/* graph_set_column_colors takes a max-index, not a count */\n+\t\t\tgraph_set_column_colors(custom_colors.argv,\n+\t\t\t\t\t\tcustom_colors.argc - 1);\n \t\t}\n-\t\t/* graph_set_column_colors takes a max-index, not a count */\n-\t\tgraph_set_column_colors(colors->argv, colors->argc - 1);\n \t}\n \n \tgraph->commit = NULL;\n"},{"id":"310486","messageId":"20170128040709.tqx4u45ktgpkbfm4@sigill.intra.peff.net","threadId":"44742","inReplyTo":"20170119163841.zhtpz6rxdieaywuy@sigill.intra.peff.net","subject":"Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-28T04:07:10Z","receivedAt":"2017-01-28T04:07:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 19, 2017 at 11:38:41AM -0500, Jeff King wrote:\n\n> On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> \n> > In this code we want to match the word \"reset\". If len is zero,\n> > strncasecmp() will return zero and we incorrectly assume it's \"reset\" as\n> > a result.\n> \n> This is probably a good idea. This _is_ user-visible, so it's possible\n> somebody was using empty config as a synonym for \"reset\". But since it\n> was never documented, I feel like relying on that is somewhat crazy.\n\nHrm. This seems to break the add--interactive script if you do not have\ncolor.diff.plain set:\n\n $ GIT_TRACE=1 git add -p\n ...\n 22:58:12.568990 [pid=11401] git.c:387 trace: built-in: git 'config' '--get-color' 'color.diff.plain' ''\n fatal: unable to parse default color value\n config --get-color color.diff.plain : command returned error: 128\n\nAs you can see, the default value the empty string, which is now an\nerror.\n\nThe default in the C code for that value is GIT_COLOR_NORMAL, which\nreally is the empty string. So I think the old code was buggy to choose\n\"reset\", but the new one is worse because it fails entirely. :)\n\nWe probably want something like this instead:\n\ndiff --git a/color.c b/color.c\nindex 190b2da96..dee61557e 100644\n--- a/color.c\n+++ b/color.c\n@@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)\n \t\tlen--;\n \t}\n \n-\tif (!len)\n-\t\treturn -1;\n+\tif (!len) {\n+\t\tdst[0] = '\\0';\n+\t\treturn 0;\n+\t}\n \n \tif (!strncasecmp(ptr, \"reset\", len)) {\n \t\txsnprintf(dst, end - dst, GIT_COLOR_RESET);\n\nThe breakage is in 'next' (it looks like it went out a few days ago; I'm\nsurprised I didn't notice until now).\n\n-Peff\n"}]}