[11/11] graph: fix coloring of octopus dashes

Message ID	ea0df1d94aa6c42eb00d59b268a90a8724322452.1570724021.git.gitgitgadget@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=0T8v=YD=vger.kernel.org=git-owner@kernel.org> Date: Thu, 10 Oct 2019 09:13:52 -0700 (PDT) Message-Id: <ea0df1d94aa6c42eb00d59b268a90a8724322452.1570724021.git.gitgitgadget@gmail.com> In-Reply-To: <pull.383.git.gitgitgadget@gmail.com> References: <pull.383.git.gitgitgadget@gmail.com> From: "James Coglan via GitGitGadget" <gitgitgadget@gmail.com> Subject: [PATCH 11/11] graph: fix coloring of octopus dashes Fcc: Sent Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MIME-Version: 1.0 To: git@vger.kernel.org Cc: Junio C Hamano <gitster@pobox.com>, James Coglan <jcoglan@gmail.com> Sender: git-owner@vger.kernel.org Precedence: bulk
Series	Improve the readability of log --graph output \| expand [00/11] Improve the readability of log --graph output [01/11] graph: automatically track visible width of `strbuf` [02/11] graph: reuse `find_new_column_by_commit()` [03/11] graph: reduce duplication in `graph_insert_into_new_columns()` [04/11] graph: remove `mapping_idx` and `graph_update_width()` [05/11] graph: extract logic for moving to GRAPH_PRE_COMMIT state [06/11] graph: tidy up display of left-skewed merges [07/11] graph: commit and post-merge lines for left-skewed merges [08/11] graph: rename `new_mapping` to `old_mapping` [09/11] graph: smooth appearance of collapsing edges on commit lines [10/11] graph: flatten edges that join to their right neighbor [11/11] graph: fix coloring of octopus dashes

Message ID

ea0df1d94aa6c42eb00d59b268a90a8724322452.1570724021.git.gitgitgadget@gmail.com (mailing list archive)

State

New, archived

Headers

Date: Thu, 10 Oct 2019 09:13:52 -0700 (PDT)
Message-Id: 
 <ea0df1d94aa6c42eb00d59b268a90a8724322452.1570724021.git.gitgitgadget@gmail.com>
In-Reply-To: <pull.383.git.gitgitgadget@gmail.com>
References: <pull.383.git.gitgitgadget@gmail.com>
From: "James Coglan via GitGitGadget" <gitgitgadget@gmail.com>
Subject: [PATCH 11/11] graph: fix coloring of octopus dashes
Fcc: Sent
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
To: git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
        James Coglan <jcoglan@gmail.com>
Sender: git-owner@vger.kernel.org
Precedence: bulk

Series

Improve the readability of log --graph output | expand

Commit Message

Derrick Stolee via GitGitGadget Oct. 10, 2019, 4:13 p.m. UTC

From: James Coglan <jcoglan@gmail.com>

In 04005834ed ("log: fix coloring of certain octopus merge shapes",
2018-09-01) there is a fix for the coloring of dashes following an
octopus merge. It makes a distinction between the case where all parents
introduce a new column, versus the case where the first parent collapses
into an existing column:

        | *-.           | *-.
        | |\ \          | |\ \
        | | | |         |/ / /

The latter case means that the columns for the merge parents begin one
place to the left in the `new_columns` array compared to the former
case.

However, the implementation only works if the commit's parents are kept
in order as they map onto the visual columns, as we get the colors by
iterating over `new_columns` as we print the dashes. In general, the
commit's parents can arbitrarily merge with existing columns, and change
their ordering in the process.

For example, in the following diagram, the number of each column
indicates which commit parent appears in each column.

        | | *---.
        | | |\ \ \
        | | |/ / /
        | |/| | /
        | |_|_|/
        |/| | |
        3 1 0 2

If the columns are colored (red, green, yellow, blue), then the dashes
will currently be colored yellow and blue, whereas they should be blue
and red.

To fix this, we need to look up each column in the `mapping` array,
which before the `GRAPH_COLLAPSING` state indicates which logical column
is displayed in each visual column. This implementation is simpler as it
doesn't have any edge cases, and it also handles how left-skewed first
parents are now displayed:

        | *-.
        |/|\ \
        | | | |
        0 1 2 3

The color of the first dashes is always the color found in `mapping` two
columns to the right of the commit symbol. Because commits are displayed
after all edges have been collapsed together and the visual columns
match the logical ones, we can find the visual offset of the commit
symbol using `commit_index`.

