Fix two bugs in graph.c #517
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
derrickstolee
force-pushed
the
graph-assert-fix
branch
2 times, most recently
from
841c949
to
f74e82b
Compare
|
/submit |
|
Submitted as pull.517.git.1578408947.gitgitgadget@gmail.com |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -1063,7 +1063,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l | |||
| int i, j; | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> 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 <peff@peff.net>
> Reported-by: Bradley Smith <brad@brad-smith.co.uk>
I don't know that I did much, but OK. :)
Thanks once again Bradley for the reproducible case.
> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> index 18709a723e..ddf6f6f5d3 100755
> --- a/t/t4215-log-skewed-merges.sh
> +++ b/t/t4215-log-skewed-merges.sh
> @@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
> EOF
> '
>
> +test_expect_success 'log --graph with multiple tips' '
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
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 1/7/2020 10:30 AM, Jeff King wrote:
> On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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...
Good points.
>> 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?
Can do.
>> 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.
You're right. These are better split. Any idea how to test the color?
(I'm pretty sure we have some tests for this... I can dig around.)
>> Helped-by: Jeff King <peff@peff.net>
>> Reported-by: Bradley Smith <brad@brad-smith.co.uk>
>
> I don't know that I did much, but OK. :)
>
> Thanks once again Bradley for the reproducible case.
>
>> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
>> index 18709a723e..ddf6f6f5d3 100755
>> --- a/t/t4215-log-skewed-merges.sh
>> +++ b/t/t4215-log-skewed-merges.sh
>> @@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
>> EOF
>> '
>>
>> +test_expect_success 'log --graph with multiple tips' '
>
> 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)?
Thanks for pointing me to existing color tests. I'll add one to my v2.
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <peff@peff.net> writes:
>> 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 so.
And with that removed, all that remains would be a removal of the
assert() plus an additional test?
>> +test_expect_success 'log --graph with multiple tips' '
>
> 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)?
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> 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 so.
>
> And with that removed, all that remains would be a removal of the
> assert() plus an additional test?
Yes, though note that the color thing is a v2.25 regression as well. So
we'd probably want both of them.
-Peff
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <peff@peff.net> writes:
> On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> >> 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 so.
>>
>> And with that removed, all that remains would be a removal of the
>> assert() plus an additional test?
>
> Yes, though note that the color thing is a v2.25 regression as well. So
> we'd probably want both of them.
Sure. Those two would make perfect pair of commits to finish -rc2 with.
Thanks.
graph.c
Outdated
| @@ -6,6 +6,8 @@ | |||
| #include "revision.h" | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The assert() macro is sometimes compiled out. Instead, switch these into
> BUG() statements using our own custom macro.
>
> Reported-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
I can buy the argument that compiling with and without NDEBUG can lead
to confusion. But if that is the case, wouldn't it be so for all of the
assert() calls, not just ones in the graph code?
Previous discussions[1] seemed to conclude that having a kernel-style
BUG_ON() is probably the right way forward. I.e., replace this:
> +#define graph_assert(exp) if (!(exp)) { BUG("assert failed: "#exp""); }
with something similar in git-compat-util.h. Even if we don't convert
everybody to it immediately, it would be available for use.
At any rate, I think this patch (and the third one) can be post-v2.25.
But we'd want the first one before the release.
-Peff
[1] https://lore.kernel.org/git/20171122223827.26773-1-sbeller@google.com/
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Tue, Jan 7, 2020 at 10:36 AM Jeff King <peff@peff.net> wrote:
> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:
> > The assert() macro is sometimes compiled out. Instead, switch these into
> > BUG() statements using our own custom macro.
>
> I can buy the argument that compiling with and without NDEBUG can lead
> to confusion. But if that is the case, wouldn't it be so for all of the
> assert() calls, not just ones in the graph code?
This wasn't just a matter of potential confusion. It's one thing to
have assert()s in the code in general, but another thing when a
scripted test specifically depends upon the asserted condition, as was
the case with the test as originally proposed. Since the final patch
series removes that particular assert() altogether, it's perhaps not
that important anymore.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 1/7/2020 10:51 AM, Eric Sunshine wrote:
> On Tue, Jan 7, 2020 at 10:36 AM Jeff King <peff@peff.net> wrote:
>> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:
>>> The assert() macro is sometimes compiled out. Instead, switch these into
>>> BUG() statements using our own custom macro.
>>
>> I can buy the argument that compiling with and without NDEBUG can lead
>> to confusion. But if that is the case, wouldn't it be so for all of the
>> assert() calls, not just ones in the graph code?
>
> This wasn't just a matter of potential confusion. It's one thing to
> have assert()s in the code in general, but another thing when a
> scripted test specifically depends upon the asserted condition, as was
> the case with the test as originally proposed. Since the final patch
> series removes that particular assert() altogether, it's perhaps not
> that important anymore.
I'm happy to drop this commit, too. I misunderstood your point.
-Stolee
| @@ -240,4 +240,109 @@ test_expect_success 'log --graph with octopus merge with column joining its penu | |||
| EOF | |||
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
> exactly the topology of a reported failure in "git log --graph". While
> investigating the fix, we realized that multiple edges that could
> collapse with horizontal lines were not doing so.
Thanks for constructing this larger case.
As for including this patch, I could take or leave it for now. I like
the idea of documenting things further, but unless it's marked
expect_failure, I don't think it's going to call anybody's attention
more than this thread already has.
So I'd love to hear what James thinks should happen here, given that
it's an extension of his other work. But I'd just as soon punt on the
patch until we decide whether it _should_ change (and then either mark
it with expect_failure, or include the test along with a patch changing
the behavior).
-Peff
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Jeff King <peff@peff.net> writes:
> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
>> exactly the topology of a reported failure in "git log --graph". While
>> investigating the fix, we realized that multiple edges that could
>> collapse with horizontal lines were not doing so.
>
> Thanks for constructing this larger case.
>
> As for including this patch, I could take or leave it for now. I like
> the idea of documenting things further, but unless it's marked
> expect_failure, I don't think it's going to call anybody's attention
> more than this thread already has.
>
> So I'd love to hear what James thinks should happen here, given that
> it's an extension of his other work. But I'd just as soon punt on the
> patch until we decide whether it _should_ change (and then either mark
> it with expect_failure, or include the test along with a patch changing
> the behavior).
... and nobody CC'ed the person from whom they want to hear opinion?
;-)
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
> >
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
> >> exactly the topology of a reported failure in "git log --graph". While
> >> investigating the fix, we realized that multiple edges that could
> >> collapse with horizontal lines were not doing so.
> >
> > Thanks for constructing this larger case.
> >
> > As for including this patch, I could take or leave it for now. I like
> > the idea of documenting things further, but unless it's marked
> > expect_failure, I don't think it's going to call anybody's attention
> > more than this thread already has.
> >
> > So I'd love to hear what James thinks should happen here, given that
> > it's an extension of his other work. But I'd just as soon punt on the
> > patch until we decide whether it _should_ change (and then either mark
> > it with expect_failure, or include the test along with a patch changing
> > the behavior).
>
> ... and nobody CC'ed the person from whom they want to hear opinion?
Doh, thank you. :) I cc'd him in the earlier thread, but GGG creates a
new one.
-Peff
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 1/7/2020 1:04 PM, Jeff King wrote:
> On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>
>>>> From: Derrick Stolee <dstolee@microsoft.com>
>>>>
>>>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate
>>>> exactly the topology of a reported failure in "git log --graph". While
>>>> investigating the fix, we realized that multiple edges that could
>>>> collapse with horizontal lines were not doing so.
>>>
>>> Thanks for constructing this larger case.
>>>
>>> As for including this patch, I could take or leave it for now. I like
>>> the idea of documenting things further, but unless it's marked
>>> expect_failure, I don't think it's going to call anybody's attention
>>> more than this thread already has.
I think that's fine. I do believe this example demonstrates that the
behavior has changed, so adding an expect_failure (with the correct
expected output) may provide more motivation to fix the issue.
>>> So I'd love to hear what James thinks should happen here, given that
>>> it's an extension of his other work. But I'd just as soon punt on the
>>> patch until we decide whether it _should_ change (and then either mark
>>> it with expect_failure, or include the test along with a patch changing
>>> the behavior).
>>
>> ... and nobody CC'ed the person from whom they want to hear opinion?
>
> Doh, thank you. :) I cc'd him in the earlier thread, but GGG creates a
> new one.
Sorry, that was my fault. I definitely intended to CC him, but forgot
when going through those who _wrote_ on the previous thread.
-Stolee
|
On the Git mailing list, Junio C Hamano wrote (reply to this): |
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 eaf158f (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: | | | | * | |_|_|/| |/| | |/ | | |/| | |/| | | * | | * | | | 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 in that case we are hitting our target now 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. The new behavior in 0f0f389 (graph: tidy up display of left-skewed merges, 2019-10-15) caused the behavior change that made this assertion failure possible. In addition to making the assert possible, it also changed how multiple edges collapse. 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. Helped-by: Jeff King <peff@peff.net> Reported-by: Bradley Smith <brad@brad-smith.co.uk> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
In some cases, horizontal lines in rendered graphs can lose 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. Add a test to t4215-log-skewed-merges.sh to prevent regression. Reported-by: Jeff King <peff@peff.net> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
derrickstolee
force-pushed
the
graph-assert-fix
branch
from
f74e82b
to
11887bd
Compare
|
/submit |
|
Submitted as pull.517.v2.git.1578432422.gitgitgadget@gmail.com |
| @@ -1219,13 +1219,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l | |||
| * | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
> 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.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <gitster@pobox.com> writes:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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
> ...
> near the beginning of this commit, though.
Will do this locally before using them in rc2.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 1/8/2020 12:34 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> 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
>> ...
>> near the beginning of this commit, though.
>
> Will do this locally before using them in rc2.
Sounds good to me. Sorry for the delay in doing it myself.
-Stolee
| @@ -1063,7 +1063,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l | |||
| int i, j; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> In some cases, horizontal lines in rendered graphs can lose 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.
>
> Add a test to t4215-log-skewed-merges.sh to prevent regression.
>
> Reported-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
Thanks. Looks good.
|
This branch is now known as |
|
This patch series was integrated into pu via git@0539b8b. |
|
This patch series was integrated into pu via git@bc14e23. |
|
This patch series was integrated into next via git@4b896fb. |
|
This patch series was integrated into pu via git@1f5f3ff. |
|
This patch series was integrated into next via git@1f5f3ff. |
|
This patch series was integrated into master via git@1f5f3ff. |
|
Closed via 1f5f3ff. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a possible fix for the bug reported in [1].
The first commit fixes the runtime failure due to the assert() statement.
The second commit replaces the assert() statements with a macro that triggers a BUG().
The third commit adds another test that shows a more complicated example and how the new code in v2.25.0-rc1 has a behavior change that is not necessarily wanted.
Thanks,
-Stolee
[1] https://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/
Cc: peff@peff.net, brad@brad-smith.co.uk, sunshine@sunshineco.com, James Coglan jcoglan@gmail.com