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

[PATCH 6/8] diff.c: decouple white space treatment from move detection algorithm

From
Stefan Beller <sbeller@google.com>
Date
May 17, 2018, 19:46 UTC
Message-ID
<20180517194653.48928-7-sbeller@google.com>
In-Reply-To
<20180517194653.48928-1-sbeller@google.com>

In the original implementation of the move detection logic the choice for ignoring white space changes is the same for the move detection as it is for the regular diff. Some cases came up where different treatment would have been nice.

Allow the user to specify that whitespace should be ignored differently
during detection of moved lines than during generation of added and removed
lines. This is done by providing analogs to the --ignore-space-at-eol,
-b, and -w options (namely,
  --color-moved-[no-]ignore-space-at-eol
  --color-moved-[no-]ignore-space-change
  --color-moved-[no-]ignore-all-space) that affect only the color of the
output, and making the existing --ignore-space-at-eol, -b, and -w options
no longer affect the color of the output.
As we change the default, we'll adjust the tests.

For now we do not infer any options to treat whitespaces in the move detection from the generic white space options given to diff. This can be tuned later to reasonable default.

Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/diff-options.txt | 13 +++++
 diff.c                         | 19 ++++++-
 diff.h                         |  1 +
 t/t4015-diff-whitespace.sh     | 90 +++++++++++++++++++++++++++++++---
 4 files changed, 114 insertions(+), 9 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index bb9f1b7cd82..7b2527b9a19 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -292,6 +292,19 @@ dimmed_zebra::
 	blocks are considered interesting, the rest is uninteresting.
 --
 
+--color-moved-[no-]ignore-space-at-eol::
+	Ignore changes in whitespace at EOL when performing the move
+	detection for --color-moved.
+--color-moved-[no-]ignore-space-change::
+	Ignore changes in amount of whitespace when performing the move
+	detection for --color-moved.  This ignores whitespace
+	at line end, and considers all other sequences of one or
+	more whitespace characters to be equivalent.
+--color-moved-[no-]ignore-all-space::
+	Ignore whitespace when comparing lines when performing the move
+	detection for --color-moved.  This ignores differences even if
+	one line has whitespace where the other line has none.
+
 --word-diff[=<mode>]::
 	Show a word diff, using the <mode> to delimit changed words.
 	By default, words are delimited by whitespace; see
diff --git a/diff.c b/diff.c
index 95c51c0b7df..b5819dd538f 100644
--- a/diff.c
+++ b/diff.c
@@ -717,10 +717,12 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,
 	const struct diff_options *diffopt = hashmap_cmp_fn_data;
 	const struct moved_entry *a = entry;
 	const struct moved_entry *b = entry_or_key;
+	unsigned flags = diffopt->color_moved_ws_handling
+			 & XDF_WHITESPACE_FLAGS;
 
 	return !xdiff_compare_lines(a->es->line, a->es->len,
 				    b->es->line, b->es->len,
-				    diffopt->xdl_opts);
+				    flags);
 }
 
 static struct moved_entry *prepare_entry(struct diff_options *o,
@@ -728,8 +730,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,
 {
 	struct moved_entry *ret = xmalloc(sizeof(*ret));
 	struct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];
+	unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;
 
-	ret->ent.hash = xdiff_hash_string(l->line, l->len, o->xdl_opts);
+	ret->ent.hash = xdiff_hash_string(l->line, l->len, flags);
 	ret->es = l;
 	ret->next_line = NULL;
 
@@ -4638,6 +4641,18 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_CR_AT_EOL);
 	else if (!strcmp(arg, "--ignore-blank-lines"))
 		DIFF_XDL_SET(options, IGNORE_BLANK_LINES);
+	else if (!strcmp(arg, "--color-moved-no-ignore-all-space"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-change"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-at-eol"))
+		options->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_AT_EOL;
+	else if (!strcmp(arg, "--color-moved-ignore-all-space"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-change"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-at-eol"))
+		options->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_AT_EOL;
 	else if (!strcmp(arg, "--indent-heuristic"))
 		DIFF_XDL_SET(options, INDENT_HEURISTIC);
 	else if (!strcmp(arg, "--no-indent-heuristic"))
diff --git a/diff.h b/diff.h
index 7bd4f182c33..de5dc680051 100644
--- a/diff.h
+++ b/diff.h
@@ -214,6 +214,7 @@ struct diff_options {
 	} color_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
+	int color_moved_ws_handling;
 };
 
 void diff_emit_submodule_del(struct diff_options *o, const char *line);
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 45091abb192..751fc478dde 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1441,7 +1441,10 @@ test_expect_success 'move detection ignoring whitespace ' '
 	line 4
 	line 5
 	EOF
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1465,7 +1468,10 @@ test_expect_success 'move detection ignoring whitespace ' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames -w --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1505,7 +1511,10 @@ test_expect_success 'move detection ignoring whitespace changes' '
 	line 5
 	EOF
 
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1529,7 +1538,10 @@ test_expect_success 'move detection ignoring whitespace changes' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames -b --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-at-eol \
+		--color-moved-ignore-space-change |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1572,7 +1584,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '
 	# avoid cluttering the output with complaints about our eol whitespace
 	test_config core.whitespace -blank-at-eol &&
 
