[PATCH] Share color list between graph and show-branch

[PATCH] Share color list between graph and show-branch

[Dan McGee]

This also adds the new colors to show-branch that were added a while
back for graph output.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---
 builtin/show-branch.c |   16 +++-------------
 color.c               |   19 +++++++++++++++++++
 color.h               |    4 ++++
 graph.c               |   21 ---------------------
 4 files changed, 26 insertions(+), 34 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index da69581..d00c0ac 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -12,16 +12,6 @@ static const char* show_branch_usage[] = {
 };
 
 static int showbranch_use_color = -1;
-static char column_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RED,
-	GIT_COLOR_GREEN,
-	GIT_COLOR_YELLOW,
-	GIT_COLOR_BLUE,
-	GIT_COLOR_MAGENTA,
-	GIT_COLOR_CYAN,
-};
-
-#define COLUMN_COLORS_MAX (ARRAY_SIZE(column_colors))
 
 static int default_num;
 static int default_alloc;
@@ -37,7 +27,7 @@ static const char **default_arg;
 static const char *get_color_code(int idx)
 {
 	if (showbranch_use_color)
-		return column_colors[idx];
+		return column_colors_ansi[idx % COLUMN_COLORS_ANSI_MAX];
 	return "";
 }
 
@@ -892,7 +882,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				for (j = 0; j < i; j++)
 					putchar(' ');
 				printf("%s%c%s [%s] ",
-				       get_color_code(i % COLUMN_COLORS_MAX),
+				       get_color_code(i),
 				       is_head ? '*' : '!',
 				       get_color_reset_code(), ref_name[i]);
 			}
@@ -954,7 +944,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				else
 					mark = '+';
 				printf("%s%c%s",
-				       get_color_code(i % COLUMN_COLORS_MAX),
+				       get_color_code(i),
 				       mark, get_color_reset_code());
 			}
 			putchar(' ');
diff --git a/color.c b/color.c
index 417cf8f..6631346 100644
--- a/color.c
+++ b/color.c
@@ -3,6 +3,25 @@
 
 int git_use_color_default = 0;
 
