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

[Junio C Hamano <gitster@pobox.com>]

Allan Caffee <allan.caffee@gmail.com> 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?