threads / rfc / 18370

[RFC] Colorization of log --graph

Subject: [RFC] Colorization of log --graph

## tl;dr

24 messages between Mar 18, 2009 and Mar 31, 2009.

replies: 23people: 10as markdown or json

Allan Caffee· Mar 18, 2009, 10:05 UTC · lore

I know that _some_ people arn't particularly fond of colors, but I was wondering how difficult it would be to colorize the edges on the --graph drawn by the log command? It can be a little tricky trying to follow them with a relatively complex history. I was thinking something like gitk already does. Is anybody else interested in seeing this?

~Allan
Johannes Schindelin· Mar 18, 2009, 11:44 UTC · re: Allan Caffee · lore

Re: [RFC] Colorization of log --graph

Hi,
On Wed, 18 Mar 2009, Allan Caffee wrote:
Show 5 quoted lines
> I know that _some_ people arn't particularly fond of colors, but I was 
> wondering how difficult it would be to colorize the edges on the --graph 
> drawn by the log command?  It can be a little tricky trying to follow 
> them with a relatively complex history.  I was thinking something like 
> gitk already does.
That's a good idea!  (And it is mentioned as a TODO in graph.c...)
> Is anybody else interested in seeing this?
Count me in.  Are you interested in implementing this?
If so:
- you need to #include "color.h" in graph.c
- you need to insert a color identifier into struct column (there is an 
  XXX comment at the correct location)
- you need to find a way to determine colors for the branches
- you need to put the handling into the function 
  graph_output_pre_commit_line() in graph.c (and probably 
  graph_output_commit_char(), graph_output_post_merge_line(), 
  graph_output_collapsing_line(), graph_padding_line(), and
  graph_output_padding_line(), too)
- it would make sense IMHO to introduce a new function that takes a 
  pointer to an strbuf, a pointer to a struct column and a char (or maybe 
  a string) that adds the appropriately colorized char (or string) to the 
  strbuf
- use the global variable diff_use_color to determine if the output should 
  be colorized at all
- probably you need to make an array of available colors or some such 
  (which might be good to put into color.[ch])

Ciao, Dscho

Allan Caffee· Mar 19, 2009, 16:59 UTC · re: Johannes Schindelin · lore

Re: [RFC] Colorization of log --graph

Hello and thanks for the speedy reply!

On Wed, Mar 18, 2009 at 7:44 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 11 quoted lines
> Hi,
>
> On Wed, 18 Mar 2009, Allan Caffee wrote:
>
> > I know that _some_ people arn't particularly fond of colors, but I was
> > wondering how difficult it would be to colorize the edges on the --graph
> > drawn by the log command?  It can be a little tricky trying to follow
> > them with a relatively complex history.  I was thinking something like
> > gitk already does.
>
> That's a good idea!  (And it is mentioned as a TODO in graph.c...)
That's me, always thinking outside the box.  ;-)
> > Is anybody else interested in seeing this?
>
> Count me in.  Are you interested in implementing this?

I'll give it a go. Been a while since I've done anything of substance in pure C so it should be a nice refresher. :)

Show 6 quoted lines
> If so:
>
> - you need to #include "color.h" in graph.c
>
> - you need to insert a color identifier into struct column (there is an
>  XXX comment at the correct location)

By color identifier I assume you mean the ANSI escape sequence, right? I didn't see a type for representing colors in color.{c,h} other than the int it seems to use internally.

> - you need to find a way to determine colors for the branches
Okay, so if we were to make this similiar to how gitk works it would involve:
If the previous commit was a merge:
	for (i = 0; i < graph->num_columns; i++)
		graph->columns[i]->color = get_next_column_color();
else
	get_current_column_color();

I was thinking of storing the current color by adding a default_column_color attribute to git_graph that serves as an index into column_colors. column_colors being the array of available colors.

Show 10 quoted lines
> - you need to put the handling into the function
>  graph_output_pre_commit_line() in graph.c (and probably
>  graph_output_commit_char(), graph_output_post_merge_line(),
>  graph_output_collapsing_line(), graph_padding_line(), and
>  graph_output_padding_line(), too)
>
> - it would make sense IMHO to introduce a new function that takes a
>  pointer to an strbuf, a pointer to a struct column and a char (or maybe
>  a string) that adds the appropriately colorized char (or string) to the
>  strbuf

That makes sense. Then we can just update the functions you mentioned above to use this.

> - use the global variable diff_use_color to determine if the output should
>  be colorized at all

The function for adding a column to an strbuf would offer a convenient place to put the condition.

> - probably you need to make an array of available colors or some such
>  (which might be good to put into color.[ch])

This would be the color_codes array I mentioned but it seems like it might belong in graph.c. There's something similiar in diff.c and it seems like this is more related to graphing then to colors in general. Although I do think it makes sense to #define some of the more common ANSI codes there so that they don't have to be duplicated. grep shows 6 occurrences of '\033[31m', the code for red foreground.

I'll begin working on a patch.  Comments/questions?
~Allan
Johannes Schindelin· Mar 19, 2009, 17:41 UTC · re: Allan Caffee · lore