Signed-off-by: James Coglan <jcoglan@gmail.com>
---
 graph.c                      | 71 +++++++++++++++++++-----------------
 t/t4214-log-graph-octopus.sh | 59 ++++++++++++++++++++++++++++--
 2 files changed, 92 insertions(+), 38 deletions(-)

Comments

Denton Liu Oct. 10, 2019, 6:16 p.m. UTC | #1

Hi James,

Nicely done! This issue was bugging me for a while!

On Thu, Oct 10, 2019 at 09:13:52AM -0700, James Coglan via GitGitGadget wrote:

[...]

> diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
> index 9ada687628..fbd485d83a 100755
> --- a/t/t4214-log-graph-octopus.sh
> +++ b/t/t4214-log-graph-octopus.sh
> @@ -42,23 +42,74 @@ test_expect_success 'set up merge history' '
>  	test_tick &&
>  	git merge -m octopus-merge 1 2 3 4 &&
>  	git checkout 1 -b L &&
> -	test_commit left
> +	test_commit left &&
> +	git checkout 4 -b R &&
> +	test_commit right
>  '
>  
>  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 &&
> +	git log --color=always --graph --date-order --pretty=tformat:%s L merge >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 &&
> +	git log --color=never --graph --date-order --pretty=tformat:%s L merge >actual.raw &&
>  	sed "s/ *\$//" actual.raw >actual &&
>  	test_cmp expect.uncolored actual
>  '
>  
> -# Repeat the previous two tests with "normal" octopus merge (i.e.,
> +# Repeat the previous two tests with an octopus merge whose final parent skews left
> +
> +test_expect_success 'log --graph with left-skewed final parent, no color' '
> +	cat >expect.uncolored <<-\EOF &&
> +	* right
> +	| *---.   octopus-merge
> +	| |\ \ \
> +	| |_|_|/
> +	|/| | |
> +	* | | | 4
> +	| | | * 3
> +	| |_|/
> +	|/| |
> +	| | * 2
> +	| |/
> +	|/|
> +	| * 1
> +	|/
> +	* initial
> +	EOF
> +	git log --color=never --graph --date-order --pretty=tformat:%s R merge >actual.raw &&
> +	sed "s/ *\$//" actual.raw >actual &&
> +	test_cmp expect.uncolored actual
> +'
> +
> +test_expect_success 'log --graph with left-skewed final parent with colors' '
> +	cat >expect.colors <<-\EOF &&
> +	* right
> +	<RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><RED>-<RESET><RED>.<RESET>   octopus-merge
> +	<RED>|<RESET> <GREEN>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <RED>\<RESET>
> +	<RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET>
> +	<RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET>
> +	* <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> 4
> +	<MAGENTA>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 3
> +	<MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>_<RESET><YELLOW>|<RESET><MAGENTA>/<RESET>
> +	<MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET>
> +	<MAGENTA>|<RESET> <GREEN>|<RESET> * 2
> +	<MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>/<RESET>
> +	<MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET>
> +	<MAGENTA>|<RESET> * 1
> +	<MAGENTA>|<RESET><MAGENTA>/<RESET>
> +	* initial
> +	EOF
> +	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
> +	git log --color=always --graph --date-order --pretty=tformat:%s R merge >actual.colors.raw &&
> +	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
> +	test_cmp expect.colors actual.colors
> +'
> +
> +# Repeat the first 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' '

So, I decided to merge 'dl/octopus-graph-bug' with your branch and I
couldn't be happier! I had to make a couple of minor tweaks to the
existing test cases but most of them only involved flipping
`test_expect_failure` to `test_expect_success`.

You can see the results of this by doing:

	$ git fetch https://github.com/Denton-L/git.git testing/graph-output
	$ git diff FETCH_HEAD^2 t/t4214-log-graph-octopus.sh

and the resulting diff is very pleasing imo.

Junio, when you pick this topic up, that branch should contain the
correct conflict resolution if that can help you out in any way.

Anyway, thanks for the work, James!

I'll give this patch my:

	Tested-by: Denton Liu <liu.denton@gmail.com>

> -- 
> gitgitgadget

Denton Liu Oct. 10, 2019, 6:28 p.m. UTC | #2

On Thu, Oct 10, 2019 at 11:16:24AM -0700, Denton Liu wrote:
> You can see the results of this by doing:
> 
> 	$ git fetch https://github.com/Denton-L/git.git testing/graph-output
> 	$ git diff FETCH_HEAD^2 t/t4214-log-graph-octopus.sh
> 
> and the resulting diff is very pleasing imo.

