Re: [PATCH 0/2] Graph horizontal fix

From: Derrick Stolee <stolee@gmail.com>
Date: Jan 8, 2020, 20:05 UTC
Message-ID: <c69afbe7-3a9c-0619-594f-f5ba980ae7b8@gmail.com>
In-Reply-To: <xmqq5zhl3jrm.fsf@gitster-ct.c.googlers.com>

On 1/8/2020 1:06 PM, Junio C Hamano wrote:

Show 23 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> This depends on ds/graph-assert-fix.
>>
>> This is a non-critical (not needed for v2.25.0) response to the previous
>> discussions [1] [2].
>>
>> While working to resolve the fix for the assert() bug, I noticed this
>> regression when multiple edges wanted to collapse with horizontal lines. It
>> takes a reasonably large graph, but real projects are likely to demonstrate
>> this behavior.
>>
>> I arranged the series into two patches: 1. the (failing) test, and 2. the
>> fix.
>>
>> The fix commit includes some details about why the change to compress merge
>> commits caused this regression, and why I feel relatively confident that
>> this is a correct resolution.
> 
> I am not sure if this is "fix" of "bug" in that what is shown
> without 2/2 (iow, "before this change" in the description of 2/2) is
> "wrong" per-se---it is just that it leaves room to be made even more
> compact.  It still is an improvement, of course, though.

I guess I was incomplete in my first example. The full horizontal behavior before 0f0f389f was

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

and 0f0f389f added a compact merge commit, but lost two horizontal lines.

	| | | | | | *
	| |_|_|_|_|/|\
	|/| | | | |/ /
	| | | | |/| /
	| | | |/| |/
	| | |/| |/|
	| |/| |/| |
	| | |/| | |

This change brings the horizontal lines back.

	| | | | | | *
	| |_|_|_|_|/|\
	|/| | | | |/ /
	| | |_|_|/| /
	| |/| | | |/
	| | | |_|/|
	| | |/| | |

Whether this qualifies as a "bug" is debatable, for sure. That's why I believe this change is below the bar for the release candidate.

Thanks, -Stolee