git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v2 0/5] t4214: cleanup and demonstrate graph bug

From
Denton Liu <liu.denton@gmail.com>
Date
Oct 4, 2019, 00:23 UTC
Message-ID
<cover.1570148053.git.liu.denton@gmail.com>
In-Reply-To
<cover.1569407150.git.liu.denton@gmail.com>

Junio, the test cases from earlier didn't exactly cover the cases Peff talked about so I added a few more test cases. These should cover those situations and a few more so we can be extra sure when the bug is fixed.

This patchset cleans up t4214 and then, in the last patch, demonstrates a bug in the way some octopus merges are colored.

While I was playing around with Pratyush's octopus merge in git-gui, I noticed that there was a bug in `git log --graph`. The horizontal lines in the octopus merge seemed to have an off-by-one error in their coloring. More detail in the last patch.

I tried my hand at fixing the bug but in the hour I spent going at it, I couldn't fix the logic up. The buggy logic is in graph.c: graph_draw_octopus_merge() in case anyone is interested.

Changes since v1:
* Add a few more test cases to demonstrate more failure (and passing)
  conditions
Denton Liu (5):
  test-lib: let test_merge() perform octopus merges
  t4214: use test_merge
  t4214: generate expect in their own test cases
  t4214: explicitly list tags in log
  t4214: demonstrate octopus graph coloring failure
 t/t4214-log-graph-octopus.sh | 329 ++++++++++++++++++++++++++++++++---
 t/test-lib-functions.sh      |   6 +-
 2 files changed, 308 insertions(+), 27 deletions(-)