Re: [RFC] Colorization of log --graph

Hi,
On Thu, 19 Mar 2009, Allan Caffee wrote:
> Hello and thanks for the speedy reply!
Heh, Git is known for raw speed ;-)
Show 11 quoted lines
> On Wed, Mar 18, 2009 at 7:44 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Wed, 18 Mar 2009, Allan Caffee wrote:
> >
> > > Is anybody else interested in seeing this?
> >
> > Count me in.  Are you interested in implementing this?
> 
> I'll give it a go.  Been a while since I've done anything of substance 
> in pure C so it should be a nice refresher.  :)
Great!
Show 10 quoted lines
> > If so:
> >
> > - you need to #include "color.h" in graph.c
> >
> > - you need to insert a color identifier into struct column (there is an
> >  XXX comment at the correct location)
> 
> By color identifier I assume you mean the ANSI escape sequence, right? I 
> didn't see a type for representing colors in color.{c,h} other than the 
> int it seems to use internally.
I'd actually add an enum color_names, or something like that.
Show 12 quoted lines
> > - you need to find a way to determine colors for the branches
> 
> Okay, so if we were to make this similiar to how gitk works it would 
> involve: If the previous commit was a merge:
> 	for (i = 0; i < graph->num_columns; i++)
> 		graph->columns[i]->color = get_next_column_color();
> else
> 	get_current_column_color();
> 
> I was thinking of storing the current color by adding a 
> default_column_color attribute to git_graph that serves as an index into 
> column_colors.  column_colors being the array of available colors.

Yep, I agree. That index could be of type "enum color_names" if you introduce the latter...

Show 13 quoted lines
> > - you need to put the handling into the function 
> >   graph_output_pre_commit_line() in graph.c (and probably 
> >   graph_output_commit_char(), graph_output_post_merge_line(), 
> >   graph_output_collapsing_line(), graph_padding_line(), and 
> >   graph_output_padding_line(), too)
> >
> > - it would make sense IMHO to introduce a new function that takes a 
> >   pointer to an strbuf, a pointer to a struct column and a char (or 
> >   maybe a string) that adds the appropriately colorized char (or 
> >   string) to the strbuf
> 
> That makes sense.  Then we can just update the functions you mentioned
> above to use this.
Right.
Show 5 quoted lines
> > - use the global variable diff_use_color to determine if the output 
> >   should be colorized at all
> 
> The function for adding a column to an strbuf would offer a convenient 
> place to put the condition.
Yes!
Show 9 quoted lines
> > - probably you need to make an array of available colors or some such 
> >   (which might be good to put into color.[ch])
> 
> This would be the color_codes array I mentioned but it seems like it
> might belong in graph.c.  There's something similiar in diff.c and it
> seems like this is more related to graphing then to colors in general.
> Although I do think it makes sense to #define some of the more common
> ANSI codes there so that they don't have to be duplicated.  grep shows 6
> occurrences of '\033[31m', the code for red foreground.

I'd actually like to see it in color.[ch], so that other code paths can use it, too.

I'd start like this:
	enum color_name {
		COLOR_RESET,
		COLOR_RED,
		COLOR_GREEN,
		COLOR_YELLOW,
		COLOR_BLUE,
		COLOR_MAGENTA,
		COLOR_CYAN,
		COLOR_WHITE
	};
Maybe the best thing would then be to add a function
	void strbuf_add_color(struct strbuf *buf, enum color_name name) {
		if (name == COLOR_RESET)
			strbuf_addf(buf, "\033[m");
		else
			strbuf_addf(buf, "\033[%dm", 31 + name - COLOR_RED);
	}

Ciao, Dscho

Nanako Shiraishi· Mar 19, 2009, 21:48 UTC · re: Johannes Schindelin · lore

Re: [RFC] Colorization of log --graph

Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
Show 12 quoted lines
> I'd start like this:
>
> 	enum color_name {
> 		COLOR_RESET,
> 		COLOR_RED,
> 		COLOR_GREEN,
> 		COLOR_YELLOW,
> 		COLOR_BLUE,
> 		COLOR_MAGENTA,
> 		COLOR_CYAN,
> 		COLOR_WHITE
> 	};
Looking for "COLOR_RED" in the archive gives:
  http://article.gmane.org/gmane.comp.version-control.git/109676
-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
Allan Caffee· Mar 20, 2009, 19:13 UTC · re: Nanako Shiraishi · lore

Re: [RFC] Colorization of log --graph

On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
Show 19 quoted lines
> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
>> I'd start like this:
>>
>>       enum color_name {
>>               COLOR_RESET,
>>               COLOR_RED,
>>               COLOR_GREEN,
>>               COLOR_YELLOW,
>>               COLOR_BLUE,
>>               COLOR_MAGENTA,
>>               COLOR_CYAN,
>>               COLOR_WHITE
>>       };
>
> Looking for "COLOR_RED" in the archive gives:
>
>  http://article.gmane.org/gmane.comp.version-control.git/109676
>

