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@xxxxxxxxx>
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
--
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
