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

[PATCH v3 06/15] diff --color-moved: avoid false short line matches and bad zerba coloring

From
PGPhillip Wood via GitGitGadget <gitgitgadget@gmail.com>
Date
Oct 27, 2021, 12:04 UTC
Message-ID
<2717ff500d2ebf82179f89c90d690718221e9fa8.1635336263.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.981.v3.git.1635336262.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>

When marking moved lines it is possible for a block of potential matched lines to extend past a change in sign when there is a sequence of added lines whose text matches the text of a sequence of deleted and added lines. Most of the time either `match` will be NULL or `pmb_advance_or_null()` will fail when the loop encounters a change of sign but there are corner cases where `match` is non-NULL and `pmb_advance_or_null()` successfully advances the moved block despite the change in sign.

One consequence of this is highlighting a short line as moved when it should not be. For example

-moved line  # Correctly highlighted as moved
+short line  # Wrongly highlighted as moved
 context
+moved line  # Correctly highlighted as moved
+short line
 context
-short line

The other consequence is coloring a moved addition following a moved deletion in the wrong color. In the example below the first "+moved line 3" should be highlighted as newMoved not newMovedAlternate.

-moved line 1 # Correctly highlighted as oldMoved
-moved line 2 # Correctly highlighted as oldMovedAlternate
+moved line 3 # Wrongly highlighted as newMovedAlternate
 context      # Everything else is highlighted correctly
+moved line 2
+moved line 3
 context
+moved line 1
-moved line 3

These false matches are more likely when using --color-moved-ws with the exception of --color-moved-ws=allow-indentation-change which ties the sign of the current whitespace delta to the sign of the line to avoid this problem. The fix is to check that the sign of the new line being matched is the same as the sign of the line that started the block of potential matches.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 diff.c                     | 17 ++++++----
 t/t4015-diff-whitespace.sh | 65 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index 53f0df75329..efba2789354 100644
--- a/diff.c
+++ b/diff.c
@@ -1176,7 +1176,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;
+	enum diff_symbol moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;
 
 
 	for (n = 0; n < o->emitted_symbols->nr; n++) {
@@ -1202,7 +1202,7 @@ static void mark_color_as_moved(struct diff_options *o,
 			flipped_block = 0;
 		}
 
-		if (!match) {
+		if (pmb_nr && (!match || l->s != moved_symbol)) {
 			int i;
 
 			if (!adjust_last_block(o, n, block_length) &&
@@ -1219,12 +1219,13 @@ static void mark_color_as_moved(struct diff_options *o,
 			pmb_nr = 0;
 			block_length = 0;
 			flipped_block = 0;
-			last_symbol = l->s;
+		}
+		if (!match) {
+			moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;
 			continue;
 		}
 
 		if (o->color_moved == COLOR_MOVED_PLAIN) {
-			last_symbol = l->s;
 			l->flags |= DIFF_SYMBOL_MOVED_LINE;
 			continue;
 		}
@@ -1251,11 +1252,16 @@ static void mark_color_as_moved(struct diff_options *o,
 							    &pmb, &pmb_alloc,
 							    &pmb_nr);
 
-			if (contiguous && pmb_nr && last_symbol == l->s)
+			if (contiguous && pmb_nr && moved_symbol == l->s)
 				flipped_block = (flipped_block + 1) % 2;
 			else
 				flipped_block = 0;
 
+			if (pmb_nr)
+				moved_symbol = l->s;
+			else
+				moved_symbol = DIFF_SYMBOL_BINARY_DIFF_HEADER;
+
 			block_length = 0;
 		}
 
@@ -1265,7 +1271,6 @@ static void mark_color_as_moved(struct diff_options *o,
 			if (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)
 				l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
 		}
-		last_symbol = l->s;
 	}
 	adjust_last_block(o, n, block_length);
 
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4e0fd76c6c5..15782c879d2 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1514,6 +1514,71 @@ test_expect_success 'zebra alternate color is only used when necessary' '
 	test_cmp expected actual
 '
 
+test_expect_success 'short lines of opposite sign do not get marked as moved' '
+	cat >old.txt <<-\EOF &&
+	this line should be marked as moved
+	unchanged
+	unchanged
+	unchanged
+	unchanged
+	too short
+	this line should be marked as oldMoved newMoved
+	this line should be marked as oldMovedAlternate newMoved
+	unchanged 1
+	unchanged 2
+	unchanged 3
+	unchanged 4
+	this line should be marked as oldMoved newMoved/newMovedAlternate
+	EOF
+	cat >new.txt <<-\EOF &&
+	too short
+	unchanged
+	unchanged
+	this line should be marked as moved
+	too short
+	unchanged
+	unchanged
+	this line should be marked as oldMoved newMoved/newMovedAlternate
+	unchanged 1
+	unchanged 2
+	this line should be marked as oldMovedAlternate newMoved
+	this line should be marked as oldMoved newMoved/newMovedAlternate
+	unchanged 3
+	this line should be marked as oldMoved newMoved
+	unchanged 4
+	EOF
+	test_expect_code 1 git diff --no-index --color --color-moved=zebra \
+		old.txt new.txt >output && cat output &&
+	grep -v index output | test_decode_color >actual &&
+	cat >expect <<-\EOF &&
+	<BOLD>diff --git a/old.txt b/new.txt<RESET>
+	<BOLD>--- a/old.txt<RESET>
+	<BOLD>+++ b/new.txt<RESET>
+	<CYAN>@@ -1,13 +1,15 @@<RESET>
+	<BOLD;MAGENTA>-this line should be marked as moved<RESET>
+	<GREEN>+<RESET><GREEN>too short<RESET>
+	 unchanged<RESET>
+	 unchanged<RESET>
+	<BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as moved<RESET>
+	<GREEN>+<RESET><GREEN>too short<RESET>
+	 unchanged<RESET>
+	 unchanged<RESET>
+	<RED>-too short<RESET>
+	<BOLD;MAGENTA>-this line should be marked as oldMoved newMoved<RESET>
+	<BOLD;BLUE>-this line should be marked as oldMovedAlternate newMoved<RESET>
+	<BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>
+	 unchanged 1<RESET>
+	 unchanged 2<RESET>
+	<BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMovedAlternate newMoved<RESET>
+	<BOLD;YELLOW>+<RESET><BOLD;YELLOW>this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>
+	 unchanged 3<RESET>
+	<BOLD;CYAN>+<RESET><BOLD;CYAN>this line should be marked as oldMoved newMoved<RESET>
+	 unchanged 4<RESET>
+	<BOLD;MAGENTA>-this line should be marked as oldMoved newMoved/newMovedAlternate<RESET>
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'cmd option assumes configured colored-moved' '
 	test_config color.diff.oldMoved "magenta" &&
 	test_config color.diff.newMoved "cyan" &&

-- 
gitgitgadget
Previous: Phillip Wood via GitGitGadgetNext: Phillip Wood via GitGitGadget
Message 43 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.