Subject: Re: [PATCH 11/11] graph: fix coloring of octopus dashes

From: Denton Liu <[email-masked]>
Date: Thu, 10 Oct 2019 11:16:24 -0700

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 <[email-masked]>

> -- 
> gitgitgadget