I guess it'd probably be nice if the result of that diff were viewable
without the extra work of fetching everything, so here it is:

diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
index 3ae8e51e50..40d27db674 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -26,15 +26,14 @@ test_expect_success 'set up merge history' '
 test_expect_success 'log --graph with tricky octopus merge, no color' '
 	cat >expect.uncolored <<-\EOF &&
 	* left
-	| *---.   octopus-merge
-	| |\ \ \
-	|/ / / /
+	| *-.   octopus-merge
+	|/|\ \
 	| | | * 4
 	| | * | 3
 	| | |/
-	| * | 2
+	| * / 2
 	| |/
-	* | 1
+	* / 1
 	|/
 	* initial
 	EOF
@@ -47,15 +46,14 @@ test_expect_success 'log --graph with tricky octopus merge with colors' '
 	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
 	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> *<MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
+	<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> 2
 	<RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
-	* <MAGENTA>|<RESET> 1
+	* <MAGENTA>/<RESET> 1
 	<MAGENTA>|<RESET><MAGENTA>/<RESET>
 	* initial
 	EOF
@@ -74,9 +72,9 @@ test_expect_success 'log --graph with normal octopus merge, no color' '
 	| | | * 4
 	| | * | 3
 	| | |/
-	| * | 2
+	| * / 2
 	| |/
-	* | 1
+	* / 1
 	|/
 	* initial
 	EOF
@@ -92,9 +90,9 @@ test_expect_success 'log --graph with normal octopus merge with colors' '
 	<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> 2
 	<RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET>
-	* <BLUE>|<RESET> 1
+	* <BLUE>/<RESET> 1
 	<BLUE>|<RESET><BLUE>/<RESET>
 	* initial
 	EOF
@@ -112,9 +110,9 @@ test_expect_success 'log --graph with normal octopus merge and child, no color'
 	| | | * 4
 	| | * | 3
 	| | |/
-	| * | 2
+	| * / 2
 	| |/
-	* | 1
+	* / 1
 	|/
 	* initial
 	EOF
@@ -123,7 +121,7 @@ test_expect_success 'log --graph with normal octopus merge and child, no color'
 	test_cmp expect.uncolored actual
 '
 
-test_expect_failure 'log --graph with normal octopus and child merge with colors' '
+test_expect_success 'log --graph with normal octopus and child merge with colors' '
 	cat >expect.colors <<-\EOF &&
 	* after-merge
 	*<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
@@ -131,9 +129,9 @@ test_expect_failure 'log --graph with normal octopus and child merge with colors
 	<GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
 	<GREEN>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3
 	<GREEN>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
-	<GREEN>|<RESET> * <MAGENTA>|<RESET> 2
+	<GREEN>|<RESET> * <MAGENTA>/<RESET> 2
 	<GREEN>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
-	* <MAGENTA>|<RESET> 1
+	* <MAGENTA>/<RESET> 1
 	<MAGENTA>|<RESET><MAGENTA>/<RESET>
 	* initial
 	EOF
@@ -147,15 +145,14 @@ test_expect_success 'log --graph with tricky octopus merge and its child, no col
 	cat >expect.uncolored <<-\EOF &&
 	* left
 	| * after-merge
-	| *---.   octopus-merge
-	| |\ \ \
-	|/ / / /
+	| *-.   octopus-merge
+	|/|\ \
 	| | | * 4
 	| | * | 3
 	| | |/
-	| * | 2
+	| * / 2
 	| |/
-	* | 1
+	* / 1
 	|/
 	* initial
 	EOF
@@ -164,20 +161,19 @@ test_expect_success 'log --graph with tricky octopus merge and its child, no col
 	test_cmp expect.uncolored actual
 '
 
-test_expect_failure 'log --graph with tricky octopus merge and its child with colors' '
+test_expect_success 'log --graph with tricky octopus merge and its child with colors' '
 	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
 	cat >expect.colors <<-\EOF &&
 	* left
 	<RED>|<RESET> * after-merge