Range-diff against v1:
1:  e77af8cde5 = 1:  e77af8cde5 test-lib: let test_merge() perform octopus merges
2:  4a93deb3f6 = 2:  4a93deb3f6 t4214: use test_merge
3:  c09f761185 = 3:  c09f761185 t4214: generate expect in their own test cases
4:  ad6d89440b = 4:  ad6d89440b t4214: explicitly list tags in log
5:  0b84bf5417 ! 5:  e58c1929bc t4214: demonstrate octopus graph coloring failure
    @@ Commit message
         dash should be the color of the line to their bottom-right. Instead, they
         are currently the color of the line to their bottom.
     
    -    Demonstrate this breakage with two sets of test cases. The first pair of
    -    test cases demonstrates the breakage with a similar case as the above.
    -    The second pair of test cases demonstrates a similar breakage but with
    -    the last parent crossing over.
    +    Demonstrate this breakage with a few sets of test cases. These test
    +    cases should show not only simple cases of the bug occuring but trickier
    +    situations that may not be handled properly in any attempt to fix the
    +    bug.
     
    -    The second pair of test cases are included as a result of my (poor)
    -    attempts at fixing the bug. This case seems particularly tricky to
    -    handle. Good luck!
    +    While we're at it, include a passing test case as a canary in case an
    +    attempt to fix the bug breaks existing operation.
     
      ## t/t4214-log-graph-octopus.sh ##
     @@ t/t4214-log-graph-octopus.sh: test_expect_success 'set up merge history' '
    @@ t/t4214-log-graph-octopus.sh: test_expect_success 'set up merge history' '
     -	test_commit left
     +	test_commit left &&
     +	git checkout 4 -b crossover &&
    -+	test_commit after-4
    ++	test_commit after-4 &&
    ++	git checkout initial -b more-L &&
    ++	test_commit after-initial
      '
      
      test_expect_success 'log --graph with tricky octopus merge, no color' '
    @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop
      	test_cmp expect.colors actual.colors
      '
     +
    -+test_expect_success 'log --graph with tricky octopus merge and its parent, no color' '
    ++test_expect_success 'log --graph with normal octopus merge and child, no color' '
    ++	cat >expect.uncolored <<-\EOF &&
    ++	* after-merge
    ++	*---.   octopus-merge
    ++	|\ \ \
    ++	| | | * 4
    ++	| | * | 3
    ++	| | |/
    ++	| * | 2
    ++	| |/
    ++	* | 1
    ++	|/
    ++	* initial
    ++	EOF
    ++	git log --color=never --graph --date-order --pretty=tformat:%s after-merge >actual.raw &&
    ++	sed "s/ *\$//" actual.raw >actual &&
    ++	test_cmp expect.uncolored actual
    ++'
    ++
    ++test_expect_failure 'log --graph with normal octopus and child merge with colors' '
    ++	cat >expect.colors <<-\EOF &&
    ++	* after-merge
    ++	*<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
    ++	<GREEN>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET>
    ++	<GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
    ++	<GREEN>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3
    ++	<GREEN>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
    ++	<GREEN>|<RESET> * <MAGENTA>|<RESET> 2
    ++	<GREEN>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
    ++	* <MAGENTA>|<RESET> 1
    ++	<MAGENTA>|<RESET><MAGENTA>/<RESET>
    ++	* initial
    ++	EOF
    ++	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
    ++	git log --color=always --graph --date-order --pretty=tformat:%s after-merge >actual.colors.raw &&
    ++	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
    ++	test_cmp expect.colors actual.colors
    ++'
    ++
    ++test_expect_success 'log --graph with tricky octopus merge and its child, no color' '
     +	cat >expect.uncolored <<-\EOF &&
     +	* left
     +	| * after-merge
    @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop
     +	test_cmp expect.uncolored actual
     +'
     +
    -+test_expect_failure 'log --graph with tricky octopus merge and its parent with colors' '
    ++test_expect_failure 'log --graph with tricky octopus merge and its child with colors' '
     +	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
     +	cat >expect.colors <<-\EOF &&
     +	* left
    @@ t/t4214-log-graph-octopus.sh: test_expect_success 'log --graph with normal octop
     +	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
     +	test_cmp expect.colors actual.colors
     +'
    ++
    ++test_expect_success 'log --graph with crossover in octopus merge and its child, no color' '
    ++	cat >expect.uncolored <<-\EOF &&
    ++	* after-4
    ++	| * after-merge
    ++	| *---.   octopus-merge
    ++	| |\ \ \
    ++	| |_|_|/
    ++	|/| | |
    ++	* | | | 4
    ++	| | | * 3
    ++	| |_|/
    ++	|/| |
    ++	| | * 2
    ++	| |/
    ++	|/|
    ++	| * 1
    ++	|/
    ++	* initial
    ++	EOF
    ++	git log --color=never --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.raw &&
    ++	sed "s/ *\$//" actual.raw >actual &&
    ++	test_cmp expect.uncolored actual
    ++'
    ++
    ++test_expect_failure 'log --graph with crossover in octopus merge and its child with colors' '
    ++	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
    ++	cat >expect.colors <<-\EOF &&
    ++	* after-4
    ++	<RED>|<RESET> * after-merge
    ++	<RED>|<RESET> *<MAGENTA>-<RESET><MAGENTA>-<RESET><RED>-<RESET><RED>.<RESET>   octopus-merge
    ++	<RED>|<RESET> <YELLOW>|<RESET><BLUE>\<RESET> <MAGENTA>\<RESET> <RED>\<RESET>
    ++	<RED>|<RESET> <YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>_<RESET><MAGENTA>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET>
    ++	* <YELLOW>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET> 4
    ++	<CYAN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 3
    ++	<CYAN>|<RESET> <YELLOW>|<RESET><CYAN>_<RESET><BLUE>|<RESET><CYAN>/<RESET>
    ++	<CYAN>|<RESET><CYAN>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET>
    ++	<CYAN>|<RESET> <YELLOW>|<RESET> * 2
    ++	<CYAN>|<RESET> <YELLOW>|<RESET><CYAN>/<RESET>
    ++	<CYAN>|<RESET><CYAN>/<RESET><YELLOW>|<RESET>
    ++	<CYAN>|<RESET> * 1
    ++	<CYAN>|<RESET><CYAN>/<RESET>
    ++	* initial
    ++	EOF
    ++	git log --color=always --graph --date-order --pretty=tformat:%s after-4 after-merge >actual.colors.raw &&
    ++	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
    ++	test_cmp expect.colors actual.colors
    ++'
    ++
    ++test_expect_success 'log --graph with unrelated commit and octopus tip, no color' '
    ++	cat >expect.uncolored <<-\EOF &&
    ++	* after-initial
    ++	| *---.   octopus-merge
    ++	| |\ \ \
    ++	| | | | * 4
    ++	| |_|_|/
    ++	|/| | |
    ++	| | | * 3
    ++	| |_|/
    ++	|/| |
    ++	| | * 2
    ++	| |/
    ++	|/|
    ++	| * 1
    ++	|/
    ++	* initial
    ++	EOF
    ++	git log --color=never --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.raw &&
    ++	sed "s/ *\$//" actual.raw >actual &&
    ++	test_cmp expect.uncolored actual
    ++'
    ++
    ++test_expect_success 'log --graph with unrelated commit and octopus tip with colors' '
    ++	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
    ++	cat >expect.colors <<-\EOF &&
    ++	* after-initial
    ++	<RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET>   octopus-merge
    ++	<RED>|<RESET> <GREEN>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET>
    ++	<RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
    ++	<RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET>
    ++	<RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 3
    ++	<RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET>
    ++	<RED>|<RESET> <GREEN>|<RESET> * 2
    ++	<RED>|<RESET> <GREEN>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><GREEN>|<RESET>
    ++	<RED>|<RESET> * 1
    ++	<RED>|<RESET><RED>/<RESET>
    ++	* initial
    ++	EOF
    ++	git log --color=always --graph --date-order --pretty=tformat:%s after-initial octopus-merge >actual.colors.raw &&
    ++	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
    ++	test_cmp expect.colors actual.colors
    ++'
    ++
    ++test_expect_success 'log --graph with unrelated commit and octopus child, no color' '
    ++	cat >expect.uncolored <<-\EOF &&
    ++	* after-initial
    ++	| * after-merge
    ++	| *---.   octopus-merge
    ++	| |\ \ \
    ++	| | | | * 4
    ++	| |_|_|/
    ++	|/| | |
    ++	| | | * 3
    ++	| |_|/
    ++	|/| |
    ++	| | * 2
    ++	| |/
    ++	|/|
    ++	| * 1
    ++	|/
    ++	* initial
    ++	EOF
    ++	git log --color=never --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.raw &&
    ++	sed "s/ *\$//" actual.raw >actual &&
    ++	test_cmp expect.uncolored actual
    ++'
    ++
    ++test_expect_failure 'log --graph with unrelated commit and octopus child with colors' '
    ++	test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
    ++	cat >expect.colors <<-\EOF &&
    ++	* after-initial
    ++	<RED>|<RESET> * after-merge
    ++	<RED>|<RESET> *<MAGENTA>-<RESET><MAGENTA>-<RESET><CYAN>-<RESET><CYAN>.<RESET>   octopus-merge
    ++	<RED>|<RESET> <YELLOW>|<RESET><BLUE>\<RESET> <MAGENTA>\<RESET> <CYAN>\<RESET>
    ++	<RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET> * 4
    ++	<RED>|<RESET> <YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>_<RESET><MAGENTA>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET>
    ++	<RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 3
    ++	<RED>|<RESET> <YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><YELLOW>|<RESET> <BLUE>|<RESET>
    ++	<RED>|<RESET> <YELLOW>|<RESET> * 2
    ++	<RED>|<RESET> <YELLOW>|<RESET><RED>/<RESET>
    ++	<RED>|<RESET><RED>/<RESET><YELLOW>|<RESET>
    ++	<RED>|<RESET> * 1
    ++	<RED>|<RESET><RED>/<RESET>
    ++	* initial
    ++	EOF
    ++	git log --color=always --graph --date-order --pretty=tformat:%s after-initial after-merge >actual.colors.raw &&
    ++	test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
    ++	test_cmp expect.colors actual.colors
    ++'
     +
      test_done
