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

Re: [PATCH 01/10] diff --color-moved=zerba: fix alternate coloring

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 15, 2021, 03:24 UTC
Message-ID
<xmqq4kdzyfm0.fsf@gitster.g>
In-Reply-To
<374dbebcbf29b686508e51205b2f7a4e72104950.1623675888.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH 01/10] diff --color-moved=zerba: fix alternate coloring
Zerba?
Show 141 quoted lines
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> b0a2ba4776 ("diff --color-moved=zebra: be stricter with color
> alternation", 2018-11-23) sought to avoid using the alternate colors
> unless there are two adjacent moved blocks of the same
> sign. Unfortunately it contains two bugs that prevented it from fixing
> the problem properly. Firstly `last_symbol` is reset at the start of
> each iteration of the loop losing the symbol of the last line and
> secondly when deciding whether to use the alternate color it should be
> checking if the current line is the same sign of the last line, not a
> different sign. The combination of the two errors means that we still
> use the alternate color when we should do but we also use it when we
> shouldn't. This is most noticable when using
> --color-moved-ws=allow-indentation-change with hunks like
>
> -this line gets indented
> +    this line gets indented
>
> where the post image is colored with newMovedAlternate rather than
> newMoved. While this does not matter much, the next commit will change
> the coloring to be correct in this case, so lets fix the bug here to
> make it clear why the output is changing and add a regression test.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  diff.c                     |  4 +--
>  t/t4015-diff-whitespace.sh | 72 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 52c791574b71..cb068f8258c0 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1142,6 +1142,7 @@ static void mark_color_as_moved(struct diff_options *o,
>  	struct moved_block *pmb = NULL; /* potentially moved blocks */
>  	int pmb_nr = 0, pmb_alloc = 0;
>  	int n, flipped_block = 0, block_length = 0;
> +	enum diff_symbol last_symbol = 0;
>  
>  
>  	for (n = 0; n < o->emitted_symbols->nr; n++) {
> @@ -1149,7 +1150,6 @@ static void mark_color_as_moved(struct diff_options *o,
>  		struct moved_entry *key;
>  		struct moved_entry *match = NULL;
>  		struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];
> -		enum diff_symbol last_symbol = 0;
>  
>  		switch (l->s) {
>  		case DIFF_SYMBOL_PLUS:
> @@ -1214,7 +1214,7 @@ static void mark_color_as_moved(struct diff_options *o,
>  			}
>  
>  			if (adjust_last_block(o, n, block_length) &&
> -			    pmb_nr && last_symbol != l->s)
> +			    pmb_nr && last_symbol == l->s)
>  				flipped_block = (flipped_block + 1) % 2;
>  			else
>  				flipped_block = 0;
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index 2c13b62d3c65..920114cd795c 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -1442,6 +1442,78 @@ test_expect_success 'detect permutations inside moved code -- dimmed-zebra' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'zebra alternate color is only used when necessary' '
> +	cat >old.txt <<-\EOF &&
> +	line 1A should be marked as oldMoved newMovedAlternate
> +	line 1B should be marked as oldMoved newMovedAlternate
> +	unchanged
> +	line 2A should be marked as oldMoved newMovedAlternate
> +	line 2B should be marked as oldMoved newMovedAlternate
> +	line 3A should be marked as oldMovedAlternate newMoved
> +	line 3B should be marked as oldMovedAlternate newMoved
> +	unchanged
> +	line 4A should be marked as oldMoved newMovedAlternate
> +	line 4B should be marked as oldMoved newMovedAlternate
> +	line 5A should be marked as oldMovedAlternate newMoved
> +	line 5B should be marked as oldMovedAlternate newMoved
> +	line 6A should be marked as oldMoved newMoved
> +	line 6B should be marked as oldMoved newMoved
> +	EOF
> +	cat >new.txt <<-\EOF &&
> +	  line 1A should be marked as oldMoved newMovedAlternate
> +	  line 1B should be marked as oldMoved newMovedAlternate
> +	unchanged
> +	  line 3A should be marked as oldMovedAlternate newMoved
> +	  line 3B should be marked as oldMovedAlternate newMoved
> +	  line 2A should be marked as oldMoved newMovedAlternate
> +	  line 2B should be marked as oldMoved newMovedAlternate
> +	unchanged
> +	  line 6A should be marked as oldMoved newMoved
> +	  line 6B should be marked as oldMoved newMoved
> +	    line 4A should be marked as oldMoved newMovedAlternate
> +	    line 4B should be marked as oldMoved newMovedAlternate
> +	  line 5A should be marked as oldMovedAlternate newMoved
> +	  line 5B should be marked as oldMovedAlternate newMoved
> +	EOF
> +	test_expect_code 1 git diff --no-index --color --color-moved=zebra \
> +		 --color-moved-ws=allow-indentation-change \
> +		 old.txt new.txt >output &&
> +	grep -v index output | test_decode_color >actual &&
> +	cat >expected <<-\EOF &&
> +	<BOLD>diff --git a/old.txt b/new.txt<RESET>
> +	<BOLD>--- a/old.txt<RESET>
> +	<BOLD>+++ b/new.txt<RESET>
> +	<CYAN>@@ -1,14 +1,14 @@<RESET>
> +	<BOLD;MAGENTA>-line 1A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;MAGENTA>-line 1B should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 1A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 1B should be marked as oldMoved newMovedAlternate<RESET>
> +	 unchanged<RESET>
> +	<BOLD;MAGENTA>-line 2A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;MAGENTA>-line 2B should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;BLUE>-line 3A should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;BLUE>-line 3B should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 3A should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 3B should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;YELLOW>+<RESET><BOLD;YELLOW>  line 2A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;YELLOW>+<RESET><BOLD;YELLOW>  line 2B should be marked as oldMoved newMovedAlternate<RESET>
> +	 unchanged<RESET>
> +	<BOLD;MAGENTA>-line 4A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;MAGENTA>-line 4B should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;BLUE>-line 5A should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;BLUE>-line 5B should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;MAGENTA>-line 6A should be marked as oldMoved newMoved<RESET>
> +	<BOLD;MAGENTA>-line 6B should be marked as oldMoved newMoved<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 6A should be marked as oldMoved newMoved<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 6B should be marked as oldMoved newMoved<RESET>
> +	<BOLD;YELLOW>+<RESET><BOLD;YELLOW>    line 4A should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;YELLOW>+<RESET><BOLD;YELLOW>    line 4B should be marked as oldMoved newMovedAlternate<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 5A should be marked as oldMovedAlternate newMoved<RESET>
> +	<BOLD;CYAN>+<RESET><BOLD;CYAN>  line 5B should be marked as oldMovedAlternate newMoved<RESET>
> +	EOF
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'cmd option assumes configured colored-moved' '
>  	test_config color.diff.oldMoved "magenta" &&
>  	test_config color.diff.newMoved "cyan" &&
Previous: Phillip Wood via GitGitGadgetNext: Phillip Wood
Message 3 of 92 in “diff --color-moved[-ws] speedups”
  1. 00/10 diff --color-moved[-ws] speedupsPhillip Wood via GitGitGadget, Jun 14, 2021
  2. 01/10 diff --color-moved=zerba: fix alternate coloringPhillip Wood via GitGitGadget, Jun 14, 2021
  3. Junio C HamanoJun 15, 2021
  4. Phillip WoodJun 15, 2021
  5. 07/10 diff --color-moved: shrink potential moved blocks as we goPhillip Wood via GitGitGadget, Jun 14, 2021
  6. 09/10 diff --color-moved-ws=allow-indentation-change: improve hash lookupsPhillip Wood via GitGitGadget, Jun 14, 2021
  7. Elijah NewrenJul 9, 2021
  8. 02/10 diff --color-moved: avoid false short line matches and bad zerba coloringPhillip Wood via GitGitGadget, Jun 14, 2021
  9. 03/10 diff: simplify allow-indentation-change delta calculationPhillip Wood via GitGitGadget, Jun 14, 2021
  10. 04/10 diff --color-moved-ws=allow-indentation-change: simplify and optimizePhillip Wood via GitGitGadget, Jun 14, 2021
  11. 05/10 diff --color-moved: call comparison function directlyPhillip Wood via GitGitGadget, Jun 14, 2021
  12. 06/10 diff --color-moved: unify moved block growth functionsPhillip Wood via GitGitGadget, Jun 14, 2021
  13. 10/10 diff --color-moved: intern stringsPhillip Wood via GitGitGadget, Jun 14, 2021
  14. 08/10 diff --color-moved: stop clearing potential moved blocksPhillip Wood via GitGitGadget, Jun 14, 2021
  15. Ævar Arnfjörð BjarmasonJun 16, 2021
  16. Phillip WoodJun 21, 2021
  17. 00/12 diff --color-moved[-ws] speedupsPhillip Wood via GitGitGadget, Jul 20, 2021
  18. 05/12 diff --color-moved-ws=allow-indentation-change: simplify and optimizePhillip Wood via GitGitGadget, Jul 20, 2021
  19. 04/12 diff: simplify allow-indentation-change delta calculationPhillip Wood via GitGitGadget, Jul 20, 2021
  20. 03/12 diff --color-moved: avoid false short line matches and bad zerba coloringPhillip Wood via GitGitGadget, Jul 20, 2021
  21. 02/12 diff --color-moved=zebra: fix alternate coloringPhillip Wood via GitGitGadget, Jul 20, 2021
  22. 01/12 diff --color-moved: add perf testsPhillip Wood via GitGitGadget, Jul 20, 2021
  23. 07/12 diff --color-moved: unify moved block growth functionsPhillip Wood via GitGitGadget, Jul 20, 2021
  24. 06/12 diff --color-moved: call comparison function directlyPhillip Wood via GitGitGadget, Jul 20, 2021
  25. 09/12 diff --color-moved: stop clearing potential moved blocksPhillip Wood via GitGitGadget, Jul 20, 2021
  26. 08/12 diff --color-moved: shrink potential moved blocks as we goPhillip Wood via GitGitGadget, Jul 20, 2021
  27. 11/12 diff: use designated initializers for emitted_diff_symbolPhillip Wood via GitGitGadget, Jul 20, 2021
  28. 10/12 diff --color-moved-ws=allow-indentation-change: improve hash lookupsPhillip Wood via GitGitGadget, Jul 20, 2021
  29. 12/12 diff --color-moved: intern stringsPhillip Wood via GitGitGadget, Jul 20, 2021
  30. Phillip WoodJul 20, 2021
  31. 00/15 diff --color-moved[-ws] speedupsPhillip Wood via GitGitGadget, Oct 27, 2021
  32. 01/15 diff --color-moved: add perf testsPhillip Wood via GitGitGadget, Oct 27, 2021
  33. Junio C HamanoOct 28, 2021
  34. Phillip WoodOct 29, 2021
  35. Ævar Arnfjörð BjarmasonOct 29, 2021
  36. Phillip WoodNov 10, 2021
  37. 02/15 diff --color-moved: clear all flags on blocks that are too shortPhillip Wood via GitGitGadget, Oct 27, 2021
  38. 03/15 diff --color-moved: factor out functionPhillip Wood via GitGitGadget, Oct 27, 2021
  39. Junio C HamanoOct 28, 2021
  40. Phillip WoodOct 29, 2021
  41. 04/15 diff --color-moved: rewind when discarding pmbPhillip Wood via GitGitGadget, Oct 27, 2021
  42. 05/15 diff --color-moved=zebra: fix alternate coloringPhillip Wood via GitGitGadget, Oct 27, 2021
  43. 06/15 diff --color-moved: avoid false short line matches and bad zerba coloringPhillip Wood via GitGitGadget, Oct 27, 2021
  44. 07/15 diff: simplify allow-indentation-change delta calculationPhillip Wood via GitGitGadget, Oct 27, 2021
  45. 08/15 diff --color-moved-ws=allow-indentation-change: simplify and optimizePhillip Wood via GitGitGadget, Oct 27, 2021
  46. 09/15 diff --color-moved: call comparison function directlyPhillip Wood via GitGitGadget, Oct 27, 2021
  47. 10/15 diff --color-moved: unify moved block growth functionsPhillip Wood via GitGitGadget, Oct 27, 2021
  48. 11/15 diff --color-moved: shrink potential moved blocks as we goPhillip Wood via GitGitGadget, Oct 27, 2021
  49. 12/15 diff --color-moved: stop clearing potential moved blocksPhillip Wood via GitGitGadget, Oct 27, 2021
  50. 13/15 diff --color-moved-ws=allow-indentation-change: improve hash lookupsPhillip Wood via GitGitGadget, Oct 27, 2021
  51. 14/15 diff: use designated initializers for emitted_diff_symbolPhillip Wood via GitGitGadget, Oct 27, 2021
  52. 15/15 diff --color-moved: intern stringsPhillip Wood via GitGitGadget, Oct 27, 2021
  53. Phillip WoodOct 27, 2021
  54. 00/15 diff --color-moved[-ws] speedupsPhillip Wood via GitGitGadget, Nov 16, 2021
  55. 01/15 diff --color-moved: add perf testsPhillip Wood via GitGitGadget, Nov 16, 2021
  56. 02/15 diff --color-moved: clear all flags on blocks that are too shortPhillip Wood via GitGitGadget, Nov 16, 2021
  57. 03/15 diff --color-moved: factor out functionPhillip Wood via GitGitGadget, Nov 16, 2021
  58. 04/15 diff --color-moved: rewind when discarding pmbPhillip Wood via GitGitGadget, Nov 16, 2021
  59. 05/15 diff --color-moved=zebra: fix alternate coloringPhillip Wood via GitGitGadget, Nov 16, 2021
  60. Johannes SchindelinNov 22, 2021
  61. 06/15 diff --color-moved: avoid false short line matches and bad zerba coloringPhillip Wood via GitGitGadget, Nov 16, 2021
  62. Johannes SchindelinNov 22, 2021
  63. Phillip WoodNov 22, 2021
  64. Johannes SchindelinNov 22, 2021
  65. 07/15 diff: simplify allow-indentation-change delta calculationPhillip Wood via GitGitGadget, Nov 16, 2021
  66. 08/15 diff --color-moved-ws=allow-indentation-change: simplify and optimizePhillip Wood via GitGitGadget, Nov 16, 2021
  67. Johannes SchindelinNov 23, 2021
  68. 11/15 diff --color-moved: shrink potential moved blocks as we goPhillip Wood via GitGitGadget, Nov 16, 2021
  69. 10/15 diff --color-moved: unify moved block growth functionsPhillip Wood via GitGitGadget, Nov 16, 2021
  70. 09/15 diff --color-moved: call comparison function directlyPhillip Wood via GitGitGadget, Nov 16, 2021
  71. Johannes SchindelinNov 23, 2021
  72. 12/15 diff --color-moved: stop clearing potential moved blocksPhillip Wood via GitGitGadget, Nov 16, 2021
  73. 13/15 diff --color-moved-ws=allow-indentation-change: improve hash lookupsPhillip Wood via GitGitGadget, Nov 16, 2021
  74. 14/15 diff: use designated initializers for emitted_diff_symbolPhillip Wood via GitGitGadget, Nov 16, 2021
  75. 15/15 diff --color-moved: intern stringsPhillip Wood via GitGitGadget, Nov 16, 2021
  76. Johannes SchindelinDec 8, 2021
  77. 00/15 diff --color-moved[-ws] speedupsPhillip Wood via GitGitGadget, Dec 9, 2021
  78. 01/15 diff --color-moved: add perf testsPhillip Wood via GitGitGadget, Dec 9, 2021
  79. 02/15 diff --color-moved: clear all flags on blocks that are too shortPhillip Wood via GitGitGadget, Dec 9, 2021
  80. 05/15 diff --color-moved=zebra: fix alternate coloringPhillip Wood via GitGitGadget, Dec 9, 2021
  81. 03/15 diff --color-moved: factor out functionPhillip Wood via GitGitGadget, Dec 9, 2021
  82. 06/15 diff --color-moved: avoid false short line matches and bad zebra coloringPhillip Wood via GitGitGadget, Dec 9, 2021
  83. 07/15 diff: simplify allow-indentation-change delta calculationPhillip Wood via GitGitGadget, Dec 9, 2021
  84. 04/15 diff --color-moved: rewind when discarding pmbPhillip Wood via GitGitGadget, Dec 9, 2021
  85. 08/15 diff --color-moved-ws=allow-indentation-change: simplify and optimizePhillip Wood via GitGitGadget, Dec 9, 2021
  86. 09/15 diff --color-moved: call comparison function directlyPhillip Wood via GitGitGadget, Dec 9, 2021
  87. 10/15 diff --color-moved: unify moved block growth functionsPhillip Wood via GitGitGadget, Dec 9, 2021
  88. 11/15 diff --color-moved: shrink potential moved blocks as we goPhillip Wood via GitGitGadget, Dec 9, 2021
  89. 12/15 diff --color-moved: stop clearing potential moved blocksPhillip Wood via GitGitGadget, Dec 9, 2021
  90. 13/15 diff --color-moved-ws=allow-indentation-change: improve hash lookupsPhillip Wood via GitGitGadget, Dec 9, 2021
  91. 15/15 diff --color-moved: intern stringsPhillip Wood via GitGitGadget, Dec 9, 2021
  92. 14/15 diff: use designated initializers for emitted_diff_symbolPhillip Wood via GitGitGadget, Dec 9, 2021

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.