graph API: Added logic for colored edges

graph.c +1 -1

@@ -662,13 +730,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

graph.c +7 -1

@@ -744,14 +813,25 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
 			else {
+				/*
+				 * Here dashless_commits represents the
+				 * number of parents which don't need
+				 * to have dashes (because their edges
+				 * fit neatly under the commit).
+				 */
+				const int dashless_commits = 2;
 				int num_dashes =
-					((graph->num_parents - 2) * 2) - 1;

graph.c struct column { · line 74

Quoted 9 lines · Allan Caffee

graph.c +2 -2

@@ -72,11 +69,21 @@ 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.
 	 */
+	const char *color;

You already use short for git_graph.default_column_color and I suspect in the longer term you want to make this one an index into column_colors[] array the same way. We may someday decide to support non-ANSI color scheme using ncurses or something, and at that point the "hardware color" constants like GIT_COLOR_RED and friends may change from strings to small integers we use to call our (yet to be written) curses interface layer with.

graph.c static struct commit_list *first_interesting_parent(struct git_graph *graph) · line 346

Quoted 5 lines

graph.c +1 -0

@@ -312,6 +343,33 @@ static struct commit_list *first_interesting_parent(struct git_graph *graph)
 	return next_interesting_parent(graph, parents);
 }
 
+static const char* graph_get_current_column_color(const struct git_graph* graph)

Style. Asterisk comes next to identifiers, not types (the parameter to graph_increment_column_color has the same issue).

static const char *graph_...color(const struct git_graph *graph)

graph.c static void graph_output_padding_line(struct git_graph *graph, · line 664

Quoted 6 lines

graph.c +1 -1

@@ -596,7 +661,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], "| ");

Hmmm, this forbids us to use reverse color in the color palette because that would highlight the trailing whitespace. Is that something we care about, or a reversed "|", "/" and "\" are already too ugly that we won't want to support them?

[...]

Quoted 19 lines

graph.c +10 -3

@@ -744,14 +813,25 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
 			else {
+				/*
+				 * Here dashless_commits represents the
+				 * number of parents which don't need
+				 * to have dashes (because their edges
+				 * fit neatly under the commit).
+				 */
+				const int dashless_commits = 2;
 				int num_dashes =
-					((graph->num_parents - 2) * 2) - 1;
+					((graph->num_parents - dashless_commits) * 2) - 1;
 				for (j = 0; j < num_dashes; j++)
-					strbuf_addch(sb, '-');
-				strbuf_addstr(sb, ". ");
+					strbuf_write_column(sb,
+							    &graph->new_columns[(j / 2) + dashless_commits],

graph.c static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) · line 831

Quoted 4 lines

graph.c +4 -0

+							    "-");
+				strbuf_write_column(sb,
+						    &graph->new_columns[(j / 2) + dashless_commits],
+						    ". ");

The nesting seems to be becoming too deep and the body of the for loop is getting too long. Time to make it a helper function that handles only one column, perhaps?

The problem with making it an index into the column_colors array is that we don't have a convenient place to test whether the user actually wants color. We can't do it in strbuf_write_column because AFAIK there's no way to get the rev-info to test the options. I suppose we could define GIT_NOT_A_COLOR to -1 and just set the color to that when we don't intend to use color. (Either way I should probably change that to an unsigned short.) What do you think?

graph.c +1 -0

index 162a516..beb622a 100644
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "commit.h"
+#include "color.h"
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"

@@ -72,11 +69,14 @@ struct column {
 	 */
 	struct commit *commit;

graph.c +5 -0

@@ -714,10 +790,30 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	strbuf_addch(sb, '*');
 }
 
+inline void graph_draw_octopus_merge(const struct git_graph *graph,
+				     struct strbuf *sb)
+{
+	/*
+	 * Here dashless_commits represents the number of parents

Quoted 23 lines · Allan Caffee

graph.c +4 -2

index 162a516..beb622a 100644
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "commit.h"
+#include "color.h"
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"

@@ -72,11 +69,14 @@ 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.  This is an
+	 * index into column_colors.
 	 */
+	unsigned short color;
 };
 