-	git diff HEAD --no-renames --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1596,7 +1611,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '
 	EOF
 	test_cmp expected actual &&
 
-	git diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |
+	git diff HEAD --no-renames --color-moved --color \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-ignore-space-at-eol |
 		grep -v "index" |
 		test_decode_color >actual &&
 	cat <<-\EOF >expected &&
@@ -1768,7 +1786,65 @@ test_expect_success 'move detection with submodules' '
 
 	# nor did we mess with it another way
 	git diff --submodule=diff --color | test_decode_color >expect &&
-	test_cmp expect decoded_actual
+	test_cmp expect decoded_actual &&
+	rm -rf bananas &&
+	git submodule deinit bananas
+'
+
+test_expect_success 'only move detection ignores white spaces' '
+	git reset --hard &&
+	q_to_tab <<-\EOF >text.txt &&
+		a long line to exceed per-line minimum
+		another long line to exceed per-line minimum
+		original file
+	EOF
+	git add text.txt &&
+	git commit -m "add text" &&
+	q_to_tab <<-\EOF >text.txt &&
+		Qa long line to exceed per-line minimum
+		Qanother long line to exceed per-line minimum
+		new file
+	EOF
+
+	# Make sure we get a different diff using -w
+	git diff --color --color-moved -w \
+		--color-moved-no-ignore-all-space \
+		--color-moved-no-ignore-space-change \
+		--color-moved-no-ignore-space-at-eol |
+		grep -v "index" |
+		test_decode_color >actual &&
+	q_to_tab <<-\EOF >expected &&
+	<BOLD>diff --git a/text.txt b/text.txt<RESET>
+	<BOLD>--- a/text.txt<RESET>
+	<BOLD>+++ b/text.txt<RESET>
+	<CYAN>@@ -1,3 +1,3 @@<RESET>
+	 Qa long line to exceed per-line minimum<RESET>
+	 Qanother long line to exceed per-line minimum<RESET>
+	<RED>-original file<RESET>
+	<GREEN>+<RESET><GREEN>new file<RESET>
+	EOF
+	test_cmp expected actual &&
+
+	# And now ignoring white space only in the move detection
+	git diff --color --color-moved \
+		--color-moved-ignore-all-space \
+		--color-moved-ignore-space-change \
+		--color-moved-ignore-space-at-eol |
+		grep -v "index" |
+		test_decode_color >actual &&
+	q_to_tab <<-\EOF >expected &&
+	<BOLD>diff --git a/text.txt b/text.txt<RESET>
+	<BOLD>--- a/text.txt<RESET>
+	<BOLD>+++ b/text.txt<RESET>
+	<CYAN>@@ -1,3 +1,3 @@<RESET>
+	<BOLD;MAGENTA>-a long line to exceed per-line minimum<RESET>
+	<BOLD;MAGENTA>-another long line to exceed per-line minimum<RESET>
+	<RED>-original file<RESET>
+	<BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>a long line to exceed per-line minimum<RESET>
+	<BOLD;YELLOW>+<RESET>Q<BOLD;YELLOW>another long line to exceed per-line minimum<RESET>
+	<GREEN>+<RESET><GREEN>new file<RESET>
+	EOF
+	test_cmp expected actual
 '
 
 test_done

