On 1/7/2020 9:04 AM, Jeff King wrote: > On Tue, Jan 07, 2020 at 08:25:59AM -0500, Derrick Stolee wrote: > >> On 1/7/2020 6:48 AM, Jeff King wrote: >>> The assertion itself is quite old, so I wondered if it was even still >>> relevant. Removing it does produce a reasonable-looking graph: >> >> As I'm digging into this case, and finding when the assertion is hit, >> I see that the issue is in the line further below your coloring issue: > > Oh, you're right. I totally missed that. > > So perhaps we have two bugs, or perhaps they have the same root cause. > >>> | | | | * dd068b4 Merge commit '8f076d8' into HEAD >>> | |_|_|/| >>> |/| | |/ >>> | | |/| >>> | |/| | >>> | * | | 8f076d8 5 >> >> What is output is actually this, above. But the logic that includes the >> assert is checking where the underscores end, and the shown underscores >> actually pass the check. The issue is that it seems like it really wants >> to show this: >> >>> | | | | * dd068b4 Merge commit '8f076d8' into HEAD >>> | |_|_|/| >>> |/| |_|/ >>> | |/| | >>> | * | | 8f076d8 5 >> >> Note that I dropped a line and compressed a slash into an underscore. It's >> on that line that this condition is being hit. > > Hrm. I could see either being acceptable, but I do think the second one > is a bit easier to read. I'm not sure which was intended for this case. Somewhat expanding the situation to test more of these collapses, I can get the following graph: * 6_M1 |\ | | * 6_M2 | | |\ | | | * 6_H | | | | * 6_M3 | | | | |\ | | | | | * 6_J | | | | * | 6_I | | | | | | * 6_M4 | |_|_|_|_|/|\ |/| | | | |/ / | | | | |/| / | | | |/| |/ | | |/| |/| | |/| |/| | | | |/| | | | | * | | | 6_G | | | |_|/ | | |/| | | | * | | 6_F | * | | | 6_E | | |/ / | |/| | | * | | 6_D | | |/ | |/| * | | 6_C | |/ |/| * | 6_B |/ * 6_A Note how 6_M4 has three parents, and the first parent has a horizontal edge. Even giving an extra padding line between horizontal edges, we _could_ output the following instead: | | | | | | * 6_M4 | |_|_|_|_|/|\ |/| | | | |/ / | | |_|_|/| / | |/| | | |/ | | | |_|/| | | |/| | | | | * | | | 6_G However, I'll stick with the fix for the coloring issue. I think it was a previous bug that just wasn't hit until now. Thanks, -Stolee