+/*
+ * The list of available column colors.
+ */
+const char *column_colors_ansi[13] = {
+	GIT_COLOR_RED,
+	GIT_COLOR_GREEN,
+	GIT_COLOR_YELLOW,
+	GIT_COLOR_BLUE,
+	GIT_COLOR_MAGENTA,
+	GIT_COLOR_CYAN,
+	GIT_COLOR_BOLD_RED,
+	GIT_COLOR_BOLD_GREEN,
+	GIT_COLOR_BOLD_YELLOW,
+	GIT_COLOR_BOLD_BLUE,
+	GIT_COLOR_BOLD_MAGENTA,
+	GIT_COLOR_BOLD_CYAN,
+	GIT_COLOR_RESET,
+};
+
 static int parse_color(const char *name, int len)
 {
 	static const char * const color_names[] = {
diff --git a/color.h b/color.h
index c0528cf..a7da793 100644
--- a/color.h
+++ b/color.h
@@ -53,6 +53,10 @@ struct strbuf;
  */
 extern int git_use_color_default;
 
+extern const char *column_colors_ansi[13];
+
+/* Ignore the RESET at the end when giving the size */
+#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
diff --git a/graph.c b/graph.c
index ef2e24e..d1dd15e 100644
--- a/graph.c
+++ b/graph.c
@@ -59,27 +59,6 @@ enum graph_state {
 	GRAPH_COLLAPSING
 };
 
-/*
- * The list of available column colors.
- */
-static const char *column_colors_ansi[] = {
-	GIT_COLOR_RED,
-	GIT_COLOR_GREEN,
-	GIT_COLOR_YELLOW,
-	GIT_COLOR_BLUE,
-	GIT_COLOR_MAGENTA,
-	GIT_COLOR_CYAN,
-	GIT_COLOR_BOLD_RED,
-	GIT_COLOR_BOLD_GREEN,
-	GIT_COLOR_BOLD_YELLOW,
-	GIT_COLOR_BOLD_BLUE,
-	GIT_COLOR_BOLD_MAGENTA,
-	GIT_COLOR_BOLD_CYAN,
-	GIT_COLOR_RESET,
-};
-
-#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)
-
 static const char **column_colors;
 static unsigned short column_colors_max;
 
-- 
1.7.4.2

Re: [PATCH] Share color list between graph and show-branch

[Junio C Hamano]

Dan McGee <dpmcgee@gmail.com> writes:

> diff --git a/color.h b/color.h
> index c0528cf..a7da793 100644
> --- a/color.h
> +++ b/color.h
> @@ -53,6 +53,10 @@ struct strbuf;
>   */
>  extern int git_use_color_default;
>  
> +extern const char *column_colors_ansi[13];
> +
> +/* Ignore the RESET at the end when giving the size */
> +#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)

Sneaky.

I first went "Huh? -- this array-size macro cannot work", expecting that
the array is not decleared with a fixed size in the header.

It may make sense to unify these two palettes whose slot assignment does
not have any meaning, but it feels that the above change totally goes
against the spirit of using ARRAY_SIZE() macro, the point of which is to
liberate programmers from having to count and adjust the size when adding
the contents to the array.

Wouldn't it make more sense to do something like

    >>> in the header <<<
    extern const char *custom_colors_ansi[];
    extern const int CUSTOM_COLORS_ANSI_MAX;

    >>> in the code <<<
    const char *custom_colors_ansi[] = {
            ... as before ...
    };
    /* Does not count the last element "RESET" */
    const int CUSTOM_COLORS_ANSI_MAX = ARRAY_SIZE(custom_colors_ansi) - 1;

to avoid mistakes?

Re: [PATCH] Share color list between graph and show-branch

[Dan McGee]

On Sun, Apr 3, 2011 at 2:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dpmcgee@gmail.com> writes:
>
>> diff --git a/color.h b/color.h
>> index c0528cf..a7da793 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -53,6 +53,10 @@ struct strbuf;
>>   */
>>  extern int git_use_color_default;
>>
>> +extern const char *column_colors_ansi[13];
>> +
>> +/* Ignore the RESET at the end when giving the size */
>> +#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)
>
> Sneaky.
>
> I first went "Huh? -- this array-size macro cannot work", expecting that
> the array is not decleared with a fixed size in the header.
>
> It may make sense to unify these two palettes whose slot assignment does
> not have any meaning, but it feels that the above change totally goes
> against the spirit of using ARRAY_SIZE() macro, the point of which is to
> liberate programmers from having to count and adjust the size when adding
> the contents to the array.
>
> Wouldn't it make more sense to do something like
>
>    >>> in the header <<<
>    extern const char *custom_colors_ansi[];
>    extern const int CUSTOM_COLORS_ANSI_MAX;
>
>    >>> in the code <<<
>    const char *custom_colors_ansi[] = {
>            ... as before ...
>    };
>    /* Does not count the last element "RESET" */
>    const int CUSTOM_COLORS_ANSI_MAX = ARRAY_SIZE(custom_colors_ansi) - 1;
>
> to avoid mistakes?

Duh. This makes way more sense, I'll resend the patch with the
necessary changes; I couldn't think of an elegant way to do it at the
time.

On another note, we also have this whole crazy "- 1" bit and the RESET
element at the end, and yet I see nowhere that slot is actually used.
It looks like this was introduced by commit 1e3d4119d21df28.

-Dan

[PATCH] Share color list between graph and show-branch

[Dan McGee]

This also adds the new colors to show-branch that were added a while
back for graph output.

Signed-off-by: Dan McGee <dpmcgee@gmail.com>
---

Updated to not require a static length definition of the array anywhere.

 builtin/show-branch.c |   16 +++-------------
 color.c               |   22 ++++++++++++++++++++++
 color.h               |    3 +++
 graph.c               |   23 +----------------------
 4 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index da69581..1abcd9e 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -12,16 +12,6 @@ static const char* show_branch_usage[] = {
 };
 
 static int showbranch_use_color = -1;
-static char column_colors[][COLOR_MAXLEN] = {
-	GIT_COLOR_RED,
-	GIT_COLOR_GREEN,
-	GIT_COLOR_YELLOW,
-	GIT_COLOR_BLUE,
-	GIT_COLOR_MAGENTA,
-	GIT_COLOR_CYAN,
-};
-
-#define COLUMN_COLORS_MAX (ARRAY_SIZE(column_colors))
 
 static int default_num;
 static int default_alloc;
@@ -37,7 +27,7 @@ static const char **default_arg;
 static const char *get_color_code(int idx)
 {
 	if (showbranch_use_color)
-		return column_colors[idx];
+		return column_colors_ansi[idx % column_colors_ansi_max];
 	return "";
 }
 
@@ -892,7 +882,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				for (j = 0; j < i; j++)
 					putchar(' ');
 				printf("%s%c%s [%s] ",
-				       get_color_code(i % COLUMN_COLORS_MAX),
+				       get_color_code(i),
 				       is_head ? '*' : '!',
 				       get_color_reset_code(), ref_name[i]);
 			}
@@ -954,7 +944,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				else
 					mark = '+';
 				printf("%s%c%s",
-				       get_color_code(i % COLUMN_COLORS_MAX),
+				       get_color_code(i),
 				       mark, get_color_reset_code());
 			}
 			putchar(' ');
