git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Noam Postavsky <npostavs@users.sourceforge.net>
Date
Oct 3, 2018, 22:32 UTC
Message-ID
<CAM-tV-88J3ZAALwZeEqTuvKXRwLzb848G0AET2Ec6ic85=7o8Q@mail.gmail.com>
In-Reply-To
<20181003042437.GA27034@sigill.intra.peff.net>
On Wed, 3 Oct 2018 at 00:24, Jeff King <peff@peff.net> wrote:
> Yeah, I really like your explanations/diagrams in the comments. It makes
> the logic very clear.

Ok good, I did have the feeling that the logic became actually clearer to me after I wrestled with the test code, so I think this means I didn't just imagine that. :)

> (there's also only one caller of this
> function, so it could be inlined; note that it's OK to use "return" in a
> test_expect block).

Oh, I think had run into some trouble with the test runner complaining about a broken &&-chain, but it seems to work fine now (perhaps I missing the && somewhere else that I fixed later).

> Why do we need the tag name to be different?

Otherwise the 'git checkout' command complains about an ambiguous ref (added that to the comment).

Show 5 quoted lines
> >       git checkout 1 -b merge &&
>
> This is assuming we just made a branch called "1", but that's one of the
> arguments. Probably this should be "$1" (or the whole thing should just
> be inlined so it is clear that what the set of parameters we expect is).
Oops, right. I've inlined it.
Show 5 quoted lines
> It might actually be worth setting up the uncolored expect file as part
> of this, since it so neatly diagrams the graph you're trying to produce.
>
> I.e., something like (completely untested; note that the leading
> indentation is all tabs, which will be removed by the "<<-" operator):

Yup, works (I again had run into some problems with &&-chaining earlier, but now it works fine)

> > * left
> > <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
[...]
Show 7 quoted lines
>
> Yikes. :) This one is pretty hard to read. I'm not sure if there's a
> good alternative. If you pipe the output of test_decode through
> this:
>
>   sed '
>         s/<RED>.<RESET>/R/g;
[...]
Show 5 quoted lines
> you get this:
>
>   * left
>   R *BBMM   octopus-merge
>   R RY B M
[...]
> which is admittedly pretty horrible, too, but at least resembles a
> graph. I dunno.

Yeah, but it's lossy, so it doesn't seem usable for the test. Maybe doubling up some characters?

** left R| **B-B-M-M. octopus-merge R| R|Y\ B\ M\ R|R/ Y/ B/ M/ R| Y| B| ** 4 R| Y| ** M| 3 R| Y| M|M/ R| ** M| 2 R| M|M/ ** M| 1 M|M/ ** initial

> I'm also not thrilled that we depend on the exact sequence of default
> colors, but I suspect it's not the first time. And it wouldn't be too
> hard to update it if that default changes.

Well, it's easy enough to set the colors explicitly. After doing this I noticed that green seems to be skipped. Not sure if that's a bug or not.

> Try not to put "git" on the left-hand side of a pipe, since it means
> we'll miss its exit code
Ok.
Show 9 quoted lines
> Leftover "debug" cruft?
>
> The same pipe comment applies as above.
>
> > test_done
> > test_done
>
> Two dones; we exit after the first one (so everything after this is
> ignored).
Oops, yeah, this script was still a bit of a rough draft.
> I think it's OK to have a dedicated script for even these two tests, if
> it makes things easier to read. However, would we also want to test the
> octopus without the problematic graph here? I think if we just omit
> "left" we get that, don't we?

t4202-log.sh already does test a "normal" octopus merge (starting around line 615, search for "octopus-a"). But that is only a 3-parent merge. And adding another test is easy enough.

From cd9415b524357c2c8b9b20a63032c94e01d46a15 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@users.sourceforge.net>
Date: Sat, 1 Sep 2018 20:07:16 -0400
Subject: [PATCH v5] log: Fix coloring of certain octupus merge shapes

For octopus merges where the first parent edge immediately merges into the next column to the left:

| *-.
| |\ \
|/ / /
then the number of columns should be one less than the usual case:
| *-.
| |\ \
| | | *

Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King.

Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net>
---
 graph.c                      |  56 +++++++++++++++++-------
 t/t4214-log-graph-octopus.sh | 102 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 15 deletions(-)
 create mode 100755 t/t4214-log-graph-octopus.sh
diff --git a/graph.c b/graph.c
index e1f6d3bdd..a3366f6da 100644
--- a/graph.c
+++ b/graph.c
@@ -842,27 +842,53 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 }
 
 /*
- * Draw an octopus merge and return the number of characters written.
+ * Draw the horizontal dashes of an octopus merge and return the number of
+ * characters written.
  */
 static int graph_draw_octopus_merge(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 + graph->commit_index;
-		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
+	 * Here dashless_parents represents the number of parents which don't
+	 * need to have dashes (the edges labeled "0" and "1").  And
+	 * dashful_parents are the remaining ones.
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * | | | | |
+	 * x 0 1 2 3
+	 *
+	 */
+	const int dashless_parents = 2;
+	int dashful_parents = graph->num_parents - dashless_parents;
+
+	/*
+	 * Usually, each parent gets its own column, like the diagram above, but
+	 * sometimes the first parent goes into an existing column, like this:
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * |/ / / /
+	 * x 0 1 2
+	 *
+	 * In which case there will be more parents than the delta of columns.
+	 */
+	int delta_cols = (graph->num_new_columns - graph->num_columns);
+	int parent_in_old_cols = graph->num_parents - delta_cols;
+
+	/*
+	 * In both cases, commit_index corresponds to the edge labeled "0".
+	 */
+	int first_col = graph->commit_index + dashless_parents
+	    - parent_in_old_cols;
+
+	int i;
+	for (i = 0; i < dashful_parents; i++) {
+		strbuf_write_column(sb, &graph->new_columns[i+first_col], '-');
+		strbuf_write_column(sb, &graph->new_columns[i+first_col],
+				    i == dashful_parents-1 ? '.' : '-');
 	}
-	col_num = (i / 2) + dashless_commits + graph->commit_index;
-	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
-	return num_dashes + 1;
+	return 2 * dashful_parents;
 }
 
 static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