-	<RED>|<RESET> *<MAGENTA>-<RESET><MAGENTA>-<RESET><CYAN>-<RESET><CYAN>.<RESET>   octopus-merge
-	<RED>|<RESET> <RED>|<RESET><BLUE>\<RESET> <MAGENTA>\<RESET> <CYAN>\<RESET>
-	<RED>|<RESET><RED>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET> <CYAN>/<RESET>
+	<RED>|<RESET> *<CYAN>-<RESET><CYAN>.<RESET>   octopus-merge
+	<RED>|<RESET><RED>/<RESET><BLUE>|<RESET><MAGENTA>\<RESET> <CYAN>\<RESET>
 	<RED>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET> * 4
 	<RED>|<RESET> <BLUE>|<RESET> * <CYAN>|<RESET> 3
 	<RED>|<RESET> <BLUE>|<RESET> <CYAN>|<RESET><CYAN>/<RESET>
-	<RED>|<RESET> * <CYAN>|<RESET> 2
+	<RED>|<RESET> * <CYAN>/<RESET> 2
 	<RED>|<RESET> <CYAN>|<RESET><CYAN>/<RESET>
-	* <CYAN>|<RESET> 1
+	* <CYAN>/<RESET> 1
 	<CYAN>|<RESET><CYAN>/<RESET>
 	* initial
 	EOF
@@ -209,7 +205,7 @@ test_expect_success 'log --graph with crossover in octopus merge, no color' '
 	test_cmp expect.uncolored actual
 '
 
-test_expect_failure 'log --graph with crossover in octopus merge with colors' '
+test_expect_success 'log --graph with crossover in octopus merge with colors' '
 	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
 	cat >expect.colors <<-\EOF &&
 	* after-4
@@ -257,7 +253,7 @@ test_expect_success 'log --graph with crossover in octopus merge and its child,
 	test_cmp expect.uncolored actual
 '
 
-test_expect_failure 'log --graph with crossover in octopus merge and its child with colors' '
+test_expect_success 'log --graph with crossover in octopus merge and its child with colors' '
 	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
 	cat >expect.colors <<-\EOF &&
 	* after-4
@@ -353,7 +349,7 @@ test_expect_success 'log --graph with unrelated commit and octopus child, no col
 	test_cmp expect.uncolored actual
 '
 
-test_expect_failure 'log --graph with unrelated commit and octopus child with colors' '
+test_expect_success 'log --graph with unrelated commit and octopus child with colors' '
 	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
 	cat >expect.colors <<-\EOF &&
 	* after-initial

Jeff King Oct. 13, 2019, 7:22 a.m. UTC | #3

On Thu, Oct 10, 2019 at 11:16:24AM -0700, Denton Liu wrote:

> >  test_expect_success 'log --graph with normal octopus merge, no color' '
> 
> So, I decided to merge 'dl/octopus-graph-bug' with your branch and I
> couldn't be happier! I had to make a couple of minor tweaks to the
> existing test cases but most of them only involved flipping
> `test_expect_failure` to `test_expect_success`.

Thanks for doing that. I also checked the output on the git.git case
discussed in:

  https://public-inbox.org/git/20190926202326.GA16664@sigill.intra.peff.net/

and it looks great. Probably because it's the same as your test cases --
the "non-collapsing type" I called it there (or maybe it's all just the
out-of-order thing James brought up; I don't recall discussing that
before, but certainly his test case exhibits the same off-by-one
coloring weirdness without the code change).

-Peff

diff --git a/graph.c b/graph.c
index 7dd2fab625..908f257018 100644
--- a/graph.c
+++ b/graph.c
@@ -665,6 +665,11 @@  static void graph_update_columns(struct git_graph *graph)
 		graph->mapping_size--;
 }
 
+static int graph_num_dashed_parents(struct git_graph *graph)
+{
+	return graph->num_parents + graph->merge_layout - 3;
+}
+
 static int graph_num_expansion_rows(struct git_graph *graph)
 {
 	/*
@@ -687,7 +692,7 @@  static int graph_num_expansion_rows(struct git_graph *graph)
 	 * 		| * \
 	 * 		|/|\ \
 	 */
-	return (graph->num_parents + graph->merge_layout - 3) * 2;
+	return graph_num_dashed_parents(graph) * 2;
 }
 
 static int graph_needs_pre_commit_line(struct git_graph *graph)
