{"thread":{"id":"47029","subject":"[PATCH 0/2] color-moved: ignore all space changes by default","startedAt":"2017-10-25T22:46:35Z","lastAt":"2017-10-26T17:59:43Z","messageCount":6,"participants":["Stefan Beller","Simon Ruderich","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"331044","messageId":"20171025224620.27657-1-sbeller@google.com","threadId":"47029","inReplyTo":null,"subject":"[PATCH 0/2] color-moved: ignore all space changes by default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-25T22:46:18Z","receivedAt":"2017-10-25T22:46:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller <sbeller@google.com> wrote[1]:\n> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>>  * As moved-lines display is mostly a presentation thing, I wonder\n>>    if it makes sense to always match loosely wrt whitespace\n>>    differences. \n>\n> Well, sometimes the user wants to know if it is byte-for-byte identical\n> (unlikely to be code, but maybe column oriented data for input;\n> think of all our FORTRAN users. ;)\n\n... and this is the implementation and the flip of the default setting\nto ignore all white space for the move detection.\n\nIt applies on \"v3 (x)diff cleanup: remove duplicate code\" [2].\n\nThanks,\nStefan\n\n[1] https://public-inbox.org/git/CAGZ79kbba9KuDX7HsxhW3jXs7ocWfZg=KSHE-GsXjntnT4PWdg@mail.gmail.com\n[2] https://public-inbox.org/git/20171025184912.21657-1-sbeller@google.com/\n\nStefan Beller (2):\n  diff: decouple white space treatment for move detection from generic\n    option\n  diff.c: ignore all white space changes by default in the move\n    detection\n\n Documentation/diff-options.txt |  15 ++++\n diff.c                         |  18 ++++-\n diff.h                         |   1 +\n t/t4015-diff-whitespace.sh     | 156 +++++++++++++++++++++++++++++++++++++++--\n 4 files changed, 181 insertions(+), 9 deletions(-)\n\n-- \n2.15.0.rc2.6.g953226eb5f\n\n"},{"id":"331045","messageId":"20171025224620.27657-2-sbeller@google.com","threadId":"47029","inReplyTo":"20171025224620.27657-1-sbeller@google.com","subject":"[PATCH 1/2] diff: decouple white space treatment for move detection from generic option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-25T22:46:19Z","receivedAt":"2017-10-25T22:46:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the original implementation of the move dection logic we assumed that\nthe choice for ignoring white space changes is the same for the move\ndetection as it is for the generic diff.\n\nIt turns out, users want to have different choices regarding white spaces\nfor the move detection and the generic diff.  For example when reviewing\na patch that is sent to the mailing list (which doesn't ignore white space\nchanges at all; it is an exact patch) refactors some nested code out\ninto a separate function, we want to have the move detection kick in\ndespite different indentation levels of the old and new code.\n\nThis patch enables the user to have a different choice for ignoring\nwhite spaces in the move detection.\n\nThe example given is covered in a test. Convert the existing tests\nto be more explicit on their choice of white space behavior in the\nmove detection as we'll change the default shortly.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/diff-options.txt |  13 ++++\n diff.c                         |  17 ++++-\n diff.h                         |   1 +\n t/t4015-diff-whitespace.sh     | 150 +++++++++++++++++++++++++++++++++++++++--\n 4 files changed, 172 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex a88c76741e..1000b53b84 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -265,6 +265,19 @@ dimmed_zebra::\n \tblocks are considered interesting, the rest is uninteresting.\n --\n \n+--color-moved-[no-]ignore-all-space::\n+\tIgnore whitespace when comparing lines when performing the move\n+\tdetection for --color-moved.  This ignores differences even if\n+\tone line has whitespace where the other line has none.\n+--color-moved-[no-]ignore-space-change::\n+\tIgnore changes in amount of whitespace when performing the move\n+\tdetection for --color-moved.  This ignores whitespace\n+\tat line end, and considers all other sequences of one or\n+\tmore whitespace characters to be equivalent.\n+--color-moved-[no-]ignore-space-at-eol::\n+\tIgnore changes in whitespace at EOL when performing the move\n+\tdetection for --color-moved.\n+\n --word-diff[=<mode>]::\n \tShow a word diff, using the <mode> to delimit changed words.\n \tBy default, words are delimited by whitespace; see\ndiff --git a/diff.c b/diff.c\nindex e6814b9e9c..2580315ab9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -714,7 +714,7 @@ static int moved_entry_cmp(const struct diff_options *diffopt,\n {\n \treturn !xdiff_compare_lines(a->es->line, a->es->len,\n \t\t\t\t    b->es->line, b->es->len,\n-\t\t\t\t    diffopt->xdl_opts);\n+\t\t\t\t    diffopt->color_moved_ignore_space);\n }\n \n static struct moved_entry *prepare_entry(struct diff_options *o,\n@@ -723,7 +723,8 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n \tstruct moved_entry *ret = xmalloc(sizeof(*ret));\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];\n \n-\tret->ent.hash = xdiff_hash_string(l->line, l->len, o->xdl_opts);\n+\tret->ent.hash = xdiff_hash_string(l->line, l->len,\n+\t\t\t\t\t  o->color_moved_ignore_space);\n \tret->es = l;\n \tret->next_line = NULL;\n \n@@ -4592,6 +4593,18 @@ int diff_opt_parse(struct diff_options *options,\n \t\tDIFF_XDL_SET(options, IGNORE_WHITESPACE_AT_EOL);\n \telse if (!strcmp(arg, \"--ignore-blank-lines\"))\n \t\tDIFF_XDL_SET(options, IGNORE_BLANK_LINES);\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-all-space\"))\n+\t\toptions->color_moved_ignore_space &= ~XDF_IGNORE_WHITESPACE;\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-change\"))\n+\t\toptions->color_moved_ignore_space &= ~XDF_IGNORE_WHITESPACE_CHANGE;\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-at-eol\"))\n+\t\toptions->color_moved_ignore_space &= ~XDF_IGNORE_WHITESPACE_AT_EOL;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-all-space\"))\n+\t\toptions->color_moved_ignore_space |= XDF_IGNORE_WHITESPACE;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-space-change\"))\n+\t\toptions->color_moved_ignore_space |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-space-at-eol\"))\n+\t\toptions->color_moved_ignore_space |= XDF_IGNORE_WHITESPACE_AT_EOL;\n \telse if (!strcmp(arg, \"--indent-heuristic\"))\n \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n \telse if (!strcmp(arg, \"--no-indent-heuristic\"))\ndiff --git a/diff.h b/diff.h\nindex aca150ba2e..6ba3f53bbd 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -196,6 +196,7 @@ struct diff_options {\n \t} color_moved;\n \t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\n+\tint color_moved_ignore_space;\n };\n \n void diff_emit_submodule_del(struct diff_options *o, const char *line);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 6c9a93b734..4ef4d6934a 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1350,7 +1350,10 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tline 4\n \tline 5\n \tEOF\n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1374,7 +1377,10 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames -w --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1414,7 +1420,10 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \tline 5\n \tEOF\n \n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1438,7 +1447,10 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames -b --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-at-eol \\\n+\t\t--color-moved-ignore-space-change |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1481,7 +1493,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '\n \t# avoid cluttering the output with complaints about our eol whitespace\n \ttest_config core.whitespace -blank-at-eol &&\n \n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1505,7 +1520,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1677,7 +1695,125 @@ test_expect_success 'move detection with submodules' '\n \n \t# nor did we mess with it another way\n \tgit diff --submodule=diff --color | test_decode_color >expect &&\n-\ttest_cmp expect decoded_actual\n+\ttest_cmp expect decoded_actual &&\n+\trm -rf bananas &&\n+\tgit submodule deinit bananas\n+'\n+\n+test_expect_success 'move detection only ignores white spaces' '\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >function.c &&\n+\tint func()\n+\t{\n+\tQif (foo) {\n+\tQQ// this part of the function\n+\tQQ// function will be very long\n+\tQQ// indeed. We must exceed both\n+\tQQ// per-line and number of line\n+\tQQ// minimums\n+\tQQ;\n+\tQ}\n+\tQbaz();\n+\tQbar();\n+\tQ// more unrelated stuff\n+\t}\n+\tEOF\n+\tgit add function.c &&\n+\tgit commit -m \"add function.c\" &&\n+\tq_to_tab <<-\\EOF >function.c &&\n+\tint do_foo()\n+\t{\n+\tQ// this part of the function\n+\tQ// function will be very long\n+\tQ// indeed. We must exceed both\n+\tQ// per-line and number of line\n+\tQ// minimums\n+\tQ;\n+\t}\n+\n+\tint func()\n+\t{\n+\tQif (foo)\n+\tQQdo_foo();\n+\tQbaz();\n+\tQbar();\n+\tQ// more unrelated stuff\n+\t}\n+\tEOF\n+\n+\t# Make sure we get a different diff using -w (\"moved function header\")\n+\tgit diff --color --color-moved -w \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\t<BOLD>diff --git a/function.c b/function.c<RESET>\n+\t<BOLD>--- a/function.c<RESET>\n+\t<BOLD>+++ b/function.c<RESET>\n+\t<CYAN>@@ -1,6 +1,5 @@<RESET>\n+\t<RED>-int func()<RESET>\n+\t<GREEN>+<RESET><GREEN>int do_foo()<RESET>\n+\t {<RESET>\n+\t<RED>-\tif (foo) {<RESET>\n+\t Q// this part of the function<RESET>\n+\t Q// function will be very long<RESET>\n+\t Q// indeed. We must exceed both<RESET>\n+\t<CYAN>@@ -8,6 +7,11 @@<RESET> <RESET>int func()<RESET>\n+\t Q// minimums<RESET>\n+\t Q;<RESET>\n+\t }<RESET>\n+\t<GREEN>+<RESET>\n+\t<GREEN>+<RESET><GREEN>int func()<RESET>\n+\t<GREEN>+<RESET><GREEN>{<RESET>\n+\t<GREEN>+<RESET>Q<GREEN>if (foo)<RESET>\n+\t<GREEN>+<RESET>QQ<GREEN>do_foo();<RESET>\n+\t Qbaz();<RESET>\n+\t Qbar();<RESET>\n+\t Q// more unrelated stuff<RESET>\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# And now ignoring white space only in the move detection\n+\tgit diff --color --color-moved \\\n+\t\t--color-moved-ignore-all-space \\\n+\t\t--color-moved-ignore-space-change \\\n+\t\t--color-moved-ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\t<BOLD>diff --git a/function.c b/function.c<RESET>\n+\t<BOLD>--- a/function.c<RESET>\n+\t<BOLD>+++ b/function.c<RESET>\n+\t<CYAN>@@ -1,13 +1,17 @@<RESET>\n+\t<GREEN>+<RESET><GREEN>int do_foo()<RESET>\n+\t<GREEN>+<RESET><GREEN>{<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// this part of the function<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// function will be very long<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// indeed. We must exceed both<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// per-line and number of line<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>// minimums<RESET>\n+\t<BOLD;CYAN>+<RESET>Q<BOLD;CYAN>;<RESET>\n+\t<BOLD;CYAN>+<RESET><BOLD;CYAN>}<RESET>\n+\t<GREEN>+<RESET>\n+\t int func()<RESET>\n+\t {<RESET>\n+\t<RED>-Qif (foo) {<RESET>\n+\t<BOLD;MAGENTA>-QQ// this part of the function<RESET>\n+\t<BOLD;MAGENTA>-QQ// function will be very long<RESET>\n+\t<BOLD;MAGENTA>-QQ// indeed. We must exceed both<RESET>\n+\t<BOLD;MAGENTA>-QQ// per-line and number of line<RESET>\n+\t<BOLD;MAGENTA>-QQ// minimums<RESET>\n+\t<BOLD;MAGENTA>-QQ;<RESET>\n+\t<BOLD;MAGENTA>-Q}<RESET>\n+\t<GREEN>+<RESET>Q<GREEN>if (foo)<RESET>\n+\t<GREEN>+<RESET>QQ<GREEN>do_foo();<RESET>\n+\t Qbaz();<RESET>\n+\t Qbar();<RESET>\n+\t Q// more unrelated stuff<RESET>\n+\tEOF\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.15.0.rc2.6.g953226eb5f\n\n"},{"id":"331046","messageId":"20171025224620.27657-3-sbeller@google.com","threadId":"47029","inReplyTo":"20171025224620.27657-1-sbeller@google.com","subject":"[PATCH 2/2] diff.c: ignore all white space changes by default in the move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-25T22:46:20Z","receivedAt":"2017-10-25T22:46:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The coloring of moved lines is currently only related to the presentation,\nas there are no options to export the move detection information into\nthe patch format. Hence we can be very loose about the default, so let's\nignore any white space change for the move detection. If a user really\ncares about move detection detecting only lines moved byte-for-byte,\nthey can exercise the --color-moved-no-ignore-* options.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/diff-options.txt | 4 +++-\n diff.c                         | 1 +\n t/t4015-diff-whitespace.sh     | 6 ++++++\n 3 files changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 1000b53b84..fdf40cbefc 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -269,14 +269,16 @@ dimmed_zebra::\n \tIgnore whitespace when comparing lines when performing the move\n \tdetection for --color-moved.  This ignores differences even if\n \tone line has whitespace where the other line has none.\n+\tThis is on by default.\n --color-moved-[no-]ignore-space-change::\n \tIgnore changes in amount of whitespace when performing the move\n \tdetection for --color-moved.  This ignores whitespace\n \tat line end, and considers all other sequences of one or\n \tmore whitespace characters to be equivalent.\n+\tThis is on by default.\n --color-moved-[no-]ignore-space-at-eol::\n \tIgnore changes in whitespace at EOL when performing the move\n-\tdetection for --color-moved.\n+\tdetection for --color-moved. This is on by default.\n \n --word-diff[=<mode>]::\n \tShow a word diff, using the <mode> to delimit changed words.\ndiff --git a/diff.c b/diff.c\nindex 2580315ab9..fef4874169 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4105,6 +4105,7 @@ void diff_setup(struct diff_options *options)\n \t}\n \n \toptions->color_moved = diff_color_moved_default;\n+\toptions->color_moved_ignore_space = XDF_WHITESPACE_FLAGS;\n }\n \n void diff_setup_done(struct diff_options *options)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 4ef4d6934a..a4ffc84f41 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1813,6 +1813,12 @@ test_expect_success 'move detection only ignores white spaces' '\n \t Qbar();<RESET>\n \t Q// more unrelated stuff<RESET>\n \tEOF\n+\ttest_cmp expected actual &&\n+\n+\t# test that this is the default:\n+\tgit diff --color --color-moved |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n \ttest_cmp expected actual\n '\n \n-- \n2.15.0.rc2.6.g953226eb5f\n\n"},{"id":"331073","messageId":"20171026072213.p6llaqrbdss7nbu4@ruderich.org","threadId":"47029","inReplyTo":"20171025224620.27657-1-sbeller@google.com","subject":"Re: [PATCH 0/2] color-moved: ignore all space changes by default","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-10-26T07:22:13Z","receivedAt":"2017-10-26T07:22:19Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:\n> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:\n>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:\n>>>\n>>>  * As moved-lines display is mostly a presentation thing, I wonder\n>>>    if it makes sense to always match loosely wrt whitespace\n>>>    differences.\n>>\n>> Well, sometimes the user wants to know if it is byte-for-byte identical\n>> (unlikely to be code, but maybe column oriented data for input;\n>> think of all our FORTRAN users. ;)\n>\n> ... and this is the implementation and the flip of the default setting\n> to ignore all white space for the move detection.\n\nHello,\n\nI'm not sure if this is a good default. I think it's not obvious\nthat moved code gets treated differently than regular changes. I\nwouldn't expect git diff to ignore whitespace changes (without me\ntelling it to) and so when I see moved code I expect they were\nmoved as is.\n\nAnd there are languages where indentation is relevant (e.g.\nPython, YAML) and as color-moved is also treated as review tool\nto detect unwanted changes this new default can be dangerous.\n\nThe new options sound like a good addition but I don't think the\ndefaults should change. However unrelated to this decision,\nplease add config settings in addition to these new options so\nusers can globally configure the behavior they want.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331078","messageId":"CA+P7+xp2S9U1y0ENj0Z=B6-DY3XWKCg16xCCzVjHDLwAcMbGZQ@mail.gmail.com","threadId":"47029","inReplyTo":"20171026072213.p6llaqrbdss7nbu4@ruderich.org","subject":"Re: [PATCH 0/2] color-moved: ignore all space changes by default","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-10-26T08:42:56Z","receivedAt":"2017-10-26T08:44:51Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Oct 26, 2017 at 12:22 AM, Simon Ruderich <simon@ruderich.org> wrote:\n> On Wed, Oct 25, 2017 at 03:46:18PM -0700, Stefan Beller wrote:\n>> On Mon, Oct 23, 2017 at 7:52 PM, Stefan Beller wrote[1]:\n>>> On Mon, Oct 23, 2017 at 6:54 PM, Junio C Hamano wrote:\n>>>>\n>>>>  * As moved-lines display is mostly a presentation thing, I wonder\n>>>>    if it makes sense to always match loosely wrt whitespace\n>>>>    differences.\n>>>\n>>> Well, sometimes the user wants to know if it is byte-for-byte identical\n>>> (unlikely to be code, but maybe column oriented data for input;\n>>> think of all our FORTRAN users. ;)\n>>\n>> ... and this is the implementation and the flip of the default setting\n>> to ignore all white space for the move detection.\n>\n> Hello,\n>\n> I'm not sure if this is a good default. I think it's not obvious\n> that moved code gets treated differently than regular changes. I\n> wouldn't expect git diff to ignore whitespace changes (without me\n> telling it to) and so when I see moved code I expect they were\n> moved as is.\n>\n> And there are languages where indentation is relevant (e.g.\n> Python, YAML) and as color-moved is also treated as review tool\n> to detect unwanted changes this new default can be dangerous.\n>\n> The new options sound like a good addition but I don't think the\n> defaults should change. However unrelated to this decision,\n> please add config settings in addition to these new options so\n> users can globally configure the behavior they want.\n>\n> Regards\n> Simon\n> --\n\nEven languages which are indentation sensitive often move blocks of\nlines between indentation levels a lot. I personally think the default\ncould change.\n\nHowever, I would suspect the best path forward is leave the default\n\"exact match\" and allow users who care and know about the feature to\nchange their config settings.\n\nThanks,\nJake\n"},{"id":"331102","messageId":"CAGZ79kZ5o6SjSa2C4rSY3ExEstThXziK7zzF93Y7uT1udE6+Yg@mail.gmail.com","threadId":"47029","inReplyTo":"CA+P7+xp2S9U1y0ENj0Z=B6-DY3XWKCg16xCCzVjHDLwAcMbGZQ@mail.gmail.com","subject":"Re: [PATCH 0/2] color-moved: ignore all space changes by default","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-26T17:59:36Z","receivedAt":"2017-10-26T17:59:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Oct 26, 2017 at 1:42 AM, Jacob Keller <jacob.keller@gmail.com> wrote:\n\n>>\n>> Hello,\n>>\n>> I'm not sure if this is a good default. I think it's not obvious\n>> that moved code gets treated differently than regular changes. I\n>> wouldn't expect git diff to ignore whitespace changes (without me\n>> telling it to) and so when I see moved code I expect they were\n>> moved as is.\n>>\n>> And there are languages where indentation is relevant (e.g.\n>> Python, YAML) and as color-moved is also treated as review tool\n>> to detect unwanted changes this new default can be dangerous.\n\nThat makes sense.\n\n>>\n>> The new options sound like a good addition but I don't think the\n>> defaults should change. However unrelated to this decision,\n>> please add config settings in addition to these new options so\n>> users can globally configure the behavior they want.\n>>\n>> Regards\n>> Simon\n>> --\n>\n> Even languages which are indentation sensitive often move blocks of\n> lines between indentation levels a lot. I personally think the default\n> could change.\n\nA safe default for such languages would be when the\nchange in whitespace across lines is taking into account, i.e.\nwhen lots of lines are copied, but all of them miss two tab indentation,\nthen it is probably fine to color it all the same. However if there would\nbe a couple of lines in that moved block of code that have\na different indentation than most other lines, this could indeed\nchange the program flow in python.\n\nSo ideally we'd compare the whitespace delta across lines.\nNote sure if that is easy. Maybe instead of ignoring whitespaces\ncompletely we'd rather make up a \"white space delta\", e.g. by using\nword-diff between the old and new line, and then keeping a hash of that\nspace delta for each line. The move detection would then also compare\nthe hashes of adjacent moved lines.\n\n> However, I would suspect the best path forward is leave the default\n> \"exact match\" and allow users who care and know about the feature to\n> change their config settings.\n\nI think that is what I'll do for now as it seems simple enough.\n\nThanks,\nStefan\n"}]}