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

On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:

From: Derrick Stolee <redacted>

A failure was reported in "git log --graph --all" with the new
graph-rendering logic. Create a test case that matches the
topology of that example and uses an explicit ref ordering instead
of the "--all" option. The test would fail with the following error:

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

The situation is a little complicated, so let's break it down.

First off, thanks for digging into this so promptly. Your solution looks
correct to me. Everything else I'll mention here are nits. :)

Your commit message starts off talking about the test, but without
describing what's interesting about it. I think the answer is that we
have two "skewed" merge parents for the same merge; maybe it would make
sense to lead with that. I.e.:

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

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

    [your diagram]

  we trigger an assert():

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

  ...and so on...

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.

Thanks for this final sentence; writing that out in English made the
purpose of the assert() much clearer.

That could perhaps be an argument in favor of writing it as a BUG()
with a similar human-readable explanation. I guess there was already a
comment in the code, but it didn't quite click with me as much as what
you wrote above.

It is actually the _second_ collapsing line that hits this assert.
The reason we are in this code path is because we are collapsing
the first line, and it in that case we are hitting our target now

s/it//

that the horizontal line is complete. However, the second line
cannot be a horizontal line, so it will collapse without horizontal
lines. In this case, it is inappropriate to assert that we have
reached our target, as we need to continue for another column
before reaching the target. Dropping the assert is safe here.

I think that makes sense. My big concern here is that the assert() was
preventing us from looking out of bounds in the graph->mapping array,
but I don't think that's the case here.

Worth mentioning that this was due to 0f0f389f12 (graph: tidy up display
of left-skewed merges, 2019-10-15), in case somebody has to later dig
deeper?

Second, the horizontal lines in that first line drop their coloring.
This is due to a use of graph_line_addch() instead of
graph_line_write_column(). Using a ternary operator to pick the
character is nice for compact code, but we actually need a column
to provide the color.

It seems like this is a totally separate bug, and could be its own
commit?

I think it's also caused by 0f0f389f12, which actually introduced the
seen_parent mechanism that you're correcting here.

Helped-by: Jeff King [off-list ref]
Reported-by: Bradley Smith <redacted>

I don't know that I did much, but OK. :)

Thanks once again Bradley for the reproducible case.

This nicely covers the assert() problem. Could we check the same case
with "--color" and test_decode_color to check the coloring issue (see
t4214 for some prior art)?

-Peff