[RFC] Colorization of log --graph

[RFC] Colorization of log --graph

[Allan Caffee]

I know that _some_ people arn't particularly fond of colors, but I was
wondering how difficult it would be to colorize the edges on the
--graph drawn by the log command?  It can be a little tricky trying to
follow them with a relatively complex history.  I was thinking something
like gitk already does.  Is anybody else interested in seeing this?

~Allan

Re: [RFC] Colorization of log --graph

[Johannes Schindelin]

Hi,

On Wed, 18 Mar 2009, Allan Caffee wrote:

> I know that _some_ people arn't particularly fond of colors, but I was 
> wondering how difficult it would be to colorize the edges on the --graph 
> drawn by the log command?  It can be a little tricky trying to follow 
> them with a relatively complex history.  I was thinking something like 
> gitk already does.

That's a good idea!  (And it is mentioned as a TODO in graph.c...)

> Is anybody else interested in seeing this?

Count me in.  Are you interested in implementing this?

If so:

- you need to #include "color.h" in graph.c

- you need to insert a color identifier into struct column (there is an 
  XXX comment at the correct location)

- you need to find a way to determine colors for the branches

- you need to put the handling into the function 
  graph_output_pre_commit_line() in graph.c (and probably 
  graph_output_commit_char(), graph_output_post_merge_line(), 
  graph_output_collapsing_line(), graph_padding_line(), and
  graph_output_padding_line(), too)

- it would make sense IMHO to introduce a new function that takes a 
  pointer to an strbuf, a pointer to a struct column and a char (or maybe 
  a string) that adds the appropriately colorized char (or string) to the 
  strbuf

- use the global variable diff_use_color to determine if the output should 
  be colorized at all

- probably you need to make an array of available colors or some such 
  (which might be good to put into color.[ch])

Ciao,
Dscho

Re: [RFC] Colorization of log --graph

[Allan Caffee]

Hello and thanks for the speedy reply!

On Wed, Mar 18, 2009 at 7:44 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 18 Mar 2009, Allan Caffee wrote:
>
> > I know that _some_ people arn't particularly fond of colors, but I was
> > wondering how difficult it would be to colorize the edges on the --graph
> > drawn by the log command?  It can be a little tricky trying to follow
> > them with a relatively complex history.  I was thinking something like
> > gitk already does.
>
> That's a good idea!  (And it is mentioned as a TODO in graph.c...)

That's me, always thinking outside the box.  ;-)

> > Is anybody else interested in seeing this?
>
> Count me in.  Are you interested in implementing this?

I'll give it a go.  Been a while since I've done anything of substance
in pure C so it should be a nice refresher.  :)

> If so:
>
> - you need to #include "color.h" in graph.c
>
> - you need to insert a color identifier into struct column (there is an
>  XXX comment at the correct location)

By color identifier I assume you mean the ANSI escape sequence, right?
I didn't see a type for representing colors in color.{c,h} other than
the int it seems to use internally.

> - you need to find a way to determine colors for the branches

Okay, so if we were to make this similiar to how gitk works it would involve:
If the previous commit was a merge:
	for (i = 0; i < graph->num_columns; i++)
		graph->columns[i]->color = get_next_column_color();
else
	get_current_column_color();

I was thinking of storing the current color by adding a
default_column_color attribute to git_graph that serves as an index into
column_colors.  column_colors being the array of available colors.

> - you need to put the handling into the function
>  graph_output_pre_commit_line() in graph.c (and probably
>  graph_output_commit_char(), graph_output_post_merge_line(),
>  graph_output_collapsing_line(), graph_padding_line(), and
>  graph_output_padding_line(), too)
>
> - it would make sense IMHO to introduce a new function that takes a
>  pointer to an strbuf, a pointer to a struct column and a char (or maybe
>  a string) that adds the appropriately colorized char (or string) to the
>  strbuf

That makes sense.  Then we can just update the functions you mentioned
above to use this.

> - use the global variable diff_use_color to determine if the output should
>  be colorized at all

The function for adding a column to an strbuf would offer a convenient
place to put the condition.

> - probably you need to make an array of available colors or some such
>  (which might be good to put into color.[ch])

This would be the color_codes array I mentioned but it seems like it
might belong in graph.c.  There's something similiar in diff.c and it
seems like this is more related to graphing then to colors in general.
Although I do think it makes sense to #define some of the more common
ANSI codes there so that they don't have to be duplicated.  grep shows 6
occurrences of '\033[31m', the code for red foreground.

I'll begin working on a patch.  Comments/questions?

~Allan

Re: [RFC] Colorization of log --graph

[Johannes Schindelin]

Hi,

On Thu, 19 Mar 2009, Allan Caffee wrote:

> Hello and thanks for the speedy reply!

Heh, Git is known for raw speed ;-)

> On Wed, Mar 18, 2009 at 7:44 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Wed, 18 Mar 2009, Allan Caffee wrote:
> >
> > > Is anybody else interested in seeing this?
> >
> > Count me in.  Are you interested in implementing this?
> 
> I'll give it a go.  Been a while since I've done anything of substance 
> in pure C so it should be a nice refresher.  :)

Great!

> > If so:
> >
> > - you need to #include "color.h" in graph.c
> >
> > - you need to insert a color identifier into struct column (there is an
> >  XXX comment at the correct location)
> 
> By color identifier I assume you mean the ANSI escape sequence, right? I 
> didn't see a type for representing colors in color.{c,h} other than the 
> int it seems to use internally.

I'd actually add an enum color_names, or something like that.

> > - you need to find a way to determine colors for the branches
> 
> Okay, so if we were to make this similiar to how gitk works it would 
> involve: If the previous commit was a merge:
> 	for (i = 0; i < graph->num_columns; i++)
> 		graph->columns[i]->color = get_next_column_color();
> else
> 	get_current_column_color();
> 
> I was thinking of storing the current color by adding a 
> default_column_color attribute to git_graph that serves as an index into 
> column_colors.  column_colors being the array of available colors.

