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

From: Jeff King <peff@peff.net>
Date: Aug 6, 2018, 21:26 UTC
Message-ID: <20180806212603.GA21026@sigill.intra.peff.net>
In-Reply-To: <CAM-tV-8sbbht7NUwf87-gq=+P=LNPyiEcv3zL+1BxfXK+ktmVA@mail.gmail.com>

On Sat, Jun 30, 2018 at 08:47:16AM -0400, Noam Postavsky wrote:

Show 15 quoted lines

> I'm still having trouble getting a big picture understanding of how
> the graph struct relates the what gets drawn on screen, but through
> some poking around with the debugger + trial & error, I've arrived at
> a new patch which seems to work. It's also a lot simpler. I hope you
> can tell me if it makes sense.
> [...]
>  	int num_dashes =
>  		((graph->num_parents - dashless_commits) * 2) - 1;
>  	for (i = 0; i < num_dashes; i++) {
> -		col_num = (i / 2) + dashless_commits + graph->commit_index;
> +		col_num = (i / 2) + dashless_commits;
>  		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
>  	}
> -	col_num = (i / 2) + dashless_commits + graph->commit_index;
> +	col_num = (i / 2) + dashless_commits;

Hmm. So this seems to work because in your example, we're showing the merge in slot 1. So the index is 1, and we have an off-by-one problem, and getting rid of that solves it.

But what if we had another line of development to the left of that? I.e., if the "c" in your script was itself on the right-hand side of a merge.

We can simulate that by adding this to your script, right before the invocation of log:

  # We traverse in commit-date order, so make sure the new commit is
  # more recent than the others. This is also the cause of your "calling
  # it x doesn't work" comment, I think (all of these commits are
  # created in a single second, so the order we hit the refs in --all
  # matters).
  sleep 1

  git checkout -b a-prime master^
  git commit --allow-empty -m a-prime

That gives me a graph like this (for d-e-f):

  * d342ed8 (HEAD -> a-prime) a-prime
  | * 14aae3a (c) c
  | | *-------.   4bacae1 (m) merge a b d e f
  | | |\ \ \ \ \  
  | |/ / / / / /  
  | | | | | | * f19c3a9 (f) f
  | |_|_|_|_|/  
  |/| | | | |   
  | | | | | * 48fd961 (e) e
  | |_|_|_|/  
  |/| | | |   
  | | | | * 3f4914f (d) d
  | |_|_|/  
  |/| | |   
  | | | * 8bef98c (b) b
  | |_|/  
  |/| |   
  | | * 253e7ba (a) a
  | |/  
  |/|   
  | * 8a60f32 (master) 1
  |/  
  * b00ba42 0

and graph->commit_index is 2.

That doesn't trigger valgrind, but all the colors are off-by-one (which makes sense; we're off-by-one towards the beginning of the array now). Using "graph->commit_index - 1" seems to yield the right results, but it feels like we're just hacking around it. And my understanding was that the "straight edge" case actually works with the current code, and we'd probably be breaking that.

I still think it makes more sense to iterate over the columns rather than over the dashes, which removes a lot of these confusing cases. This is what I came up with:

diff --git a/graph.c b/graph.c
index c782590202..d0a3e0858b 100644
--- a/graph.c
+++ b/graph.c
@@ -847,22 +847,24 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 static int graph_draw_octopus_merge(struct git_graph *graph,
 				    struct strbuf *sb)
 {
+	int col_num, first_col, last_col;
+
+	/* Skip the current commit, since we've already drawn its asterisk. */
+	first_col = graph->commit_index + 1;
 	/*
-	 * Here dashless_commits represents the number of parents
-	 * which don't need to have dashes (because their edges fit
-	 * neatly under the commit).
+	 * We subtract three, one each for:
+	 *  - the commit we're directly on top of
+	 *  - the commit to our left that we're merged into
+	 *  - we want to point to the final column, not one past
 	 */
-	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;
+	last_col = first_col + graph->num_parents - 3;
+
+	for (col_num = first_col; col_num <= last_col; col_num++) {
 		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
+		strbuf_write_column(sb, &graph->new_columns[col_num],
+				    col_num == last_col ? '.' : '-');
 	}
-	col_num = (i / 2) + dashless_commits;
-	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
-	return num_dashes + 1;
+	return 2 * (last_col - first_col + 1);
 }
 
 static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)

I suspect it still has a bug, which is that it is handling this
first-parent-goes-left case, but probably gets the straight-parent case
wrong. But at least in this form, I think it is obvious to see where
that bug is (the "three" in the comment is not accurate in that latter
case, and it should be two). Which I think is what your original fix was
getting at: we need to set first/last to start off with, and then
"shrink" them with a conditional depending on which form we're seeing.

-Peff