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

[Allan Caffee <allan.caffee@gmail.com>]

On Sun, 12 Apr 2009, Junio C Hamano wrote:

> Allan Caffee <allan.caffee@gmail.com> writes:
>
> > diff --git a/graph.c b/graph.c
> > index 162a516..beb622a 100644
> > --- a/graph.c
> > +++ b/graph.c
> > @@ -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;
> >  };
> >
> > +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?

> > @@ -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
> > +	 * 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.

Okay.  I'll remove the inline specifier from all the functions I added.