graph.c struct column { · line 78

Quoted 1 line

graph.c +1 -0

+const unsigned short GIT_NOT_A_COLOR = -1;

That (-1) is an unusual value for an *unsigned* short variable.

Quoted 9 lines

graph.c +5 -0

@@ -714,10 +790,30 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	strbuf_addch(sb, '*');
 }
 
+inline void graph_draw_octopus_merge(const struct git_graph *graph,
+				     struct strbuf *sb)
+{
+	/*
+	 * Here dashless_commits represents the number of parents

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 807

Quoted 10 lines

graph.c +10 -0

+	 * which don't need to have dashes (because their edges fit
+	 * neatly under the commit).
+	 */
+	const int dashless_commits = 2;
+	int col_num, i;
+	int num_dashes =
+		((graph->num_parents - dashless_commits) * 2) - 1;
+	for (i = 0; i < num_dashes; i++) {
+		col_num = (i / 2) + dashless_commits;
+		strbuf_write_column(sb, &graph->new_columns[col_num], '-');

graph.c: In function 'graph_draw_octopus_merge':
graph.c:807: error: 'strbuf_write_column' is static but used in inline function 'graph_draw_octopus_merge' which is not static graph.c:810: error: 'strbuf_write_column' is static but used in inline function 'graph_draw_octopus_merge' which is not static
make: *** [graph.o] Error 1

In general, I'd prefer people not to say "inline" unless (1) they know what they are doing, and (2) the code is really performance critical.

At least I do not think the colored commit graph is performance critical, especially a function that only deals with octopus merges.

Quoted 3 lines

Quoted 1 line

+const unsigned short GIT_NOT_A_COLOR = -1;

That (-1) is an unusual value for an *unsigned* short variable.

Perhaps you would prefer USHRT_MAX? I noticed that none of the existing code #includes limits.h. Is it safe to assume this header is present?

Quoted 8 lines · Allan Caffee

Quoted 3 lines

Quoted 1 line

+const unsigned short GIT_NOT_A_COLOR = -1;

That (-1) is an unusual value for an *unsigned* short variable.

Perhaps you would prefer USHRT_MAX? I noticed that none of the existing
code #includes limits.h. Is it safe to assume this header is present?

I expected to see something like

      #define COLUMN_COLORS_MAX (ARRAY_SIZE(column_colors)+1)

      write_with_color(...) {
                if (c->color < COLUMN_COLORS_MAX)
                        add color prefix;
                add string
                if (c->color < COLUMN_COLORS_MAX)
                        add color suffix;
      }

instead, actually, and was a bit surprised with (-1).

I assumed that the +1 in your example was a typo since AFAIKS ARRAY_SIZE should give us one past the last index.

Also if git is to be expanded allow the use of non-ANSI color codes (or already does so) the strbuf_escape_sequence_length needs to be updated to accept the relevant escape codes.

strbuf.c +9 -0

+/*
+ * Return the length of the escape sequence in a string buffer
+ * starting at index i.  If there is no escape sequence starting at
+ * return 0.
+ */
+size_t strbuf_esc_sequence_length(const struct strbuf *sb, size_t i)
+{
+	size_t start = i;
+	if (sb->buf[i] != '\033')

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

Quoted 2 lines · Allan Caffee

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

I like the colored graph very much, thanks. Unfortunately there are some problems with aligning of log messages and headers. For example, try this in git.git repository:

$ git log -1 --graph 796b137

* commit 796b13781aecce551b8f92049a66646e60f31dce

|\ Merge: 6da14ee db12d97
| | Author: Junio C Hamano <[email protected]>
| | Date:   2009-04-08 23:41:27 -0700

Without colors or without your patch the alignment is correct:

$ git log -1 --graph --no-color 796b137

* commit 796b13781aecce551b8f92049a66646e60f31dce

|\  Merge: 6da14ee db12d97
| | Author: Junio C Hamano <[email protected]>
| | Date:   2009-04-08 23:41:27 -0700

(Perhaps the "Merge:" header could have two spaces befor the data, but this is unrelated to --graph.)