Re: [PATCH v2 1/2] graph: fix case that hit assert()

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2020, 21:50 UTC
Message-ID: <xmqqd0bv3pin.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <c05fe2c37a87b254157eec1c8a0f8ca206e1cd31.1578432422.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 22 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
> Subject: Re: [PATCH v2 1/2] graph: fix case that hit assert()
>
> A failure was reported in "git log --graph --all" with the new
> graph-rendering logic. The code fails an assert() statement when
> collapsing multiple edges from a merge.
>
> The assert was introduced by eaf158f8 (graph API: Use horizontal
> lines for more compact graphs, 2009-04-21), which is quite old.
> This assert is trying to say that when we complete a horizontal
> line with a single slash, it is because we have reached our target.
>
> This assertion is hit when we have two collapsing lines from the
> same merge commit, as follows:
>
>     | | | | *
>     | |_|_|/|
>     |/| | |/
>     | | |/|
>     | |/| |
>     | * | |
>     * | | |

I was sort-of expecting to see

    graph: drop assert() for merge with two collapsing parents

    When "git log --graph" shows a merge commit that has two collapsing
    lines, like:

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

    we trigger an assert():

            graph.c:1228: graph_output_collapsing_line: Assertion
                          `graph->mapping[i - 3] == target' failed.

    The assert was introduced by eaf158f8 ("graph API: Use horizontal
    lines for more compact graphs", 2009-04-21), which is quite old.
    ...

near the beginning of this commit, though.

Show 27 quoted lines

> In a larger example, the current code will output a collapse
> as follows:
>
> 	| | | | | | *
> 	| |_|_|_|_|/|\
> 	|/| | | | |/ /
> 	| | | | |/| /
> 	| | | |/| |/
> 	| | |/| |/|
> 	| |/| |/| |
> 	| | |/| | |
> 	| | * | | |
>
> However, the intended collapse should allow multiple horizontal lines
> as follows:
>
> 	| | | | | | *
> 	| |_|_|_|_|/|\
> 	|/| | | | |/ /
> 	| | |_|_|/| /
> 	| |/| | | |/
> 	| | | |_|/|
> 	| | |/| | |
> 	| | * | | |
>
> This behavior is not corrected by this change, but is noted for a later
> update.

This part is new and looks like a good thing to mention.