{"thread":{"id":"52582","subject":"[PATCH 0/3] Fix two bugs in graph.c","startedAt":"2020-01-07T14:55:51Z","lastAt":"2020-01-08T19:14:42Z","messageCount":24,"participants":["Derrick Stolee via GitGitGadget","Jeff King","Eric Sunshine","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"389376","messageId":"pull.517.git.1578408947.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":null,"subject":"[PATCH 0/3] Fix two bugs in graph.c","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T14:55:44Z","receivedAt":"2020-01-07T14:55:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is a possible fix for the bug reported in [1].\n\nThe first commit fixes the runtime failure due to the assert() statement.\n\nThe second commit replaces the assert() statements with a macro that\ntriggers a BUG().\n\nThe third commit adds another test that shows a more complicated example and\nhow the new code in v2.25.0-rc1 has a behavior change that is not\nnecessarily wanted.\n\nThanks, -Stolee\n\n[1] \nhttps://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/\n\nDerrick Stolee (3):\n graph: fix case that hit assert()\n graph: replace assert() with graph_assert() macro\n t4215: add bigger graph collapse test\n\n graph.c | 39 +++++++------\n t/t4215-log-skewed-merges.sh | 105 +++++++++++++++++++++++++++++++++++\n 2 files changed, 126 insertions(+), 18 deletions(-)\n\n\nbase-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-517%2Fderrickstolee%2Fgraph-assert-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-517/derrickstolee/graph-assert-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/517\n-- \ngitgitgadget\n"},{"id":"389377","messageId":"5dd305d2f0de43a70b46336c8f1a62437e0511e1.1578408947.git.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.git.1578408947.gitgitgadget@gmail.com","subject":"[PATCH 2/3] graph: replace assert() with graph_assert() macro","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T14:55:46Z","receivedAt":"2020-01-07T14:55:53Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe assert() macro is sometimes compiled out. Instead, switch these into\nBUG() statements using our own custom macro.\n\nReported-by: Eric Sunshine \nSigned-off-by: Derrick Stolee \n---\n graph.c | 22 ++++++++++++----------\n 1 file changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex aaf97069bd..ac78bdf96c 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -6,6 +6,8 @@\n #include \"revision.h\"\n #include \"argv-array.h\"\n \n+#define graph_assert(exp) if (!(exp)) { BUG(\"assert failed: \"#exp\"\"); }\n+\n /* Internal API */\n \n /*\n@@ -316,7 +318,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void\n \tstruct git_graph *graph = data;\n \tstatic struct strbuf msgbuf = STRBUF_INIT;\n \n-\tassert(opt);\n+\tgraph_assert(opt);\n \n \tstrbuf_reset(&msgbuf);\n \tif (opt->line_prefix)\n@@ -865,13 +867,13 @@ static void graph_output_pre_commit_line(struct git_graph *graph,\n \t *\n \t * We need 2 extra rows for every parent over 2.\n \t */\n-\tassert(graph->num_parents >= 3);\n+\tgraph_assert(graph->num_parents >= 3);\n \n \t/*\n \t * graph->expansion_row tracks the current expansion row we are on.\n \t * It should be in the range [0, num_expansion_rows - 1]\n \t */\n-\tassert(0 <= graph->expansion_row &&\n+\tgraph_assert(0 <= graph->expansion_row &&\n \t graph->expansion_row < graph_num_expansion_rows(graph));\n \n \t/*\n@@ -923,13 +925,13 @@ static void graph_output_commit_char(struct git_graph *graph, struct graph_line\n \t * (We should only see boundary commits when revs->boundary is set.)\n \t */\n \tif (graph->commit->object.flags & BOUNDARY) {\n-\t\tassert(graph->revs->boundary);\n+\t\tgraph_assert(graph->revs->boundary);\n \t\tgraph_line_addch(line, 'o');\n \t\treturn;\n \t}\n \n \t/*\n-\t * get_revision_mark() handles all other cases without assert()\n+\t * get_revision_mark() handles all other cases without graph_assert()\n \t */\n \tgraph_line_addstr(line, get_revision_mark(graph->revs, graph->commit));\n }\n@@ -1094,7 +1096,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \n \t\t\tfor (j = 0; j < graph->num_parents; j++) {\n \t\t\t\tpar_column = graph_find_new_column_by_commit(graph, parents->item);\n-\t\t\t\tassert(par_column >= 0);\n+\t\t\t\tgraph_assert(par_column >= 0);\n \n \t\t\t\tc = merge_chars[idx];\n \t\t\t\tgraph_line_write_column(line, &graph->new_columns[par_column], c);\n@@ -1172,14 +1174,14 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l\n \t\t * the graph much more legible, since whenever branches\n \t\t * cross, only one is moving directions.\n \t\t */\n-\t\tassert(target * 2 <= i);\n+\t\tgraph_assert(target * 2 <= i);\n \n \t\tif (target * 2 == i) {\n \t\t\t/*\n \t\t\t * This column is already in the\n \t\t\t * correct place\n \t\t\t */\n-\t\t\tassert(graph->mapping[i] == -1);\n+\t\t\tgraph_assert(graph->mapping[i] == -1);\n \t\t\tgraph->mapping[i] = target;\n \t\t} else if (graph->mapping[i - 1] < 0) {\n \t\t\t/*\n@@ -1225,8 +1227,8 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n \t\t\t */\n-\t\t\tassert(graph->mapping[i - 1] > target);\n-\t\t\tassert(graph->mapping[i - 2] < 0);\n+\t\t\tgraph_assert(graph->mapping[i - 1] > target);\n+\t\t\tgraph_assert(graph->mapping[i - 2] < 0);\n \t\t\tgraph->mapping[i - 2] = target;\n \t\t\t/*\n \t\t\t * Mark this branch as the horizontal edge to\n-- \ngitgitgadget\n\n"},{"id":"389378","messageId":"65186f3ded251e0bcf1fcb18160163a3efd97c37.1578408947.git.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.git.1578408947.gitgitgadget@gmail.com","subject":"[PATCH 1/3] graph: fix case that hit assert()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T14:55:45Z","receivedAt":"2020-01-07T14:55:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nA failure was reported in \"git log --graph --all\" with the new\ngraph-rendering logic. Create a test case that matches the\ntopology of that example and uses an explicit ref ordering instead\nof the \"--all\" option. The test would fail with the following error:\n\n\tgraph.c:1228: graph_output_collapsing_line: Assertion\n\t\t `graph->mapping[i - 3] == target' failed.\n\nThe situation is a little complicated, so let's break it down.\n\nThe assert was introduced by eaf158f8 (\"graph API: Use horizontal\nlines for more compact graphs\", 2009-04-21), which is quite old.\nThis assert is trying to say that when we complete a horizontal\nline with a single slash, it is because we have reached our target.\n\nThis assertion is hit when we have two collapsing lines from the\nsame merge commit, as follows:\n\n | | | | *\n | |_|_|/|\n |/| | |/\n | | |/|\n | |/| |\n | * | |\n * | | |\n\nIt is actually the _second_ collapsing line that hits this assert.\nThe reason we are in this code path is because we are collapsing\nthe first line, and it in that case we are hitting our target now\nthat the horizontal line is complete. However, the second line\ncannot be a horizontal line, so it will collapse without horizontal\nlines. In this case, it is inappropriate to assert that we have\nreached our target, as we need to continue for another column\nbefore reaching the target. Dropping the assert is safe here.\n\nSecond, the horizontal lines in that first line drop their coloring.\nThis is due to a use of graph_line_addch() instead of\ngraph_line_write_column(). Using a ternary operator to pick the\ncharacter is nice for compact code, but we actually need a column\nto provide the color.\n\nHelped-by: Jeff King \nReported-by: Bradley Smith \nSigned-off-by: Derrick Stolee \n---\n graph.c | 17 ++++++++-------\n t/t4215-log-skewed-merges.sh | 42 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 8 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 66ae18add8..aaf97069bd 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1063,7 +1063,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \tint i, j;\n \n \tstruct commit_list *first_parent = first_interesting_parent(graph);\n-\tint seen_parent = 0;\n+\tstruct column *parent_col = NULL;\n \n \t/*\n \t * Output the post-merge row\n@@ -1117,12 +1117,17 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \t\t\tgraph_line_addch(line, ' ');\n \t\t} else {\n \t\t\tgraph_line_write_column(line, col, '|');\n-\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1)\n-\t\t\t\tgraph_line_addch(line, seen_parent ? '_' : ' ');\n+\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1) {\n+\t\t\t\tif (parent_col)\n+\t\t\t\t\tgraph_line_write_column(\n+\t\t\t\t\t\tline, parent_col, '_');\n+\t\t\t\telse\n+\t\t\t\t\tgraph_line_addch(line, ' ');\n+\t\t\t}\n \t\t}\n \n \t\tif (col_commit == first_parent->item)\n-\t\t\tseen_parent = 1;\n+\t\t\tparent_col = col;\n \t}\n \n \t/*\n@@ -1219,13 +1224,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l\n \t\t\t *\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n-\t\t\t *\n-\t\t\t * The branch to the left of that space\n-\t\t\t * should be our eventual target.\n \t\t\t */\n \t\t\tassert(graph->mapping[i - 1] > target);\n \t\t\tassert(graph->mapping[i - 2] < 0);\n-\t\t\tassert(graph->mapping[i - 3] == target);\n \t\t\tgraph->mapping[i - 2] = target;\n \t\t\t/*\n \t\t\t * Mark this branch as the horizontal edge to\ndiff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\nindex 18709a723e..ddf6f6f5d3 100755\n--- a/t/t4215-log-skewed-merges.sh\n+++ b/t/t4215-log-skewed-merges.sh\n@@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu\n \tEOF\n '\n \n+test_expect_success 'log --graph with multiple tips' '\n+\tgit checkout --orphan 6_1 &&\n+\ttest_commit 6_A &&\n+\tgit branch 6_2 &&\n+\tgit branch 6_4 &&\n+\ttest_commit 6_B &&\n+\tgit branch 6_3 &&\n+\ttest_commit 6_C &&\n+\tgit checkout 6_2 && test_commit 6_D &&\n+\tgit checkout 6_3 && test_commit 6_E &&\n+\tgit checkout -b 6_5 6_1 &&\n+\tgit merge --no-ff 6_2 -m 6_F &&\n+\tgit checkout 6_4 && test_commit 6_G &&\n+\tgit checkout 6_3 &&\n+\tgit merge --no-ff 6_4 -m 6_H &&\n+\tgit checkout 6_1 &&\n+\tgit merge --no-ff 6_2 -m 6_I &&\n+\n+\tcheck_graph 6_1 6_3 6_5 <<-\\EOF\n+\t* 6_I\n+\t|\\\n+\t| | * 6_H\n+\t| | |\\\n+\t| | | * 6_G\n+\t| | * | 6_E\n+\t| | | | * 6_F\n+\t| |_|_|/|\n+\t|/| | |/\n+\t| | |/|\n+\t| |/| |\n+\t| * | | 6_D\n+\t| | |/\n+\t| |/|\n+\t* | | 6_C\n+\t| |/\n+\t|/|\n+\t* | 6_B\n+\t|/\n+\t* 6_A\n+\tEOF\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"389379","messageId":"f74e82bea68701beb734537cafd147162d1bb2c6.1578408947.git.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.git.1578408947.gitgitgadget@gmail.com","subject":"[PATCH 3/3] t4215: add bigger graph collapse test","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T14:55:47Z","receivedAt":"2020-01-07T14:55:55Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nA previous test in t4215-log-skewed-merges.sh was added to demonstrate\nexactly the topology of a reported failure in \"git log --graph\". While\ninvestigating the fix, we realized that multiple edges that could\ncollapse with horizontal lines were not doing so.\n\nSpecifically, examine this section of the graph:\n\n\t| | | | | | * 7_M4\n\t| |_|_|_|_|/|\\\n\t|/| | | | |/ /\n\t| | | | |/| /\n\t| | | |/| |/\n\t| | |/| |/|\n\t| |/| |/| |\n\t| | |/| | |\n\t| | * | | | 7_G\n\nDocument this behavior with a test. This behavior is new, as the\nbehavior in v2.24.1 has the following output:\n\n\t| | | | | | *-. 7_M4\n\t| | | | | | |\\ \\\n\t| |_|_|_|_|/ / /\n\t|/| | | | | / /\n\t| | |_|_|_|/ /\n\t| |/| | | | /\n\t| | | |_|_|/\n\t| | |/| | |\n\t| | * | | | 7_G\n\nThe behavior changed logically in 479db18b (\"graph: smooth appearance\nof collapsing edges on commit lines\", 2019-10-15), but was actually\nbroken due to an assert() bug in 458152cc (\"graph: example of graph\noutput that can be simplified\", 2019-10-15). A future change could\nmodify this behavior to do the following instead:\n\n\t| | | | | | * 7_M4\n\t| |_|_|_|_|/|\\\n\t|/| | | | |/ /\n\t| | |_|_|/| /\n\t| |/| | | |/\n\t| | | |_|/|\n\t| | |/| | |\n\t| | * | | | 7_G\n\nSigned-off-by: Derrick Stolee \n---\n t/t4215-log-skewed-merges.sh | 63 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 63 insertions(+)\n\ndiff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\nindex ddf6f6f5d3..be2c24564b 100755\n--- a/t/t4215-log-skewed-merges.sh\n+++ b/t/t4215-log-skewed-merges.sh\n@@ -282,4 +282,67 @@ test_expect_success 'log --graph with multiple tips' '\n \tEOF\n '\n \n+test_expect_success 'log --graph with multiple tips' '\n+\tgit checkout --orphan 7_1 &&\n+\ttest_commit 7_A &&\n+\ttest_commit 7_B &&\n+\ttest_commit 7_C &&\n+\tgit checkout -b 7_2 7_1~2 &&\n+\ttest_commit 7_D &&\n+\ttest_commit 7_E &&\n+\tgit checkout -b 7_3 7_1~1 &&\n+\ttest_commit 7_F &&\n+\ttest_commit 7_G &&\n+\tgit checkout -b 7_4 7_2~1 &&\n+\ttest_commit 7_H &&\n+\tgit checkout -b 7_5 7_1~2 &&\n+\ttest_commit 7_I &&\n+\tgit checkout -b 7_6 7_3~1 &&\n+\ttest_commit 7_J &&\n+\tgit checkout -b M_1 7_1 &&\n+\tgit merge --no-ff 7_2 -m 7_M1 &&\n+\tgit checkout -b M_3 7_3 &&\n+\tgit merge --no-ff 7_4 -m 7_M2 &&\n+\tgit checkout -b M_5 7_5 &&\n+\tgit merge --no-ff 7_6 -m 7_M3 &&\n+\tgit checkout -b M_7 7_1 &&\n+\tgit merge --no-ff 7_2 7_3 -m 7_M4 &&\n+\n+\tcheck_graph M_1 M_3 M_5 M_7 <<-\\EOF\n+\t* 7_M1\n+\t|\\\n+\t| | * 7_M2\n+\t| | |\\\n+\t| | | * 7_H\n+\t| | | | * 7_M3\n+\t| | | | |\\\n+\t| | | | | * 7_J\n+\t| | | | * | 7_I\n+\t| | | | | | * 7_M4\n+\t| |_|_|_|_|/|\\\n+\t|/| | | | |/ /\n+\t| | | | |/| /\n+\t| | | |/| |/\n+\t| | |/| |/|\n+\t| |/| |/| |\n+\t| | |/| | |\n+\t| | * | | | 7_G\n+\t| | | |_|/\n+\t| | |/| |\n+\t| | * | | 7_F\n+\t| * | | | 7_E\n+\t| | |/ /\n+\t| |/| |\n+\t| * | | 7_D\n+\t| | |/\n+\t| |/|\n+\t* | | 7_C\n+\t| |/\n+\t|/|\n+\t* | 7_B\n+\t|/\n+\t* 7_A\n+\tEOF\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"389384","messageId":"20200107153006.GA20591@coredump.intra.peff.net","threadId":"52582","inReplyTo":"65186f3ded251e0bcf1fcb18160163a3efd97c37.1578408947.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] graph: fix case that hit assert()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T15:30:06Z","receivedAt":"2020-01-07T15:30:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> A failure was reported in \"git log --graph --all\" with the new\n> graph-rendering logic. Create a test case that matches the\n> topology of that example and uses an explicit ref ordering instead\n> of the \"--all\" option. The test would fail with the following error:\n> \n> \tgraph.c:1228: graph_output_collapsing_line: Assertion\n> \t\t `graph->mapping[i - 3] == target' failed.\n> \n> The situation is a little complicated, so let's break it down.\n\nFirst off, thanks for digging into this so promptly. Your solution looks\ncorrect to me. Everything else I'll mention here are nits. :)\n\nYour commit message starts off talking about the test, but without\ndescribing what's interesting about it. I think the answer is that we\nhave two \"skewed\" merge parents for the same merge; maybe it would make\nsense to lead with that. I.e.:\n\n Subject: graph: drop assert() for merge with two collapsing parents\n\n When \"git log --graph\" shows a merge commit that has two collapsing\n lines, like:\n\n [your diagram]\n\n we trigger an assert():\n\n graph.c:1228: graph_output_collapsing_line: Assertion\n `graph->mapping[i - 3] == target' failed.\n\n ...and so on...\n\n> The assert was introduced by eaf158f8 (\"graph API: Use horizontal\n> lines for more compact graphs\", 2009-04-21), which is quite old.\n> This assert is trying to say that when we complete a horizontal\n> line with a single slash, it is because we have reached our target.\n\nThanks for this final sentence; writing that out in English made the\npurpose of the assert() much clearer.\n\nThat could perhaps be an argument in favor of writing it as a BUG()\nwith a similar human-readable explanation. I guess there was already a\ncomment in the code, but it didn't quite click with me as much as what\nyou wrote above.\n\n> It is actually the _second_ collapsing line that hits this assert.\n> The reason we are in this code path is because we are collapsing\n> the first line, and it in that case we are hitting our target now\n\ns/it//\n\n> that the horizontal line is complete. However, the second line\n> cannot be a horizontal line, so it will collapse without horizontal\n> lines. In this case, it is inappropriate to assert that we have\n> reached our target, as we need to continue for another column\n> before reaching the target. Dropping the assert is safe here.\n\nI think that makes sense. My big concern here is that the assert() was\npreventing us from looking out of bounds in the graph->mapping array,\nbut I don't think that's the case here.\n\nWorth mentioning that this was due to 0f0f389f12 (graph: tidy up display\nof left-skewed merges, 2019-10-15), in case somebody has to later dig\ndeeper?\n\n> Second, the horizontal lines in that first line drop their coloring.\n> This is due to a use of graph_line_addch() instead of\n> graph_line_write_column(). Using a ternary operator to pick the\n> character is nice for compact code, but we actually need a column\n> to provide the color.\n\nIt seems like this is a totally separate bug, and could be its own\ncommit?\n\nI think it's also caused by 0f0f389f12, which actually introduced the\nseen_parent mechanism that you're correcting here.\n\n> Helped-by: Jeff King \n> Reported-by: Bradley Smith \n\nI don't know that I did much, but OK. :)\n\nThanks once again Bradley for the reproducible case.\n\n> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\n> index 18709a723e..ddf6f6f5d3 100755\n> --- a/t/t4215-log-skewed-merges.sh\n> +++ b/t/t4215-log-skewed-merges.sh\n> @@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu\n> \tEOF\n> '\n> \n> +test_expect_success 'log --graph with multiple tips' '\n\nThis nicely covers the assert() problem. Could we check the same case\nwith \"--color\" and test_decode_color to check the coloring issue (see\nt4214 for some prior art)?\n\n-Peff\n"},{"id":"389385","messageId":"20200107153651.GB20591@coredump.intra.peff.net","threadId":"52582","inReplyTo":"5dd305d2f0de43a70b46336c8f1a62437e0511e1.1578408947.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] graph: replace assert() with graph_assert() macro","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T15:36:51Z","receivedAt":"2020-01-07T15:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> The assert() macro is sometimes compiled out. Instead, switch these into\n> BUG() statements using our own custom macro.\n> \n> Reported-by: Eric Sunshine \n> Signed-off-by: Derrick Stolee \n\nI can buy the argument that compiling with and without NDEBUG can lead\nto confusion. But if that is the case, wouldn't it be so for all of the\nassert() calls, not just ones in the graph code?\n\nPrevious discussions[1] seemed to conclude that having a kernel-style\nBUG_ON() is probably the right way forward. I.e., replace this:\n\n> +#define graph_assert(exp) if (!(exp)) { BUG(\"assert failed: \"#exp\"\"); }\n\nwith something similar in git-compat-util.h. Even if we don't convert\neverybody to it immediately, it would be available for use.\n\nAt any rate, I think this patch (and the third one) can be post-v2.25.\nBut we'd want the first one before the release.\n\n-Peff\n\n[1] https://lore.kernel.org/git/20171122223827.26773-1-sbeller@google.com/\n"},{"id":"389387","messageId":"20200107153922.GC20591@coredump.intra.peff.net","threadId":"52582","inReplyTo":"f74e82bea68701beb734537cafd147162d1bb2c6.1578408947.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] t4215: add bigger graph collapse test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T15:39:22Z","receivedAt":"2020-01-07T15:39:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n> \n> A previous test in t4215-log-skewed-merges.sh was added to demonstrate\n> exactly the topology of a reported failure in \"git log --graph\". While\n> investigating the fix, we realized that multiple edges that could\n> collapse with horizontal lines were not doing so.\n\nThanks for constructing this larger case.\n\nAs for including this patch, I could take or leave it for now. I like\nthe idea of documenting things further, but unless it's marked\nexpect_failure, I don't think it's going to call anybody's attention\nmore than this thread already has.\n\nSo I'd love to hear what James thinks should happen here, given that\nit's an extension of his other work. But I'd just as soon punt on the\npatch until we decide whether it _should_ change (and then either mark\nit with expect_failure, or include the test along with a patch changing\nthe behavior).\n\n-Peff\n"},{"id":"389388","messageId":"CAPig+cTu=iAeQNm8z53cyG8C1dgokpZBvRVgev091nBFg8tCXQ@mail.gmail.com","threadId":"52582","inReplyTo":"20200107153651.GB20591@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] graph: replace assert() with graph_assert() macro","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-07T15:51:39Z","receivedAt":"2020-01-07T15:51:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 7, 2020 at 10:36 AM Jeff King wrote:\n> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:\n> > The assert() macro is sometimes compiled out. Instead, switch these into\n> > BUG() statements using our own custom macro.\n>\n> I can buy the argument that compiling with and without NDEBUG can lead\n> to confusion. But if that is the case, wouldn't it be so for all of the\n> assert() calls, not just ones in the graph code?\n\nThis wasn't just a matter of potential confusion. It's one thing to\nhave assert()s in the code in general, but another thing when a\nscripted test specifically depends upon the asserted condition, as was\nthe case with the test as originally proposed. Since the final patch\nseries removes that particular assert() altogether, it's perhaps not\nthat important anymore.\n"},{"id":"389399","messageId":"xmqqk1635gwu.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"pull.517.git.1578408947.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/3] Fix two bugs in graph.c","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T17:13:05Z","receivedAt":"2020-01-07T17:13:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This is a possible fix for the bug reported in [1].\n>\n> The first commit fixes the runtime failure due to the assert() statement.\n>\n> The second commit replaces the assert() statements with a macro that\n> triggers a BUG().\n>\n> The third commit adds another test that shows a more complicated example and\n> how the new code in v2.25.0-rc1 has a behavior change that is not\n> necessarily wanted.\n>\n> Thanks, -Stolee\n\nThanks, all, for so quickly resolving the issue.\n\nWill queue.\n\n>\n> [1] \n> https://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/\n\n\n\n>\n> Derrick Stolee (3):\n> graph: fix case that hit assert()\n> graph: replace assert() with graph_assert() macro\n> t4215: add bigger graph collapse test\n>\n> graph.c | 39 +++++++------\n> t/t4215-log-skewed-merges.sh | 105 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 126 insertions(+), 18 deletions(-)\n>\n>\n> base-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-517%2Fderrickstolee%2Fgraph-assert-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-517/derrickstolee/graph-assert-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/517\n"},{"id":"389402","messageId":"xmqqftgr5elz.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"20200107153922.GC20591@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] t4215: add bigger graph collapse test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T18:02:48Z","receivedAt":"2020-01-07T18:02:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:\n>\n>> From: Derrick Stolee \n>> \n>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate\n>> exactly the topology of a reported failure in \"git log --graph\". While\n>> investigating the fix, we realized that multiple edges that could\n>> collapse with horizontal lines were not doing so.\n>\n> Thanks for constructing this larger case.\n>\n> As for including this patch, I could take or leave it for now. I like\n> the idea of documenting things further, but unless it's marked\n> expect_failure, I don't think it's going to call anybody's attention\n> more than this thread already has.\n>\n> So I'd love to hear what James thinks should happen here, given that\n> it's an extension of his other work. But I'd just as soon punt on the\n> patch until we decide whether it _should_ change (and then either mark\n> it with expect_failure, or include the test along with a patch changing\n> the behavior).\n\n... and nobody CC'ed the person from whom they want to hear opinion?\n\n;-)\n\n"},{"id":"389404","messageId":"20200107180415.GB48806@coredump.intra.peff.net","threadId":"52582","inReplyTo":"xmqqftgr5elz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] t4215: add bigger graph collapse test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T18:04:15Z","receivedAt":"2020-01-07T18:04:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:\n> >\n> >> From: Derrick Stolee \n> >> \n> >> A previous test in t4215-log-skewed-merges.sh was added to demonstrate\n> >> exactly the topology of a reported failure in \"git log --graph\". While\n> >> investigating the fix, we realized that multiple edges that could\n> >> collapse with horizontal lines were not doing so.\n> >\n> > Thanks for constructing this larger case.\n> >\n> > As for including this patch, I could take or leave it for now. I like\n> > the idea of documenting things further, but unless it's marked\n> > expect_failure, I don't think it's going to call anybody's attention\n> > more than this thread already has.\n> >\n> > So I'd love to hear what James thinks should happen here, given that\n> > it's an extension of his other work. But I'd just as soon punt on the\n> > patch until we decide whether it _should_ change (and then either mark\n> > it with expect_failure, or include the test along with a patch changing\n> > the behavior).\n> \n> ... and nobody CC'ed the person from whom they want to hear opinion?\n\nDoh, thank you. :) I cc'd him in the earlier thread, but GGG creates a\nnew one.\n\n-Peff\n"},{"id":"389405","messageId":"31fb4e1f-be0b-d81f-8c53-81d6b0b95741@gmail.com","threadId":"52582","inReplyTo":"20200107180415.GB48806@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] t4215: add bigger graph collapse test","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-07T18:44:41Z","receivedAt":"2020-01-07T18:44:44Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/7/2020 1:04 PM, Jeff King wrote:\n> On Tue, Jan 07, 2020 at 10:02:48AM -0800, Junio C Hamano wrote:\n> \n>> Jeff King writes:\n>>\n>>> On Tue, Jan 07, 2020 at 02:55:47PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>>\n>>>> From: Derrick Stolee \n>>>>\n>>>> A previous test in t4215-log-skewed-merges.sh was added to demonstrate\n>>>> exactly the topology of a reported failure in \"git log --graph\". While\n>>>> investigating the fix, we realized that multiple edges that could\n>>>> collapse with horizontal lines were not doing so.\n>>>\n>>> Thanks for constructing this larger case.\n>>>\n>>> As for including this patch, I could take or leave it for now. I like\n>>> the idea of documenting things further, but unless it's marked\n>>> expect_failure, I don't think it's going to call anybody's attention\n>>> more than this thread already has.\n\nI think that's fine. I do believe this example demonstrates that the\nbehavior has changed, so adding an expect_failure (with the correct\nexpected output) may provide more motivation to fix the issue.\n\n>>> So I'd love to hear what James thinks should happen here, given that\n>>> it's an extension of his other work. But I'd just as soon punt on the\n>>> patch until we decide whether it _should_ change (and then either mark\n>>> it with expect_failure, or include the test along with a patch changing\n>>> the behavior).\n>>\n>> ... and nobody CC'ed the person from whom they want to hear opinion?\n> \n> Doh, thank you. :) I cc'd him in the earlier thread, but GGG creates a\n> new one.\n\nSorry, that was my fault. I definitely intended to CC him, but forgot\nwhen going through those who _wrote_ on the previous thread.\n\n-Stolee\n"},{"id":"389406","messageId":"0d024980-5d5e-5818-3c90-6eb0c7e112be@gmail.com","threadId":"52582","inReplyTo":"CAPig+cTu=iAeQNm8z53cyG8C1dgokpZBvRVgev091nBFg8tCXQ@mail.gmail.com","subject":"Re: [PATCH 2/3] graph: replace assert() with graph_assert() macro","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-07T18:45:37Z","receivedAt":"2020-01-07T18:45:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/7/2020 10:51 AM, Eric Sunshine wrote:\n> On Tue, Jan 7, 2020 at 10:36 AM Jeff King wrote:\n>> On Tue, Jan 07, 2020 at 02:55:46PM +0000, Derrick Stolee via GitGitGadget wrote:\n>>> The assert() macro is sometimes compiled out. Instead, switch these into\n>>> BUG() statements using our own custom macro.\n>>\n>> I can buy the argument that compiling with and without NDEBUG can lead\n>> to confusion. But if that is the case, wouldn't it be so for all of the\n>> assert() calls, not just ones in the graph code?\n> \n> This wasn't just a matter of potential confusion. It's one thing to\n> have assert()s in the code in general, but another thing when a\n> scripted test specifically depends upon the asserted condition, as was\n> the case with the test as originally proposed. Since the final patch\n> series removes that particular assert() altogether, it's perhaps not\n> that important anymore.\n\nI'm happy to drop this commit, too. I misunderstood your point.\n\n-Stolee\n\n"},{"id":"389407","messageId":"d7a2e725-9866-61be-10ea-a21f16b88c2d@gmail.com","threadId":"52582","inReplyTo":"20200107153006.GA20591@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] graph: fix case that hit assert()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-07T18:47:54Z","receivedAt":"2020-01-07T18:47:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/7/2020 10:30 AM, Jeff King wrote:\n> On Tue, Jan 07, 2020 at 02:55:45PM +0000, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> A failure was reported in \"git log --graph --all\" with the new\n>> graph-rendering logic. Create a test case that matches the\n>> topology of that example and uses an explicit ref ordering instead\n>> of the \"--all\" option. The test would fail with the following error:\n>>\n>> \tgraph.c:1228: graph_output_collapsing_line: Assertion\n>> \t\t `graph->mapping[i - 3] == target' failed.\n>>\n>> The situation is a little complicated, so let's break it down.\n> \n> First off, thanks for digging into this so promptly. Your solution looks\n> correct to me. Everything else I'll mention here are nits. :)\n> \n> Your commit message starts off talking about the test, but without\n> describing what's interesting about it. I think the answer is that we\n> have two \"skewed\" merge parents for the same merge; maybe it would make\n> sense to lead with that. I.e.:\n> \n> Subject: graph: drop assert() for merge with two collapsing parents\n> \n> When \"git log --graph\" shows a merge commit that has two collapsing\n> lines, like:\n> \n> [your diagram]\n> \n> we trigger an assert():\n> \n> graph.c:1228: graph_output_collapsing_line: Assertion\n> `graph->mapping[i - 3] == target' failed.\n> \n> ...and so on...\n\nGood points.\n\n>> The assert was introduced by eaf158f8 (\"graph API: Use horizontal\n>> lines for more compact graphs\", 2009-04-21), which is quite old.\n>> This assert is trying to say that when we complete a horizontal\n>> line with a single slash, it is because we have reached our target.\n> \n> Thanks for this final sentence; writing that out in English made the\n> purpose of the assert() much clearer.\n> \n> That could perhaps be an argument in favor of writing it as a BUG()\n> with a similar human-readable explanation. I guess there was already a\n> comment in the code, but it didn't quite click with me as much as what\n> you wrote above.\n> \n>> It is actually the _second_ collapsing line that hits this assert.\n>> The reason we are in this code path is because we are collapsing\n>> the first line, and it in that case we are hitting our target now\n> \n> s/it//\n> \n>> that the horizontal line is complete. However, the second line\n>> cannot be a horizontal line, so it will collapse without horizontal\n>> lines. In this case, it is inappropriate to assert that we have\n>> reached our target, as we need to continue for another column\n>> before reaching the target. Dropping the assert is safe here.\n> \n> I think that makes sense. My big concern here is that the assert() was\n> preventing us from looking out of bounds in the graph->mapping array,\n> but I don't think that's the case here.\n> \n> Worth mentioning that this was due to 0f0f389f12 (graph: tidy up display\n> of left-skewed merges, 2019-10-15), in case somebody has to later dig\n> deeper?\n\nCan do.\n\n>> Second, the horizontal lines in that first line drop their coloring.\n>> This is due to a use of graph_line_addch() instead of\n>> graph_line_write_column(). Using a ternary operator to pick the\n>> character is nice for compact code, but we actually need a column\n>> to provide the color.\n> \n> It seems like this is a totally separate bug, and could be its own\n> commit?\n>\n> I think it's also caused by 0f0f389f12, which actually introduced the\n> seen_parent mechanism that you're correcting here.\n\nYou're right. These are better split. Any idea how to test the color?\n(I'm pretty sure we have some tests for this... I can dig around.)\n \n>> Helped-by: Jeff King \n>> Reported-by: Bradley Smith \n> \n> I don't know that I did much, but OK. :)\n> \n> Thanks once again Bradley for the reproducible case.\n> \n>> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\n>> index 18709a723e..ddf6f6f5d3 100755\n>> --- a/t/t4215-log-skewed-merges.sh\n>> +++ b/t/t4215-log-skewed-merges.sh\n>> @@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu\n>> \tEOF\n>> '\n>> \n>> +test_expect_success 'log --graph with multiple tips' '\n> \n> This nicely covers the assert() problem. Could we check the same case\n> with \"--color\" and test_decode_color to check the coloring issue (see\n> t4214 for some prior art)?\n\nThanks for pointing me to existing color tests. I'll add one to my v2.\n\n-Stolee\n"},{"id":"389415","messageId":"xmqqblrf5azn.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"20200107153006.GA20591@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] graph: fix case that hit assert()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T19:21:00Z","receivedAt":"2020-01-07T19:21:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Second, the horizontal lines in that first line drop their coloring.\n>> This is due to a use of graph_line_addch() instead of\n>> graph_line_write_column(). Using a ternary operator to pick the\n>> character is nice for compact code, but we actually need a column\n>> to provide the color.\n>\n> It seems like this is a totally separate bug, and could be its own\n> commit?\n\nI think so.\n\nAnd with that removed, all that remains would be a removal of the\nassert() plus an additional test?\n\n>> +test_expect_success 'log --graph with multiple tips' '\n>\n> This nicely covers the assert() problem. Could we check the same case\n> with \"--color\" and test_decode_color to check the coloring issue (see\n> t4214 for some prior art)?\n\n"},{"id":"389417","messageId":"20200107193143.GA56858@coredump.intra.peff.net","threadId":"52582","inReplyTo":"xmqqblrf5azn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] graph: fix case that hit assert()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-07T19:31:43Z","receivedAt":"2020-01-07T19:31:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> Second, the horizontal lines in that first line drop their coloring.\n> >> This is due to a use of graph_line_addch() instead of\n> >> graph_line_write_column(). Using a ternary operator to pick the\n> >> character is nice for compact code, but we actually need a column\n> >> to provide the color.\n> >\n> > It seems like this is a totally separate bug, and could be its own\n> > commit?\n> \n> I think so.\n> \n> And with that removed, all that remains would be a removal of the\n> assert() plus an additional test?\n\nYes, though note that the color thing is a v2.25 regression as well. So\nwe'd probably want both of them.\n\n-Peff\n"},{"id":"389423","messageId":"xmqqlfqj3tn2.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"20200107193143.GA56858@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] graph: fix case that hit assert()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T20:21:05Z","receivedAt":"2020-01-07T20:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 07, 2020 at 11:21:00AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> >> Second, the horizontal lines in that first line drop their coloring.\n>> >> This is due to a use of graph_line_addch() instead of\n>> >> graph_line_write_column(). Using a ternary operator to pick the\n>> >> character is nice for compact code, but we actually need a column\n>> >> to provide the color.\n>> >\n>> > It seems like this is a totally separate bug, and could be its own\n>> > commit?\n>> \n>> I think so.\n>> \n>> And with that removed, all that remains would be a removal of the\n>> assert() plus an additional test?\n>\n> Yes, though note that the color thing is a v2.25 regression as well. So\n> we'd probably want both of them.\n\nSure. Those two would make perfect pair of commits to finish -rc2 with.\n\nThanks.\n"},{"id":"389427","messageId":"c05fe2c37a87b254157eec1c8a0f8ca206e1cd31.1578432422.git.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.v2.git.1578432422.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] graph: fix case that hit assert()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T21:27:01Z","receivedAt":"2020-01-07T21:27:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nA failure was reported in \"git log --graph --all\" with the new\ngraph-rendering logic. The code fails an assert() statement when\ncollapsing multiple edges from a merge.\n\nThe assert was introduced by eaf158f8 (graph API: Use horizontal\nlines for more compact graphs, 2009-04-21), which is quite old.\nThis assert is trying to say that when we complete a horizontal\nline with a single slash, it is because we have reached our target.\n\nThis assertion is hit when we have two collapsing lines from the\nsame merge commit, as follows:\n\n | | | | *\n | |_|_|/|\n |/| | |/\n | | |/|\n | |/| |\n | * | |\n * | | |\n\nIt is actually the _second_ collapsing line that hits this assert.\nThe reason we are in this code path is because we are collapsing\nthe first line, and in that case we are hitting our target now\nthat the horizontal line is complete. However, the second line\ncannot be a horizontal line, so it will collapse without horizontal\nlines. In this case, it is inappropriate to assert that we have\nreached our target, as we need to continue for another column\nbefore reaching the target. Dropping the assert is safe here.\n\nThe new behavior in 0f0f389f12 (graph: tidy up display of\nleft-skewed merges, 2019-10-15) caused the behavior change that\nmade this assertion failure possible. In addition to making the\nassert possible, it also changed how multiple edges collapse.\n\nIn a larger example, the current code will output a collapse\nas follows:\n\n\t| | | | | | *\n\t| |_|_|_|_|/|\\\n\t|/| | | | |/ /\n\t| | | | |/| /\n\t| | | |/| |/\n\t| | |/| |/|\n\t| |/| |/| |\n\t| | |/| | |\n\t| | * | | |\n\nHowever, the intended collapse should allow multiple horizontal lines\nas follows:\n\n\t| | | | | | *\n\t| |_|_|_|_|/|\\\n\t|/| | | | |/ /\n\t| | |_|_|/| /\n\t| |/| | | |/\n\t| | | |_|/|\n\t| | |/| | |\n\t| | * | | |\n\nThis behavior is not corrected by this change, but is noted for a later\nupdate.\n\nHelped-by: Jeff King \nReported-by: Bradley Smith \nSigned-off-by: Derrick Stolee \n---\n graph.c | 4 ----\n t/t4215-log-skewed-merges.sh | 42 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 4 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 66ae18add8..f514ed3efa 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1219,13 +1219,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l\n \t\t\t *\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n-\t\t\t *\n-\t\t\t * The branch to the left of that space\n-\t\t\t * should be our eventual target.\n \t\t\t */\n \t\t\tassert(graph->mapping[i - 1] > target);\n \t\t\tassert(graph->mapping[i - 2] < 0);\n-\t\t\tassert(graph->mapping[i - 3] == target);\n \t\t\tgraph->mapping[i - 2] = target;\n \t\t\t/*\n \t\t\t * Mark this branch as the horizontal edge to\ndiff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\nindex 18709a723e..ddf6f6f5d3 100755\n--- a/t/t4215-log-skewed-merges.sh\n+++ b/t/t4215-log-skewed-merges.sh\n@@ -240,4 +240,46 @@ test_expect_success 'log --graph with octopus merge with column joining its penu\n \tEOF\n '\n \n+test_expect_success 'log --graph with multiple tips' '\n+\tgit checkout --orphan 6_1 &&\n+\ttest_commit 6_A &&\n+\tgit branch 6_2 &&\n+\tgit branch 6_4 &&\n+\ttest_commit 6_B &&\n+\tgit branch 6_3 &&\n+\ttest_commit 6_C &&\n+\tgit checkout 6_2 && test_commit 6_D &&\n+\tgit checkout 6_3 && test_commit 6_E &&\n+\tgit checkout -b 6_5 6_1 &&\n+\tgit merge --no-ff 6_2 -m 6_F &&\n+\tgit checkout 6_4 && test_commit 6_G &&\n+\tgit checkout 6_3 &&\n+\tgit merge --no-ff 6_4 -m 6_H &&\n+\tgit checkout 6_1 &&\n+\tgit merge --no-ff 6_2 -m 6_I &&\n+\n+\tcheck_graph 6_1 6_3 6_5 <<-\\EOF\n+\t* 6_I\n+\t|\\\n+\t| | * 6_H\n+\t| | |\\\n+\t| | | * 6_G\n+\t| | * | 6_E\n+\t| | | | * 6_F\n+\t| |_|_|/|\n+\t|/| | |/\n+\t| | |/|\n+\t| |/| |\n+\t| * | | 6_D\n+\t| | |/\n+\t| |/|\n+\t* | | 6_C\n+\t| |/\n+\t|/|\n+\t* | 6_B\n+\t|/\n+\t* 6_A\n+\tEOF\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"389428","messageId":"pull.517.v2.git.1578432422.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.git.1578408947.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Fix two bugs in graph.c","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T21:27:00Z","receivedAt":"2020-01-07T21:27:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This is a possible fix for the bug reported in [1].\n\nThe first commit fixes the runtime failure due to the assert() statement.\n\nThe second commit replaces the assert() statements with a macro that\ntriggers a BUG().\n\nThe third commit adds another test that shows a more complicated example and\nhow the new code in v2.25.0-rc1 has a behavior change that is not\nnecessarily wanted.\n\nThanks, -Stolee\n\n[1] \nhttps://lore.kernel.org/git/CAHt=fUXTHc4JPsapvHKnw5vHhp2cBOYRNfdaSDWBUnKt8fWfeA@mail.gmail.com/\n\nDerrick Stolee (2):\n graph: fix case that hit assert()\n graph: fix lack of color in horizontal lines\n\n graph.c | 17 +++++----\n t/t4215-log-skewed-merges.sh | 71 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 80 insertions(+), 8 deletions(-)\n\n\nbase-commit: 8679ef24ed64018bb62170c43ce73e0261c0600a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-517%2Fderrickstolee%2Fgraph-assert-fix-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-517/derrickstolee/graph-assert-fix-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/517\n\nRange-diff vs v1:\n\n 1: 65186f3ded ! 1: c05fe2c37a graph: fix case that hit assert()\n @@ -3,17 +3,11 @@\n graph: fix case that hit assert()\n \n A failure was reported in \"git log --graph --all\" with the new\n - graph-rendering logic. Create a test case that matches the\n - topology of that example and uses an explicit ref ordering instead\n - of the \"--all\" option. The test would fail with the following error:\n + graph-rendering logic. The code fails an assert() statement when\n + collapsing multiple edges from a merge.\n \n - graph.c:1228: graph_output_collapsing_line: Assertion\n - `graph->mapping[i - 3] == target' failed.\n -\n - The situation is a little complicated, so let's break it down.\n -\n - The assert was introduced by eaf158f8 (\"graph API: Use horizontal\n - lines for more compact graphs\", 2009-04-21), which is quite old.\n + The assert was introduced by eaf158f8 (graph API: Use horizontal\n + lines for more compact graphs, 2009-04-21), which is quite old.\n This assert is trying to say that when we complete a horizontal\n line with a single slash, it is because we have reached our target.\n \n @@ -30,18 +24,45 @@\n \n It is actually the _second_ collapsing line that hits this assert.\n The reason we are in this code path is because we are collapsing\n - the first line, and it in that case we are hitting our target now\n + the first line, and in that case we are hitting our target now\n that the horizontal line is complete. However, the second line\n cannot be a horizontal line, so it will collapse without horizontal\n lines. In this case, it is inappropriate to assert that we have\n reached our target, as we need to continue for another column\n before reaching the target. Dropping the assert is safe here.\n \n - Second, the horizontal lines in that first line drop their coloring.\n - This is due to a use of graph_line_addch() instead of\n - graph_line_write_column(). Using a ternary operator to pick the\n - character is nice for compact code, but we actually need a column\n - to provide the color.\n + The new behavior in 0f0f389f12 (graph: tidy up display of\n + left-skewed merges, 2019-10-15) caused the behavior change that\n + made this assertion failure possible. In addition to making the\n + assert possible, it also changed how multiple edges collapse.\n +\n + In a larger example, the current code will output a collapse\n + as follows:\n +\n + | | | | | | *\n + | |_|_|_|_|/|\\\n + |/| | | | |/ /\n + | | | | |/| /\n + | | | |/| |/\n + | | |/| |/|\n + | |/| |/| |\n + | | |/| | |\n + | | * | | |\n +\n + However, the intended collapse should allow multiple horizontal lines\n + as follows:\n +\n + | | | | | | *\n + | |_|_|_|_|/|\\\n + |/| | | | |/ /\n + | | |_|_|/| /\n + | |/| | | |/\n + | | | |_|/|\n + | | |/| | |\n + | | * | | |\n +\n + This behavior is not corrected by this change, but is noted for a later\n + update.\n \n Helped-by: Jeff King \n Reported-by: Bradley Smith \n @@ -50,36 +71,6 @@\n diff --git a/graph.c b/graph.c\n --- a/graph.c\n +++ b/graph.c\n -@@\n - \tint i, j;\n - \n - \tstruct commit_list *first_parent = first_interesting_parent(graph);\n --\tint seen_parent = 0;\n -+\tstruct column *parent_col = NULL;\n - \n - \t/*\n - \t * Output the post-merge row\n -@@\n - \t\t\tgraph_line_addch(line, ' ');\n - \t\t} else {\n - \t\t\tgraph_line_write_column(line, col, '|');\n --\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1)\n --\t\t\t\tgraph_line_addch(line, seen_parent ? '_' : ' ');\n -+\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1) {\n -+\t\t\t\tif (parent_col)\n -+\t\t\t\t\tgraph_line_write_column(\n -+\t\t\t\t\t\tline, parent_col, '_');\n -+\t\t\t\telse\n -+\t\t\t\t\tgraph_line_addch(line, ' ');\n -+\t\t\t}\n - \t\t}\n - \n - \t\tif (col_commit == first_parent->item)\n --\t\t\tseen_parent = 1;\n -+\t\t\tparent_col = col;\n - \t}\n - \n - \t/*\n @@\n \t\t\t *\n \t\t\t * The space just to the left of this\n 2: 5dd305d2f0 ! 2: 11887bd38d graph: replace assert() with graph_assert() macro\n @@ -1,100 +1,86 @@\n Author: Derrick Stolee \n \n - graph: replace assert() with graph_assert() macro\n + graph: fix lack of color in horizontal lines\n \n - The assert() macro is sometimes compiled out. Instead, switch these into\n - BUG() statements using our own custom macro.\n + In some cases, horizontal lines in rendered graphs can lose their\n + coloring. This is due to a use of graph_line_addch() instead of\n + graph_line_write_column(). Using a ternary operator to pick the\n + character is nice for compact code, but we actually need a column to\n + provide the color.\n \n - Reported-by: Eric Sunshine \n + Add a test to t4215-log-skewed-merges.sh to prevent regression.\n +\n + Reported-by: Jeff King \n Signed-off-by: Derrick Stolee \n \n diff --git a/graph.c b/graph.c\n --- a/graph.c\n +++ b/graph.c\n @@\n - #include \"revision.h\"\n - #include \"argv-array.h\"\n - \n -+#define graph_assert(exp) if (!(exp)) { BUG(\"assert failed: \"#exp\"\"); }\n -+\n - /* Internal API */\n - \n - /*\n -@@\n - \tstruct git_graph *graph = data;\n - \tstatic struct strbuf msgbuf = STRBUF_INIT;\n - \n --\tassert(opt);\n -+\tgraph_assert(opt);\n - \n - \tstrbuf_reset(&msgbuf);\n - \tif (opt->line_prefix)\n -@@\n - \t *\n - \t * We need 2 extra rows for every parent over 2.\n - \t */\n --\tassert(graph->num_parents >= 3);\n -+\tgraph_assert(graph->num_parents >= 3);\n + \tint i, j;\n \n - \t/*\n - \t * graph->expansion_row tracks the current expansion row we are on.\n - \t * It should be in the range [0, num_expansion_rows - 1]\n - \t */\n --\tassert(0 <= graph->expansion_row &&\n -+\tgraph_assert(0 <= graph->expansion_row &&\n - \t graph->expansion_row < graph_num_expansion_rows(graph));\n + \tstruct commit_list *first_parent = first_interesting_parent(graph);\n +-\tint seen_parent = 0;\n ++\tstruct column *parent_col = NULL;\n \n \t/*\n + \t * Output the post-merge row\n @@\n - \t * (We should only see boundary commits when revs->boundary is set.)\n - \t */\n - \tif (graph->commit->object.flags & BOUNDARY) {\n --\t\tassert(graph->revs->boundary);\n -+\t\tgraph_assert(graph->revs->boundary);\n - \t\tgraph_line_addch(line, 'o');\n - \t\treturn;\n + \t\t\tgraph_line_addch(line, ' ');\n + \t\t} else {\n + \t\t\tgraph_line_write_column(line, col, '|');\n +-\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1)\n +-\t\t\t\tgraph_line_addch(line, seen_parent ? '_' : ' ');\n ++\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1) {\n ++\t\t\t\tif (parent_col)\n ++\t\t\t\t\tgraph_line_write_column(\n ++\t\t\t\t\t\tline, parent_col, '_');\n ++\t\t\t\telse\n ++\t\t\t\t\tgraph_line_addch(line, ' ');\n ++\t\t\t}\n + \t\t}\n + \n + \t\tif (col_commit == first_parent->item)\n +-\t\t\tseen_parent = 1;\n ++\t\t\tparent_col = col;\n \t}\n \n \t/*\n --\t * get_revision_mark() handles all other cases without assert()\n -+\t * get_revision_mark() handles all other cases without graph_assert()\n - \t */\n - \tgraph_line_addstr(line, get_revision_mark(graph->revs, graph->commit));\n - }\n -@@\n - \n - \t\t\tfor (j = 0; j < graph->num_parents; j++) {\n - \t\t\t\tpar_column = graph_find_new_column_by_commit(graph, parents->item);\n --\t\t\t\tassert(par_column >= 0);\n -+\t\t\t\tgraph_assert(par_column >= 0);\n - \n - \t\t\t\tc = merge_chars[idx];\n - \t\t\t\tgraph_line_write_column(line, &graph->new_columns[par_column], c);\n +\n + diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\n + --- a/t/t4215-log-skewed-merges.sh\n + +++ b/t/t4215-log-skewed-merges.sh\n @@\n - \t\t * the graph much more legible, since whenever branches\n - \t\t * cross, only one is moving directions.\n - \t\t */\n --\t\tassert(target * 2 <= i);\n -+\t\tgraph_assert(target * 2 <= i);\n + \tEOF\n + '\n \n - \t\tif (target * 2 == i) {\n - \t\t\t/*\n - \t\t\t * This column is already in the\n - \t\t\t * correct place\n - \t\t\t */\n --\t\t\tassert(graph->mapping[i] == -1);\n -+\t\t\tgraph_assert(graph->mapping[i] == -1);\n - \t\t\tgraph->mapping[i] = target;\n - \t\t} else if (graph->mapping[i - 1] < 0) {\n - \t\t\t/*\n -@@\n - \t\t\t * The space just to the left of this\n - \t\t\t * branch should always be empty.\n - \t\t\t */\n --\t\t\tassert(graph->mapping[i - 1] > target);\n --\t\t\tassert(graph->mapping[i - 2] < 0);\n -+\t\t\tgraph_assert(graph->mapping[i - 1] > target);\n -+\t\t\tgraph_assert(graph->mapping[i - 2] < 0);\n - \t\t\tgraph->mapping[i - 2] = target;\n - \t\t\t/*\n - \t\t\t * Mark this branch as the horizontal edge to\n ++test_expect_success 'log --graph with multiple tips and colors' '\n ++\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n ++\tcat >expect.colors <<-\\EOF &&\n ++\t* 6_I\n ++\t|\\\n ++\t| | * 6_H\n ++\t| | |\\\n ++\t| | | * 6_G\n ++\t| | | | * 6_F\n ++\t| |_|_|/|\n ++\t|/| | |/\n ++\t| | |/|\n ++\t| |/| |\n ++\t| | * | 6_E\n ++\t| * | | 6_D\n ++\t| | |/\n ++\t| |/|\n ++\t* | | 6_C\n ++\t| |/\n ++\t|/|\n ++\t* | 6_B\n ++\t|/\n ++\t* 6_A\n ++\tEOF\n ++\tgit log --color=always --graph --date-order --pretty=tformat:%s 6_1 6_3 6_5 >actual.colors.raw &&\n ++\ttest_decode_color actual.colors &&\n ++\ttest_cmp expect.colors actual.colors\n ++'\n ++\n + test_done\n 3: f74e82bea6 < -: ---------- t4215: add bigger graph collapse test\n\n-- \ngitgitgadget\n"},{"id":"389429","messageId":"11887bd38d73b48c6677ef1e8fe2b9f0c295c8a0.1578432422.git.gitgitgadget@gmail.com","threadId":"52582","inReplyTo":"pull.517.v2.git.1578432422.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] graph: fix lack of color in horizontal lines","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T21:27:02Z","receivedAt":"2020-01-07T21:27:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nIn some cases, horizontal lines in rendered graphs can lose their\ncoloring. This is due to a use of graph_line_addch() instead of\ngraph_line_write_column(). Using a ternary operator to pick the\ncharacter is nice for compact code, but we actually need a column to\nprovide the color.\n\nAdd a test to t4215-log-skewed-merges.sh to prevent regression.\n\nReported-by: Jeff King \nSigned-off-by: Derrick Stolee \n---\n graph.c | 13 +++++++++----\n t/t4215-log-skewed-merges.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 38 insertions(+), 4 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex f514ed3efa..aaf97069bd 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1063,7 +1063,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \tint i, j;\n \n \tstruct commit_list *first_parent = first_interesting_parent(graph);\n-\tint seen_parent = 0;\n+\tstruct column *parent_col = NULL;\n \n \t/*\n \t * Output the post-merge row\n@@ -1117,12 +1117,17 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l\n \t\t\tgraph_line_addch(line, ' ');\n \t\t} else {\n \t\t\tgraph_line_write_column(line, col, '|');\n-\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1)\n-\t\t\t\tgraph_line_addch(line, seen_parent ? '_' : ' ');\n+\t\t\tif (graph->merge_layout != 0 || i != graph->commit_index - 1) {\n+\t\t\t\tif (parent_col)\n+\t\t\t\t\tgraph_line_write_column(\n+\t\t\t\t\t\tline, parent_col, '_');\n+\t\t\t\telse\n+\t\t\t\t\tgraph_line_addch(line, ' ');\n+\t\t\t}\n \t\t}\n \n \t\tif (col_commit == first_parent->item)\n-\t\t\tseen_parent = 1;\n+\t\t\tparent_col = col;\n \t}\n \n \t/*\ndiff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh\nindex ddf6f6f5d3..5661ed5881 100755\n--- a/t/t4215-log-skewed-merges.sh\n+++ b/t/t4215-log-skewed-merges.sh\n@@ -282,4 +282,33 @@ test_expect_success 'log --graph with multiple tips' '\n \tEOF\n '\n \n+test_expect_success 'log --graph with multiple tips and colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* 6_I\n+\t|\\\n+\t| | * 6_H\n+\t| | |\\\n+\t| | | * 6_G\n+\t| | | | * 6_F\n+\t| |_|_|/|\n+\t|/| | |/\n+\t| | |/|\n+\t| |/| |\n+\t| | * | 6_E\n+\t| * | | 6_D\n+\t| | |/\n+\t| |/|\n+\t* | | 6_C\n+\t| |/\n+\t|/|\n+\t* | 6_B\n+\t|/\n+\t* 6_A\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s 6_1 6_3 6_5 >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"389430","messageId":"xmqqd0bv3pin.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"c05fe2c37a87b254157eec1c8a0f8ca206e1cd31.1578432422.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] graph: fix case that hit assert()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T21:50:08Z","receivedAt":"2020-01-07T21:50:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n> Subject: Re: [PATCH v2 1/2] graph: fix case that hit assert()\n>\n> A failure was reported in \"git log --graph --all\" with the new\n> graph-rendering logic. The code fails an assert() statement when\n> collapsing multiple edges from a merge.\n>\n> The assert was introduced by eaf158f8 (graph API: Use horizontal\n> lines for more compact graphs, 2009-04-21), which is quite old.\n> This assert is trying to say that when we complete a horizontal\n> line with a single slash, it is because we have reached our target.\n>\n> This assertion is hit when we have two collapsing lines from the\n> same merge commit, as follows:\n>\n> | | | | *\n> | |_|_|/|\n> |/| | |/\n> | | |/|\n> | |/| |\n> | * | |\n> * | | |\n\nI was sort-of expecting to see\n\n graph: drop assert() for merge with two collapsing parents\n\n When \"git log --graph\" shows a merge commit that has two collapsing\n lines, like:\n\n | | | | *\n | |_|_|/|\n |/| | |/\n | | |/|\n | |/| |\n | * | |\n * | | |\n\n we trigger an assert():\n\n graph.c:1228: graph_output_collapsing_line: Assertion\n `graph->mapping[i - 3] == target' failed.\n\n The assert was introduced by eaf158f8 (\"graph API: Use horizontal\n lines for more compact graphs\", 2009-04-21), which is quite old.\n ...\n\nnear the beginning of this commit, though.\n\n> In a larger example, the current code will output a collapse\n> as follows:\n>\n> \t| | | | | | *\n> \t| |_|_|_|_|/|\\\n> \t|/| | | | |/ /\n> \t| | | | |/| /\n> \t| | | |/| |/\n> \t| | |/| |/|\n> \t| |/| |/| |\n> \t| | |/| | |\n> \t| | * | | |\n>\n> However, the intended collapse should allow multiple horizontal lines\n> as follows:\n>\n> \t| | | | | | *\n> \t| |_|_|_|_|/|\\\n> \t|/| | | | |/ /\n> \t| | |_|_|/| /\n> \t| |/| | | |/\n> \t| | | |_|/|\n> \t| | |/| | |\n> \t| | * | | |\n>\n> This behavior is not corrected by this change, but is noted for a later\n> update.\n\nThis part is new and looks like a good thing to mention.\n\n"},{"id":"389431","messageId":"xmqq8smj3pg5.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"11887bd38d73b48c6677ef1e8fe2b9f0c295c8a0.1578432422.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] graph: fix lack of color in horizontal lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-07T21:51:38Z","receivedAt":"2020-01-07T21:51:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> In some cases, horizontal lines in rendered graphs can lose their\n> coloring. This is due to a use of graph_line_addch() instead of\n> graph_line_write_column(). Using a ternary operator to pick the\n> character is nice for compact code, but we actually need a column to\n> provide the color.\n>\n> Add a test to t4215-log-skewed-merges.sh to prevent regression.\n>\n> Reported-by: Jeff King \n> Signed-off-by: Derrick Stolee \n> ---\n\nThanks. Looks good.\n"},{"id":"389489","messageId":"xmqqa76x3l8o.fsf@gitster-ct.c.googlers.com","threadId":"52582","inReplyTo":"xmqqd0bv3pin.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] graph: fix case that hit assert()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-08T17:34:47Z","receivedAt":"2020-01-08T17:34:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Derrick Stolee via GitGitGadget\" writes:\n>\n>> From: Derrick Stolee \n>> Subject: Re: [PATCH v2 1/2] graph: fix case that hit assert()\n>>\n>> A failure was reported in \"git log --graph --all\" with the new\n>> graph-rendering logic. The code fails an assert() statement when\n>> collapsing multiple edges from a merge.\n>>\n>> The assert was introduced by eaf158f8 (graph API: Use horizontal\n>> lines for more compact graphs, 2009-04-21), which is quite old.\n>> This assert is trying to say that when we complete a horizontal\n>> line with a single slash, it is because we have reached our target.\n>>\n>> This assertion is hit when we have two collapsing lines from the\n>> same merge commit, as follows:\n>>\n>> | | | | *\n>> | |_|_|/|\n>> |/| | |/\n>> | | |/|\n>> | |/| |\n>> | * | |\n>> * | | |\n>\n> I was sort-of expecting to see\n> ...\n> near the beginning of this commit, though.\n\nWill do this locally before using them in rc2.\n\nThanks.\n"},{"id":"389498","messageId":"c7df7e59-9e37-72eb-e1b0-aa21bb6045dc@gmail.com","threadId":"52582","inReplyTo":"xmqqa76x3l8o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 1/2] graph: fix case that hit assert()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-08T19:14:38Z","receivedAt":"2020-01-08T19:14:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/8/2020 12:34 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> \"Derrick Stolee via GitGitGadget\" writes:\n>>\n>>> From: Derrick Stolee \n>>> Subject: Re: [PATCH v2 1/2] graph: fix case that hit assert()\n>>>\n>>> A failure was reported in \"git log --graph --all\" with the new\n>>> graph-rendering logic. The code fails an assert() statement when\n>>> collapsing multiple edges from a merge.\n>>>\n>>> The assert was introduced by eaf158f8 (graph API: Use horizontal\n>>> lines for more compact graphs, 2009-04-21), which is quite old.\n>>> This assert is trying to say that when we complete a horizontal\n>>> line with a single slash, it is because we have reached our target.\n>>>\n>>> This assertion is hit when we have two collapsing lines from the\n>>> same merge commit, as follows:\n>>>\n>>> | | | | *\n>>> | |_|_|/|\n>>> |/| | |/\n>>> | | |/|\n>>> | |/| |\n>>> | * | |\n>>> * | | |\n>>\n>> I was sort-of expecting to see\n>> ...\n>> near the beginning of this commit, though.\n> \n> Will do this locally before using them in rc2.\n\nSounds good to me. Sorry for the delay in doing it myself.\n\n-Stolee\n"}]}