Yep, I agree.  That index could be of type "enum color_names" if you 
introduce the latter...

> > - you need to put the handling into the function 
> >   graph_output_pre_commit_line() in graph.c (and probably 
> >   graph_output_commit_char(), graph_output_post_merge_line(), 
> >   graph_output_collapsing_line(), graph_padding_line(), and 
> >   graph_output_padding_line(), too)
> >
> > - it would make sense IMHO to introduce a new function that takes a 
> >   pointer to an strbuf, a pointer to a struct column and a char (or 
> >   maybe a string) that adds the appropriately colorized char (or 
> >   string) to the strbuf
> 
> That makes sense.  Then we can just update the functions you mentioned
> above to use this.

Right.

> > - use the global variable diff_use_color to determine if the output 
> >   should be colorized at all
> 
> The function for adding a column to an strbuf would offer a convenient 
> place to put the condition.

Yes!

> > - probably you need to make an array of available colors or some such 
> >   (which might be good to put into color.[ch])
> 
> This would be the color_codes array I mentioned but it seems like it
> might belong in graph.c.  There's something similiar in diff.c and it
> seems like this is more related to graphing then to colors in general.
> Although I do think it makes sense to #define some of the more common
> ANSI codes there so that they don't have to be duplicated.  grep shows 6
> occurrences of '\033[31m', the code for red foreground.

I'd actually like to see it in color.[ch], so that other code paths can 
use it, too.

I'd start like this:

	enum color_name {
		COLOR_RESET,
		COLOR_RED,
		COLOR_GREEN,
		COLOR_YELLOW,
		COLOR_BLUE,
		COLOR_MAGENTA,
		COLOR_CYAN,
		COLOR_WHITE
	};

Maybe the best thing would then be to add a function

	void strbuf_add_color(struct strbuf *buf, enum color_name name) {
		if (name == COLOR_RESET)
			strbuf_addf(buf, "\033[m");
		else
			strbuf_addf(buf, "\033[%dm", 31 + name - COLOR_RED);
	}

Ciao,
Dscho

Re: [RFC] Colorization of log --graph

[Nanako Shiraishi]

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:

> I'd start like this:
>
> 	enum color_name {
> 		COLOR_RESET,
> 		COLOR_RED,
> 		COLOR_GREEN,
> 		COLOR_YELLOW,
> 		COLOR_BLUE,
> 		COLOR_MAGENTA,
> 		COLOR_CYAN,
> 		COLOR_WHITE
> 	};

Looking for "COLOR_RED" in the archive gives:

  http://article.gmane.org/gmane.comp.version-control.git/109676

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Re: [RFC] Colorization of log --graph

[Allan Caffee]

On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
>> I'd start like this:
>>
>>       enum color_name {
>>               COLOR_RESET,
>>               COLOR_RED,
>>               COLOR_GREEN,
>>               COLOR_YELLOW,
>>               COLOR_BLUE,
>>               COLOR_MAGENTA,
>>               COLOR_CYAN,
>>               COLOR_WHITE
>>       };
>
> Looking for "COLOR_RED" in the archive gives:
>
>  http://article.gmane.org/gmane.comp.version-control.git/109676
>

Duly noted.  Perhaps those #defines should be relocated to color.h?  If
we still wanted to provide a color_name type we could use
GIT_COLOR_NAME_RESET et al.  That would give us something like:

#define GIT_COLOR_NORMAL	""
#define GIT_COLOR_RESET		"\033[m"
#define GIT_COLOR_BOLD		"\033[1m"
#define GIT_COLOR_RED		"\033[31m"
#define GIT_COLOR_GREEN		"\033[32m"
#define GIT_COLOR_YELLOW	"\033[33m"
#define GIT_COLOR_BLUE		"\033[34m"
#define GIT_COLOR_CYAN		"\033[36m"
#define GIT_COLOR_BG_RED	"\033[41m"

enum color_name {
	GIT_COLOR_NAME_NORMAL
	GIT_COLOR_NAME_RESET,
	GIT_COLOR_NAME_RED,
	GIT_COLOR_NAME_GREEN,
	GIT_COLOR_NAME_YELLOW,
	GIT_COLOR_NAME_BLUE,
	GIT_COLOR_NAME_MAGENTA,
	GIT_COLOR_NAME_CYAN,
	GIT_COLOR_NAME_WHITE
	GIT_COLOR_NAME_BG_RED
};

/*
 * Map names to ANSI escape sequences.  Consider putting this in color.c
 * and providing color_name_get_ansi_code(enum color_name).
 */
const char* git_color_codes[] {
	GIT_COLOR_RESET,
	GIT_COLOR_BOLD,
	GIT_COLOR_RED,
	GIT_COLOR_GREEN,
	GIT_COLOR_YELLOW,
	GIT_COLOR_BLUE,
	GIT_COLOR_CYAN,
	GIT_COLOR_BG_RED,
};

That conveniently offers clients access to both the raw escape codes and
a clear type for storing/handling colors.

~Allan

Re: [RFC] Colorization of log --graph

[Jeff King]

On Fri, Mar 20, 2009 at 03:13:53PM -0400, Allan Caffee wrote:

> /*
>  * Map names to ANSI escape sequences.  Consider putting this in color.c
>  * and providing color_name_get_ansi_code(enum color_name).
>  */
> const char* git_color_codes[] {
> 	GIT_COLOR_RESET,
> 	GIT_COLOR_BOLD,
> 	GIT_COLOR_RED,
> 	GIT_COLOR_GREEN,
> 	GIT_COLOR_YELLOW,
> 	GIT_COLOR_BLUE,
> 	GIT_COLOR_CYAN,
> 	GIT_COLOR_BG_RED,
> };
> 
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.

