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

[PATCH v2 3/3] diff: check MIN_BLOCK_LENGTH at start of new block

From
JTJonathan Tan <jonathantanmy@google.com>
Date
Aug 14, 2017, 21:31 UTC
Message-ID
<288c6c29d212088175b13074dba23c9dbdaa2c67.1502745892.git.jonathantanmy@google.com>
In-Reply-To
<cover.1502745892.git.jonathantanmy@google.com>

The existing documentation states "If there are fewer than 3 adjacent moved lines, they are not marked up as moved", which is ambiguous as to whether "adjacent moved lines" must be adjacent both at the source and at the destination, or be adjacent merely at the source or merely at the destination. The behavior of the current code takes the latter interpretation, but the behavior of blocks being conceptually painted as blocks and then "unpainted" as lines is confusing to me.

Therefore, clarify the ambiguity in the documentation in the stricter direction - a block is completely painted or not at all - and update the code accordingly.

This requires a change in the test "detect malicious moved code, inside file" in that the malicious change is now marked without the move colors (because the blocks involved are too small), contrasting with the subsequent test where the non-malicious change is marked with move colors.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 Documentation/diff-options.txt |  6 ++--
 diff.c                         |  6 +++-
 t/t4015-diff-whitespace.sh     | 71 +++++++++++++++++++++++++++++++++---------
 3 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bc52bd0b9..1ee3ca3f6 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -257,10 +257,10 @@ zebra::
 	Blocks of moved code are detected greedily. The detected blocks are
 	painted using either the 'color.diff.{old,new}Moved' color or
 	'color.diff.{old,new}MovedAlternative'. The change between
-	the two colors indicates that a new block was detected. If there
-	are fewer than 3 adjacent moved lines, they are not marked up
+	the two colors indicates that a new block was detected. If a block
+	has fewer than 3 adjacent moved lines, it is not marked up
 	as moved, but the regular colors 'color.diff.{old,new}' will be
-	used.
+	used instead.
 dimmed_zebra::
 	Similar to 'zebra', but additional dimming of uninteresting parts
 	of moved code is performed. The bordering lines of two adjacent
diff --git a/diff.c b/diff.c
index f598d8a3a..20b784736 100644
--- a/diff.c
+++ b/diff.c
@@ -923,7 +923,6 @@ static void mark_color_as_moved(struct diff_options *o,
 		}
 
 		l->flags |= DIFF_SYMBOL_MOVED_LINE;
-		block_length++;
 
 		if (o->color_moved == COLOR_MOVED_PLAIN)
 			continue;
@@ -953,8 +952,13 @@ static void mark_color_as_moved(struct diff_options *o,
 			}
 
 			flipped_block = (flipped_block + 1) % 2;
+
+			adjust_last_block(o, n, block_length);
+			block_length = 0;
 		}
 
+		block_length++;
+
 		if (flipped_block)
 			l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
 	}
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6f7758e5c..d0613a189 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1097,13 +1097,13 @@ test_expect_success 'detect malicious moved code, inside file' '
 	 printf("World\n");<RESET>
 	 }<RESET>
 	 <RESET>
-	<BRED>-int secure_foo(struct user *u)<RESET>
-	<BRED>-{<RESET>
-	<BLUE>-if (!u->is_allowed_foo)<RESET>
-	<BLUE>-return;<RESET>
-	<BRED>-foo(u);<RESET>
-	<BLUE>-}<RESET>
-	<BLUE>-<RESET>
+	<RED>-int secure_foo(struct user *u)<RESET>
+	<RED>-{<RESET>
+	<RED>-if (!u->is_allowed_foo)<RESET>
+	<RED>-return;<RESET>
+	<RED>-foo(u);<RESET>
+	<RED>-}<RESET>
+	<RED>-<RESET>
 	 int main()<RESET>
 	 {<RESET>
 	 foo();<RESET>
@@ -1115,13 +1115,13 @@ test_expect_success 'detect malicious moved code, inside file' '
 	 printf("Hello World, but different\n");<RESET>
 	 }<RESET>
 	 <RESET>
