[PATCH 0/2] color-moved: ignore all space changes by default

[PATCH 0/2] color-moved: ignore all space changes by default

[Stefan Beller]

On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller <sbeller@google.com> wrote[1]:
> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>  * As moved-lines display is mostly a presentation thing, I wonder
>>    if it makes sense to always match loosely wrt whitespace
>>    differences. 
>
> Well, sometimes the user wants to know if it is byte-for-byte identical
> (unlikely to be code, but maybe column oriented data for input;
> think of all our FORTRAN users. ;)

... and this is the implementation and the flip of the default setting
to ignore all white space for the move detection.

It applies on "v3 (x)diff cleanup: remove duplicate code" [2].

Thanks,
Stefan

[1] https://public-inbox.org/git/CAGZ79kbba9KuDX7HsxhW3jXs7ocWfZg=KSHE-GsXjntnT4PWdg@mail.gmail.com
[2] https://public-inbox.org/git/20171025184912.21657-1-sbeller@google.com/

Stefan Beller (2):
  diff: decouple white space treatment for move detection from generic
    option
  diff.c: ignore all white space changes by default in the move
    detection

 Documentation/diff-options.txt |  15 ++++
 diff.c                         |  18 ++++-
 diff.h                         |   1 +
 t/t4015-diff-whitespace.sh     | 156 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 181 insertions(+), 9 deletions(-)

-- 
2.15.0.rc2.6.g953226eb5f

[PATCH 1/2] diff: decouple white space treatment for move detection from generic option

[Stefan Beller]

In the original implementation of the move dection logic we assumed that
the choice for ignoring white space changes is the same for the move
detection as it is for the generic diff.

It turns out, users want to have different choices regarding white spaces
for the move detection and the generic diff.  For example when reviewing
a patch that is sent to the mailing list (which doesn't ignore white space
changes at all; it is an exact patch) refactors some nested code out
into a separate function, we want to have the move detection kick in
despite different indentation levels of the old and new code.

This patch enables the user to have a different choice for ignoring
white spaces in the move detection.

The example given is covered in a test. Convert the existing tests
to be more explicit on their choice of white space behavior in the
move detection as we'll change the default shortly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/diff-options.txt |  13 ++++
 diff.c                         |  17 ++++-
 diff.h                         |   1 +
 t/t4015-diff-whitespace.sh     | 150 +++++++++++++++++++++++++++++++++++++++--
 4 files changed, 172 insertions(+), 9 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index a88c76741e..1000b53b84 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -265,6 +265,19 @@ dimmed_zebra::
 	blocks are considered interesting, the rest is uninteresting.
 --
 
+--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.
+--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-space-at-eol::
+	Ignore changes in whitespace at EOL when performing the move
+	detection for --color-moved.
+
 --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 e6814b9e9c..2580315ab9 100644
--- a/diff.c
+++ b/diff.c
@@ -714,7 +714,7 @@ static int moved_entry_cmp(const struct diff_options *diffopt,
 {
 	return !xdiff_compare_lines(a->es->line, a->es->len,
 				    b->es->line, b->es->len,
-				    diffopt->xdl_opts);
+				    diffopt->color_moved_ignore_space);
 }
 
 static struct moved_entry *prepare_entry(struct diff_options *o,
@@ -723,7 +723,8 @@ 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];
 
-	ret->ent.hash = xdiff_hash_string(l->line, l->len, o->xdl_opts);
+	ret->ent.hash = xdiff_hash_string(l->line, l->len,
+					  o->color_moved_ignore_space);
 	ret->es = l;
 	ret->next_line = NULL;
 