Duly noted. Perhaps those #defines should be relocated to color.h? If we still wanted to provide a color_name type we could use GIT_COLOR_NAME_RESET et al. That would give us something like:

#define GIT_COLOR_NORMAL "" #define GIT_COLOR_RESET "\033[m" #define GIT_COLOR_BOLD "\033[1m" #define GIT_COLOR_RED "\033[31m" #define GIT_COLOR_GREEN "\033[32m" #define GIT_COLOR_YELLOW "\033[33m" #define GIT_COLOR_BLUE "\033[34m" #define GIT_COLOR_CYAN "\033[36m" #define GIT_COLOR_BG_RED "\033[41m"

enum color_name {
	GIT_COLOR_NAME_NORMAL
	GIT_COLOR_NAME_RESET,
	GIT_COLOR_NAME_RED,
	GIT_COLOR_NAME_GREEN,
	GIT_COLOR_NAME_YELLOW,
	GIT_COLOR_NAME_BLUE,
	GIT_COLOR_NAME_MAGENTA,
	GIT_COLOR_NAME_CYAN,
	GIT_COLOR_NAME_WHITE
	GIT_COLOR_NAME_BG_RED
};
/*
 * Map names to ANSI escape sequences.  Consider putting this in color.c
 * and providing color_name_get_ansi_code(enum color_name).
 */
const char* git_color_codes[] {
	GIT_COLOR_RESET,
	GIT_COLOR_BOLD,
	GIT_COLOR_RED,
	GIT_COLOR_GREEN,
	GIT_COLOR_YELLOW,
	GIT_COLOR_BLUE,
	GIT_COLOR_CYAN,
	GIT_COLOR_BG_RED,
};

That conveniently offers clients access to both the raw escape codes and a clear type for storing/handling colors.

~Allan
Jeff King· Mar 20, 2009, 19:58 UTC · re: Allan Caffee · lore

Re: [RFC] Colorization of log --graph

On Fri, Mar 20, 2009 at 03:13:53PM -0400, Allan Caffee wrote:
Show 17 quoted lines
> /*
>  * Map names to ANSI escape sequences.  Consider putting this in color.c
>  * and providing color_name_get_ansi_code(enum color_name).
>  */
> const char* git_color_codes[] {
> 	GIT_COLOR_RESET,
> 	GIT_COLOR_BOLD,
> 	GIT_COLOR_RED,
> 	GIT_COLOR_GREEN,
> 	GIT_COLOR_YELLOW,
> 	GIT_COLOR_BLUE,
> 	GIT_COLOR_CYAN,
> 	GIT_COLOR_BG_RED,
> };
> 
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.

I want to point out one thing: an enum or a list like this is actually a subset of the useful color codes that git can represent. Actual configured colors can have attributes, foreground, and background colors. So they need to be stored in a character array.

Adding an enum for GIT_COLOR_RED and using it throughout the code can be helpful for simple cases, but it doesn't give you an easy way of saying "red background, blue foreground". Maybe that is enough for git internal usage, since we tend not to use backgrounds or attributes for defaults. But maybe it makes more sense to do this as:

  const char *ansi_color(enum color fg, enum color bg, enum attribute attr);

and return a pointer to a static array representing the color (and even cycle through a list the way sha1_to_hex or git_path does). And you could even use it to simplify and share code with the config color parsing in color.c.

-Peff
Junio C Hamano· Mar 20, 2009, 20:13 UTC · re: Allan Caffee · lore

Re: [RFC] Colorization of log --graph

Allan Caffee <allan.caffee@gmail.com> writes:
Show 22 quoted lines
> On Thu, Mar 19, 2009 at 5:48 PM, Nanako Shiraishi <nanako3@lavabit.com> wrote:
>> Quoting Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>>> I'd start like this:
>>>
>>>       enum color_name {
>>>               COLOR_RESET,
>>>               COLOR_RED,
>>>               COLOR_GREEN,
>>>               COLOR_YELLOW,
>>>               COLOR_BLUE,
>>>               COLOR_MAGENTA,
>>>               COLOR_CYAN,
>>>               COLOR_WHITE
>>>       };
>>
>> Looking for "COLOR_RED" in the archive gives:
>>
>>  http://article.gmane.org/gmane.comp.version-control.git/109676
>>
>
> Duly noted.  Perhaps those #defines should be relocated to color.h?

Heh, I did not even realize the above 109676 was referring to what I wrote sometime ago.