I want to point out one thing: an enum or a list like this is actually a
subset of the useful color codes that git can represent.  Actual
configured colors can have attributes, foreground, and background
colors. So they need to be stored in a character array.

Adding an enum for GIT_COLOR_RED and using it throughout the code can be
helpful for simple cases, but it doesn't give you an easy way of saying
"red background, blue foreground". Maybe that is enough for git internal
usage, since we tend not to use backgrounds or attributes for defaults.
But maybe it makes more sense to do this as:

  const char *ansi_color(enum color fg, enum color bg, enum attribute attr);

and return a pointer to a static array representing the color (and even
cycle through a list the way sha1_to_hex or git_path does). And you
could even use it to simplify and share code with the config color
parsing in color.c.

-Peff

[RFC/PATCH] graph API: Added logic for colored edges.

[Allan Caffee]

Modified the graph drawing logic to colorize edges based on parent-child
relationships similiarly to gitk.

Signed-off-by: Allan Caffee <allan.caffee@gmail.com>
---

I havn't gotten the chance to do any of the color clean up that's been
discussed on this thread.  I'll try to throw something together in a seperate
patch series.

Also this patch isn't respecting the --no-color option which I imagine means
that diff_use_color_default isn't the right variable to be checking.  Johannes
mentioned using diff_use_color but the only instance I see is a parameter to
diff_get_color.  What am I missing?

~Allan

 color.h |    1 +
 graph.c |  167 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 144 insertions(+), 24 deletions(-)

diff --git a/color.h b/color.h
index 6846be1..18abeb7 100644
--- a/color.h
+++ b/color.h
@@ -11,6 +11,7 @@
 #define GIT_COLOR_GREEN		"\033[32m"
 #define GIT_COLOR_YELLOW	"\033[33m"
 #define GIT_COLOR_BLUE		"\033[34m"
+#define GIT_COLOR_MAGENTA	"\033[35m"
 #define GIT_COLOR_CYAN		"\033[36m"
 #define GIT_COLOR_BG_RED	"\033[41m"
 
diff --git a/graph.c b/graph.c
index 162a516..2929c8b 100644
--- a/graph.c
+++ b/graph.c
@@ -1,9 +1,11 @@
 #include "cache.h"
 #include "commit.h"
+#include "color.h"
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"
 
+extern int diff_use_color_default;
 /* Internal API */
 
 /*
@@ -72,11 +74,22 @@ struct column {
 	 */
 	struct commit *commit;
 	/*
-	 * XXX: Once we add support for colors, struct column could also
-	 * contain the color of its branch line.
+	 * The color to (optionally) print this column in.
 	 */
+	char *color;
 };
 