@@ -4592,6 +4593,18 @@ int diff_opt_parse(struct diff_options *options,
 		DIFF_XDL_SET(options, IGNORE_WHITESPACE_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_ignore_space &= ~XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-change"))
+		options->color_moved_ignore_space &= ~XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-no-ignore-space-at-eol"))
+		options->color_moved_ignore_space &= ~XDF_IGNORE_WHITESPACE_AT_EOL;
+	else if (!strcmp(arg, "--color-moved-ignore-all-space"))
+		options->color_moved_ignore_space |= XDF_IGNORE_WHITESPACE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-change"))
+		options->color_moved_ignore_space |= XDF_IGNORE_WHITESPACE_CHANGE;
+	else if (!strcmp(arg, "--color-moved-ignore-space-at-eol"))
+		options->color_moved_ignore_space |= 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 aca150ba2e..6ba3f53bbd 100644
--- a/diff.h
+++ b/diff.h
@@ -196,6 +196,7 @@ struct diff_options {
 	} color_moved;
 	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
 	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
+	int color_moved_ignore_space;
 };
 
 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 6c9a93b734..4ef4d6934a 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1350,7 +1350,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 &&
@@ -1374,7 +1377,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 &&
@@ -1414,7 +1420,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 &&
@@ -1438,7 +1447,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 &&
@@ -1481,7 +1493,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 &&
@@ -1505,7 +1520,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 &&
@@ -1677,7 +1695,125 @@ 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 'move detection only ignores white spaces' '
+	git reset --hard &&
+	q_to_tab <<-\EOF >function.c &&
+	int func()
+	{
+	Qif (foo) {
+	QQ// this part of the function
+	QQ// function will be very long
+	QQ// indeed. We must exceed both
+	QQ// per-line and number of line
+	QQ// minimums
+	QQ;
+	Q}
+	Qbaz();
+	Qbar();
+	Q// more unrelated stuff
+	}
+	EOF
+	git add function.c &&
+	git commit -m "add function.c" &&
+	q_to_tab <<-\EOF >function.c &&
+	int do_foo()
+	{
+	Q// this part of the function
+	Q// function will be very long
+	Q// indeed. We must exceed both
+	Q// per-line and number of line
+	Q// minimums
+	Q;
+	}
+
+	int func()
+	{
+	Qif (foo)
+	QQdo_foo();
+	Qbaz();
+	Qbar();
+	Q// more unrelated stuff
+	}
+	EOF
+
+	# Make sure we get a different diff using -w ("moved function header")
+	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/function.c b/function.c<RESET>
+	<BOLD>--- a/function.c<RESET>
+	<BOLD>+++ b/function.c<RESET>
+	<CYAN>@@ -1,6 +1,5 @@<RESET>
+	<RED>-int func()<RESET>
+	<GREEN>+<RESET><GREEN>int do_foo()<RESET>
+	 {<RESET>
+	<RED>-	if (foo) {<RESET>
+	 Q// this part of the function<RESET>
+	 Q// function will be very long<RESET>
+	 Q// indeed. We must exceed both<RESET>
+	<CYAN>@@ -8,6 +7,11 @@<RESET> <RESET>int func()<RESET>
+	 Q// minimums<RESET>
+	 Q;<RESET>
+	 }<RESET>
+	<GREEN>+<RESET>
+	<GREEN>+<RESET><GREEN>int func()<RESET>
+	<GREEN>+<RESET><GREEN>{<RESET>
+	<GREEN>+<RESET>Q<GREEN>if (foo)<RESET>
+	<GREEN>+<RESET>QQ<GREEN>do_foo();<RESET>
+	 Qbaz();<RESET>
+	 Qbar();<RESET>
+	 Q// more unrelated stuff<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/function.c b/function.c<RESET>
+	<BOLD>--- a/function.c<RESET>
+	<BOLD>+++ b/function.c<RESET>
+	<CYAN>@@ -1,13 +1,17 @@<RESET>
+	<GREEN>+<RESET><GREEN>int do_foo()<RESET>
+	<GREEN>+<RESET><GREEN>{<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// this part of the function<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// function will be very long<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// indeed. We must exceed both<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// per-line and number of line<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// minimums<RESET>
+	<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>;<RESET>
+	<BOLD;CYAN>+<RESET><BOLD;CYAN>}<RESET>
+	<GREEN>+<RESET>
+	 int func()<RESET>
+	 {<RESET>
+	<RED>-Qif (foo) {<RESET>
+	<BOLD;MAGENTA>-QQ// this part of the function<RESET>
+	<BOLD;MAGENTA>-QQ// function will be very long<RESET>
+	<BOLD;MAGENTA>-QQ// indeed. We must exceed both<RESET>
+	<BOLD;MAGENTA>-QQ// per-line and number of line<RESET>
+	<BOLD;MAGENTA>-QQ// minimums<RESET>
+	<BOLD;MAGENTA>-QQ;<RESET>
+	<BOLD;MAGENTA>-Q}<RESET>
+	<GREEN>+<RESET>Q<GREEN>if (foo)<RESET>
+	<GREEN>+<RESET>QQ<GREEN>do_foo();<RESET>
+	 Qbaz();<RESET>
+	 Qbar();<RESET>
+	 Q// more unrelated stuff<RESET>
+	EOF
+	test_cmp expected actual
 '
 
 test_done
-- 
2.15.0.rc2.6.g953226eb5f

[PATCH 2/2] diff.c: ignore all white space changes by default in the move detection

[Stefan Beller]

The coloring of moved lines is currently only related to the presentation,
as there are no options to export the move detection information into
the patch format. Hence we can be very loose about the default, so let's
ignore any white space change for the move detection. If a user really
cares about move detection detecting only lines moved byte-for-byte,
they can exercise the --color-moved-no-ignore-* options.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/diff-options.txt | 4 +++-
 diff.c                         | 1 +
 t/t4015-diff-whitespace.sh     | 6 ++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 1000b53b84..fdf40cbefc 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -269,14 +269,16 @@ dimmed_zebra::
 	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.
+	This is on by default.
 --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.
+	This is on by default.
 --color-moved-[no-]ignore-space-at-eol::
 	Ignore changes in whitespace at EOL when performing the move
-	detection for --color-moved.
+	detection for --color-moved. This is on by default.
 
 --word-diff[=<mode>]::
 	Show a word diff, using the <mode> to delimit changed words.
diff --git a/diff.c b/diff.c
index 2580315ab9..fef4874169 100644
--- a/diff.c
+++ b/diff.c
@@ -4105,6 +4105,7 @@ void diff_setup(struct diff_options *options)
 	}
 
 	options->color_moved = diff_color_moved_default;
+	options->color_moved_ignore_space = XDF_WHITESPACE_FLAGS;
 }
 
 void diff_setup_done(struct diff_options *options)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 4ef4d6934a..a4ffc84f41 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -1813,6 +1813,12 @@ test_expect_success 'move detection only ignores white spaces' '
 	 Qbar();<RESET>
 	 Q// more unrelated stuff<RESET>
 	EOF
+	test_cmp expected actual &&
+
+	# test that this is the default:
+	git diff --color --color-moved |
+		grep -v "index" |
+		test_decode_color >actual &&
 	test_cmp expected actual
 '
 
-- 
2.15.0.rc2.6.g953226eb5f

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Simon Ruderich]

On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:
> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:
>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:
>>>
>>>  * As moved-lines display is mostly a presentation thing, I wonder
>>>    if it makes sense to always match loosely wrt whitespace
>>>    differences.
>>
>> Well, sometimes the user wants to know if it is byte-for-byte identical
>> (unlikely to be code, but maybe column oriented data for input;
>> think of all our FORTRAN users. ;)
>
> ... and this is the implementation and the flip of the default setting
> to ignore all white space for the move detection.

Hello,

I'm not sure if this is a good default. I think it's not obvious
that moved code gets treated differently than regular changes. I
wouldn't expect git diff to ignore whitespace changes (without me
telling it to) and so when I see moved code I expect they were
moved as is.