diff --git a/color.c b/color.c
index 417cf8f..3db214c 100644
--- a/color.c
+++ b/color.c
@@ -3,6 +3,28 @@
 
 int git_use_color_default = 0;
 
+/*
+ * The list of available column colors.
+ */
+const char *column_colors_ansi[] = {
+	GIT_COLOR_RED,
+	GIT_COLOR_GREEN,
+	GIT_COLOR_YELLOW,
+	GIT_COLOR_BLUE,
+	GIT_COLOR_MAGENTA,
+	GIT_COLOR_CYAN,
+	GIT_COLOR_BOLD_RED,
+	GIT_COLOR_BOLD_GREEN,
+	GIT_COLOR_BOLD_YELLOW,
+	GIT_COLOR_BOLD_BLUE,
+	GIT_COLOR_BOLD_MAGENTA,
+	GIT_COLOR_BOLD_CYAN,
+	GIT_COLOR_RESET,
+};
+
+/* Ignore the RESET at the end when giving the size */
+const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
+
 static int parse_color(const char *name, int len)
 {
 	static const char * const color_names[] = {
diff --git a/color.h b/color.h
index c0528cf..68a926a 100644
--- a/color.h
+++ b/color.h
@@ -53,6 +53,9 @@ struct strbuf;
  */
 extern int git_use_color_default;
 
+/* A default list of colors to use for commit graphs and show-branch output */
+extern const char *column_colors_ansi[];
+extern const int column_colors_ansi_max;
 
 /*
  * Use this instead of git_default_config if you need the value of color.ui.
diff --git a/graph.c b/graph.c
index ef2e24e..2f6893d 100644
--- a/graph.c
+++ b/graph.c
@@ -59,27 +59,6 @@ enum graph_state {
 	GRAPH_COLLAPSING
 };
 
-/*
- * The list of available column colors.
- */
-static const char *column_colors_ansi[] = {
-	GIT_COLOR_RED,
-	GIT_COLOR_GREEN,
-	GIT_COLOR_YELLOW,
-	GIT_COLOR_BLUE,
-	GIT_COLOR_MAGENTA,
-	GIT_COLOR_CYAN,
-	GIT_COLOR_BOLD_RED,
-	GIT_COLOR_BOLD_GREEN,
-	GIT_COLOR_BOLD_YELLOW,
-	GIT_COLOR_BOLD_BLUE,
-	GIT_COLOR_BOLD_MAGENTA,
-	GIT_COLOR_BOLD_CYAN,
-	GIT_COLOR_RESET,
-};
-
-#define COLUMN_COLORS_ANSI_MAX (ARRAY_SIZE(column_colors_ansi) - 1)
-
 static const char **column_colors;
 static unsigned short column_colors_max;
 
@@ -228,7 +207,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 
 	if (!column_colors)
 		graph_set_column_colors(column_colors_ansi,
-					COLUMN_COLORS_ANSI_MAX);
+					column_colors_ansi_max);
 
 	graph->commit = NULL;
 	graph->revs = opt;
-- 
1.7.4.2

Re: [PATCH] Share color list between graph and show-branch

[Johan Herland]

On Tuesday 05 April 2011, Dan McGee wrote:
> On another note, we also have this whole crazy "- 1" bit and the RESET
> element at the end, and yet I see nowhere that slot is actually used.
> It looks like this was introduced by commit 1e3d4119d21df28.

Read that commit again. You'll see that in graph.c:strbuf_write_column() it 
replaces

  strbuf_addstr(sb, GIT_COLOR_RESET);

with

  strbuf_addstr(sb, column_get_color_code(column_colors_max));

which resolves to the same thing. The reason for that extra indirection is 
to enable replacing the column_colors_ansi array with a different color 
array, to do graph coloring in non-ANSI contexts. Specifically, it was done 
to enable HTML/CSS coloring of graphs in CGit: 
http://hjemli.net/git/cgit/commit/?id=268b34af23cdcac87aed3300bfe6154cbc65753e

It should be obvious that if we replace the ANSI coloring scheme with some 
other coloring scheme, we also need to change the RESET entry (resetting a 
HTML "color" with the ANSI reset code is nonsense). Therefore I opted to 
move the RESET code into the column_colors array, and make column_colors_max 
indicate both (a) the length of the column_colors array, and (b) the index 
of the RESET code in that same array. That's why we need the crazy "- 1" bit 
when defining COLUMN_COLORS_ANSI_MAX.

BTW, this is documented graph.h:graph_set_column_colors() from the same 
1e3d4119d21df28 commit.


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net