Show 43 quoted lines
> If we still wanted to provide a color_name type we could use
> GIT_COLOR_NAME_RESET et al.  That would give us something like:
>
> #define GIT_COLOR_NORMAL	""
> #define GIT_COLOR_RESET		"\033[m"
> #define GIT_COLOR_BOLD		"\033[1m"
> #define GIT_COLOR_RED		"\033[31m"
> #define GIT_COLOR_GREEN		"\033[32m"
> #define GIT_COLOR_YELLOW	"\033[33m"
> #define GIT_COLOR_BLUE		"\033[34m"
> #define GIT_COLOR_CYAN		"\033[36m"
> #define GIT_COLOR_BG_RED	"\033[41m"
>
> enum color_name {
> 	GIT_COLOR_NAME_NORMAL
> 	GIT_COLOR_NAME_RESET,
> 	GIT_COLOR_NAME_RED,
> 	GIT_COLOR_NAME_GREEN,
> 	GIT_COLOR_NAME_YELLOW,
> 	GIT_COLOR_NAME_BLUE,
> 	GIT_COLOR_NAME_MAGENTA,
> 	GIT_COLOR_NAME_CYAN,
> 	GIT_COLOR_NAME_WHITE
> 	GIT_COLOR_NAME_BG_RED
> };
>
> /*
>  * Map names to ANSI escape sequences.  Consider putting this in color.c
>  * and providing color_name_get_ansi_code(enum color_name).
>  */
> const char* git_color_codes[] {
> 	GIT_COLOR_RESET,
> 	GIT_COLOR_BOLD,
> 	GIT_COLOR_RED,
> 	GIT_COLOR_GREEN,
> 	GIT_COLOR_YELLOW,
> 	GIT_COLOR_BLUE,
> 	GIT_COLOR_CYAN,
> 	GIT_COLOR_BG_RED,
> };
>
> That conveniently offers clients access to both the raw escape codes and
> a clear type for storing/handling colors.
Is git_color_codes[GIT_COLOR_NAME_FOO] supposed to give you GIT_COLOR_FOO?

Are you consolidating various pieces of physical color definition to one place? That sounds sensible.

The corrent code does:
diff.c::
	user says "meta" is "purple"
        -> parse_diff_color_slot() says "meta" is slot 2
        -> git_diff_basic_config() asks color_parse() to place the ANSI
           representation of the "purple" in slot 2
	-> code uses diff_get_color() to grab "meta" color from the slot
           and sends it to the terminal

builtin-branch.c duplicates the exact same logic with a separate tables and a set of slots. builtin-grep.c cheats and does not give the end user any customizability, which needs to be fixed.

The "slots" are defined in terms of what the color is used for, the meaning, e.g. "a line from the file before the change (DIFF_FILE_OLD)"; we cannot avoid having application specific set of slots, but the parsing should be able to share the code.

Once the slot number is known, we ask color_parse() to put the final physical string (suitable for the terminal's consumption) to fill the slot. But for that, I do not think git_color_codes[] nor GIT_COLOR_FOO need to be exposed to the applications (i.e. "diff", "branch", "grep"). It is an implementation detail that color_parse() always uses ANSI escape sequences right now, but we could encapsulate that in color.c and later perhaps start looking up from the terminfo database, for example.

But that leaves the question of initialization. I think it would give a better abstraction if we changed the type of values stored in a color-table like diff.c::diff_colors[] from physical string sent to the terminal to a color name (your enum color_name). Then the application code can initialize their own color-table for each application-specific slots with GIT_COLOR_NAME_RED, let the configuration mechanism to customize it for the user. The codepath that currently assume the color table contains strings that can be sent to the terminal need to be modified to ask color_code_to_terminal_string(GIT_COLOR_NAME_YELLOW) or something. Which means:

(1) Physical color representation should be known only to color.c.  I.e.
	#define GIT_COLOR_BOLD "\033[1m"
    does not belong to color.h (public header for application consumption)
    nor diff.c (application);
(2) Logical color name and the ways to convert it for terminal consumption
    belongs to color.h.  I.e.
	enum color_name {
        	GIT_COLOR_NAME_YELLOW,
                ...
	}
    should go to color.h;
    color_fprintf() should be changed to take "enum color_name color"
    instead of "const char *color";
    We would need strbuf variant for callers that prepare the string in
    core before giving it to fprintf().
(3) "static const char *git_color_codes[]" would be an implementation
    detail of the current "ANSI-only" one, hidden inside color.c, for
    color_fprintf() and its strbuf cousin to look at.
Eric Raible· Mar 18, 2009, 16:52 UTC · re: Allan Caffee · lore

Re: [RFC] Colorization of log --graph

Allan Caffee <allan.caffee <at> gmail.com> writes:
Show 8 quoted lines
> 
> I know that _some_ people arn't particularly fond of colors, but I was
> wondering how difficult it would be to colorize the edges on the
> --graph drawn by the log command?  It can be a little tricky trying to
> follow them with a relatively complex history.  I was thinking something
> like gitk already does.  Is anybody else interested in seeing this?
> 
> ~Allan

This may be clueless (I suspect that it is) but I have never understood the meaning of the different line colors in gitk. They seems arbitrary to me.

I get that the current HEAD is represented as a yellow dot, but that's it. (As an aside, it might be nice if merges had a different color dot than normal commits).

Can anyone clue me in?
- Eric
Santi Béjar· Mar 18, 2009, 17:04 UTC · re: Eric Raible · lore

Re: [RFC] Colorization of log --graph

2009/3/18 Eric Raible <raible+git@gmail.com>:
Show 8 quoted lines
> This may be clueless (I suspect that it is) but I have never understood
> the meaning of the different line colors in gitk.  They seems arbitrary to me.
>
> I get that the current HEAD is represented as a yellow dot, but that's it.
> (As an aside, it might be nice if merges had a different color dot than
> normal commits).
>
> Can anyone clue me in?

Gitk paints lines of development (lineal history without merges nor forks) with the same color.

HTH, Santi

Eric Raible· Mar 18, 2009, 17:29 UTC · re: Santi Béjar · lore

Re: [RFC] Colorization of log --graph

On Wed, Mar 18, 2009 at 10:04 AM, Santi Béjar <santi@agolina.net> wrote:
Show 5 quoted lines
> Gitk paints lines of development (lineal history without merges nor
> forks) with the same color.
>
> HTH,
> Santi

Thanks for the quick reply. I suppose I realized that but it just doesn't seem that profound. Don't get me wrong - I like gitk and still prefer it to any of the alternatives. But its of color seems more flashy than useful to me.

Perhaps I'd be happier if the color of the nth parent of a merge (selectable, first by default) emerged from the merge? I dunno.

Markus Heidelberg· Mar 19, 2009, 19:32 UTC · re: Eric Raible · lore

Re: [RFC] Colorization of log --graph

Eric Raible, 18.03.2009:
Show 11 quoted lines
> On Wed, Mar 18, 2009 at 10:04 AM, Santi Béjar <santi@agolina.net> wrote:
> > Gitk paints lines of development (lineal history without merges nor
> > forks) with the same color.
> >
> > HTH,
> > Santi
> 
> Thanks for the quick reply.  I suppose I realized that but it just
> doesn't seem that profound.
> Don't get me wrong - I like gitk and still prefer it to any of the alternatives.
> But its of color seems more flashy than useful to me.

If scrolling through a history with many branches (so many parallel lines) like git.git, colors help you to follow a particular line.

I don't find it easy to follow a line in a PCB, where you normally don't have colors :)