new file mode 100755
index 000000000..dab96c89a
--- /dev/null
+++ b/t/t4214-log-graph-octopus.sh
@@ -0,0 +1,102 @@
+#!/bin/sh
+
+test_description='git log --graph of skewed left octopus merge.'
+
+. ./test-lib.sh
+
+test_expect_success 'set up merge history' '
+	cat >expect.uncolored <<-\EOF &&
+	* left
+	| *---.   octopus-merge
+	| |\ \ \
+	|/ / / /
+	| | | * 4
+	| | * | 3
+	| | |/
+	| * | 2
+	| |/
+	* | 1
+	|/
+	* initial
+	EOF
+	cat >expect.colors <<-\EOF &&
+	* left
+	<RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
+	<RED>|<RESET> <RED>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET>
+	<RED>|<RESET><RED>/<RESET> <YELLOW>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET>
+	<RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
+	<RED>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3
+	<RED>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
+	<RED>|<RESET> * <MAGENTA>|<RESET> 2
+	<RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
+	* <MAGENTA>|<RESET> 1
+	<MAGENTA>|<RESET><MAGENTA>/<RESET>
+	* initial
+	EOF
+	test_commit initial &&
+	for i in 1 2 3 4 ; do
+		git checkout master -b $i || return $?
+		# Make tag name different from branch name, to avoid
+		# ambiguity error when calling checkout.
+		test_commit $i $i $i tag$i || return $?
+	done &&
+	git checkout 1 -b merge &&
+	test_tick &&
+	git merge -m octopus-merge 1 2 3 4 &&
+	git checkout 1 -b L &&
+	test_commit left
+'
+
+test_expect_success 'log --graph with tricky octopus merge with colors' '
+	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
+	git log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw &&
+	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
+	test_cmp expect.colors actual.colors
+'
+
+test_expect_success 'log --graph with tricky octopus merge, no color' '
+	git log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&
+	sed "s/ *\$//" actual.raw >actual &&
+	test_cmp expect.uncolored actual
+'
+
+# Repeat the previous two tests with "normal" octopus merge (i.e.,
+# without the first parent skewing to the "left" branch column).
+
+test_expect_success 'log --graph with normal octopus merge, no color' '
+	cat >expect.uncolored <<-\EOF &&
+	*---.   octopus-merge
+	|\ \ \
+	| | | * 4
+	| | * | 3
+	| | |/
+	| * | 2
+	| |/
+	* | 1
+	|/
+	* initial
+	EOF
+	git log --color=never --graph --date-order --pretty=tformat:%s merge >actual.raw &&
+	sed "s/ *\$//" actual.raw >actual &&
+	test_cmp expect.uncolored actual
+'
+
+test_expect_success 'log --graph with normal octopus merge with colors' '
+	cat >expect.colors <<-\EOF &&
+	*<YELLOW>-<RESET><YELLOW>-<RESET><BLUE>-<RESET><BLUE>.<RESET>   octopus-merge
+	<RED>|<RESET><GREEN>\<RESET> <YELLOW>\<RESET> <BLUE>\<RESET>
+	<RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 4
+	<RED>|<RESET> <GREEN>|<RESET> * <BLUE>|<RESET> 3
+	<RED>|<RESET> <GREEN>|<RESET> <BLUE>|<RESET><BLUE>/<RESET>
+	<RED>|<RESET> * <BLUE>|<RESET> 2
+	<RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET>
+	* <BLUE>|<RESET> 1
+	<BLUE>|<RESET><BLUE>/<RESET>
+	* initial
+	EOF
+	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
+	git log --color=always --graph --date-order --pretty=tformat:%s merge >actual.colors.raw &&
+	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
+	test_cmp expect.colors actual.colors
+'
+test_done

-- 
2.11.0
Previous: Jeff KingNext: Jeff King
Message 18 of 22 in “[BUG] A part of an edge from an octopus merge gets colored, even with --color=never”
  1. Noam PostavskyMay 15, 2016
  2. Johannes SixtMay 17, 2016
  3. Jeff KingMay 17, 2016
  4. Jeff KingMay 17, 2016
  5. Jeff KingMay 17, 2016
  6. Noam PostavskyMay 20, 2016
  7. Noam PostavskyJun 23, 2018
  8. Jeff KingJun 25, 2018
  9. Noam PostavskyJun 30, 2018
  10. Noam PostavskyAug 6, 2018
  11. Jeff KingAug 6, 2018
  12. Jeff KingAug 6, 2018
  13. Noam PostavskySep 2, 2018
  14. Jeff KingSep 8, 2018
  15. Noam PostavskySep 25, 2018
  16. Noam PostavskyOct 3, 2018
  17. Jeff KingOct 3, 2018
  18. Noam PostavskyOct 3, 2018
  19. Jeff KingOct 9, 2018
  20. Noam PostavskyOct 10, 2018
  21. Johannes SixtMay 17, 2016
  22. Jeff KingMay 17, 2016

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.