-	<BGREEN>+<RESET><BGREEN>int secure_foo(struct user *u)<RESET>
-	<BGREEN>+<RESET><BGREEN>{<RESET>
-	<YELLOW>+<RESET><YELLOW>foo(u);<RESET>
-	<BGREEN>+<RESET><BGREEN>if (!u->is_allowed_foo)<RESET>
-	<BGREEN>+<RESET><BGREEN>return;<RESET>
-	<YELLOW>+<RESET><YELLOW>}<RESET>
-	<YELLOW>+<RESET>
+	<GREEN>+<RESET><GREEN>int secure_foo(struct user *u)<RESET>
+	<GREEN>+<RESET><GREEN>{<RESET>
+	<GREEN>+<RESET><GREEN>foo(u);<RESET>
+	<GREEN>+<RESET><GREEN>if (!u->is_allowed_foo)<RESET>
+	<GREEN>+<RESET><GREEN>return;<RESET>
+	<GREEN>+<RESET><GREEN>}<RESET>
+	<GREEN>+<RESET>
 	 int another_function()<RESET>
 	 {<RESET>
 	 bar();<RESET>
@@ -1417,6 +1417,49 @@ test_expect_success '--color-moved block at end of diff output respects MIN_BLOC
 	test_cmp expected actual
 '
 
+test_expect_success '--color-moved treats adjacent blocks as separate for MIN_BLOCK_LENGTH' '
+	git reset --hard &&
+	cat <<-\EOF >foo &&
+	line1
+	irrelevant_line
+	line2
+	line3
+	EOF
+	>bar &&
+	git add foo bar &&
+	git commit -m x &&
+
+	cat <<-\EOF >foo &&
+	irrelevant_line
+	EOF
+	cat <<-\EOF >bar &&
+	line2
+	line3
+	line1
+	EOF
+
+	git diff HEAD --color-moved=zebra --no-renames | grep -v "index" | test_decode_color >actual &&
+	cat >expected <<-\EOF &&
+	<BOLD>diff --git a/bar b/bar<RESET>
+	<BOLD>--- a/bar<RESET>
+	<BOLD>+++ b/bar<RESET>
+	<CYAN>@@ -0,0 +1,3 @@<RESET>
+	<GREEN>+<RESET><GREEN>line2<RESET>
+	<GREEN>+<RESET><GREEN>line3<RESET>
+	<GREEN>+<RESET><GREEN>line1<RESET>
+	<BOLD>diff --git a/foo b/foo<RESET>
+	<BOLD>--- a/foo<RESET>
+	<BOLD>+++ b/foo<RESET>
+	<CYAN>@@ -1,4 +1 @@<RESET>
+	<RED>-line1<RESET>
+	 irrelevant_line<RESET>
+	<RED>-line2<RESET>
+	<RED>-line3<RESET>
+	EOF
+
+	test_cmp expected actual
+'
+
 test_expect_success 'move detection with submodules' '
 	test_create_repo bananas &&
 	echo ripe >bananas/recipe &&

-- 
2.14.1.480.gb18f417b89-goog
Previous: Jonathan TanNext: Stefan Beller
Message 16 of 30 in “Fixes to "diff --color-moved" MIN_BLOCK_LENGTH handling”
  1. 0/3 Fixes to "diff --color-moved" MIN_BLOCK_LENGTH handlingJonathan Tan, Aug 11, 2017
  2. 1/3 diff: avoid redundantly clearing a flagJonathan Tan, Aug 11, 2017
  3. Stefan BellerAug 14, 2017
  4. 2/3 diff: respect MIN_BLOCK_LENGTH for last blockJonathan Tan, Aug 11, 2017
  5. Stefan BellerAug 14, 2017
  6. 3/3 diff: check MIN_BLOCK_LENGTH at start of new blockJonathan Tan, Aug 11, 2017
  7. Stefan BellerAug 14, 2017
  8. Junio C HamanoAug 12, 2017
  9. Stefan BellerAug 14, 2017
  10. Junio C HamanoAug 14, 2017
  11. Stefan BellerAug 14, 2017
  12. Junio C HamanoAug 15, 2017
  13. 0/3 Fixes to "diff --color-moved" MIN_BLOCK_LENGTH handlingJonathan Tan, Aug 14, 2017
  14. 1/3 diff: avoid redundantly clearing a flagJonathan Tan, Aug 14, 2017
  15. 2/3 diff: respect MIN_BLOCK_LENGTH for last blockJonathan Tan, Aug 14, 2017
  16. 3/3 diff: check MIN_BLOCK_LENGTH at start of new blockJonathan Tan, Aug 14, 2017
  17. Stefan BellerAug 14, 2017
  18. 0/3 "diff --color-moved" with different heuristicJonathan Tan, Aug 14, 2017
  19. 1/3 diff: avoid redundantly clearing a flagJonathan Tan, Aug 14, 2017
  20. 2/3 diff: respect MIN_BLOCK_LENGTH for last blockJonathan Tan, Aug 14, 2017
  21. 3/3 diff: define block by number of non-space charsJonathan Tan, Aug 14, 2017
  22. Stefan BellerAug 15, 2017
  23. Junio C HamanoAug 15, 2017
  24. Stefan BellerAug 15, 2017
  25. Junio C HamanoAug 15, 2017
  26. 0/3 "diff --color-moved" with yet another heuristicJonathan Tan, Aug 16, 2017
  27. 1/3 diff: avoid redundantly clearing a flagJonathan Tan, Aug 16, 2017
  28. 2/3 diff: respect MIN_BLOCK_LENGTH for last blockJonathan Tan, Aug 16, 2017
  29. 3/3 diff: define block by number of alphanumeric charsJonathan Tan, Aug 16, 2017
  30. Stefan BellerAug 16, 2017

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.