Markus
Eric Raible· Mar 19, 2009, 19:52 UTC · re: Markus Heidelberg · lore

Re: [RFC] Colorization of log --graph

2009/3/19 Markus Heidelberg <markus.heidelberg@web.de>:
Show 8 quoted lines
>
> If scrolling through a history with many branches (so many parallel
> lines) like git.git, colors help you to follow a particular line.
>
> I don't find it easy to follow a line in a PCB, where you normally don't
> have colors :)
>
> Markus

I find it easier to simply click on the line and then click the parent in the lower window. Which I'm only mentioning b.c. others might not realize that it's possible.

The meta question is: anyone know of any gitk/git gui documentation aside from Junio's excellent "Fun with msysgit 1.6.1 preview" (http://gitster.livejournal.com/24080.html)?

Anything additional would be useful in my quest to get $dayjob to switch to git.
- Eric
Markus Heidelberg· Mar 19, 2009, 20:04 UTC · re: Eric Raible · lore

Re: [RFC] Colorization of log --graph

Eric Raible, 19.03.2009:
Show 13 quoted lines
> 2009/3/19 Markus Heidelberg <markus.heidelberg@web.de>:
> >
> > If scrolling through a history with many branches (so many parallel
> > lines) like git.git, colors help you to follow a particular line.
> >
> > I don't find it easy to follow a line in a PCB, where you normally don't
> > have colors :)
> >
> > Markus
> 
> I find it easier to simply click on the line and then click the parent
> in the lower window.  Which I'm only mentioning b.c. others might
> not realize that it's possible.

Oh, right. I know about it, but since I don't use gitk often, I didn't think of it.

Markus
Allan Caffee· Mar 30, 2009, 14:13 UTC · lore

[RFC/PATCH] graph API: Added logic for colored edges.

Modified the graph drawing logic to colorize edges based on parent-child relationships similiarly to gitk.

Signed-off-by: Allan Caffee <allan.caffee@gmail.com>
---

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?

~Allan
 color.h |    1 +
 graph.c |  167 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 144 insertions(+), 24 deletions(-)
diff --git a/color.h b/color.h
index 6846be1..18abeb7 100644
--- a/color.h
+++ b/color.h
@@ -11,6 +11,7 @@
 #define GIT_COLOR_GREEN		"\033[32m"
 #define GIT_COLOR_YELLOW	"\033[33m"
 #define GIT_COLOR_BLUE		"\033[34m"
+#define GIT_COLOR_MAGENTA	"\033[35m"
 #define GIT_COLOR_CYAN		"\033[36m"
 #define GIT_COLOR_BG_RED	"\033[41m"
 
diff --git a/graph.c b/graph.c
index 162a516..2929c8b 100644
--- a/graph.c
+++ b/graph.c
@@ -1,9 +1,11 @@
 #include "cache.h"
 #include "commit.h"
+#include "color.h"
 #include "graph.h"
 #include "diff.h"
 #include "revision.h"
 