@@ -930,47 +935,45 @@  static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 static void graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb)
 {
 	/*
-	 * 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.
+	 * The parents of a merge commit can be arbitrarily reordered as they
+	 * are mapped onto display columns, for example this is a valid merge:
 	 *
-	 * | *---.
-	 * | |\ \ \
-	 * | | | | |
-	 * x 0 1 2 3
+	 *	| | *---.
+	 *	| | |\ \ \
+	 *	| | |/ / /
+	 *	| |/| | /
+	 *	| |_|_|/
+	 *	|/| | |
+	 *	3 1 0 2
 	 *
-	 */
-	const int dashless_parents = 3 - graph->merge_layout;
-	int dashful_parents = graph->num_parents - dashless_parents;
-
-	/*
-	 * Usually, we add one new column for each parent (like the diagram
-	 * above) but sometimes the first parent goes into an existing column,
-	 * like this:
+	 * The numbers denote which parent of the merge each visual column
+	 * corresponds to; we can't assume that the parents will initially
+	 * display in the order given by new_columns.
 	 *
-	 * | *-.
-	 * |/|\ \
-	 * | | | |
-	 * x 0 1 2
+	 * To find the right color for each dash, we need to consult the
+	 * mapping array, starting from the column 2 places to the right of the
+	 * merge commit, and use that to find out which logical column each
+	 * edge will collapse to.
 	 *
-	 * In which case the number of parents will be one greater than the
-	 * number of added columns.
+	 * Commits are rendered once all edges have collapsed to their correct
+	 * logcial column, so commit_index gives us the right visual offset for
+	 * the merge commit.
 	 */
-	int added_cols = (graph->num_new_columns - graph->num_columns);
-	int parent_in_old_cols = graph->num_parents - added_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, j;
+	struct column *col;
 
-	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 ? '.' : '-');
+	int dashed_parents = graph_num_dashed_parents(graph);
+
+	for (i = 0; i < dashed_parents; i++) {
+		j = graph->mapping[(graph->commit_index + i + 2) * 2];
+		col = &graph->new_columns[j];
+
+		strbuf_write_column(sb, col, '-');
+		strbuf_write_column(sb, col, (i == dashed_parents - 1) ? '.' : '-');
 	}
+
+	return;
 }
 
 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
index 9ada687628..fbd485d83a 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -42,23 +42,74 @@  test_expect_success 'set up merge history' '
 	test_tick &&
 	git merge -m octopus-merge 1 2 3 4 &&
 	git checkout 1 -b L &&
-	test_commit left
+	test_commit left &&
+	git checkout 4 -b R &&
+	test_commit right
 '
 
 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 &&
+	git log --color=always --graph --date-order --pretty=tformat:%s L merge >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 &&
+	git log --color=never --graph --date-order --pretty=tformat:%s L merge >actual.raw &&
 	sed "s/ *\$//" actual.raw >actual &&
 	test_cmp expect.uncolored actual
 '
 
-# Repeat the previous two tests with "normal" octopus merge (i.e.,
+# Repeat the previous two tests with an octopus merge whose final parent skews left
+
+test_expect_success 'log --graph with left-skewed final parent, no color' '
+	cat >expect.uncolored <<-\EOF &&
+	* right
+	| *---.   octopus-merge
+	| |\ \ \
+	| |_|_|/
+	|/| | |
+	* | | | 4
+	| | | * 3
+	| |_|/
+	|/| |
+	| | * 2
+	| |/
+	|/|
+	| * 1
+	|/
+	* initial
+	EOF
+	git log --color=never --graph --date-order --pretty=tformat:%s R merge >actual.raw &&
+	sed "s/ *\$//" actual.raw >actual &&
+	test_cmp expect.uncolored actual
+'
+
+test_expect_success 'log --graph with left-skewed final parent with colors' '
+	cat >expect.colors <<-\EOF &&
+	* right
+	<RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><RED>-<RESET><RED>.<RESET>   octopus-merge
+	<RED>|<RESET> <GREEN>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <RED>\<RESET>
+	<RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET>
+	<RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET>
+	* <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> 4
+	<MAGENTA>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 3
+	<MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>_<RESET><YELLOW>|<RESET><MAGENTA>/<RESET>
+	<MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET>
+	<MAGENTA>|<RESET> <GREEN>|<RESET> * 2
+	<MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>/<RESET>
+	<MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET>
+	<MAGENTA>|<RESET> * 1
+	<MAGENTA>|<RESET><MAGENTA>/<RESET>
+	* initial
+	EOF
+	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
+	git log --color=always --graph --date-order --pretty=tformat:%s R merge >actual.colors.raw &&
+	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
+	test_cmp expect.colors actual.colors
+'
+
+# Repeat the first 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' '

[11/11] graph: fix coloring of octopus dashes

Commit Message

Comments

Patch