From: Jeff King <peff@peff.net>
Date: Tue, 17 May 2016 19:55:41 GMT
Subject: Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never
Message-ID: <20160517195541.GC11289@sigill.intra.peff.net>
In-Reply-To: <20160517195136.GB11289@sigill.intra.peff.net>

On Tue, May 17, 2016 at 03:51:37PM -0400, Jeff King wrote:

> On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote:
> 
> > 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.
> 
> Looking at the valgrind output reveals that. Here's an assert() that
> catches it reliably for me:
> 
> diff --git a/graph.c b/graph.c
> index 1350bdd..964bbd1 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph *graph,
>  		((graph->num_parents - dashless_commits) * 2) - 1;
>  	for (i = 0; i < num_dashes; i++) {
>  		col_num = (i / 2) + dashless_commits + graph->commit_index;
> +		assert(col_num < graph->num_new_columns);
>  		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
>  	}
>  	col_num = (i / 2) + dashless_commits + graph->commit_index;
> +	assert(col_num < graph->num_new_columns);
>  	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
>  	return num_dashes + 1;
>  }
> 
> (It's actually the first one which triggers). I'm not familiar enough
> with the code to know whether the col_num computation is bogus, or
> whether we needed to earlier increase the size of the "new_columns"
> field.

And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d
seems to fix this (author cc'd). It's the extra "commit_index" addition
that causes the problem. But I'm still not sure what the correct
solution is.

-Peff