And there are languages where indentation is relevant (e.g.
Python, YAML) and as color-moved is also treated as review tool
to detect unwanted changes this new default can be dangerous.

The new options sound like a good addition but I don't think the
defaults should change. However unrelated to this decision,
please add config settings in addition to these new options so
users can globally configure the behavior they want.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Jacob Keller]

On Thu, Oct 26, 2017 at 12:22 AM, Simon Ruderich <simon@ruderich.org> wrote:
> On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:
>> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:
>>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:
>>>>
>>>>  * As moved-lines display is mostly a presentation thing, I wonder
>>>>    if it makes sense to always match loosely wrt whitespace
>>>>    differences.
>>>
>>> Well, sometimes the user wants to know if it is byte-for-byte identical
>>> (unlikely to be code, but maybe column oriented data for input;
>>> think of all our FORTRAN users. ;)
>>
>> ... and this is the implementation and the flip of the default setting
>> to ignore all white space for the move detection.
>
> Hello,
>
> I'm not sure if this is a good default. I think it's not obvious
> that moved code gets treated differently than regular changes. I
> wouldn't expect git diff to ignore whitespace changes (without me
> telling it to) and so when I see moved code I expect they were
> moved as is.
>
> And there are languages where indentation is relevant (e.g.
> Python, YAML) and as color-moved is also treated as review tool
> to detect unwanted changes this new default can be dangerous.
>
> The new options sound like a good addition but I don't think the
> defaults should change. However unrelated to this decision,
> please add config settings in addition to these new options so
> users can globally configure the behavior they want.
>
> Regards
> Simon
> --

Even languages which are indentation sensitive often move blocks of
lines between indentation levels a lot. I personally think the default
could change.

However, I would suspect the best path forward is leave the default
"exact match" and allow users who care and know about the feature to
change their config settings.

Thanks,
Jake

Re: [PATCH 0/2] color-moved: ignore all space changes by default

[Stefan Beller]

On Thu, Oct 26, 2017 at 1:42 AM, Jacob Keller <jacob.keller@gmail.com> wrote:

>>
>> Hello,
>>
>> I'm not sure if this is a good default. I think it's not obvious
>> that moved code gets treated differently than regular changes. I
>> wouldn't expect git diff to ignore whitespace changes (without me
>> telling it to) and so when I see moved code I expect they were
>> moved as is.
>>
>> And there are languages where indentation is relevant (e.g.
>> Python, YAML) and as color-moved is also treated as review tool
>> to detect unwanted changes this new default can be dangerous.

That makes sense.

>>
>> The new options sound like a good addition but I don't think the
>> defaults should change. However unrelated to this decision,
>> please add config settings in addition to these new options so
>> users can globally configure the behavior they want.
>>
>> Regards
>> Simon
>> --
>
> Even languages which are indentation sensitive often move blocks of
> lines between indentation levels a lot. I personally think the default
> could change.

A safe default for such languages would be when the
change in whitespace across lines is taking into account, i.e.
when lots of lines are copied, but all of them miss two tab indentation,
then it is probably fine to color it all the same. However if there would
be a couple of lines in that moved block of code that have
a different indentation than most other lines, this could indeed
change the program flow in python.

So ideally we'd compare the whitespace delta across lines.
Note sure if that is easy. Maybe instead of ignoring whitespaces
completely we'd rather make up a "white space delta", e.g. by using
word-diff between the old and new line, and then keeping a hash of that
space delta for each line. The move detection would then also compare
the hashes of adjacent moved lines.

> However, I would suspect the best path forward is leave the default
> "exact match" and allow users who care and know about the feature to
> change their config settings.

I think that is what I'll do for now as it seems simple enough.

Thanks,
Stefan