{"thread":{"id":"51914","subject":"[BUG/PATCH 0/5] t4214: cleanup and demonstrate graph bug","startedAt":"2019-09-25T10:27:04Z","lastAt":"2019-10-04T05:51:07Z","messageCount":16,"participants":["Denton Liu","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"382906","messageId":"cover.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":null,"subject":"[BUG/PATCH 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:26:57Z","receivedAt":"2019-09-25T10:27:04Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"This patchset cleans up t4214 and then, in the last patch, demonstrates\na bug in the way some octopus merges are colored.\n\nWhile I was playing around with Pratyush's octopus merge in git-gui, I\nnoticed that there was a bug in `git log --graph`. The horizontal lines\nin the octopus merge seemed to have an off-by-one error in their\ncoloring. More detail in the last patch.\n\nI tried my hand at fixing the bug but in the hour I spent going at it, I\ncouldn't fix the logic up. The buggy logic is in graph.c:\ngraph_draw_octopus_merge() in case anyone is interested.\n\n\nDenton Liu (5):\n test-lib: let test_merge() perform octopus merges\n t4214: use test_merge\n t4214: generate expect in their own test cases\n t4214: explicitly list tags in log\n t4214: demonstrate octopus graph coloring failure\n\n t/t4214-log-graph-octopus.sh | 143 +++++++++++++++++++++++++++++------\n t/test-lib-functions.sh | 6 +-\n 2 files changed, 122 insertions(+), 27 deletions(-)\n\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382907","messageId":"e77af8cde5380c3ba43f074232a50a262ef79791.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[BUG/PATCH 1/5] test-lib: let test_merge() perform octopus merges","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:27:01Z","receivedAt":"2019-09-25T10:27:05Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently test_merge() only allows developers to merge in one branch.\nHowever, this restriction is artificial and there is no reason why it\nneeds to be this way.\n\nExtend test_merge() to allow the specification of multiple branches so\nthat octopus merges can be performed.\n\nSigned-off-by: Denton Liu \n---\n t/test-lib-functions.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 87bf3a2287..b299ecc326 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -228,9 +228,11 @@ test_commit () {\n # can be a tag pointing to the commit-to-merge.\n \n test_merge () {\n+\tlabel=\"$1\" &&\n+\tshift &&\n \ttest_tick &&\n-\tgit merge -m \"$1\" \"$2\" &&\n-\tgit tag \"$1\"\n+\tgit merge -m \"$label\" \"$@\" &&\n+\tgit tag \"$label\"\n }\n \n # Efficiently create commits, each with a unique number (from 1 to \n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382908","messageId":"4a93deb3f62088b9bdb615adfb126581659914b4.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[BUG/PATCH 2/5] t4214: use test_merge","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:27:03Z","receivedAt":"2019-09-25T10:27:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the previous commit, we extended test_merge() so that it could\nperform octopus merges. Now that the restriction is lifted, use\ntest_merge() to perform the octopus merge instead of manually\nduplicating test_merge() functionality.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex dab96c89aa..f6e22ec825 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -41,8 +41,7 @@ test_expect_success 'set up merge history' '\n \t\ttest_commit $i $i $i tag$i || return $?\n \tdone &&\n \tgit checkout 1 -b merge &&\n-\ttest_tick &&\n-\tgit merge -m octopus-merge 1 2 3 4 &&\n+\ttest_merge octopus-merge 1 2 3 4 &&\n \tgit checkout 1 -b L &&\n \ttest_commit left\n '\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382909","messageId":"c09f761185859998d33c4944fe3a7317e3a8c987.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[BUG/PATCH 3/5] t4214: generate expect in their own test cases","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:27:05Z","receivedAt":"2019-09-25T10:27:09Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, the expect files of the test case were being generated in the\nsetup method. However, it would make more sense to generate these files\nwithin the test cases that actually use them so that it's obvious to\nfuture readers where the expected values are coming from.\n\nMove the generation of the expect files in their own respective test\ncases.\n\nWhile we're at it, we want to establish a pattern in this test suite\nthat, firstly, a non-colored test case is given then, immediately after,\nthe colored version is given.\n\nSwitch test cases \"log --graph with tricky octopus merge, no color\" and\n\"log --graph with tricky octopus merge with colors\" so that the \"no\ncolor\" version appears first.\n\nThis patch is best viewed with `--color-moved`.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 42 ++++++++++++++++++------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex f6e22ec825..16776e347c 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -5,6 +5,20 @@ test_description='git log --graph of skewed left octopus merge.'\n . ./test-lib.sh\n \n test_expect_success 'set up merge history' '\n+\ttest_commit initial &&\n+\tfor i in 1 2 3 4 ; do\n+\t\tgit checkout master -b $i || return $?\n+\t\t# Make tag name different from branch name, to avoid\n+\t\t# ambiguity error when calling checkout.\n+\t\ttest_commit $i $i $i tag$i || return $?\n+\tdone &&\n+\tgit checkout 1 -b merge &&\n+\ttest_merge octopus-merge 1 2 3 4 &&\n+\tgit checkout 1 -b L &&\n+\ttest_commit left\n+'\n+\n+test_expect_success 'log --graph with tricky octopus merge, no color' '\n \tcat >expect.uncolored <<-\\EOF &&\n \t* left\n \t| *---. octopus-merge\n@@ -19,6 +33,13 @@ test_expect_success 'set up merge history' '\n \t|/\n \t* initial\n \tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_success 'log --graph with tricky octopus merge with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n \tcat >expect.colors <<-\\EOF &&\n \t* left\n \t| *---. octopus-merge\n@@ -33,32 +54,11 @@ test_expect_success 'set up merge history' '\n \t|/\n \t* initial\n \tEOF\n-\ttest_commit initial &&\n-\tfor i in 1 2 3 4 ; do\n-\t\tgit checkout master -b $i || return $?\n-\t\t# Make tag name different from branch name, to avoid\n-\t\t# ambiguity error when calling checkout.\n-\t\ttest_commit $i $i $i tag$i || return $?\n-\tdone &&\n-\tgit checkout 1 -b merge &&\n-\ttest_merge octopus-merge 1 2 3 4 &&\n-\tgit checkout 1 -b L &&\n-\ttest_commit left\n-'\n-\n-test_expect_success 'log --graph with tricky octopus merge with colors' '\n-\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n \tgit log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n \n-test_expect_success 'log --graph with tricky octopus merge, no color' '\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n-\tsed \"s/ *\\$//\" actual.raw >actual &&\n-\ttest_cmp expect.uncolored actual\n-'\n-\n # Repeat the previous two tests with \"normal\" octopus merge (i.e.,\n # without the first parent skewing to the \"left\" branch column).\n \n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382910","messageId":"ad6d89440be6e5a26d107dd7b11bad322df1f0cb.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[BUG/PATCH 4/5] t4214: explicitly list tags in log","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:27:07Z","receivedAt":"2019-09-25T10:27:11Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future test case, we will be extending the commit graph. As a\nresult, explicitly list the tags that will generate the graph so that\nwhen future additions are made, the current graph illustrations won't be\naffected.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex 16776e347c..097151da39 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -33,7 +33,7 @@ test_expect_success 'log --graph with tricky octopus merge, no color' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s left octopus-merge >actual.raw &&\n \tsed \"s/ *\\$//\" actual.raw >actual &&\n \ttest_cmp expect.uncolored actual\n '\n@@ -54,7 +54,7 @@ test_expect_success 'log --graph with tricky octopus merge with colors' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s left octopus-merge >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n@@ -75,7 +75,7 @@ test_expect_success 'log --graph with normal octopus merge, no color' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s merge >actual.raw &&\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s octopus-merge >actual.raw &&\n \tsed \"s/ *\\$//\" actual.raw >actual &&\n \ttest_cmp expect.uncolored actual\n '\n@@ -94,7 +94,7 @@ test_expect_success 'log --graph with normal octopus merge with colors' '\n \t* initial\n \tEOF\n \ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n-\tgit log --color=always --graph --date-order --pretty=tformat:%s merge >actual.colors.raw &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s octopus-merge >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382911","messageId":"0b84bf54175cd8ee967ab46ac393c2d5d18d772e.1569407150.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[BUG/PATCH 5/5] t4214: demonstrate octopus graph coloring failure","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T10:27:09Z","receivedAt":"2019-09-25T10:27:14Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The graph coloring logic for octopus merges currently has a bug. This\ncan be seen git.git with 74c7cfa875 (Merge of\nhttp://members.cox.net/junkio/git-jc.git, 2005-05-05), whose second\nchild is 211232bae6 (Octopus merge of the following five patches.,\n2005-05-05).\n\nIf one runs\n\n\tgit log --graph 74c7cfa875\n\none can see that the octopus merge is colored incorrectly. In\nparticular, the horizontal dashes are off by one color. Each horizontal\ndash should be the color of the line to their bottom-right. Instead, they\nare currently the color of the line to their bottom.\n\nDemonstrate this breakage with two sets of test cases. The first pair of\ntest cases demonstrates the breakage with a similar case as the above.\nThe second pair of test cases demonstrates a similar breakage but with\nthe last parent crossing over.\n\nThe second pair of test cases are included as a result of my (poor)\nattempts at fixing the bug. This case seems particularly tricky to\nhandle. Good luck!\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 96 +++++++++++++++++++++++++++++++++++-\n 1 file changed, 95 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex 097151da39..99e0ea034e 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -14,8 +14,11 @@ test_expect_success 'set up merge history' '\n \tdone &&\n \tgit checkout 1 -b merge &&\n \ttest_merge octopus-merge 1 2 3 4 &&\n+\ttest_commit after-merge &&\n \tgit checkout 1 -b L &&\n-\ttest_commit left\n+\ttest_commit left &&\n+\tgit checkout 4 -b crossover &&\n+\ttest_commit after-4\n '\n \n test_expect_success 'log --graph with tricky octopus merge, no color' '\n@@ -98,4 +101,95 @@ test_expect_success 'log --graph with normal octopus merge with colors' '\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n+\n+test_expect_success 'log --graph with tricky octopus merge and its parent, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* left\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t|/ / / /\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s left after-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with tricky octopus merge and its parent with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* left\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t|/ / / /\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s left after-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with crossover in octopus merge, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-4\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-4 octopus-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with crossover in octopus merge with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-4\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-4 octopus-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n test_done\n-- \n2.23.0.248.g3a9dd8fb08\n\n"},{"id":"382935","messageId":"20190925170902.GA11547@dentonliu-ltm.internal.salesforce.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"Re: [BUG/PATCH 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-09-25T17:09:02Z","receivedAt":"2019-09-25T17:09:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Wed, Sep 25, 2019 at 03:26:57AM -0700, Denton Liu wrote:\n> I tried my hand at fixing the bug but in the hour I spent going at it, I\n> couldn't fix the logic up. The buggy logic is in graph.c:\n> graph_draw_octopus_merge() in case anyone is interested.\n\nI guess for the record, this was the final patch that I ended up with.\nTwo issues with it, though: \n\n1. It assumes that there can be no parallel paths on the right side,\nwhich I'm not sure is a correct assumption.\n\n2. It _still_ fails my last proposed test case.\n\n-- >8 --\n\ndiff --git a/graph.c b/graph.c\nindex f53135485f..f9395a2327 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -881,8 +881,7 @@ static int graph_draw_octopus_merge(struct git_graph *graph,\n \t/*\n \t * In both cases, commit_index corresponds to the edge labeled \"0\".\n \t */\n-\tint first_col = graph->commit_index + dashless_parents\n-\t - parent_in_old_cols;\n+\tint first_col = graph->num_new_columns - dashful_parents;\n \n \tint i;\n \tfor (i = 0; i < dashful_parents; i++) {\n"},{"id":"383024","messageId":"20190926202326.GA16664@sigill.intra.peff.net","threadId":"51914","inReplyTo":"20190925170902.GA11547@dentonliu-ltm.internal.salesforce.com","subject":"Re: [BUG/PATCH 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-26T20:23:27Z","receivedAt":"2019-09-26T20:23:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 25, 2019 at 10:09:02AM -0700, Denton Liu wrote:\n\n> On Wed, Sep 25, 2019 at 03:26:57AM -0700, Denton Liu wrote:\n> > I tried my hand at fixing the bug but in the hour I spent going at it, I\n> > couldn't fix the logic up. The buggy logic is in graph.c:\n> > graph_draw_octopus_merge() in case anyone is interested.\n> \n> I guess for the record, this was the final patch that I ended up with.\n> Two issues with it, though: \n> \n> 1. It assumes that there can be no parallel paths on the right side,\n> which I'm not sure is a correct assumption.\n> \n> 2. It _still_ fails my last proposed test case.\n> \n> -- >8 --\n> \n> diff --git a/graph.c b/graph.c\n> index f53135485f..f9395a2327 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -881,8 +881,7 @@ static int graph_draw_octopus_merge(struct git_graph *graph,\n> \t/*\n> \t * In both cases, commit_index corresponds to the edge labeled \"0\".\n> \t */\n> -\tint first_col = graph->commit_index + dashless_parents\n> -\t - parent_in_old_cols;\n> +\tint first_col = graph->num_new_columns - dashful_parents;\n> \n> \tint i;\n> \tfor (i = 0; i < dashful_parents; i++) {\n\nHmm. Looking at the broken case from \"git log -2 --graph 74c7cfa875\", I\nsee that added_cols is \"4\", which is right (we had 2 lines coming in,\nand then with the 5 parents that becomes 6). But one of those lines is\nalready counted as \"dashless\". We account for that with\nparent_in_old_cols, which is 1, and subtract that way from first_col.\nThat works for the diagram in the code:\n\n | *---.\n | |\\ \\ \\\n |/ / / /\n x 0 1 2\n\nwhere one of the parent lines is collapsing back to the left. But not\nfor this more mundane case:\n\n | *-----. commit 211232bae64bcc60bbf5d1b5e5b2344c22ed767e\n | |\\ \\ \\ \\ Merge: fc54a9c30c 9e30dd7c0e c4b83e618f 660265909f b28858bf65\n | | | | | | \n\nwhere we go straight down. I'm not sure I've fully grasped it, but it\nfeels like that distinction is the source of the off-by-one. I'm not\nsure how to tell the difference here, though. I think it relies on the\nnext commit on the left-hand line being the same as the first parent (or\nmaybe any parent?).\n\nIf I remove the use of parent_in_old_cols entirely, the merge looks\nright, but the \"collapsing\" one is broken (and t4214 fails).\n\nBy the way, a useful trick I stumbled on to look at the coloring across\nmany such merges:\n\n git log --graph --format=%h --color | grep -A2 -e - | less -S\n\nIt looks like every octopus in git.git is colored wrong (because they're\nthe non-collapsing type).\n\n-Peff\n"},{"id":"383367","messageId":"xmqqo8yxbhkt.fsf@gitster-ct.c.googlers.com","threadId":"51914","inReplyTo":"20190926202326.GA16664@sigill.intra.peff.net","subject":"Re: [BUG/PATCH 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-03T22:16:34Z","receivedAt":"2019-10-03T22:16:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That works for the diagram in the code:\n>\n> | *---.\n> | |\\ \\ \\\n> |/ / / /\n> x 0 1 2\n>\n> where one of the parent lines is collapsing back to the left. But not\n> for this more mundane case:\n>\n> | *-----. commit 211232bae64bcc60bbf5d1b5e5b2344c22ed767e\n> | |\\ \\ \\ \\ Merge: fc54a9c30c 9e30dd7c0e c4b83e618f 660265909f b28858bf65\n> | | | | | | \n>\n> where we go straight down. I'm not sure I've fully grasped it, but it\n> feels like that distinction is the source of the off-by-one. I'm not\n> sure how to tell the difference here, though. I think it relies on the\n> next commit on the left-hand line being the same as the first parent (or\n> maybe any parent?).\n>\n> If I remove the use of parent_in_old_cols entirely, the merge looks\n> right, but the \"collapsing\" one is broken (and t4214 fails).\n>\n> By the way, a useful trick I stumbled on to look at the coloring across\n> many such merges:\n>\n> git log --graph --format=%h --color | grep -A2 -e - | less -S\n>\n> It looks like every octopus in git.git is colored wrong (because they're\n> the non-collapsing type).\n\nThanks for analysing further. I wonder if new tests added by\nDenton's BUG/PATCH series cover both kinds? It would be good\nto make sure that any \"fix\" corrects known-broken cases while\nkeeping the working cases still working.\n\nThanks.\n"},{"id":"383386","messageId":"cover.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1569407150.git.liu.denton@gmail.com","subject":"[PATCH v2 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:09Z","receivedAt":"2019-10-04T00:23:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Junio, the test cases from earlier didn't exactly cover the cases Peff\ntalked about so I added a few more test cases. These should cover those\nsituations and a few more so we can be extra sure when the bug is fixed.\n\n\nThis patchset cleans up t4214 and then, in the last patch, demonstrates\na bug in the way some octopus merges are colored.\n\nWhile I was playing around with Pratyush's octopus merge in git-gui, I\nnoticed that there was a bug in `git log --graph`. The horizontal lines\nin the octopus merge seemed to have an off-by-one error in their\ncoloring. More detail in the last patch.\n\nI tried my hand at fixing the bug but in the hour I spent going at it, I\ncouldn't fix the logic up. The buggy logic is in graph.c:\ngraph_draw_octopus_merge() in case anyone is interested.\n\nChanges since v1:\n\n* Add a few more test cases to demonstrate more failure (and passing)\n conditions\n\n\nDenton Liu (5):\n test-lib: let test_merge() perform octopus merges\n t4214: use test_merge\n t4214: generate expect in their own test cases\n t4214: explicitly list tags in log\n t4214: demonstrate octopus graph coloring failure\n\n t/t4214-log-graph-octopus.sh | 329 ++++++++++++++++++++++++++++++++---\n t/test-lib-functions.sh | 6 +-\n 2 files changed, 308 insertions(+), 27 deletions(-)\n\nRange-diff against v1:\n1: e77af8cde5 = 1: e77af8cde5 test-lib: let test_merge() perform octopus merges\n2: 4a93deb3f6 = 2: 4a93deb3f6 t4214: use test_merge\n3: c09f761185 = 3: c09f761185 t4214: generate expect in their own test cases\n4: ad6d89440b = 4: ad6d89440b t4214: explicitly list tags in log\n5: 0b84bf5417 ! 5: e58c1929bc t4214: demonstrate octopus graph coloring failure\n @@ Commit message\n dash should be the color of the line to their bottom-right. Instead, they\n are currently the color of the line to their bottom.\n \n - Demonstrate this breakage with two sets of test cases. The first pair of\n - test cases demonstrates the breakage with a similar case as the above.\n - The second pair of test cases demonstrates a similar breakage but with\n - the last parent crossing over.\n + Demonstrate this breakage with a few sets of test cases. These test\n + cases should show not only simple cases of the bug occuring but trickier\n + situations that may not be handled properly in any attempt to fix the\n + bug.\n \n - The second pair of test cases are included as a result of my (poor)\n - attempts at fixing the bug. This case seems particularly tricky to\n - handle. Good luck!\n + While we're at it, include a passing test case as a canary in case an\n + attempt to fix the bug breaks existing operation.\n \n ## t/t4214-log-graph-octopus.sh ##\n @@ t/t4214-log-graph-octopus.sh: test_expect_success 'set up merge history' '\n @@ t/t4214-log-graph-octopus.sh: test_expect_success 'set up merge history' '\n -\ttest_commit left\n +\ttest_commit left &&\n +\tgit checkout 4 -b crossover &&\n -+\ttest_commit after-4\n ++\ttest_commit after-4 &&\n ++\tgit checkout initial -b more-L &&\n ++\ttest_commit after-initial\n '\n \n test_expect_success 'log --graph with tricky octopus merge, no color' '\n @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop\n \ttest_cmp expect.colors actual.colors\n '\n +\n -+test_expect_success 'log --graph with tricky octopus merge and its parent, no color' '\n ++test_expect_success 'log --graph with normal octopus merge and child, no color' '\n ++\tcat >expect.uncolored <<-\\EOF &&\n ++\t* after-merge\n ++\t*---. octopus-merge\n ++\t|\\ \\ \\\n ++\t| | | * 4\n ++\t| | * | 3\n ++\t| | |/\n ++\t| * | 2\n ++\t| |/\n ++\t* | 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=never --graph --date-order --pretty=tformat:%s after-merge >actual.raw &&\n ++\tsed \"s/ *\\$//\" actual.raw >actual &&\n ++\ttest_cmp expect.uncolored actual\n ++'\n ++\n ++test_expect_failure 'log --graph with normal octopus and child merge with colors' '\n ++\tcat >expect.colors <<-\\EOF &&\n ++\t* after-merge\n ++\t*---. octopus-merge\n ++\t|\\ \\ \\\n ++\t| | | * 4\n ++\t| | * | 3\n ++\t| | |/\n ++\t| * | 2\n ++\t| |/\n ++\t* | 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n ++\tgit log --color=always --graph --date-order --pretty=tformat:%s after-merge >actual.colors.raw &&\n ++\ttest_decode_color actual.colors &&\n ++\ttest_cmp expect.colors actual.colors\n ++'\n ++\n ++test_expect_success 'log --graph with tricky octopus merge and its child, no color' '\n +\tcat >expect.uncolored <<-\\EOF &&\n +\t* left\n +\t| * after-merge\n @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop\n +\ttest_cmp expect.uncolored actual\n +'\n +\n -+test_expect_failure 'log --graph with tricky octopus merge and its parent with colors' '\n ++test_expect_failure 'log --graph with tricky octopus merge and its child with colors' '\n +\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n +\tcat >expect.colors <<-\\EOF &&\n +\t* left\n @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop\n +\ttest_decode_color actual.colors &&\n +\ttest_cmp expect.colors actual.colors\n +'\n ++\n ++test_expect_success 'log --graph with crossover in octopus merge and its child, no color' '\n ++\tcat >expect.uncolored <<-\\EOF &&\n ++\t* after-4\n ++\t| * after-merge\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t* | | | 4\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=never --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.raw &&\n ++\tsed \"s/ *\\$//\" actual.raw >actual &&\n ++\ttest_cmp expect.uncolored actual\n ++'\n ++\n ++test_expect_failure 'log --graph with crossover in octopus merge and its child with colors' '\n ++\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n ++\tcat >expect.colors <<-\\EOF &&\n ++\t* after-4\n ++\t| * after-merge\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t* | | | 4\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=always --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.colors.raw &&\n ++\ttest_decode_color actual.colors &&\n ++\ttest_cmp expect.colors actual.colors\n ++'\n ++\n ++test_expect_success 'log --graph with unrelated commit and octopus tip, no color' '\n ++\tcat >expect.uncolored <<-\\EOF &&\n ++\t* after-initial\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| | | | * 4\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=never --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.raw &&\n ++\tsed \"s/ *\\$//\" actual.raw >actual &&\n ++\ttest_cmp expect.uncolored actual\n ++'\n ++\n ++test_expect_success 'log --graph with unrelated commit and octopus tip with colors' '\n ++\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n ++\tcat >expect.colors <<-\\EOF &&\n ++\t* after-initial\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| | | | * 4\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=always --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.colors.raw &&\n ++\ttest_decode_color actual.colors &&\n ++\ttest_cmp expect.colors actual.colors\n ++'\n ++\n ++test_expect_success 'log --graph with unrelated commit and octopus child, no color' '\n ++\tcat >expect.uncolored <<-\\EOF &&\n ++\t* after-initial\n ++\t| * after-merge\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| | | | * 4\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=never --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.raw &&\n ++\tsed \"s/ *\\$//\" actual.raw >actual &&\n ++\ttest_cmp expect.uncolored actual\n ++'\n ++\n ++test_expect_failure 'log --graph with unrelated commit and octopus child with colors' '\n ++\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n ++\tcat >expect.colors <<-\\EOF &&\n ++\t* after-initial\n ++\t| * after-merge\n ++\t| *---. octopus-merge\n ++\t| |\\ \\ \\\n ++\t| | | | * 4\n ++\t| |_|_|/\n ++\t|/| | |\n ++\t| | | * 3\n ++\t| |_|/\n ++\t|/| |\n ++\t| | * 2\n ++\t| |/\n ++\t|/|\n ++\t| * 1\n ++\t|/\n ++\t* initial\n ++\tEOF\n ++\tgit log --color=always --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.colors.raw &&\n ++\ttest_decode_color actual.colors &&\n ++\ttest_cmp expect.colors actual.colors\n ++'\n +\n test_done\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383387","messageId":"e77af8cde5380c3ba43f074232a50a262ef79791.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"[PATCH v2 1/5] test-lib: let test_merge() perform octopus merges","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:13Z","receivedAt":"2019-10-04T00:23:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Currently test_merge() only allows developers to merge in one branch.\nHowever, this restriction is artificial and there is no reason why it\nneeds to be this way.\n\nExtend test_merge() to allow the specification of multiple branches so\nthat octopus merges can be performed.\n\nSigned-off-by: Denton Liu \n---\n t/test-lib-functions.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 87bf3a2287..b299ecc326 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -228,9 +228,11 @@ test_commit () {\n # can be a tag pointing to the commit-to-merge.\n \n test_merge () {\n+\tlabel=\"$1\" &&\n+\tshift &&\n \ttest_tick &&\n-\tgit merge -m \"$1\" \"$2\" &&\n-\tgit tag \"$1\"\n+\tgit merge -m \"$label\" \"$@\" &&\n+\tgit tag \"$label\"\n }\n \n # Efficiently create commits, each with a unique number (from 1 to \n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383388","messageId":"ad6d89440be6e5a26d107dd7b11bad322df1f0cb.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"[PATCH v2 4/5] t4214: explicitly list tags in log","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:20Z","receivedAt":"2019-10-04T00:23:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a future test case, we will be extending the commit graph. As a\nresult, explicitly list the tags that will generate the graph so that\nwhen future additions are made, the current graph illustrations won't be\naffected.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex 16776e347c..097151da39 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -33,7 +33,7 @@ test_expect_success 'log --graph with tricky octopus merge, no color' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s left octopus-merge >actual.raw &&\n \tsed \"s/ *\\$//\" actual.raw >actual &&\n \ttest_cmp expect.uncolored actual\n '\n@@ -54,7 +54,7 @@ test_expect_success 'log --graph with tricky octopus merge with colors' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s left octopus-merge >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n@@ -75,7 +75,7 @@ test_expect_success 'log --graph with normal octopus merge, no color' '\n \t|/\n \t* initial\n \tEOF\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s merge >actual.raw &&\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s octopus-merge >actual.raw &&\n \tsed \"s/ *\\$//\" actual.raw >actual &&\n \ttest_cmp expect.uncolored actual\n '\n@@ -94,7 +94,7 @@ test_expect_success 'log --graph with normal octopus merge with colors' '\n \t* initial\n \tEOF\n \ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n-\tgit log --color=always --graph --date-order --pretty=tformat:%s merge >actual.colors.raw &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s octopus-merge >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383389","messageId":"4a93deb3f62088b9bdb615adfb126581659914b4.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"[PATCH v2 2/5] t4214: use test_merge","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:15Z","receivedAt":"2019-10-04T00:23:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In the previous commit, we extended test_merge() so that it could\nperform octopus merges. Now that the restriction is lifted, use\ntest_merge() to perform the octopus merge instead of manually\nduplicating test_merge() functionality.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex dab96c89aa..f6e22ec825 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -41,8 +41,7 @@ test_expect_success 'set up merge history' '\n \t\ttest_commit $i $i $i tag$i || return $?\n \tdone &&\n \tgit checkout 1 -b merge &&\n-\ttest_tick &&\n-\tgit merge -m octopus-merge 1 2 3 4 &&\n+\ttest_merge octopus-merge 1 2 3 4 &&\n \tgit checkout 1 -b L &&\n \ttest_commit left\n '\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383390","messageId":"c09f761185859998d33c4944fe3a7317e3a8c987.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"[PATCH v2 3/5] t4214: generate expect in their own test cases","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:17Z","receivedAt":"2019-10-04T00:23:25Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Before, the expect files of the test case were being generated in the\nsetup method. However, it would make more sense to generate these files\nwithin the test cases that actually use them so that it's obvious to\nfuture readers where the expected values are coming from.\n\nMove the generation of the expect files in their own respective test\ncases.\n\nWhile we're at it, we want to establish a pattern in this test suite\nthat, firstly, a non-colored test case is given then, immediately after,\nthe colored version is given.\n\nSwitch test cases \"log --graph with tricky octopus merge, no color\" and\n\"log --graph with tricky octopus merge with colors\" so that the \"no\ncolor\" version appears first.\n\nThis patch is best viewed with `--color-moved`.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 42 ++++++++++++++++++------------------\n 1 file changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex f6e22ec825..16776e347c 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -5,6 +5,20 @@ test_description='git log --graph of skewed left octopus merge.'\n . ./test-lib.sh\n \n test_expect_success 'set up merge history' '\n+\ttest_commit initial &&\n+\tfor i in 1 2 3 4 ; do\n+\t\tgit checkout master -b $i || return $?\n+\t\t# Make tag name different from branch name, to avoid\n+\t\t# ambiguity error when calling checkout.\n+\t\ttest_commit $i $i $i tag$i || return $?\n+\tdone &&\n+\tgit checkout 1 -b merge &&\n+\ttest_merge octopus-merge 1 2 3 4 &&\n+\tgit checkout 1 -b L &&\n+\ttest_commit left\n+'\n+\n+test_expect_success 'log --graph with tricky octopus merge, no color' '\n \tcat >expect.uncolored <<-\\EOF &&\n \t* left\n \t| *---. octopus-merge\n@@ -19,6 +33,13 @@ test_expect_success 'set up merge history' '\n \t|/\n \t* initial\n \tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_success 'log --graph with tricky octopus merge with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n \tcat >expect.colors <<-\\EOF &&\n \t* left\n \t| *---. octopus-merge\n@@ -33,32 +54,11 @@ test_expect_success 'set up merge history' '\n \t|/\n \t* initial\n \tEOF\n-\ttest_commit initial &&\n-\tfor i in 1 2 3 4 ; do\n-\t\tgit checkout master -b $i || return $?\n-\t\t# Make tag name different from branch name, to avoid\n-\t\t# ambiguity error when calling checkout.\n-\t\ttest_commit $i $i $i tag$i || return $?\n-\tdone &&\n-\tgit checkout 1 -b merge &&\n-\ttest_merge octopus-merge 1 2 3 4 &&\n-\tgit checkout 1 -b L &&\n-\ttest_commit left\n-'\n-\n-test_expect_success 'log --graph with tricky octopus merge with colors' '\n-\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n \tgit log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw &&\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n \n-test_expect_success 'log --graph with tricky octopus merge, no color' '\n-\tgit log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw &&\n-\tsed \"s/ *\\$//\" actual.raw >actual &&\n-\ttest_cmp expect.uncolored actual\n-'\n-\n # Repeat the previous two tests with \"normal\" octopus merge (i.e.,\n # without the first parent skewing to the \"left\" branch column).\n \n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383391","messageId":"e58c1929bc40dadc468a3e37e49a8de04c46af99.1570148053.git.liu.denton@gmail.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"[PATCH v2 5/5] t4214: demonstrate octopus graph coloring failure","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-10-04T00:23:22Z","receivedAt":"2019-10-04T00:23:28Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"The graph coloring logic for octopus merges currently has a bug. This\ncan be seen git.git with 74c7cfa875 (Merge of\nhttp://members.cox.net/junkio/git-jc.git, 2005-05-05), whose second\nchild is 211232bae6 (Octopus merge of the following five patches.,\n2005-05-05).\n\nIf one runs\n\n\tgit log --graph 74c7cfa875\n\none can see that the octopus merge is colored incorrectly. In\nparticular, the horizontal dashes are off by one color. Each horizontal\ndash should be the color of the line to their bottom-right. Instead, they\nare currently the color of the line to their bottom.\n\nDemonstrate this breakage with a few sets of test cases. These test\ncases should show not only simple cases of the bug occuring but trickier\nsituations that may not be handled properly in any attempt to fix the\nbug.\n\nWhile we're at it, include a passing test case as a canary in case an\nattempt to fix the bug breaks existing operation.\n\nSigned-off-by: Denton Liu \n---\n t/t4214-log-graph-octopus.sh | 282 ++++++++++++++++++++++++++++++++++-\n 1 file changed, 281 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh\nindex 097151da39..3ae8e51e50 100755\n--- a/t/t4214-log-graph-octopus.sh\n+++ b/t/t4214-log-graph-octopus.sh\n@@ -14,8 +14,13 @@ test_expect_success 'set up merge history' '\n \tdone &&\n \tgit checkout 1 -b merge &&\n \ttest_merge octopus-merge 1 2 3 4 &&\n+\ttest_commit after-merge &&\n \tgit checkout 1 -b L &&\n-\ttest_commit left\n+\ttest_commit left &&\n+\tgit checkout 4 -b crossover &&\n+\ttest_commit after-4 &&\n+\tgit checkout initial -b more-L &&\n+\ttest_commit after-initial\n '\n \n test_expect_success 'log --graph with tricky octopus merge, no color' '\n@@ -98,4 +103,279 @@ test_expect_success 'log --graph with normal octopus merge with colors' '\n \ttest_decode_color actual.colors &&\n \ttest_cmp expect.colors actual.colors\n '\n+\n+test_expect_success 'log --graph with normal octopus merge and child, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-merge\n+\t*---. octopus-merge\n+\t|\\ \\ \\\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with normal octopus and child merge with colors' '\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-merge\n+\t*---. octopus-merge\n+\t|\\ \\ \\\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with tricky octopus merge and its child, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* left\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t|/ / / /\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s left after-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with tricky octopus merge and its child with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* left\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t|/ / / /\n+\t| | | * 4\n+\t| | * | 3\n+\t| | |/\n+\t| * | 2\n+\t| |/\n+\t* | 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s left after-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with crossover in octopus merge, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-4\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-4 octopus-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with crossover in octopus merge with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-4\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-4 octopus-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with crossover in octopus merge and its child, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-4\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with crossover in octopus merge and its child with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-4\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| |_|_|/\n+\t|/| | |\n+\t* | | | 4\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with unrelated commit and octopus tip, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-initial\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| | | | * 4\n+\t| |_|_|/\n+\t|/| | |\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_success 'log --graph with unrelated commit and octopus tip with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-initial\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| | | | * 4\n+\t| |_|_|/\n+\t|/| | |\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n+test_expect_success 'log --graph with unrelated commit and octopus child, no color' '\n+\tcat >expect.uncolored <<-\\EOF &&\n+\t* after-initial\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| | | | * 4\n+\t| |_|_|/\n+\t|/| | |\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=never --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.raw &&\n+\tsed \"s/ *\\$//\" actual.raw >actual &&\n+\ttest_cmp expect.uncolored actual\n+'\n+\n+test_expect_failure 'log --graph with unrelated commit and octopus child with colors' '\n+\ttest_config log.graphColors red,green,yellow,blue,magenta,cyan &&\n+\tcat >expect.colors <<-\\EOF &&\n+\t* after-initial\n+\t| * after-merge\n+\t| *---. octopus-merge\n+\t| |\\ \\ \\\n+\t| | | | * 4\n+\t| |_|_|/\n+\t|/| | |\n+\t| | | * 3\n+\t| |_|/\n+\t|/| |\n+\t| | * 2\n+\t| |/\n+\t|/|\n+\t| * 1\n+\t|/\n+\t* initial\n+\tEOF\n+\tgit log --color=always --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.colors.raw &&\n+\ttest_decode_color actual.colors &&\n+\ttest_cmp expect.colors actual.colors\n+'\n+\n test_done\n-- \n2.23.0.565.g1cc52d20df\n\n"},{"id":"383397","messageId":"xmqqblux83et.fsf@gitster-ct.c.googlers.com","threadId":"51914","inReplyTo":"cover.1570148053.git.liu.denton@gmail.com","subject":"Re: [PATCH v2 0/5] t4214: cleanup and demonstrate graph bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-04T05:50:50Z","receivedAt":"2019-10-04T05:51:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Range-diff against v1:\n> 1: e77af8cde5 = 1: e77af8cde5 test-lib: let test_merge() perform octopus merges\n\nmicronit: I would say s/let/allow/ if I were writing this.\n\n> 2: 4a93deb3f6 = 2: 4a93deb3f6 t4214: use test_merge\n> 3: c09f761185 = 3: c09f761185 t4214: generate expect in their own test cases\n> 4: ad6d89440b = 4: ad6d89440b t4214: explicitly list tags in log\n> 5: 0b84bf5417 ! 5: e58c1929bc t4214: demonstrate octopus graph coloring failure\n\nQueued and pushed out. Thanks.\n"}]}