-- 
2.17.0.582.gccdcbd54c44.dirty
Previous: Stefan BellerNext: Simon Ruderich
Message 73 of 95 in “What's cooking in git.git (May 2018, #02; Thu, 17)”
  1. Junio C HamanoMay 17, 2018
  2. jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))Kaartic Sivaraam, May 17, 2018
  3. Ævar Arnfjörð BjarmasonMay 17, 2018
  4. Kaartic SivaraamMay 17, 2018
  5. Ævar Arnfjörð BjarmasonMay 17, 2018
  6. Jeff KingMay 17, 2018
  7. jk/branch-l-0-deprecation (was Re: What's cooking in git.git (May 2018, #02; Thu, 17))Kaartic Sivaraam, May 24, 2018
  8. Jeff KingMay 24, 2018
  9. branch: issue "-l" deprecation warning after pager startsJeff King, May 24, 2018
  10. Junio C HamanoMay 25, 2018
  11. Jeff KingMay 25, 2018
  12. Junio C HamanoMay 25, 2018
  13. Junio C HamanoMay 25, 2018
  14. Jeff KingMay 25, 2018
  15. Junio C HamanoMay 26, 2018
  16. 0/3 usage: prefix all lines in `vreportf()`, not just the firstMartin Ågren, May 25, 2018
  17. 1/3 usage: extract `prefix_suffix_lines()` from `advise()`Martin Ågren, May 25, 2018
  18. Junio C HamanoMay 28, 2018
  19. Duy NguyenMay 28, 2018
  20. Jeff KingMay 29, 2018
  21. Jeff KingMay 29, 2018
  22. Junio C HamanoMay 30, 2018
  23. Junio C HamanoMay 30, 2018
  24. Martin ÅgrenMay 30, 2018
  25. Jeff KingMay 31, 2018
  26. 2/3 usage: prefix all lines in `vreportf()`, not just the firstMartin Ågren, May 25, 2018
  27. Junio C HamanoMay 28, 2018
  28. Duy NguyenMay 28, 2018
  29. Junio C HamanoMay 28, 2018
  30. Martin ÅgrenMay 29, 2018
  31. Junio C HamanoMay 29, 2018
  32. Martin ÅgrenMay 29, 2018
  33. Junio C HamanoMay 29, 2018
  34. Duy NguyenMay 29, 2018
  35. Martin ÅgrenMay 30, 2018
  36. Jeff KingMay 29, 2018
  37. Martin ÅgrenMay 30, 2018
  38. 3/3 usage: translate the "error: "-prefix and othersMartin Ågren, May 25, 2018
  39. Junio C HamanoMay 26, 2018
  40. Junio C HamanoMay 26, 2018
  41. Jeff KingMay 29, 2018
  42. Jeff KingMay 29, 2018
  43. Junio C HamanoMay 30, 2018
  44. Jeff KingMay 31, 2018
  45. Kaartic SivaraamMay 26, 2018
  46. Duy NguyenJun 2, 2018
  47. Jeff KingJun 2, 2018
  48. Kaartic SivaraamMay 26, 2018
  49. Jeff KingMay 29, 2018
  50. Junio C HamanoMay 30, 2018
  51. Jeff KingMay 31, 2018
  52. Junio C HamanoJun 1, 2018
  53. Kaartic.SivaraamMay 31, 2018
  54. Derrick StoleeMay 17, 2018
  55. Stefan BellerMay 17, 2018
  56. 0/2 Reroll 2 last commits of sb/object-store-replaceStefan Beller, May 17, 2018
  57. 1/2 object.c: free replace map in raw_object_store_clearStefan Beller, May 17, 2018
  58. 2/2 replace-object.c: remove the_repository from prepare_replace_objectStefan Beller, May 17, 2018
  59. merge-recursive: give notice when submodule commit gets fast-forwardedStefan Beller, May 17, 2018
  60. 0/1 rebased: inform about auto submodule ffLeif Middelschulte, May 18, 2018
  61. 0/1 rebased: inform about auto submodule ffLeif Middelschulte, May 18, 2018
  62. 1/1 Inform about fast-forwarding of submodules during mergeLeif Middelschulte, May 18, 2018
  63. Elijah NewrenMay 18, 2018
  64. Junio C HamanoMay 21, 2018
  65. 0/8 Reroll of sb/diff-color-move-moreStefan Beller, May 17, 2018
  66. 1/8 xdiff/xdiff.h: remove unused flagsStefan Beller, May 17, 2018
  67. 2/8 xdiff/xdiffi.c: remove unneeded function declarationsStefan Beller, May 17, 2018
  68. 4/8 diff.c: adjust hash function signature to match hashmap expectationStefan Beller, May 17, 2018
  69. 5/8 diff.c: add a blocks mode for moved code detectionStefan Beller, May 17, 2018
  70. 7/8 diff.c: add --color-moved-ignore-space-delta optionStefan Beller, May 17, 2018
  71. 8/8 diff: color-moved white space handling options imply color-movedStefan Beller, May 17, 2018
  72. 3/8 diff.c: do not pass diff options as keydata to hashmapStefan Beller, May 17, 2018
  73. 6/8 diff.c: decouple white space treatment from move detection algorithmStefan Beller, May 17, 2018
  74. Simon RuderichMay 18, 2018
  75. Stefan BellerMay 18, 2018
  76. Jonathan TanMay 17, 2018
  77. Jacob KellerJun 7, 2018
  78. Junio C HamanoMay 17, 2018
  79. Stefan BellerMay 17, 2018
  80. Junio C HamanoMay 17, 2018
  81. Stefan BellerMay 17, 2018
  82. brian m. carlsonMay 21, 2018
  83. Stefan BellerMay 21, 2018
  84. sb/submodule-move-nested breaks t7411 under GIT_FSMONITOR_TESTÆvar Arnfjörð Bjarmason, May 25, 2018
  85. Stefan BellerMay 25, 2018
  86. Stefan BellerMay 25, 2018
  87. Ævar Arnfjörð BjarmasonSep 6, 2018
  88. Stefan BellerSep 6, 2018
  89. Ben PeartSep 6, 2018
  90. Stefan BellerSep 6, 2018
  91. git-mv: allow submodules and fsmonitor to work togetherStefan Beller, Sep 6, 2018
  92. Ben PeartSep 10, 2018
  93. git-mv: allow submodules and fsmonitor to work togetherBen Peart, Sep 10, 2018
  94. Stefan BellerSep 10, 2018
  95. Ben PeartSep 10, 2018

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.