-- 
2.23.0.565.g1cc52d20df
Previous: Junio C HamanoNext: Denton Liu
Message 10 of 16 in “t4214: cleanup and demonstrate graph bug”
  1. 0/5 t4214: cleanup and demonstrate graph bugDenton Liu, Sep 25, 2019
  2. 1/5 test-lib: let test_merge() perform octopus mergesDenton Liu, Sep 25, 2019
  3. 2/5 t4214: use test_mergeDenton Liu, Sep 25, 2019
  4. 3/5 t4214: generate expect in their own test casesDenton Liu, Sep 25, 2019
  5. 4/5 t4214: explicitly list tags in logDenton Liu, Sep 25, 2019
  6. 5/5 t4214: demonstrate octopus graph coloring failureDenton Liu, Sep 25, 2019
  7. Denton LiuSep 25, 2019
  8. Jeff KingSep 26, 2019
  9. Junio C HamanoOct 3, 2019
  10. 0/5 t4214: cleanup and demonstrate graph bugDenton Liu, Oct 4, 2019
  11. 1/5 test-lib: let test_merge() perform octopus mergesDenton Liu, Oct 4, 2019
  12. 4/5 t4214: explicitly list tags in logDenton Liu, Oct 4, 2019
  13. 2/5 t4214: use test_mergeDenton Liu, Oct 4, 2019
  14. 3/5 t4214: generate expect in their own test casesDenton Liu, Oct 4, 2019
  15. 5/5 t4214: demonstrate octopus graph coloring failureDenton Liu, Oct 4, 2019
  16. Junio C HamanoOct 4, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.