+extern int diff_use_color_default;
 /* Internal API */
 
 /*
@@ -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);
+
+
 enum graph_state {
 	GRAPH_PADDING,
 	GRAPH_SKIP,
@@ -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
@@ -185,6 +216,11 @@ struct git_graph {
 	 * temporary array each time we have to output a collapsing line.
 	 */
 	int *new_mapping;
+	/*
+	 * The current default column color being used.  This is
+	 * stored as an index into the array column_colors.
+	 */
+	short default_column_color;
 };
 
 struct git_graph *graph_init(struct rev_info *opt)
@@ -201,6 +237,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	graph->num_columns = 0;
 	graph->num_new_columns = 0;
 	graph->mapping_size = 0;
+	graph->default_column_color = 0;
 
 	/*
 	 * Allocate a reasonably large default number of columns
@@ -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;
+		}
+	}
 
 	/*
 	 * If the commit is already in the new_columns list, we don't need to
@@ -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); */
+	graph->new_columns[graph->num_new_columns].color = color;
 	graph->mapping[*mapping_index] = graph->num_new_columns;
 	*mapping_index += 2;
 	graph->num_new_columns++;
@@ -445,6 +492,12 @@ static void graph_update_columns(struct git_graph *graph)
 			for (parent = first_interesting_parent(graph);
 			     parent;
 			     parent = next_interesting_parent(graph, parent)) {
+				/*
+				 * If this is a merge increment the current
+				 * color.
+				 */
+				if (graph->num_parents > 1)
+					get_next_column_color(graph);
 				graph_insert_into_new_columns(graph,
 							      parent->item,
 							      &mapping_idx);
@@ -596,7 +649,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], "| ");
 	}
 
 	graph_pad_horizontally(graph, sb);