+static void strbuf_write_column(struct strbuf *sb, const struct column *c,
+		const char *s);
+
+static char* get_current_column_color (const struct git_graph* graph);
+
+/*
+ * Update the default column color and return the new value.
+ */
+static char* get_next_column_color(struct git_graph* graph);
+
+
 enum graph_state {
 	GRAPH_PADDING,
 	GRAPH_SKIP,
@@ -86,6 +99,24 @@ enum graph_state {
 	GRAPH_COLLAPSING
 };
 
+/*
+ * The list of available column colors.
+ */
+static char column_colors[][COLOR_MAXLEN] = {
+	GIT_COLOR_RED,
+	GIT_COLOR_GREEN,
+	GIT_COLOR_YELLOW,
+	GIT_COLOR_BLUE,
+	GIT_COLOR_MAGENTA,
+	GIT_COLOR_CYAN,
+	GIT_COLOR_BOLD GIT_COLOR_RED,
+	GIT_COLOR_BOLD GIT_COLOR_GREEN,
+	GIT_COLOR_BOLD GIT_COLOR_YELLOW,
+	GIT_COLOR_BOLD GIT_COLOR_BLUE,
+	GIT_COLOR_BOLD GIT_COLOR_MAGENTA,
+	GIT_COLOR_BOLD GIT_COLOR_CYAN,
+};
+
 struct git_graph {
 	/*
 	 * The commit currently being processed
@@ -185,6 +216,11 @@ struct git_graph {
 	 * temporary array each time we have to output a collapsing line.
 	 */
 	int *new_mapping;
+	/*
+	 * The current default column color being used.  This is
+	 * stored as an index into the array column_colors.
+	 */
+	short default_column_color;
 };
 
 struct git_graph *graph_init(struct rev_info *opt)
@@ -201,6 +237,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	graph->num_columns = 0;
 	graph->num_new_columns = 0;
 	graph->mapping_size = 0;
+	graph->default_column_color = 0;
 
 	/*
 	 * Allocate a reasonably large default number of columns
@@ -317,6 +354,14 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 					  int *mapping_index)
 {
 	int i;
+	char *color = get_current_column_color(graph);
+
+	for (i = 0; i < graph->num_columns; i++) {
+		if (graph->columns[i].commit == commit) {
+			color = graph->columns[i].color;
+			break;
+		}
+	}
 
 	/*
 	 * If the commit is already in the new_columns list, we don't need to
@@ -334,6 +379,8 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 	 * This commit isn't already in new_columns.  Add it.
 	 */
 	graph->new_columns[graph->num_new_columns].commit = commit;
+/*         fprintf(stderr,"adding the %scommit%s\n", color, GIT_COLOR_RESET); */
+	graph->new_columns[graph->num_new_columns].color = color;
 	graph->mapping[*mapping_index] = graph->num_new_columns;
 	*mapping_index += 2;
 	graph->num_new_columns++;
@@ -445,6 +492,12 @@ static void graph_update_columns(struct git_graph *graph)
 			for (parent = first_interesting_parent(graph);
 			     parent;
 			     parent = next_interesting_parent(graph, parent)) {
+				/*
+				 * If this is a merge increment the current
+				 * color.
+				 */
+				if (graph->num_parents > 1)
+					get_next_column_color(graph);
 				graph_insert_into_new_columns(graph,
 							      parent->item,
 							      &mapping_idx);
@@ -596,7 +649,7 @@ static void graph_output_padding_line(struct git_graph *graph,
 	 * Output a padding row, that leaves all branch lines unchanged
 	 */
 	for (i = 0; i < graph->num_new_columns; i++) {
-		strbuf_addstr(sb, "| ");
+		strbuf_write_column(sb, &graph->new_columns[i], "| ");
 	}
 
 	graph_pad_horizontally(graph, sb);
@@ -649,7 +702,10 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 		struct column *col = &graph->columns[i];
 		if (col->commit == graph->commit) {
 			seen_this = 1;
-			strbuf_addf(sb, "| %*s", graph->expansion_row, "");
+			struct strbuf tmp = STRBUF_INIT;
+			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
+			strbuf_write_column(sb, col, tmp.buf);
+			strbuf_release(&tmp);
 		} else if (seen_this && (graph->expansion_row == 0)) {
 			/*
 			 * This is the first line of the pre-commit output.
@@ -662,13 +718,13 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 			 */
 			if (graph->prev_state == GRAPH_POST_MERGE &&
 			    graph->prev_commit_index < i)
-				strbuf_addstr(sb, "\\ ");
+				strbuf_write_column(sb, col, "\\ ");
 			else
-				strbuf_addstr(sb, "| ");
+				strbuf_write_column(sb, col, "| ");
 		} else if (seen_this && (graph->expansion_row > 0)) {
-			strbuf_addstr(sb, "\\ ");
+			strbuf_write_column(sb, col, "\\ ");
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -728,6 +784,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 	 */
 	seen_this = 0;
 	for (i = 0; i <= graph->num_columns; i++) {
+		struct column *col = &graph->columns[i];
 		struct commit *col_commit;
 		if (i == graph->num_columns) {
 			if (seen_this)
@@ -751,7 +808,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 				strbuf_addstr(sb, ". ");
 			}
 		} else if (seen_this && (graph->num_parents > 2)) {
-			strbuf_addstr(sb, "\\ ");
+			strbuf_write_column(sb, col, "\\ ");
 		} else if (seen_this && (graph->num_parents == 2)) {
 			/*
 			 * This is a 2-way merge commit.
@@ -768,11 +825,11 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			 */
 			if (graph->prev_state == GRAPH_POST_MERGE &&
 			    graph->prev_commit_index < i)
-				strbuf_addstr(sb, "\\ ");
+				strbuf_write_column(sb, col, "\\ ");
 			else
-				strbuf_addstr(sb, "| ");
+				strbuf_write_column(sb, col, "| ");
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -789,6 +846,17 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
+inline struct column* find_new_column_by_commit(struct git_graph *graph,
+						struct commit *commit)
+{
+	int i;
+	for (i = 0; i < graph->num_new_columns; i++) {
+		if (graph->new_columns[i].commit == commit)
+			return &graph->new_columns[i];
+	}
+	return 0;
+}
+
 static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
@@ -798,24 +866,43 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
 	 * Output the post-merge row
 	 */
 	for (i = 0; i <= graph->num_columns; i++) {
+		struct column *col = &graph->columns[i];
 		struct commit *col_commit;
 		if (i == graph->num_columns) {
 			if (seen_this)
 				break;
 			col_commit = graph->commit;
 		} else {
-			col_commit = graph->columns[i].commit;
+			col_commit = col->commit;
 		}
 
 		if (col_commit == graph->commit) {
+			/*
+			 * Since the current commit is a merge find
+			 * the columns for the parent commits in
+			 * new_columns and use those to format the
+			 * edges.
+			 */
+			struct commit_list *parents = NULL;
+			struct column *par_column;
 			seen_this = 1;
-			strbuf_addch(sb, '|');
-			for (j = 0; j < graph->num_parents - 1; j++)
-				strbuf_addstr(sb, "\\ ");
+			parents = first_interesting_parent(graph);
+			assert(parents);
+			par_column = find_new_column_by_commit(graph,parents->item);
+			assert(par_column);
+
+			strbuf_write_column(sb, par_column, "|");
+			for (j = 0; j < graph->num_parents - 1; j++) {
+				parents = next_interesting_parent(graph, parents);
+				assert(parents);
+				par_column = find_new_column_by_commit(graph,parents->item);
+				assert(par_column);
+				strbuf_write_column(sb, par_column, "\\ ");
+			}
 		} else if (seen_this) {
-			strbuf_addstr(sb, "\\ ");
+			strbuf_write_column(sb, col, "\\ ");
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -834,6 +921,8 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 {
 	int i;
 	int *tmp_mapping;
+	static int collapsing_columns[255];
+	int collapsing_seen_so_far = 0;
 
 	/*
 	 * Clear out the new_mapping array
@@ -912,9 +1001,11 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 		if (target < 0)
 			strbuf_addch(sb, ' ');
 		else if (target * 2 == i)
-			strbuf_addch(sb, '|');
-		else
-			strbuf_addch(sb, '/');
+			strbuf_write_column(sb, &graph->new_columns[target], "|");
+		else {
+			strbuf_write_column(sb, &graph->new_columns[target], "/");
+
+		}
 	}
 
 	graph_pad_horizontally(graph, sb);
@@ -979,9 +1070,10 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 	 * children that we have already processed.)
 	 */
 	for (i = 0; i < graph->num_columns; i++) {
-		struct commit *col_commit = graph->columns[i].commit;
+		struct column *col = &graph->columns[i];
+		struct commit *col_commit = col->commit;
 		if (col_commit == graph->commit) {
-			strbuf_addch(sb, '|');
+			strbuf_write_column(sb, col, "|");
 
 			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
@@ -991,7 +1083,7 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 					strbuf_addch(sb, ' ');
 			}
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -1154,3 +1246,30 @@ void graph_show_commit_msg(struct git_graph *graph,
 			putchar('\n');
 	}
 }
+
+static void strbuf_write_column(struct strbuf *sb, const struct column *c,
+		const char *s)
+{
+	/*
+	 * TODO: I get the creeping suspicion that this isn't the
+	 * right flag to be checking since --no-color doesn't turn
+	 * this off.
+	 */
+	if (diff_use_color_default)
+		strbuf_addstr(sb, c->color);
+	strbuf_addstr(sb, s);
+	if (diff_use_color_default)
+		strbuf_addstr(sb, GIT_COLOR_RESET);
+}
+
+static char* get_current_column_color (const struct git_graph* graph)
+{
+	return column_colors[graph->default_column_color];
+}
+
+static char* get_next_column_color(struct git_graph* graph)
+{
+	graph->default_column_color = (graph->default_column_color + 1) %
+		ARRAY_SIZE(column_colors);
+	return (get_current_column_color(graph));
+}
-- 
1.5.4.3

[PATCH 1/2] graph.c: avoid compile warnings

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	I'd actually like to see this and the next patch squashed in.

 graph.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/graph.c b/graph.c
index 2929c8b..5e2f224 100644
--- a/graph.c
+++ b/graph.c
@@ -701,8 +701,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 	for (i = 0; i < graph->num_columns; i++) {
 		struct column *col = &graph->columns[i];
 		if (col->commit == graph->commit) {
-			seen_this = 1;
 			struct strbuf tmp = STRBUF_INIT;
+			seen_this = 1;
 			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
 			strbuf_write_column(sb, col, tmp.buf);
 			strbuf_release(&tmp);
@@ -921,8 +921,6 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 {
 	int i;
 	int *tmp_mapping;
-	static int collapsing_columns[255];
-	int collapsing_seen_so_far = 0;
 
 	/*
 	 * Clear out the new_mapping array
-- 
1.6.2.1.493.g67cf3

Re: [PATCH 1/2] graph.c: avoid compile warnings

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	I'd actually like to see this and the next patch squashed in.
>
>  graph.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index 2929c8b..5e2f224 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -701,8 +701,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
>  	for (i = 0; i < graph->num_columns; i++) {
>  		struct column *col = &graph->columns[i];
>  		if (col->commit == graph->commit) {
> -			seen_this = 1;
>  			struct strbuf tmp = STRBUF_INIT;
> +			seen_this = 1;

Which codebase are you working on top of?

>  			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
>  			strbuf_write_column(sb, col, tmp.buf);
>  			strbuf_release(&tmp);
> @@ -921,8 +921,6 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
>  {
>  	int i;
>  	int *tmp_mapping;
> -	static int collapsing_columns[255];
> -	int collapsing_seen_so_far = 0;
>  
>  	/*
>  	 * Clear out the new_mapping array
> -- 
> 1.6.2.1.493.g67cf3

Re: [PATCH 1/2] graph.c: avoid compile warnings

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/graph.c b/graph.c
>> index 2929c8b..5e2f224 100644
>> --- a/graph.c
>> +++ b/graph.c
>> @@ -701,8 +701,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
>>  	for (i = 0; i < graph->num_columns; i++) {
>>  		struct column *col = &graph->columns[i];
>>  		if (col->commit == graph->commit) {
>> -			seen_this = 1;
>>  			struct strbuf tmp = STRBUF_INIT;
>> +			seen_this = 1;
>
> Which codebase are you working on top of?

Nevermind.  I didn't realize it was "here are to help whipping your series
into shape" meant for Allan.

[PATCH 2/2] --graph: respect --no-color

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/graph.c b/graph.c
index 5e2f224..6a8622c 100644
--- a/graph.c
+++ b/graph.c
@@ -5,7 +5,6 @@
 #include "diff.h"
 #include "revision.h"
 
-extern int diff_use_color_default;
 /* Internal API */
 
 /*
@@ -1253,15 +1252,17 @@ static void strbuf_write_column(struct strbuf *sb, const struct column *c,
 	 * right flag to be checking since --no-color doesn't turn
 	 * this off.
 	 */
-	if (diff_use_color_default)
+	if (c->color)
 		strbuf_addstr(sb, c->color);
 	strbuf_addstr(sb, s);
-	if (diff_use_color_default)
+	if (c->color)
 		strbuf_addstr(sb, GIT_COLOR_RESET);
 }
 
 static char* get_current_column_color (const struct git_graph* graph)
 {
+	if (!DIFF_OPT_TST(&graph->revs->diffopt, COLOR_DIFF))
+		return NULL;
 	return column_colors[graph->default_column_color];
 }
 
-- 
1.6.2.1.493.g67cf3

Re: [RFC/PATCH] graph API: Added logic for colored edges.

[Johannes Schindelin]

Hi,

On Mon, 30 Mar 2009, Allan Caffee wrote:

> Modified the graph drawing logic to colorize edges based on parent-child 
> relationships similiarly to gitk.
> 
> Signed-off-by: Allan Caffee <allan.caffee@gmail.com>

Nice!

> I havn't gotten the chance to do any of the color clean up that's been 
> discussed on this thread.  I'll try to throw something together in a 
> seperate patch series.
> 
> Also this patch isn't respecting the --no-color option which I imagine 
> means that diff_use_color_default isn't the right variable to be 
> checking.  Johannes mentioned using diff_use_color but the only instance 
> I see is a parameter to diff_get_color.  What am I missing?

The patch I sent you should work...

> diff --git a/graph.c b/graph.c
> index 162a516..2929c8b 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -72,11 +74,22 @@ struct column {
>  	 */
>  	struct commit *commit;
>  	/*
> -	 * XXX: Once we add support for colors, struct column could also
> -	 * contain the color of its branch line.
> +	 * The color to (optionally) print this column in.
>  	 */
> +	char *color;
>  };
>  
> +static void strbuf_write_column(struct strbuf *sb, const struct column *c,
> +		const char *s);
> +
> +static char* get_current_column_color (const struct git_graph* graph);
> +
> +/*
> + * Update the default column color and return the new value.
> + */
> +static char* get_next_column_color(struct git_graph* graph);
> +
> +

Please just insert the definitions here, instead of using a forward 
declaration.

> @@ -86,6 +99,24 @@ enum graph_state {
>  	GRAPH_COLLAPSING
>  };
>  
> +/*
> + * The list of available column colors.
> + */
> +static char column_colors[][COLOR_MAXLEN] = {
> +	GIT_COLOR_RED,
> +	GIT_COLOR_GREEN,
> +	GIT_COLOR_YELLOW,
> +	GIT_COLOR_BLUE,
> +	GIT_COLOR_MAGENTA,
> +	GIT_COLOR_CYAN,
> +	GIT_COLOR_BOLD GIT_COLOR_RED,
> +	GIT_COLOR_BOLD GIT_COLOR_GREEN,
> +	GIT_COLOR_BOLD GIT_COLOR_YELLOW,
> +	GIT_COLOR_BOLD GIT_COLOR_BLUE,
> +	GIT_COLOR_BOLD GIT_COLOR_MAGENTA,
> +	GIT_COLOR_BOLD GIT_COLOR_CYAN,
> +};
> +
>  struct git_graph {
>  	/*
>  	 * The commit currently being processed

I imagine that this is a good start.  Whether to make a patch that moves 
this into color.[ch] before or after this patch is up to Junio, I guess 
(even if I would prefer it to be done before, so that it gets done).

> @@ -317,6 +354,14 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
>  					  int *mapping_index)
>  {
>  	int i;
> +	char *color = get_current_column_color(graph);
> +
> +	for (i = 0; i < graph->num_columns; i++) {
> +		if (graph->columns[i].commit == commit) {
> +			color = graph->columns[i].color;
> +			break;
> +		}
> +	}

I imagine that this would be better done using a struct decorate mapping 
commits to the color strings.

Also, I'd only call get_current_column_color() if there was no color 
assigned to the commit (instead of calling it all the time).

It might not be a performance bottleneck here, but I guess it is better 
not to get used to that pattern anyway.

> @@ -334,6 +379,8 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
>  	 * This commit isn't already in new_columns.  Add it.
>  	 */
>  	graph->new_columns[graph->num_new_columns].commit = commit;
> +/*         fprintf(stderr,"adding the %scommit%s\n", color, GIT_COLOR_RESET); */

Please remove this line.

> @@ -649,7 +702,10 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
>  		struct column *col = &graph->columns[i];
>  		if (col->commit == graph->commit) {
>  			seen_this = 1;
> -			strbuf_addf(sb, "| %*s", graph->expansion_row, "");
> +			struct strbuf tmp = STRBUF_INIT;
> +			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
> +			strbuf_write_column(sb, col, tmp.buf);
> +			strbuf_release(&tmp);

Maybe it would be better to add functions

const char *column_color(struct column *c)
{
	return c->color ? c->color : "";
}

const char *column_color_reset(struct column *c)
{
	return c->color ? GIT_COLOR_RESET : "";
}

?

Sorry, I have to stop the review here, ran out of time...

If nobody beats me to it, I will continue here later.

Thanks!
Dscho

Re: [RFC/PATCH] graph API: Added logic for colored edges.

[Johannes Schindelin]

Hi,

On Mon, 30 Mar 2009, Allan Caffee wrote:

> +static void strbuf_write_column(struct strbuf *sb, const struct column *c,
> +		const char *s)
> +{
> +	/*
> +	 * TODO: I get the creeping suspicion that this isn't the
> +	 * right flag to be checking since --no-color doesn't turn
> +	 * this off.
> +	 */

Heh, of course I forgot to remove this with my to-be-squashed-in patch...

> +	if (diff_use_color_default)
> +		strbuf_addstr(sb, c->color);
> +	strbuf_addstr(sb, s);
> +	if (diff_use_color_default)
> +		strbuf_addstr(sb, GIT_COLOR_RESET);
> +}

How about this function instead?

static void strbuf_add_column(struct strbuf *sb,
	const struct column *column, const char *fmt, ...)
{
        va_list ap;

        va_start(ap, fmt);
	if (column->color)
		strbuf_addstr(sb, column->color);
        strbuf_vaddf(sb, fmt, ap);
	if (column->color)
		strbuf_addstr(sb, GIT_COLOR_RESET);
        va_end(ap);
}

Hmm?

Ciao,
Dscho

Re: [RFC/PATCH] graph API: Added logic for colored edges.

[Johannes Sixt]

Johannes Schindelin schrieb:
> How about this function instead?
> 
> static void strbuf_add_column(struct strbuf *sb,
> 	const struct column *column, const char *fmt, ...)
> {
>         va_list ap;
> 
>         va_start(ap, fmt);
> 	if (column->color)
> 		strbuf_addstr(sb, column->color);
>         strbuf_vaddf(sb, fmt, ap);
> 	if (column->color)
> 		strbuf_addstr(sb, GIT_COLOR_RESET);
>         va_end(ap);
> }
> 
> Hmm?

Except the strbuf_vaddf() is only in your private repository ;)

-- Hannes

Re: [RFC/PATCH] graph API: Added logic for colored edges.

[Johannes Schindelin]

Hi,

On Tue, 31 Mar 2009, Johannes Sixt wrote:

> Johannes Schindelin schrieb:
> > How about this function instead?
> > 
> > static void strbuf_add_column(struct strbuf *sb,
> > 	const struct column *column, const char *fmt, ...)
> > {
> >         va_list ap;
> > 
> >         va_start(ap, fmt);
> > 	if (column->color)
> > 		strbuf_addstr(sb, column->color);
> >         strbuf_vaddf(sb, fmt, ap);
> > 	if (column->color)
> > 		strbuf_addstr(sb, GIT_COLOR_RESET);
> >         va_end(ap);
> > }
> > 
> > Hmm?
> 
> Except the strbuf_vaddf() is only in your private repository ;)

LOL & schenkelklopf!

You're absolutely correct... I'm sorry,
Dscho

Re: [RFC/PATCH] graph API: Added logic for colored edges.

[Johannes Schindelin]

Hi,

On Mon, 30 Mar 2009, Allan Caffee wrote:

> @@ -445,6 +492,12 @@ static void graph_update_columns(struct git_graph *graph)
>  			for (parent = first_interesting_parent(graph);
>  			     parent;
>  			     parent = next_interesting_parent(graph, parent)) {
> +				/*
> +				 * If this is a merge increment the current
> +				 * color.
> +				 */
> +				if (graph->num_parents > 1)
> +					get_next_column_color(graph);
>  				graph_insert_into_new_columns(graph,
>  							      parent->item,
>  							      &mapping_idx);

Hmm.  I would have expected the color to be an argument to 
graph_insert_into_new_columns()...

Oh, and please forget about my stupid babbling about using struct 
decoration for colors: the column already knows commit and color, so if 
you need the color in a functino in addition to the commit, you should 
pass either the column struct instead, or the commit and the color as 
individual parameters.

This concludes my review ;-)

Thanks,
Dscho

Re: [RFC] Colorization of log --graph

[Junio C Hamano]

Allan Caffee <allan.caffee@gmail.com> writes:

> On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
>> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>>> I'd start like this:
>>>
>>>       enum color_name {
>>>               COLOR_RESET,
>>>               COLOR_RED,
>>>               COLOR_GREEN,
>>>               COLOR_YELLOW,
>>>               COLOR_BLUE,
>>>               COLOR_MAGENTA,
>>>               COLOR_CYAN,
>>>               COLOR_WHITE
>>>       };
>>
>> Looking for "COLOR_RED" in the archive gives:
>>
>>  http://article.gmane.org/gmane.comp.version-control.git/109676
>>
>
> Duly noted.  Perhaps those #defines should be relocated to color.h?

Heh, I did not even realize the above 109676 was referring to what I wrote
sometime ago.

> If we still wanted to provide a color_name type we could use
> GIT_COLOR_NAME_RESET et al.  That would give us something like:
>
> #define GIT_COLOR_NORMAL	""
> #define GIT_COLOR_RESET		"\033[m"
> #define GIT_COLOR_BOLD		"\033[1m"
> #define GIT_COLOR_RED		"\033[31m"
> #define GIT_COLOR_GREEN		"\033[32m"
> #define GIT_COLOR_YELLOW	"\033[33m"
> #define GIT_COLOR_BLUE		"\033[34m"
> #define GIT_COLOR_CYAN		"\033[36m"
> #define GIT_COLOR_BG_RED	"\033[41m"
>
> enum color_name {
> 	GIT_COLOR_NAME_NORMAL
> 	GIT_COLOR_NAME_RESET,
> 	GIT_COLOR_NAME_RED,
> 	GIT_COLOR_NAME_GREEN,
> 	GIT_COLOR_NAME_YELLOW,
> 	GIT_COLOR_NAME_BLUE,
> 	GIT_COLOR_NAME_MAGENTA,
> 	GIT_COLOR_NAME_CYAN,
> 	GIT_COLOR_NAME_WHITE
> 	GIT_COLOR_NAME_BG_RED
> };
>
> /*
>  * Map names to ANSI escape sequences.  Consider putting this in color.c
>  * and providing color_name_get_ansi_code(enum color_name).
>  */
> const char* git_color_codes[] {
> 	GIT_COLOR_RESET,
> 	GIT_COLOR_BOLD,
> 	GIT_COLOR_RED,
> 	GIT_COLOR_GREEN,
> 	GIT_COLOR_YELLOW,
> 	GIT_COLOR_BLUE,
> 	GIT_COLOR_CYAN,
> 	GIT_COLOR_BG_RED,
> };
>
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.

Is git_color_codes[GIT_COLOR_NAME_FOO] supposed to give you GIT_COLOR_FOO?

Are you consolidating various pieces of physical color definition to one
place?  That sounds sensible.

The corrent code does:

diff.c::
	user says "meta" is "purple"
        -> parse_diff_color_slot() says "meta" is slot 2
        -> git_diff_basic_config() asks color_parse() to place the ANSI
           representation of the "purple" in slot 2
	-> code uses diff_get_color() to grab "meta" color from the slot
           and sends it to the terminal

builtin-branch.c duplicates the exact same logic with a separate tables
and a set of slots.  builtin-grep.c cheats and does not give the end user
any customizability, which needs to be fixed.

The "slots" are defined in terms of what the color is used for, the
meaning, e.g. "a line from the file before the change (DIFF_FILE_OLD)"; we
cannot avoid having application specific set of slots, but the parsing
should be able to share the code.

Once the slot number is known, we ask color_parse() to put the final
physical string (suitable for the terminal's consumption) to fill the
slot.  But for that, I do not think git_color_codes[] nor GIT_COLOR_FOO
need to be exposed to the applications (i.e. "diff", "branch", "grep").
It is an implementation detail that color_parse() always uses ANSI escape
sequences right now, but we could encapsulate that in color.c and later
perhaps start looking up from the terminfo database, for example.

But that leaves the question of initialization.  I think it would give a
better abstraction if we changed the type of values stored in a
color-table like diff.c::diff_colors[] from physical string sent to the
terminal to a color name (your enum color_name).  Then the application
code can initialize their own color-table for each application-specific
slots with GIT_COLOR_NAME_RED, let the configuration mechanism to
customize it for the user.  The codepath that currently assume the color
table contains strings that can be sent to the terminal need to be
modified to ask color_code_to_terminal_string(GIT_COLOR_NAME_YELLOW) or
something.  Which means:

(1) Physical color representation should be known only to color.c.  I.e.

	#define GIT_COLOR_BOLD "\033[1m"

    does not belong to color.h (public header for application consumption)
    nor diff.c (application);

(2) Logical color name and the ways to convert it for terminal consumption
    belongs to color.h.  I.e.

	enum color_name {
        	GIT_COLOR_NAME_YELLOW,
                ...
	}

    should go to color.h;

    color_fprintf() should be changed to take "enum color_name color"
    instead of "const char *color";

    We would need strbuf variant for callers that prepare the string in
    core before giving it to fprintf().

(3) "static const char *git_color_codes[]" would be an implementation
    detail of the current "ANSI-only" one, hidden inside color.c, for
    color_fprintf() and its strbuf cousin to look at.

Re: [RFC] Colorization of log --graph

[Eric Raible]

Allan Caffee <allan.caffee <at> gmail.com> writes:

> 
> I know that _some_ people arn't particularly fond of colors, but I was
> wondering how difficult it would be to colorize the edges on the
> --graph drawn by the log command?  It can be a little tricky trying to
> follow them with a relatively complex history.  I was thinking something
> like gitk already does.  Is anybody else interested in seeing this?
> 
> ~Allan

This may be clueless (I suspect that it is) but I have never understood
the meaning of the different line colors in gitk.  They seems arbitrary to me.

I get that the current HEAD is represented as a yellow dot, but that's it.
(As an aside, it might be nice if merges had a different color dot than
normal commits).

Can anyone clue me in?

- Eric

Re: [RFC] Colorization of log --graph

[Santi Béjar]

2009/3/18 Eric Raible <raible+git@gmail.com>:
> This may be clueless (I suspect that it is) but I have never understood
> the meaning of the different line colors in gitk.  They seems arbitrary to me.
>
> I get that the current HEAD is represented as a yellow dot, but that's it.
> (As an aside, it might be nice if merges had a different color dot than
> normal commits).
>
> Can anyone clue me in?

Gitk paints lines of development (lineal history without merges nor
forks) with the same color.

HTH,
Santi

Re: [RFC] Colorization of log --graph

[Eric Raible]

On Wed, Mar 18, 2009 at 10:04 AM, Santi Béjar <santi@agolina.net> wrote:
> Gitk paints lines of development (lineal history without merges nor
> forks) with the same color.
>
> HTH,
> Santi

Thanks for the quick reply.  I suppose I realized that but it just
doesn't seem that profound.
Don't get me wrong - I like gitk and still prefer it to any of the alternatives.
But its of color seems more flashy than useful to me.

Perhaps I'd be happier if the color of the nth parent of a merge
(selectable, first by default)
emerged from the merge?  I dunno.

Re: [RFC] Colorization of log --graph

[Markus Heidelberg]

Eric Raible, 18.03.2009:
> On Wed, Mar 18, 2009 at 10:04 AM, Santi Béjar <santi@agolina.net> wrote:
> > Gitk paints lines of development (lineal history without merges nor
> > forks) with the same color.
> >
> > HTH,
> > Santi
> 
> Thanks for the quick reply.  I suppose I realized that but it just
> doesn't seem that profound.
> Don't get me wrong - I like gitk and still prefer it to any of the alternatives.
> But its of color seems more flashy than useful to me.

If scrolling through a history with many branches (so many parallel
lines) like git.git, colors help you to follow a particular line.

I don't find it easy to follow a line in a PCB, where you normally don't
have colors :)

Markus

Re: [RFC] Colorization of log --graph

[Eric Raible]

2009/3/19 Markus Heidelberg <markus.heidelberg@web.de>:
>
> If scrolling through a history with many branches (so many parallel
> lines) like git.git, colors help you to follow a particular line.
>
> I don't find it easy to follow a line in a PCB, where you normally don't
> have colors :)
>
> Markus

I find it easier to simply click on the line and then click the parent
in the lower window.  Which I'm only mentioning b.c. others might
not realize that it's possible.

The meta question is: anyone know of any gitk/git gui documentation
aside from Junio's excellent "Fun with msysgit 1.6.1 preview"
(http://gitster.livejournal.com/24080.html)?

Anything additional would be useful in my quest to get $dayjob to switch to git.

- Eric

Re: [RFC] Colorization of log --graph

[Markus Heidelberg]

Eric Raible, 19.03.2009:
> 2009/3/19 Markus Heidelberg <markus.heidelberg@web.de>:
> >
> > If scrolling through a history with many branches (so many parallel
> > lines) like git.git, colors help you to follow a particular line.
> >
> > I don't find it easy to follow a line in a PCB, where you normally don't
> > have colors :)
> >
> > Markus
> 
> I find it easier to simply click on the line and then click the parent
> in the lower window.  Which I'm only mentioning b.c. others might
> not realize that it's possible.

Oh, right. I know about it, but since I don't use gitk often, I didn't
think of it.

Markus