Allan Caffee <allan.caffee@xxxxxxxxx> writes:
> @@ -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.
> @@ -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)
> @@ -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?
> @@ -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.
> @@ -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.
> @@ -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?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html