@@ -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);
 		} else if (seen_this && (graph->expansion_row == 0)) {
 			/*
 			 * This is the first line of the pre-commit output.
@@ -662,13 +718,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, "| ");
 		}
 	}
 
@@ -728,6 +784,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 	 */
 	seen_this = 0;
 	for (i = 0; i <= graph->num_columns; i++) {
+		struct column *col = &graph->columns[i];
 		struct commit *col_commit;
 		if (i == graph->num_columns) {
 			if (seen_this)
@@ -751,7 +808,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 				strbuf_addstr(sb, ". ");
 			}
 		} else if (seen_this && (graph->num_parents > 2)) {
-			strbuf_addstr(sb, "\\ ");
+			strbuf_write_column(sb, col, "\\ ");
 		} else if (seen_this && (graph->num_parents == 2)) {
 			/*
 			 * This is a 2-way merge commit.
@@ -768,11 +825,11 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			 */
 			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 {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -789,6 +846,17 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
+inline struct column* find_new_column_by_commit(struct git_graph *graph,
+						struct commit *commit)
+{
+	int i;
+	for (i = 0; i < graph->num_new_columns; i++) {
+		if (graph->new_columns[i].commit == commit)
+			return &graph->new_columns[i];
+	}
+	return 0;
+}
+
 static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 {
 	int seen_this = 0;
@@ -798,24 +866,43 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
 	 * Output the post-merge row
 	 */
 	for (i = 0; i <= graph->num_columns; i++) {
+		struct column *col = &graph->columns[i];
 		struct commit *col_commit;
 		if (i == graph->num_columns) {
 			if (seen_this)
 				break;
 			col_commit = graph->commit;
 		} else {
-			col_commit = graph->columns[i].commit;
+			col_commit = col->commit;
 		}
 
 		if (col_commit == graph->commit) {
+			/*
+			 * Since the current commit is a merge find
+			 * the columns for the parent commits in
+			 * new_columns and use those to format the
+			 * edges.
+			 */
+			struct commit_list *parents = NULL;
+			struct column *par_column;
 			seen_this = 1;
-			strbuf_addch(sb, '|');
-			for (j = 0; j < graph->num_parents - 1; j++)
-				strbuf_addstr(sb, "\\ ");
+			parents = first_interesting_parent(graph);
+			assert(parents);
+			par_column = find_new_column_by_commit(graph,parents->item);
+			assert(par_column);
+
+			strbuf_write_column(sb, par_column, "|");
+			for (j = 0; j < graph->num_parents - 1; j++) {
+				parents = next_interesting_parent(graph, parents);
+				assert(parents);
+				par_column = find_new_column_by_commit(graph,parents->item);
+				assert(par_column);
+				strbuf_write_column(sb, par_column, "\\ ");
+			}
 		} else if (seen_this) {
-			strbuf_addstr(sb, "\\ ");
+			strbuf_write_column(sb, col, "\\ ");
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -834,6 +921,8 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 {
 	int i;
 	int *tmp_mapping;
+	static int collapsing_columns[255];
+	int collapsing_seen_so_far = 0;
 
 	/*
 	 * Clear out the new_mapping array
@@ -912,9 +1001,11 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 		if (target < 0)
 			strbuf_addch(sb, ' ');
 		else if (target * 2 == i)
-			strbuf_addch(sb, '|');
-		else
-			strbuf_addch(sb, '/');
+			strbuf_write_column(sb, &graph->new_columns[target], "|");
+		else {
+			strbuf_write_column(sb, &graph->new_columns[target], "/");
+
+		}
 	}
 
 	graph_pad_horizontally(graph, sb);
@@ -979,9 +1070,10 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 	 * children that we have already processed.)
 	 */
 	for (i = 0; i < graph->num_columns; i++) {
-		struct commit *col_commit = graph->columns[i].commit;
+		struct column *col = &graph->columns[i];
+		struct commit *col_commit = col->commit;
 		if (col_commit == graph->commit) {
-			strbuf_addch(sb, '|');
+			strbuf_write_column(sb, col, "|");
 
 			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
@@ -991,7 +1083,7 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 					strbuf_addch(sb, ' ');
 			}
 		} else {
-			strbuf_addstr(sb, "| ");
+			strbuf_write_column(sb, col, "| ");
 		}
 	}
 
@@ -1154,3 +1246,30 @@ void graph_show_commit_msg(struct git_graph *graph,
 			putchar('\n');
 	}
 }
+
+static void strbuf_write_column(struct strbuf *sb, const struct column *c,
+		const char *s)
+{
+	/*
+	 * TODO: I get the creeping suspicion that this isn't the
+	 * right flag to be checking since --no-color doesn't turn
+	 * this off.
+	 */
+	if (diff_use_color_default)
+		strbuf_addstr(sb, c->color);
+	strbuf_addstr(sb, s);
+	if (diff_use_color_default)
+		strbuf_addstr(sb, GIT_COLOR_RESET);
+}
+
+static char* get_current_column_color (const struct git_graph* graph)
+{
+	return column_colors[graph->default_column_color];
+}
+
+static char* get_next_column_color(struct git_graph* graph)
+{
+	graph->default_column_color = (graph->default_column_color + 1) %
+		ARRAY_SIZE(column_colors);
+	return (get_current_column_color(graph));
+}

-- 
1.5.4.3
Johannes Schindelin· Mar 30, 2009, 16:04 UTC · re: Allan Caffee · lore

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

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@gmail.com>
Nice!
Show 8 quoted lines
> 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...
Show 26 quoted lines
> 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.

Show 25 quoted lines
> @@ -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).

Show 12 quoted lines
> @@ -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.

Show 5 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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

Johannes Schindelin· Mar 31, 2009, 10:13 UTC · re: Allan Caffee · lore

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

Hi,
On Mon, 30 Mar 2009, Allan Caffee wrote:
Show 8 quoted lines
> +static void strbuf_write_column(struct strbuf *sb, const struct column *c,
> +		const char *s)
> +{
> +	/*
> +	 * TODO: I get the creeping suspicion that this isn't the
> +	 * right flag to be checking since --no-color doesn't turn
> +	 * this off.
> +	 */
Heh, of course I forgot to remove this with my to-be-squashed-in patch...
Show 6 quoted lines
> +	if (diff_use_color_default)
> +		strbuf_addstr(sb, c->color);
> +	strbuf_addstr(sb, s);
> +	if (diff_use_color_default)
> +		strbuf_addstr(sb, GIT_COLOR_RESET);
> +}
How about this function instead?
static void strbuf_add_column(struct strbuf *sb,
	const struct column *column, const char *fmt, ...)
{
        va_list ap;
        va_start(ap, fmt);
	if (column->color)
		strbuf_addstr(sb, column->color);
        strbuf_vaddf(sb, fmt, ap);
	if (column->color)
		strbuf_addstr(sb, GIT_COLOR_RESET);
        va_end(ap);
}
Hmm?

Ciao, Dscho

Johannes Sixt· Mar 31, 2009, 10:26 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin schrieb:
Show 17 quoted lines
> How about this function instead?
> 
> static void strbuf_add_column(struct strbuf *sb,
> 	const struct column *column, const char *fmt, ...)
> {
>         va_list ap;
> 
>         va_start(ap, fmt);
> 	if (column->color)
> 		strbuf_addstr(sb, column->color);
>         strbuf_vaddf(sb, fmt, ap);
> 	if (column->color)
> 		strbuf_addstr(sb, GIT_COLOR_RESET);
>         va_end(ap);
> }
> 
> Hmm?
Except the strbuf_vaddf() is only in your private repository ;)
-- Hannes
Johannes Schindelin· Mar 31, 2009, 12:09 UTC · re: Johannes Sixt · lore

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

Hi,
On Tue, 31 Mar 2009, Johannes Sixt wrote:
Show 20 quoted lines
> Johannes Schindelin schrieb:
> > How about this function instead?
> > 
> > static void strbuf_add_column(struct strbuf *sb,
> > 	const struct column *column, const char *fmt, ...)
> > {
> >         va_list ap;
> > 
> >         va_start(ap, fmt);
> > 	if (column->color)
> > 		strbuf_addstr(sb, column->color);
> >         strbuf_vaddf(sb, fmt, ap);
> > 	if (column->color)
> > 		strbuf_addstr(sb, GIT_COLOR_RESET);
> >         va_end(ap);
> > }
> > 
> > Hmm?
> 
> Except the strbuf_vaddf() is only in your private repository ;)
LOL & schenkelklopf!

You're absolutely correct... I'm sorry, Dscho

Johannes Schindelin· Mar 31, 2009, 10:21 UTC · re: Allan Caffee · lore

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

Hi,
On Mon, 30 Mar 2009, Allan Caffee wrote:
Show 13 quoted lines
> @@ -445,6 +492,12 @@ static void graph_update_columns(struct git_graph *graph)
>  			for (parent = first_interesting_parent(graph);
>  			     parent;
>  			     parent = next_interesting_parent(graph, parent)) {
> +				/*
> +				 * If this is a merge increment the current
> +				 * color.
> +				 */
> +				if (graph->num_parents > 1)
> +					get_next_column_color(graph);
>  				graph_insert_into_new_columns(graph,
>  							      parent->item,
>  							      &mapping_idx);

Hmm. I would have expected the color to be an argument to graph_insert_into_new_columns()...

Oh, and please forget about my stupid babbling about using struct decoration for colors: the column already knows commit and color, so if you need the color in a functino in addition to the commit, you should pass either the column struct instead, or the commit and the color as individual parameters.

This concludes my review ;-)

Thanks, Dscho

Johannes Schindelin· Mar 30, 2009, 15:49 UTC · lore

[PATCH 1/2] graph.c: avoid compile warnings

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
	I'd actually like to see this and the next patch squashed in.
 graph.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
diff --git a/graph.c b/graph.c
index 2929c8b..5e2f224 100644
--- a/graph.c
+++ b/graph.c
@@ -701,8 +701,8 @@ 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) {
-			seen_this = 1;
 			struct strbuf tmp = STRBUF_INIT;
+			seen_this = 1;
 			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
 			strbuf_write_column(sb, col, tmp.buf);
 			strbuf_release(&tmp);
@@ -921,8 +921,6 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
 {
 	int i;
 	int *tmp_mapping;
-	static int collapsing_columns[255];
-	int collapsing_seen_so_far = 0;
 
 	/*
 	 * Clear out the new_mapping array

-- 
1.6.2.1.493.g67cf3
Junio C Hamano· Mar 30, 2009, 15:58 UTC · re: Johannes Schindelin · lore

Re: [PATCH 1/2] graph.c: avoid compile warnings

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 19 quoted lines
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	I'd actually like to see this and the next patch squashed in.
>
>  graph.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index 2929c8b..5e2f224 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -701,8 +701,8 @@ 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) {
> -			seen_this = 1;
>  			struct strbuf tmp = STRBUF_INIT;
> +			seen_this = 1;
Which codebase are you working on top of?
Show 14 quoted lines
>  			strbuf_addf(&tmp, "| %*s", graph->expansion_row, "");
>  			strbuf_write_column(sb, col, tmp.buf);
>  			strbuf_release(&tmp);
> @@ -921,8 +921,6 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
>  {
>  	int i;
>  	int *tmp_mapping;
> -	static int collapsing_columns[255];
> -	int collapsing_seen_so_far = 0;
>  
>  	/*
>  	 * Clear out the new_mapping array
> -- 
> 1.6.2.1.493.g67cf3
Junio C Hamano· Mar 30, 2009, 16:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] graph.c: avoid compile warnings

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
>> diff --git a/graph.c b/graph.c
>> index 2929c8b..5e2f224 100644
>> --- a/graph.c
>> +++ b/graph.c
>> @@ -701,8 +701,8 @@ 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) {
>> -			seen_this = 1;
>>  			struct strbuf tmp = STRBUF_INIT;
>> +			seen_this = 1;
>
> Which codebase are you working on top of?

Nevermind. I didn't realize it was "here are to help whipping your series into shape" meant for Allan.

Johannes Schindelin· Mar 30, 2009, 15:49 UTC · lore

[PATCH 2/2] --graph: respect --no-color

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 graph.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/graph.c b/graph.c
index 5e2f224..6a8622c 100644
--- a/graph.c
+++ b/graph.c
@@ -5,7 +5,6 @@
 #include "diff.h"
 #include "revision.h"
 
-extern int diff_use_color_default;
 /* Internal API */
 
 /*
@@ -1253,15 +1252,17 @@ static void strbuf_write_column(struct strbuf *sb, const struct column *c,
 	 * right flag to be checking since --no-color doesn't turn
 	 * this off.
 	 */
-	if (diff_use_color_default)
+	if (c->color)
 		strbuf_addstr(sb, c->color);
 	strbuf_addstr(sb, s);
-	if (diff_use_color_default)
+	if (c->color)
 		strbuf_addstr(sb, GIT_COLOR_RESET);
 }
 
 static char* get_current_column_color (const struct git_graph* graph)
 {
+	if (!DIFF_OPT_TST(&graph->revs->diffopt, COLOR_DIFF))
+		return NULL;
 	return column_colors[graph->default_column_color];
 }
 

-- 
1.6.2.1.493.g67cf3

← back to recent threads