Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

From: Jeff King <peff@peff.net>
Date: May 17, 2016, 19:45 UTC
Message-ID: <20160517194533.GA11289@sigill.intra.peff.net>
In-Reply-To: <573B6BF5.1090004@kdbg.org>

On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote:

Show 19 quoted lines

> Am 15.05.2016 um 15:05 schrieb Noam Postavsky:
> > With a certain topology involving an octopus merge, git log --graph
> > --oneline --all --color=never produces output which includes some ANSI
> > escape code coloring. Attached is a script to reproduce the problem
> > (creates a git repository in subdir log-format-test), along with
> > sample graph and valgrind output (indicates some unitialialized memory
> > access).
> > 
> > I've observed the problem with Windows git versions 2.7.0, 2.5.3.
> > I've NOT observed it with 1.9.5,
> > 
> > On GNU/Linux the symptom only appears when running with valgrind, I
> > tried versions
> > 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)
> > 
> 
> Sorry, I can't reproduce your observation. I ran the script you provided
> with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
> valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.

Interesting. It replicates out of the box for me. It looks like the column pointer we are passing is bogus. If I instrument git like this:

diff --git a/graph.c b/graph.c
index 1350bdd..62a5810 100644
--- a/graph.c
+++ b/graph.c
@@ -76,6 +76,7 @@ static const char *column_get_color_code(unsigned short color)
 static void strbuf_write_column(struct strbuf *sb, const struct column *c,
 				char col_char)
 {
+	warning("c=%p, c->color = %d, max=%d", c, c->color, column_colors_max);
 	if (c->color < column_colors_max)
 		strbuf_addstr(sb, column_get_color_code(c->color));
 	strbuf_addch(sb, col_char);
@@ -390,6 +391,9 @@ static void graph_insert_into_new_columns(struct git_graph *graph,
 	 */
 	graph->new_columns[graph->num_new_columns].commit = commit;
 	graph->new_columns[graph->num_new_columns].color = graph_find_commit_color(graph, commit);
+	warning("assigned %p, %d",
+		&graph->new_columns[graph->num_new_columns],
+		graph->new_columns[graph->num_new_columns].color);
 	graph->mapping[*mapping_index] = graph->num_new_columns;
 	*mapping_index += 2;
 	graph->num_new_columns++;

Then I get this output:

    warning: assigned 0x20d21d0, 12
    * 163b784 (c) c
    warning: assigned 0x20d8f90, 12
    warning: assigned 0x20d8fa0, 12
    warning: assigned 0x20d8fb0, 12
    warning: c=0x20d21d0, c->color = 12, max=12
    warning: c=0x20d8fc0, c->color = 0, max=12
    warning: c=0x20d8fc0, c->color = 0, max=12
    | *-.   a9a6975 (HEAD -> m) merge a b

Note that we set up f90, fa0, and fb0, but then pass fc0 into
strbuf_write_column (and it has bogus color values). It looks like we're
reading one past the end of our array, but I haven't figured out where
or why.

-Peff