Re: [PATCH] graph API: Added logic for colored edges
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:34
Allan Caffee [off-list ref] writes:
quoted hunk
@@ -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.
quoted hunk
@@ -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)
quoted hunk
@@ -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 hunk
@@ -648,8 +713,11 @@ 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) { + struct strbuf tmp = STRBUF_INIT; seen_this = 1; - strbuf_addf(sb, "| %*s", graph->expansion_row, ""); + strbuf_addf(&tmp, "| %*s", graph->expansion_row, ""); + strbuf_write_column(sb, col, tmp.buf); + strbuf_release(&tmp);
Same here.
quoted hunk
@@ -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 - 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, "| "); } }
Likewise.
quoted hunk
@@ -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], + "-"); + 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?