{"thread":{"id":"48758","subject":"[PATCH v3 0/8] Moved code detection: ignore space on uniform indentation","startedAt":"2018-06-22T01:57:41Z","lastAt":"2018-07-02T19:07:35Z","messageCount":25,"participants":["Stefan Beller","Junio C Hamano","SZEDER Gábor","Brandon Williams"],"isPatch":true,"patchVersion":3,"patchTotal":8},"messages":[{"id":"350702","messageId":"20180622015725.219575-1-sbeller@google.com","threadId":"48758","inReplyTo":null,"subject":"[PATCH v3 0/8] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:17Z","receivedAt":"2018-06-22T01:57:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This replaces sb/diff-color-move-more and is also available at\nhttps://github.com/stefanbeller/git/tree/color-moved-only\nIt applies on v2.18.0.\n\nMove detection is a nice feature, but doesn't work well with indentation\nor dedentation. Make it possible to indent/dedent code and still have\nit recognized as moved code in the diff.\n\nThe fun is in the last patch, which allows white space sensitive\nlanguages to trust the move detection, too. Each block that is marked as\nmoved will have the same delta in {in-, de-}dentation.\nI would think this mode might be a reasonable default eventually.\n\nThanks,\nStefan\n\nv3:\n This is a complete rewrite of the actual patch, with slight modifications]\n on the refactoring how to decouple the white space treatment from the\n move detection. See range-diff below.\n\nv2: https://public-inbox.org/git/20180424210330.87861-1-sbeller@google.com/\n\nv1: https://public-inbox.org/git/20180402224854.86922-1-sbeller@google.com/\n\nStefan Beller (8):\n xdiff/xdiff.h: remove unused flags\n xdiff/xdiffi.c: remove unneeded function declarations\n diff.c: do not pass diff options as keydata to hashmap\n diff.c: adjust hash function signature to match hashmap expectation\n diff.c: add a blocks mode for moved code detection\n diff.c: decouple white space treatment from move detection algorithm\n diff.c: factor advance_or_nullify out of mark_color_as_moved\n diff.c: add white space mode to move detection that allows indent\n changes\n\n Documentation/diff-options.txt | 29 +++-\n diff.c | 253 +++++++++++++++++++++++++++++----\n diff.h | 9 +-\n t/t4015-diff-whitespace.sh | 202 +++++++++++++++++++++++++-\n xdiff/xdiff.h | 8 --\n xdiff/xdiffi.c | 17 ---\n 6 files changed, 458 insertions(+), 60 deletions(-)\n\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\ngit branch-diff fe0a9eaf31dd0c349ae4308498c33a5c3794b293..origin/sb/diff-color-move-more origin/master..HEAD\n\n1: a7a7af6b76b = 1: 7e01bd9a972 xdiff/xdiff.h: remove unused flags\n2: a7b6aaf7bc0 = 2: 46e11a99bb7 xdiff/xdiffi.c: remove unneeded function declarations\n3: d9e57cc6b05 = 3: 8fd0ce94aaf diff.c: do not pass diff options as keydata to hashmap\n4: 87111ba726d = 4: 4a07b39163c diff.c: adjust hash function signature to match hashmap expectation\n5: 9559b8cb456 = 5: ef1976a301d diff.c: add a blocks mode for moved code detection\n6: 41a70464209 ! 6: a60a3f0de9d diff.c: decouple white space treatment from move detection algorithm\n @@ -7,24 +7,30 @@\n for the regular diff. Some cases came up where different treatment would\n have been nice.\n \n - Allow the user to specify that whitespace should be ignored differently\n + Allow the user to specify that white space should be ignored differently\n during detection of moved lines than during generation of added and removed\n lines. This is done by providing analogs to the --ignore-space-at-eol,\n - -b, and -w options (namely,\n - --color-moved-[no-]ignore-space-at-eol\n - --color-moved-[no-]ignore-space-change\n - --color-moved-[no-]ignore-all-space) that affect only the color of the\n - output, and making the existing --ignore-space-at-eol, -b, and -w options\n - no longer affect the color of the output.\n + -b, and -w options by introducing the option --color-moved-ws=\n + with the modes named \"ignore-space-at-eol\", \"ignore-space-change\" and\n + \"ignore-all-space\", which is used only during the move detection phase.\n \n As we change the default, we'll adjust the tests.\n \n - For now we do not infer any options to treat whitespaces in the move\n + For now we do not infer any options to treat white spaces in the move\n detection from the generic white space options given to diff.\n This can be tuned later to reasonable default.\n \n + As we plan on adding more white space related options in a later patch,\n + that interferes with the current white space options, use a flag field\n + and clamp it down to XDF_WHITESPACE_FLAGS, as that (a) allows to easily\n + check at parse time if we give invalid combinations and (b) can reuse\n + parts of this patch.\n +\n + By having the white space treatment in its own option, we'll also\n + make it easier for a later patch to have an config option for\n + spaces in the move detection.\n +\n Signed-off-by: Stefan Beller \n - Signed-off-by: Junio C Hamano \n \n diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n --- a/Documentation/diff-options.txt\n @@ -33,18 +39,21 @@\n \tblocks are considered interesting, the rest is uninteresting.\n --\n \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 -+--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 ++--color-moved-ws=::\n ++\tThis configures how white spaces are ignored when performing the\n ++\tmove detection for `--color-moved`. These modes can be given\n ++\tas a comma separated list:\n +++\n ++--\n ++ignore-space-at-eol::\n ++\tIgnore changes in whitespace at EOL.\n ++ignore-space-change::\n ++\tIgnore changes in amount of whitespace. 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-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 ++ignore-all-space::\n ++\tIgnore whitespace when comparing lines. This ignores differences\n ++\teven if one line has whitespace where the other line has none.\n +\n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\n @@ -53,6 +62,43 @@\n diff --git a/diff.c b/diff.c\n --- a/diff.c\n +++ b/diff.c\n +@@\n + \t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n + }\n + \n ++static int parse_color_moved_ws(const char *arg)\n ++{\n ++\tint ret = 0;\n ++\tstruct string_list l = STRING_LIST_INIT_DUP;\n ++\tstruct string_list_item *i;\n ++\n ++\tstring_list_split(&l, arg, ',', -1);\n ++\n ++\tfor_each_string_list_item(i, &l) {\n ++\t\tstruct strbuf sb = STRBUF_INIT;\n ++\t\tstrbuf_addstr(&sb, i->string);\n ++\t\tstrbuf_trim(&sb);\n ++\n ++\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n ++\t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n ++\t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n ++\t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n ++\t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n ++\t\t\tret |= XDF_IGNORE_WHITESPACE;\n ++\t\telse\n ++\t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n ++\n ++\t\tstrbuf_release(&sb);\n ++\t}\n ++\n ++\tstring_list_clear(&l, 0);\n ++\n ++\treturn ret;\n ++}\n ++\n + int git_diff_ui_config(const char *var, const char *value, void *cb)\n + {\n + \tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n @@\n \tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n \tconst struct moved_entry *a = entry;\n @@ -79,24 +125,14 @@\n \tret->next_line = NULL;\n \n @@\n - \t\tDIFF_XDL_SET(options, IGNORE_CR_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_ws_handling &= ~XDF_IGNORE_WHITESPACE;\n -+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-change\"))\n -+\t\toptions->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_CHANGE;\n -+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-at-eol\"))\n -+\t\toptions->color_moved_ws_handling &= ~XDF_IGNORE_WHITESPACE_AT_EOL;\n -+\telse if (!strcmp(arg, \"--color-moved-ignore-all-space\"))\n -+\t\toptions->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n -+\telse if (!strcmp(arg, \"--color-moved-ignore-space-change\"))\n -+\t\toptions->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE_CHANGE;\n -+\telse if (!strcmp(arg, \"--color-moved-ignore-space-at-eol\"))\n -+\t\toptions->color_moved_ws_handling |= 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\"))\n + \t\tif (cm < 0)\n + \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n + \t\toptions->color_moved = cm;\n ++\t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n ++\t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n + \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n + \t\toptions->use_color = 1;\n + \t\toptions->word_diff = DIFF_WORDS_COLOR;\n \n diff --git a/diff.h b/diff.h\n --- a/diff.h\n @@ -113,39 +149,13 @@\n diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n --- a/t/t4015-diff-whitespace.sh\n +++ b/t/t4015-diff-whitespace.sh\n -@@\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 @@\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 -@@\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\t--color-moved-ws=ignore-all-space |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n @@ -155,21 +165,7 @@\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 -@@\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\t--color-moved-ws=ignore-space-change |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n @@ -179,9 +175,7 @@\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\t--color-moved-ws=ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n @@ -211,10 +205,7 @@\n +\tEOF\n +\n +\t# Make sure we get a different diff using -w\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 ++\tgit diff --color --color-moved -w |\n +\t\tgrep -v \"index\" |\n +\t\ttest_decode_color >actual &&\n +\tq_to_tab <<-\\EOF >expected &&\n @@ -231,9 +222,7 @@\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\t--color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n +\t\tgrep -v \"index\" |\n +\t\ttest_decode_color >actual &&\n +\tq_to_tab <<-\\EOF >expected &&\n7: ce99fa38553 < -: ----------- diff.c: add --color-moved-ignore-space-delta option\n8: 39c5337cd9e < -: ----------- diff: color-moved white space handling options imply color-moved\n-: ----------- > 7: b76faee22fe diff.c: factor advance_or_nullify out of mark_color_as_moved\n-: ----------- > 8: ab003ed7e27 diff.c: add white space mode to move detection that allows indent changes\n"},{"id":"350703","messageId":"20180622015725.219575-2-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 1/8] xdiff/xdiff.h: remove unused flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:18Z","receivedAt":"2018-06-22T01:57:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"These flags were there since the beginning (3443546f6e (Use a *real*\nbuilt-in diff generator, 2006-03-24), but were never used. Remove them.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n xdiff/xdiff.h | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex c1937a29112..2356da5f784 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -52,14 +52,6 @@ extern \"C\" {\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \n-#define XDL_MMB_READONLY (1 << 0)\n-\n-#define XDL_MMF_ATOMIC (1 << 0)\n-\n-#define XDL_BDOP_INS 1\n-#define XDL_BDOP_CPY 2\n-#define XDL_BDOP_INSB 3\n-\n /* merge simplification levels */\n #define XDL_MERGE_MINIMAL 0\n #define XDL_MERGE_EAGER 1\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350704","messageId":"20180622015725.219575-3-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 2/8] xdiff/xdiffi.c: remove unneeded function declarations","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:19Z","receivedAt":"2018-06-22T01:57:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There is no need to forward-declare these functions, as they are used\nafter their implementation only.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n xdiff/xdiffi.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 0de1ef463bf..3e8aff92bc4 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -22,34 +22,17 @@\n \n #include \"xinclude.h\"\n \n-\n-\n #define XDL_MAX_COST_MIN 256\n #define XDL_HEUR_MIN_COST 256\n #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)\n #define XDL_SNAKE_CNT 20\n #define XDL_K_HEUR 4\n \n-\n-\n typedef struct s_xdpsplit {\n \tlong i1, i2;\n \tint min_lo, min_hi;\n } xdpsplit_t;\n \n-\n-\n-\n-static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n-\t\t unsigned long const *ha2, long off2, long lim2,\n-\t\t long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,\n-\t\t xdalgoenv_t *xenv);\n-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2);\n-\n-\n-\n-\n-\n /*\n * See \"An O(ND) Difference Algorithm and its Variations\", by Eugene Myers.\n * Basically considers a \"box\" (off1, off2, lim1, lim2) and scan from both\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350705","messageId":"20180622015725.219575-5-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 4/8] diff.c: adjust hash function signature to match hashmap expectation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:21Z","receivedAt":"2018-06-22T01:57:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes the follow up patch easier.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 112e6af2cc8..2a1a21d3b7a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -707,11 +707,15 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n-static int moved_entry_cmp(const struct diff_options *diffopt,\n-\t\t\t const struct moved_entry *a,\n-\t\t\t const struct moved_entry *b,\n+static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n+\t\t\t const void *entry,\n+\t\t\t const void *entry_or_key,\n \t\t\t const void *keydata)\n {\n+\tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n+\tconst struct moved_entry *a = entry;\n+\tconst struct moved_entry *b = entry_or_key;\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@@ -5541,10 +5545,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\tif (o->color_moved) {\n \t\t\tstruct hashmap add_lines, del_lines;\n \n-\t\t\thashmap_init(&del_lines,\n-\t\t\t\t (hashmap_cmp_fn)moved_entry_cmp, o, 0);\n-\t\t\thashmap_init(&add_lines,\n-\t\t\t\t (hashmap_cmp_fn)moved_entry_cmp, o, 0);\n+\t\t\thashmap_init(&del_lines, moved_entry_cmp, o, 0);\n+\t\t\thashmap_init(&add_lines, moved_entry_cmp, o, 0);\n \n \t\t\tadd_lines_to_move_detection(o, &add_lines, &del_lines);\n \t\t\tmark_color_as_moved(o, &add_lines, &del_lines);\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350706","messageId":"20180622015725.219575-6-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 5/8] diff.c: add a blocks mode for moved code detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:22Z","receivedAt":"2018-06-22T01:57:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new \"blocks\" mode provides a middle ground between plain and zebra.\nIt is as intuitive (few colors) as plain, but still has the requirement\nfor a minimum of lines/characters to count a block as moved.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \n (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-options.txt | 8 ++++--\n diff.c | 6 +++--\n diff.h | 5 ++--\n t/t4015-diff-whitespace.sh | 48 +++++++++++++++++++++++++++++++++-\n 4 files changed, 60 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 41064909ee4..2e20794f9ed 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -276,10 +276,14 @@ plain::\n \tthat are added somewhere else in the diff. This mode picks up any\n \tmoved line, but it is not very useful in a review to determine\n \tif a block of code was moved without permutation.\n-zebra::\n+blocks:\n \tBlocks of moved text of at least 20 alphanumeric characters\n \tare detected greedily. The detected blocks are\n-\tpainted using either the 'color.diff.{old,new}Moved' color or\n+\tpainted using either the 'color.diff.{old,new}Moved' color.\n+\tAdjacent blocks cannot be told apart.\n+zebra::\n+\tBlocks of moved text are detected as in 'blocks' mode. The blocks\n+\tare painted using either the 'color.diff.{old,new}Moved' color or\n \t'color.diff.{old,new}MovedAlternative'. The change between\n \tthe two colors indicates that a new block was detected.\n dimmed_zebra::\ndiff --git a/diff.c b/diff.c\nindex 2a1a21d3b7a..b9575901eb2 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -271,6 +271,8 @@ static int parse_color_moved(const char *arg)\n \t\treturn COLOR_MOVED_NO;\n \telse if (!strcmp(arg, \"plain\"))\n \t\treturn COLOR_MOVED_PLAIN;\n+\telse if (!strcmp(arg, \"blocks\"))\n+\t\treturn COLOR_MOVED_BLOCKS;\n \telse if (!strcmp(arg, \"zebra\"))\n \t\treturn COLOR_MOVED_ZEBRA;\n \telse if (!strcmp(arg, \"default\"))\n@@ -278,7 +280,7 @@ static int parse_color_moved(const char *arg)\n \telse if (!strcmp(arg, \"dimmed_zebra\"))\n \t\treturn COLOR_MOVED_ZEBRA_DIM;\n \telse\n-\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'\"));\n+\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n }\n \n int git_diff_ui_config(const char *var, const char *value, void *cb)\n@@ -903,7 +905,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \n \t\tblock_length++;\n \n-\t\tif (flipped_block)\n+\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n \t}\n \tadjust_last_block(o, n, block_length);\ndiff --git a/diff.h b/diff.h\nindex dedac472ca5..d8009597937 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -208,8 +208,9 @@ struct diff_options {\n \tenum {\n \t\tCOLOR_MOVED_NO = 0,\n \t\tCOLOR_MOVED_PLAIN = 1,\n-\t\tCOLOR_MOVED_ZEBRA = 2,\n-\t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n+\t\tCOLOR_MOVED_BLOCKS = 2,\n+\t\tCOLOR_MOVED_ZEBRA = 3,\n+\t\tCOLOR_MOVED_ZEBRA_DIM = 4,\n \t} color_moved;\n \t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 17df491a3ab..45091abb192 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n+test_expect_success 'detect blocks of moved code' '\n \tgit reset --hard &&\n \tcat <<-\\EOF >lines.txt &&\n \t\tlong line 1\n@@ -1271,6 +1271,52 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n \ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n+\n+\tgit diff HEAD --no-renames --color-moved=blocks --color |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,16 +1,16 @@\n+\t-long line 1\n+\t-long line 2\n+\t-long line 3\n+\t line 4\n+\t line 5\n+\t line 6\n+\t line 7\n+\t line 8\n+\t line 9\n+\t+long line 1\n+\t+long line 2\n+\t+long line 3\n+\t+long line 14\n+\t+long line 15\n+\t+long line 16\n+\t line 10\n+\t line 11\n+\t line 12\n+\t line 13\n+\t-long line 14\n+\t-long line 15\n+\t-long line 16\n+\tEOF\n+\ttest_cmp expected actual\n+\n+'\n+\n+test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n+\t# reuse setup from test before!\n+\ttest_config color.diff.oldMoved \"magenta\" &&\n+\ttest_config color.diff.newMoved \"cyan\" &&\n+\ttest_config color.diff.oldMovedAlternative \"blue\" &&\n+\ttest_config color.diff.newMovedAlternative \"yellow\" &&\n+\ttest_config color.diff.oldMovedDimmed \"normal magenta\" &&\n+\ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n+\ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n+\ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n \tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350707","messageId":"20180622015725.219575-4-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 3/8] diff.c: do not pass diff options as keydata to hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:20Z","receivedAt":"2018-06-22T01:57:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When we initialize the hashmap, we give it a pointer to the\ndiff_options, which it then passes along to each call of the\nhashmap_cmp_fn function. There's no need to pass it a second time as\nthe \"keydata\" parameter, and our comparison functions never look at\nkeydata.\n\nThis was a mistake left over from an earlier round of 2e2d5ac184\n(diff.c: color moved lines differently, 2017-06-30), before hashmap\nlearned to pass the data pointer for us.\n\nExplanation-by: Jeff King \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 136d44b4556..112e6af2cc8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -842,13 +842,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tcase DIFF_SYMBOL_PLUS:\n \t\t\thm = del_lines;\n \t\t\tkey = prepare_entry(o, n);\n-\t\t\tmatch = hashmap_get(hm, key, o);\n+\t\t\tmatch = hashmap_get(hm, key, NULL);\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tcase DIFF_SYMBOL_MINUS:\n \t\t\thm = add_lines;\n \t\t\tkey = prepare_entry(o, n);\n-\t\t\tmatch = hashmap_get(hm, key, o);\n+\t\t\tmatch = hashmap_get(hm, key, NULL);\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tdefault:\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350708","messageId":"20180622015725.219575-7-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 6/8] diff.c: decouple white space treatment from move detection algorithm","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:23Z","receivedAt":"2018-06-22T01:57:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the original implementation of the move detection logic the choice for\nignoring white space changes is the same for the move detection as it is\nfor the regular diff. Some cases came up where different treatment would\nhave been nice.\n\nAllow the user to specify that white space should be ignored differently\nduring detection of moved lines than during generation of added and removed\nlines. This is done by providing analogs to the --ignore-space-at-eol,\n-b, and -w options by introducing the option --color-moved-ws=\nwith the modes named \"ignore-space-at-eol\", \"ignore-space-change\" and\n\"ignore-all-space\", which is used only during the move detection phase.\n\nAs we change the default, we'll adjust the tests.\n\nFor now we do not infer any options to treat white spaces in the move\ndetection from the generic white space options given to diff.\nThis can be tuned later to reasonable default.\n\nAs we plan on adding more white space related options in a later patch,\nthat interferes with the current white space options, use a flag field\nand clamp it down to XDF_WHITESPACE_FLAGS, as that (a) allows to easily\ncheck at parse time if we give invalid combinations and (b) can reuse\nparts of this patch.\n\nBy having the white space treatment in its own option, we'll also\nmake it easier for a later patch to have an config option for\nspaces in the move detection.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 16 +++++++++\n diff.c | 39 +++++++++++++++++++--\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 64 +++++++++++++++++++++++++++++++---\n 4 files changed, 114 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 2e20794f9ed..d174ed1dd0e 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -292,6 +292,22 @@ dimmed_zebra::\n \tblocks are considered interesting, the rest is uninteresting.\n --\n \n+--color-moved-ws=::\n+\tThis configures how white spaces are ignored when performing the\n+\tmove detection for `--color-moved`. These modes can be given\n+\tas a comma separated list:\n++\n+--\n+ignore-space-at-eol::\n+\tIgnore changes in whitespace at EOL.\n+ignore-space-change::\n+\tIgnore changes in amount of whitespace. This ignores whitespace\n+\tat line end, and considers all other sequences of one or\n+\tmore whitespace characters to be equivalent.\n+ignore-all-space::\n+\tIgnore whitespace when comparing lines. This ignores differences\n+\teven if one line has whitespace where the other line has none.\n+\n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\n \tBy default, words are delimited by whitespace; see\ndiff --git a/diff.c b/diff.c\nindex b9575901eb2..6c523bafddf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -283,6 +283,36 @@ static int parse_color_moved(const char *arg)\n \t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n }\n \n+static int parse_color_moved_ws(const char *arg)\n+{\n+\tint ret = 0;\n+\tstruct string_list l = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *i;\n+\n+\tstring_list_split(&l, arg, ',', -1);\n+\n+\tfor_each_string_list_item(i, &l) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sb, i->string);\n+\t\tstrbuf_trim(&sb);\n+\n+\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n+\t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE;\n+\t\telse\n+\t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n+\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tstring_list_clear(&l, 0);\n+\n+\treturn ret;\n+}\n+\n int git_diff_ui_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n@@ -717,10 +747,12 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n \tconst struct moved_entry *a = entry;\n \tconst struct moved_entry *b = entry_or_key;\n+\tunsigned flags = diffopt->color_moved_ws_handling\n+\t\t\t & XDF_WHITESPACE_FLAGS;\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 flags);\n }\n \n static struct moved_entry *prepare_entry(struct diff_options *o,\n@@ -728,8 +760,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n {\n \tstruct moved_entry *ret = xmalloc(sizeof(*ret));\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];\n+\tunsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;\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, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n \n@@ -4717,6 +4750,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n+\t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n+\t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\ndiff --git a/diff.h b/diff.h\nindex d8009597937..94d4adfe0f3 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -214,6 +214,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_ws_handling;\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 45091abb192..aad0870c8a1 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1465,7 +1465,8 @@ 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-ws=ignore-all-space |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1529,7 +1530,8 @@ 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-ws=ignore-space-change |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1596,7 +1598,8 @@ 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-ws=ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1768,7 +1771,60 @@ 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 'only move detection ignores white spaces' '\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\t\ta long line to exceed per-line minimum\n+\t\tanother long line to exceed per-line minimum\n+\t\toriginal file\n+\tEOF\n+\tgit add text.txt &&\n+\tgit commit -m \"add text\" &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\t\tQa long line to exceed per-line minimum\n+\t\tQanother long line to exceed per-line minimum\n+\t\tnew file\n+\tEOF\n+\n+\t# Make sure we get a different diff using -w\n+\tgit diff --color --color-moved -w |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,3 +1,3 @@\n+\t Qa long line to exceed per-line minimum\n+\t Qanother long line to exceed per-line minimum\n+\t-original file\n+\t+new file\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-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,3 +1,3 @@\n+\t-a long line to exceed per-line minimum\n+\t-another long line to exceed per-line minimum\n+\t-original file\n+\t+Qa long line to exceed per-line minimum\n+\t+Qanother long line to exceed per-line minimum\n+\t+new file\n+\tEOF\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350709","messageId":"20180622015725.219575-8-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 7/8] diff.c: factor advance_or_nullify out of mark_color_as_moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:24Z","receivedAt":"2018-06-22T01:58:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This moves the part of code that checks if we're still in a block\ninto its own function. We'll need a different approach on advancing\nthe blocks in a later patch, so having it as a separate function will\nprove useful.\n\nWhile at it rename the variable `p` to `prev` to indicate that it refers\nto the previous line. This is as pmb[i] was assigned in the last iteration\nof the outmost for loop.\n\nFurther rename `pnext` to `cur` to indicate that this should match up with\nthe current line of the outmost for loop.\n\nAlso replace the advancement of pmb[i] to reuse `cur` instead of\nusing `p->next` (which is how the name for pnext could be explained.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 6c523bafddf..040b46545e5 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -801,6 +801,25 @@ static void add_lines_to_move_detection(struct diff_options *o,\n \t}\n }\n \n+static void pmb_advance_or_null(struct diff_options *o,\n+\t\t\t\tstruct moved_entry *match,\n+\t\t\t\tstruct hashmap *hm,\n+\t\t\t\tstruct moved_entry **pmb,\n+\t\t\t\tint pmb_nr)\n+{\n+\tint i;\n+\tfor (i = 0; i < pmb_nr; i++) {\n+\t\tstruct moved_entry *prev = pmb[i];\n+\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n+\t\t\t\tprev->next_line : NULL;\n+\t\tif (cur && !hm->cmpfn(o, cur, match, NULL)) {\n+\t\t\tpmb[i] = cur;\n+\t\t} else {\n+\t\t\tpmb[i] = NULL;\n+\t\t}\n+\t}\n+}\n+\n static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \t\t\t\t\t int pmb_nr)\n {\n@@ -875,7 +894,6 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tstruct moved_entry *key;\n \t\tstruct moved_entry *match = NULL;\n \t\tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\t\tint i;\n \n \t\tswitch (l->s) {\n \t\tcase DIFF_SYMBOL_PLUS:\n@@ -906,17 +924,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\n \n-\t\t/* Check any potential block runs, advance each or nullify */\n-\t\tfor (i = 0; i < pmb_nr; i++) {\n-\t\t\tstruct moved_entry *p = pmb[i];\n-\t\t\tstruct moved_entry *pnext = (p && p->next_line) ?\n-\t\t\t\t\tp->next_line : NULL;\n-\t\t\tif (pnext && !hm->cmpfn(o, pnext, match, NULL)) {\n-\t\t\t\tpmb[i] = p->next_line;\n-\t\t\t} else {\n-\t\t\t\tpmb[i] = NULL;\n-\t\t\t}\n-\t\t}\n+\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n \n \t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n \n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350710","messageId":"20180622015725.219575-9-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v3 8/8] diff.c: add white space mode to move detection that allows indent changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-22T01:57:25Z","receivedAt":"2018-06-22T01:58:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The option of --color-moved has proven to be useful as observed on the\nmailing list. However when refactoring sometimes the indentation changes,\nfor example when partitioning a functions into smaller helper functions\nthe code usually mostly moved around except for a decrease in indentation.\n\nTo just review the moved code ignoring the change in indentation, a mode\nto ignore spaces in the move detection as implemented in a previous patch\nwould be enough. However the whole move coloring as motivated in commit\n2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought\nup the notion of the reviewer being able to trust the move of a \"block\".\n\nAs there are languages such as python, which depend on proper relative\nindentation for the control flow of the program, ignoring any white space\nchange in a block would not uphold the promises of 2e2d5ac that allows\nreviewers to pay less attention to the inside of a block, as inside\nthe reviewer wants to assume the same program flow.\n\nThis new mode of white space ignorance will take this into account and will\nonly allow the same white space changes per line in each block. This patch\neven allows only for the same change at the beginning of the lines.\n\nAs this is a white space mode, it is made exclusive to other white space\nmodes in the move detection.\n\nThis patch brings some challenges, related to the detection of blocks.\nWe need a white net the catch the possible moved lines, but then need to\nnarrow down to check if the blocks are still in tact. Consider this\nexample (ignoring block sizes):\n\n - A\n - B\n - C\n + A\n + B\n + C\n\nAt the beginning of a block when checking if there is a counterpart\nfor A, we have to ignore all space changes. However at the following\nlines we have to check if the indent change stayed the same.\n\nChecking if the indentation change did stay the same, is done by computing\nthe indentation change by the difference in line length, and then assume\nthe change is only in the beginning of the longer line, the common tail\nis the same. That is why the test contains lines like:\n\n - A\n ...\n + A \n ...\n\nAs the first line starting a block is caught using a compare function that\nignores white spaces unlike the rest of the block, where the white space\ndelta is taken into account for the comparison, we also have to think about\nthe following situation:\n\n - A\n - B\n - A\n - B\n + A\n + B\n + A\n + B\n\nWhen checking if the first A (both in the + and - lines) is a start of\na block, we have to check all 'A' and record all the white space deltas\nsuch that we can find the example above to be just one block that is\nindented.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 5 ++\n diff.c | 158 ++++++++++++++++++++++++++++++++-\n diff.h | 3 +\n t/t4015-diff-whitespace.sh | 90 +++++++++++++++++++\n 4 files changed, 254 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex d174ed1dd0e..d9ff0bb8a58 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -307,6 +307,11 @@ ignore-space-change::\n ignore-all-space::\n \tIgnore whitespace when comparing lines. This ignores differences\n \teven if one line has whitespace where the other line has none.\n+allow-indentation-change::\n+\tInitially ignore any white spaces in the move detection, then\n+\tgroup the moved code blocks only into a block if the change in\n+\twhitespace is the same per line. This is incompatible with the\n+\tother modes.\n \n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\ndiff --git a/diff.c b/diff.c\nindex 040b46545e5..9e357111864 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -302,12 +302,18 @@ static int parse_color_moved_ws(const char *arg)\n \t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n \t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE;\n+\t\telse if (!strcmp(sb.buf, \"allow-indentation-change\"))\n+\t\t\tret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n \t\telse\n \t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n \n \t\tstrbuf_release(&sb);\n \t}\n \n+\tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n+\t (ret & XDF_WHITESPACE_FLAGS))\n+\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n+\n \tstring_list_clear(&l, 0);\n \n \treturn ret;\n@@ -737,7 +743,91 @@ struct moved_entry {\n \tstruct hashmap_entry ent;\n \tconst struct emitted_diff_symbol *es;\n \tstruct moved_entry *next_line;\n+\tstruct ws_delta *wsd;\n+};\n+\n+/**\n+ * The struct ws_delta holds white space differences between moved lines, i.e.\n+ * between '+' and '-' lines that have been detected to be a move.\n+ * The string contains the difference in leading white spaces, before the\n+ * rest of the line is compared using the white space config for move\n+ * coloring. The current_longer indicates if the first string in the\n+ * comparision is longer than the second.\n+ */\n+struct ws_delta {\n+\tchar *string;\n+\tint current_longer : 1;\n };\n+#define WS_DELTA_INIT { NULL, 0 }\n+\n+static int compute_ws_delta(const struct emitted_diff_symbol *a,\n+\t\t\t const struct emitted_diff_symbol *b,\n+\t\t\t struct ws_delta *out)\n+{\n+\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n+\tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n+\tint d = longer->len - shorter->len;\n+\n+\tout->string = xmemdupz(longer->line, d);\n+\tout->current_longer = (a == longer);\n+\n+\treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n+}\n+\n+static int cmp_in_block_with_wsd(const struct diff_options *o,\n+\t\t\t\t const struct moved_entry *cur,\n+\t\t\t\t const struct moved_entry *match,\n+\t\t\t\t struct moved_entry *pmb,\n+\t\t\t\t int n)\n+{\n+\tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n+\tint al = cur->es->len, cl = l->len;\n+\tconst char *a = cur->es->line,\n+\t\t *b = match->es->line,\n+\t\t *c = l->line;\n+\n+\tint wslen;\n+\n+\t/*\n+\t * We need to check if 'cur' is equal to 'match'.\n+\t * As those are from the same (+/-) side, we do not need to adjust for\n+\t * indent changes. However these were found using fuzzy matching\n+\t * so we do have to check if they are equal.\n+\t */\n+\tif (strcmp(a, b))\n+\t\treturn 1;\n+\n+\tif (!pmb->wsd)\n+\t\t/*\n+\t\t * No white space delta was carried forward? This can happen\n+\t\t * when we exit early in this function and do not carry\n+\t\t * forward ws.\n+\t\t */\n+\t\treturn 1;\n+\n+\t/*\n+\t * The indent changes of the block are known and carried forward in\n+\t * pmb->wsd; however we need to check if the indent changes of the\n+\t * current line are still the same as before.\n+\t *\n+\t * To do so we need to compare 'l' to 'cur', adjusting the\n+\t * one of them for the white spaces, depending which was longer.\n+\t */\n+\n+\twslen = strlen(pmb->wsd->string);\n+\tif (pmb->wsd->current_longer) {\n+\t\tc += wslen;\n+\t\tcl -= wslen;\n+\t} else {\n+\t\ta += wslen;\n+\t\tal -= wslen;\n+\t}\n+\n+\tif (strcmp(a, c))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n \n static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \t\t\t const void *entry,\n@@ -750,6 +840,16 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \tunsigned flags = diffopt->color_moved_ws_handling\n \t\t\t & XDF_WHITESPACE_FLAGS;\n \n+\tif (diffopt->color_moved_ws_handling &\n+\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t/*\n+\t\t * As there is not specific white space config given,\n+\t\t * we'd need to check for a new block, so ignore all\n+\t\t * white space. The setup of the white space\n+\t\t * configuration for the next block is done else where\n+\t\t */\n+\t\tflags |= XDF_IGNORE_WHITESPACE;\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 flags);\n@@ -765,6 +865,7 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n \tret->ent.hash = xdiff_hash_string(l->line, l->len, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n+\tret->wsd = NULL;\n \n \treturn ret;\n }\n@@ -820,6 +921,37 @@ static void pmb_advance_or_null(struct diff_options *o,\n \t}\n }\n \n+static void pmb_advance_or_null_multi_match(struct diff_options *o,\n+\t\t\t\t\t struct moved_entry *match,\n+\t\t\t\t\t struct hashmap *hm,\n+\t\t\t\t\t struct moved_entry **pmb,\n+\t\t\t\t\t int pmb_nr, int n)\n+{\n+\tint i;\n+\tchar *got_match = xcalloc(1, pmb_nr);\n+\n+\tfor (; match; match = hashmap_get_next(hm, match)) {\n+\t\tfor (i = 0; i < pmb_nr; i++) {\n+\t\t\tstruct moved_entry *prev = pmb[i];\n+\t\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n+\t\t\t\t\tprev->next_line : NULL;\n+\t\t\tif (!cur)\n+\t\t\t\tcontinue;\n+\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, pmb[i], n))\n+\t\t\t\tgot_match[i] |= 1;\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i next_line->wsd = pmb[i]->wsd;\n+\t\t\tpmb[i] = pmb[i]->next_line;\n+\t\t} else\n+\t\t\tpmb[i] = NULL;\n+\t}\n+}\n+\n static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \t\t\t\t\t int pmb_nr)\n {\n@@ -837,6 +969,10 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n+\t\t\tif (pmb[rp]->wsd) {\n+\t\t\t\tfree(pmb[rp]->wsd->string);\n+\t\t\t\tFREE_AND_NULL(pmb[rp]->wsd);\n+\t\t\t}\n \t\t\tpmb[rp] = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n@@ -924,7 +1060,11 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\n \n-\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n+\t\tif (o->color_moved_ws_handling &\n+\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\tpmb_advance_or_null_multi_match(o, match, hm, pmb, pmb_nr, n);\n+\t\telse\n+\t\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n \n \t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n \n@@ -935,7 +1075,17 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t */\n \t\t\tfor (; match; match = hashmap_get_next(hm, match)) {\n \t\t\t\tALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);\n-\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\tif (o->color_moved_ws_handling &\n+\t\t\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n+\t\t\t\t\tstruct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n+\t\t\t\t\tif (compute_ws_delta(l, match->es, wsd)) {\n+\t\t\t\t\t\tmatch->wsd = wsd;\n+\t\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t\t} else\n+\t\t\t\t\t\tfree(wsd);\n+\t\t\t\t} else {\n+\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tflipped_block = (flipped_block + 1) % 2;\n@@ -5590,6 +5740,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\tif (o->color_moved) {\n \t\t\tstruct hashmap add_lines, del_lines;\n \n+\t\t\tif (o->color_moved_ws_handling &\n+\t\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\t\to->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n+\n \t\t\thashmap_init(&del_lines, moved_entry_cmp, o, 0);\n \t\t\thashmap_init(&add_lines, moved_entry_cmp, o, 0);\n \ndiff --git a/diff.h b/diff.h\nindex 94d4adfe0f3..a14895bb824 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -214,6 +214,9 @@ 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+\n+\t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n+\t#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)\n \tint color_moved_ws_handling;\n };\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex aad0870c8a1..13b20be591e 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1827,4 +1827,94 @@ test_expect_success 'only move detection ignores white spaces' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'compare whitespace delta across moved blocks' '\n+\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\tQIndented\n+\tQText across\n+\tQsome lines\n+\tQBut! <- this stands out\n+\tQAdjusting with\n+\tQQdifferent starting\n+\tQwhite spaces\n+\tQAnother outlier\n+\tQQQIndented\n+\tQQQText across\n+\tQQQfive lines\n+\tQQQthat has similar lines\n+\tQQQto previous blocks, but with different indent\n+\tQQQYetQAnotherQoutlierQ\n+\tEOF\n+\n+\tgit add text.txt &&\n+\tgit commit -m \"add text.txt\" &&\n+\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\tQQIndented\n+\tQQText across\n+\tQQsome lines\n+\tQQQBut! <- this stands out\n+\tAdjusting with\n+\tQdifferent starting\n+\twhite spaces\n+\tAnotherQoutlier\n+\tQQIndented\n+\tQQText across\n+\tQQfive lines\n+\tQQthat has similar lines\n+\tQQto previous blocks, but with different indent\n+\tQQYetQAnotherQoutlier\n+\tEOF\n+\n+\tgit diff --color --color-moved --color-moved-ws=allow-indentation-change |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\n+\tq_to_tab <<-\\EOF >expected &&\n+\t\tdiff --git a/text.txt b/text.txt\n+\t\t--- a/text.txt\n+\t\t+++ b/text.txt\n+\t\t@@ -1,14 +1,14 @@\n+\t\t-QIndented\n+\t\t-QText across\n+\t\t-Qsome lines\n+\t\t-QBut! <- this stands out\n+\t\t-QAdjusting with\n+\t\t-QQdifferent starting\n+\t\t-Qwhite spaces\n+\t\t-QAnother outlier\n+\t\t-QQQIndented\n+\t\t-QQQText across\n+\t\t-QQQfive lines\n+\t\t-QQQthat has similar lines\n+\t\t-QQQto previous blocks, but with different indent\n+\t\t-QQQYetQAnotherQoutlierQ\n+\t\t+QQIndented\n+\t\t+QQText across\n+\t\t+QQsome lines\n+\t\t+QQQBut! <- this stands out\n+\t\t+Adjusting with\n+\t\t+Qdifferent starting\n+\t\t+white spaces\n+\t\t+AnotherQoutlier\n+\t\t+QQIndented\n+\t\t+QQText across\n+\t\t+QQfive lines\n+\t\t+QQthat has similar lines\n+\t\t+QQto previous blocks, but with different indent\n+\t\t+QQYetQAnotherQoutlier\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'compare whitespace delta incompatible with other space options' '\n+\ttest_must_fail git diff \\\n+\t\t--color-moved-ws=allow-indentation-change,ignore-all-space \\\n+\t\t2>err &&\n+\tgrep allow-indentation-change err\n+'\n+\n+\n test_done\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"350772","messageId":"xmqqvaaaphwf.fsf@gitster-ct.c.googlers.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"Re: [PATCH v3 0/8] Moved code detection: ignore space on uniform indentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-22T22:37:20Z","receivedAt":"2018-06-22T22:37:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This seems to break the documentation build rather badly; I have a\nmonkey-fix queued at the tip fo the topic branch for tonight's push.\n"},{"id":"350790","messageId":"20180623165233.23653-1-szeder.dev@gmail.com","threadId":"48758","inReplyTo":"20180622015725.219575-9-sbeller@google.com","subject":"Re: [PATCH v3 8/8] diff.c: add white space mode to move detection that allows indent changes","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-06-23T16:52:33Z","receivedAt":"2018-06-23T16:52:49Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> diff --git a/diff.c b/diff.c\n> index 040b46545e5..9e357111864 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -302,12 +302,18 @@ static int parse_color_moved_ws(const char *arg)\n> \t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n> \t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n> \t\t\tret |= XDF_IGNORE_WHITESPACE;\n> +\t\telse if (!strcmp(sb.buf, \"allow-indentation-change\"))\n> +\t\t\tret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n> \t\telse\n> \t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n> \n> \t\tstrbuf_release(&sb);\n> \t}\n> \n> +\tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n> +\t (ret & XDF_WHITESPACE_FLAGS))\n> +\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n\nNote that this is a translated error message.\n\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index aad0870c8a1..13b20be591e 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1827,4 +1827,94 @@ test_expect_success 'only move detection ignores white spaces' '\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'compare whitespace delta across moved blocks' '\n> +\n> +\tgit reset --hard &&\n> +\tq_to_tab <<-\\EOF >text.txt &&\n> +\tQIndented\n> +\tQText across\n> +\tQsome lines\n> +\tQBut! <- this stands out\n> +\tQAdjusting with\n> +\tQQdifferent starting\n> +\tQwhite spaces\n> +\tQAnother outlier\n> +\tQQQIndented\n> +\tQQQText across\n> +\tQQQfive lines\n> +\tQQQthat has similar lines\n> +\tQQQto previous blocks, but with different indent\n> +\tQQQYetQAnotherQoutlierQ\n> +\tEOF\n> +\n> +\tgit add text.txt &&\n> +\tgit commit -m \"add text.txt\" &&\n> +\n> +\tq_to_tab <<-\\EOF >text.txt &&\n> +\tQQIndented\n> +\tQQText across\n> +\tQQsome lines\n> +\tQQQBut! <- this stands out\n> +\tAdjusting with\n> +\tQdifferent starting\n> +\twhite spaces\n> +\tAnotherQoutlier\n> +\tQQIndented\n> +\tQQText across\n> +\tQQfive lines\n> +\tQQthat has similar lines\n> +\tQQto previous blocks, but with different indent\n> +\tQQYetQAnotherQoutlier\n> +\tEOF\n> +\n> +\tgit diff --color --color-moved --color-moved-ws=allow-indentation-change |\n> +\t\tgrep -v \"index\" |\n> +\t\ttest_decode_color >actual &&\n\nPlease use an intermediate file for the output of 'git diff', because\nrunning it upstream of a pipe hides its exit code.\n\n> +\n> +\tq_to_tab <<-\\EOF >expected &&\n> +\t\tdiff --git a/text.txt b/text.txt\n> +\t\t--- a/text.txt\n> +\t\t+++ b/text.txt\n> +\t\t@@ -1,14 +1,14 @@\n> +\t\t-QIndented\n> +\t\t-QText across\n> +\t\t-Qsome lines\n> +\t\t-QBut! <- this stands out\n> +\t\t-QAdjusting with\n> +\t\t-QQdifferent starting\n> +\t\t-Qwhite spaces\n> +\t\t-QAnother outlier\n> +\t\t-QQQIndented\n> +\t\t-QQQText across\n> +\t\t-QQQfive lines\n> +\t\t-QQQthat has similar lines\n> +\t\t-QQQto previous blocks, but with different indent\n> +\t\t-QQQYetQAnotherQoutlierQ\n> +\t\t+QQIndented\n> +\t\t+QQText across\n> +\t\t+QQsome lines\n> +\t\t+QQQBut! <- this stands out\n> +\t\t+Adjusting with\n> +\t\t+Qdifferent starting\n> +\t\t+white spaces\n> +\t\t+AnotherQoutlier\n> +\t\t+QQIndented\n> +\t\t+QQText across\n> +\t\t+QQfive lines\n> +\t\t+QQthat has similar lines\n> +\t\t+QQto previous blocks, but with different indent\n> +\t\t+QQYetQAnotherQoutlier\n> +\tEOF\n> +\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'compare whitespace delta incompatible with other space options' '\n> +\ttest_must_fail git diff \\\n> +\t\t--color-moved-ws=allow-indentation-change,ignore-all-space \\\n> +\t\t2>err &&\n> +\tgrep allow-indentation-change err\n\nA translated error message should be checked with 'test_i18ngrep'.\n\n"},{"id":"351278","messageId":"20180629001958.85143-1-sbeller@google.com","threadId":"48758","inReplyTo":"20180622015725.219575-1-sbeller@google.com","subject":"[PATCH v4 0/9] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:49Z","receivedAt":"2018-06-29T00:20:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v4:\n * see range diff below\n * brought best practices to t4015 and have git not upstream of a pipe\n (new patch 3)\n * squashed in the SQUASH patches\n * fixed the translation as well.\n\nv3:\n This is a complete rewrite of the actual patch, with slight modifications]\n on the refactoring how to decouple the white space treatment from the\n move detection. See range-diff below.\n https://public-inbox.org/git/20180622015725.219575-1-sbeller@google.com/\n\nv2: https://public-inbox.org/git/20180424210330.87861-1-sbeller@google.com/\n\nv1: https://public-inbox.org/git/20180402224854.86922-1-sbeller@google.com/\n\nStefan Beller (9):\n xdiff/xdiff.h: remove unused flags\n xdiff/xdiffi.c: remove unneeded function declarations\n t4015: avoid git as a pipe input\n diff.c: do not pass diff options as keydata to hashmap\n diff.c: adjust hash function signature to match hashmap expectation\n diff.c: add a blocks mode for moved code detection\n diff.c: decouple white space treatment from move detection algorithm\n diff.c: factor advance_or_nullify out of mark_color_as_moved\n diff.c: add white space mode to move detection that allows indent\n changes\n\n Documentation/diff-options.txt | 30 +++-\n diff.c | 253 +++++++++++++++++++++++++++++----\n diff.h | 9 +-\n t/t4015-diff-whitespace.sh | 243 ++++++++++++++++++++++++++-----\n xdiff/xdiff.h | 8 --\n xdiff/xdiffi.c | 17 ---\n 6 files changed, 472 insertions(+), 88 deletions(-)\n\n-- \n2.18.0.399.gad0ab374a1-goog\n\n1: ace2dc2bc11 = 1: ace2dc2bc11 xdiff/xdiff.h: remove unused flags\n2: 53b3574564e = 2: 53b3574564e xdiff/xdiffi.c: remove unneeded function declarations\n-: ----------- > 3: 9b58621e0d8 t4015: avoid git as a pipe input\n3: 34850b565df = 4: be0ea0717e1 diff.c: do not pass diff options as keydata to hashmap\n4: 8148e51178f = 5: ff7e8721afa diff.c: adjust hash function signature to match hashmap expectation\n5: 9d1de6a208e ! 6: 73c2801a5e3 diff.c: add a blocks mode for moved code detection\n @@ -19,7 +19,7 @@\n \tmoved line, but it is not very useful in a review to determine\n \tif a block of code was moved without permutation.\n -zebra::\n -+blocks:\n ++blocks::\n \tBlocks of moved text of at least 20 alphanumeric characters\n \tare detected greedily. The detected blocks are\n -\tpainted using either the 'color.diff.{old,new}Moved' color or\n @@ -95,10 +95,8 @@\n \ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n -+\n -+\tgit diff HEAD --no-renames --color-moved=blocks --color |\n -+\t\tgrep -v \"index\" |\n -+\t\ttest_decode_color >actual &&\n ++\tgit diff HEAD --no-renames --color-moved=blocks --color >actual.raw &&\n ++\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n +\tcat <<-\\EOF >expected &&\n +\tdiff --git a/lines.txt b/lines.txt\n +\t--- a/lines.txt\n @@ -141,6 +139,16 @@\n +\ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n +\ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n +\ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n - \tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color |\n - \t\tgrep -v \"index\" |\n - \t\ttest_decode_color >actual &&\n + \tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color >actual.raw &&\n + \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n + \tcat <<-\\EOF >expected &&\n +@@\n + \t7charsA\n + \tEOF\n + \n +-\tgit diff HEAD --color-moved=zebra --color --no-renames | grep -v \"index\" | test_decode_color >actual &&\n ++\tgit diff HEAD --color-moved=zebra --color --no-renames >actual.raw &&\n ++\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n + \tcat >expected <<-\\EOF &&\n + \tdiff --git a/bar b/bar\n + \t--- a/bar\n6: e37bb7b1fc8 ! 7: 87a8919c260 diff.c: decouple white space treatment from move detection algorithm\n @@ -55,6 +55,7 @@\n +ignore-all-space::\n +\tIgnore whitespace when comparing lines. This ignores differences\n +\teven if one line has whitespace where the other line has none.\n ++--\n +\n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\n @@ -154,32 +155,32 @@\n \tEOF\n \ttest_cmp expected actual &&\n \n --\tgit diff HEAD --no-renames -w --color-moved --color |\n +-\tgit diff HEAD --no-renames -w --color-moved --color >actual.raw &&\n +\tgit diff HEAD --no-renames --color-moved --color \\\n -+\t\t--color-moved-ws=ignore-all-space |\n - \t\tgrep -v \"index\" |\n - \t\ttest_decode_color >actual &&\n ++\t\t--color-moved-ws=ignore-all-space >actual.raw &&\n + \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n + \tdiff --git a/lines.txt b/lines.txt\n @@\n \tEOF\n \ttest_cmp expected actual &&\n \n --\tgit diff HEAD --no-renames -b --color-moved --color |\n +-\tgit diff HEAD --no-renames -b --color-moved --color >actual.raw &&\n +\tgit diff HEAD --no-renames --color-moved --color \\\n -+\t\t--color-moved-ws=ignore-space-change |\n - \t\tgrep -v \"index\" |\n - \t\ttest_decode_color >actual &&\n ++\t\t--color-moved-ws=ignore-space-change >actual.raw &&\n + \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n + \tdiff --git a/lines.txt b/lines.txt\n @@\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 --ignore-space-at-eol --color-moved --color >actual.raw &&\n +\tgit diff HEAD --no-renames --color-moved --color \\\n -+\t\t--color-moved-ws=ignore-space-at-eol |\n - \t\tgrep -v \"index\" |\n - \t\ttest_decode_color >actual &&\n ++\t\t--color-moved-ws=ignore-space-at-eol >actual.raw &&\n + \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n + \tdiff --git a/lines.txt b/lines.txt\n @@\n \n \t# nor did we mess with it another way\n7: a58e68b8880 = 8: 25279c42f25 diff.c: factor advance_or_nullify out of mark_color_as_moved\n8: f2d78d2c672 ! 9: 305f8b27589 diff.c: add white space mode to move detection that allows indent changes\n @@ -86,9 +86,9 @@\n +\tgroup the moved code blocks only into a block if the change in\n +\twhitespace is the same per line. This is incompatible with the\n +\tother modes.\n + --\n \n --word-diff[=]::\n - \tShow a word diff, using the to delimit changed words.\n \n diff --git a/diff.c b/diff.c\n --- a/diff.c\n @@ -129,7 +129,7 @@\n + */\n +struct ws_delta {\n +\tchar *string;\n -+\tint current_longer : 1;\n ++\tunsigned int current_longer : 1;\n };\n +#define WS_DELTA_INIT { NULL, 0 }\n +\n @@ -339,6 +339,30 @@\n diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n --- a/t/t4015-diff-whitespace.sh\n +++ b/t/t4015-diff-whitespace.sh\n +@@\n + \tEOF\n + \n + \t# Make sure we get a different diff using -w\n +-\tgit diff --color --color-moved -w |\n +-\t\tgrep -v \"index\" |\n +-\t\ttest_decode_color >actual &&\n ++\tgit diff --color --color-moved -w >actual.raw &&\n ++\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n + \tq_to_tab <<-\\EOF >expected &&\n + \tdiff --git a/text.txt b/text.txt\n + \t--- a/text.txt\n +@@\n + \n + \t# And now ignoring white space only in the move detection\n + \tgit diff --color --color-moved \\\n +-\t\t--color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n +-\t\tgrep -v \"index\" |\n +-\t\ttest_decode_color >actual &&\n ++\t\t--color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol >actual.raw &&\n ++\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n + \tq_to_tab <<-\\EOF >expected &&\n + \tdiff --git a/text.txt b/text.txt\n + \t--- a/text.txt\n @@\n \ttest_cmp expected actual\n '\n @@ -383,9 +407,8 @@\n +\tQQYetQAnotherQoutlier\n +\tEOF\n +\n -+\tgit diff --color --color-moved --color-moved-ws=allow-indentation-change |\n -+\t\tgrep -v \"index\" |\n -+\t\ttest_decode_color >actual &&\n ++\tgit diff --color --color-moved --color-moved-ws=allow-indentation-change >actual.raw &&\n ++\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n +\n +\tq_to_tab <<-\\EOF >expected &&\n +\t\tdiff --git a/text.txt b/text.txt\n @@ -429,8 +452,7 @@\n +\ttest_must_fail git diff \\\n +\t\t--color-moved-ws=allow-indentation-change,ignore-all-space \\\n +\t\t2>err &&\n -+\tgrep allow-indentation-change err\n ++\ttest_i18ngrep allow-indentation-change err\n +'\n -+\n +\n test_done\n9: 76db2b1328f < -: ----------- SQUASH????? Documentation breakage emergency fix\n10: d484589fad1 < -: ----------- SQUASH??? t/4015 GETTEXT_POISON emergency fix\n11: fceb91fc99d < -: ----------- diff: fix a sparse 'dubious one-bit signed bitfield' error\n"},{"id":"351279","messageId":"20180629001958.85143-2-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 1/9] xdiff/xdiff.h: remove unused flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:50Z","receivedAt":"2018-06-29T00:20:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"These flags were there since the beginning (3443546f6e (Use a *real*\nbuilt-in diff generator, 2006-03-24), but were never used. Remove them.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n xdiff/xdiff.h | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex c1937a29112..2356da5f784 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -52,14 +52,6 @@ extern \"C\" {\n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n \n-#define XDL_MMB_READONLY (1 << 0)\n-\n-#define XDL_MMF_ATOMIC (1 << 0)\n-\n-#define XDL_BDOP_INS 1\n-#define XDL_BDOP_CPY 2\n-#define XDL_BDOP_INSB 3\n-\n /* merge simplification levels */\n #define XDL_MERGE_MINIMAL 0\n #define XDL_MERGE_EAGER 1\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351280","messageId":"20180629001958.85143-3-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 2/9] xdiff/xdiffi.c: remove unneeded function declarations","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:51Z","receivedAt":"2018-06-29T00:20:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"There is no need to forward-declare these functions, as they are used\nafter their implementation only.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n xdiff/xdiffi.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 0de1ef463bf..3e8aff92bc4 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -22,34 +22,17 @@\n \n #include \"xinclude.h\"\n \n-\n-\n #define XDL_MAX_COST_MIN 256\n #define XDL_HEUR_MIN_COST 256\n #define XDL_LINE_MAX (long)((1UL << (CHAR_BIT * sizeof(long) - 1)) - 1)\n #define XDL_SNAKE_CNT 20\n #define XDL_K_HEUR 4\n \n-\n-\n typedef struct s_xdpsplit {\n \tlong i1, i2;\n \tint min_lo, min_hi;\n } xdpsplit_t;\n \n-\n-\n-\n-static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n-\t\t unsigned long const *ha2, long off2, long lim2,\n-\t\t long *kvdf, long *kvdb, int need_min, xdpsplit_t *spl,\n-\t\t xdalgoenv_t *xenv);\n-static xdchange_t *xdl_add_change(xdchange_t *xscr, long i1, long i2, long chg1, long chg2);\n-\n-\n-\n-\n-\n /*\n * See \"An O(ND) Difference Algorithm and its Variations\", by Eugene Myers.\n * Basically considers a \"box\" (off1, off2, lim1, lim2) and scan from both\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351281","messageId":"20180629001958.85143-5-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 4/9] diff.c: do not pass diff options as keydata to hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:53Z","receivedAt":"2018-06-29T00:20:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When we initialize the hashmap, we give it a pointer to the\ndiff_options, which it then passes along to each call of the\nhashmap_cmp_fn function. There's no need to pass it a second time as\nthe \"keydata\" parameter, and our comparison functions never look at\nkeydata.\n\nThis was a mistake left over from an earlier round of 2e2d5ac184\n(diff.c: color moved lines differently, 2017-06-30), before hashmap\nlearned to pass the data pointer for us.\n\nExplanation-by: Jeff King \nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f9..ce7bedc1b92 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -842,13 +842,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tcase DIFF_SYMBOL_PLUS:\n \t\t\thm = del_lines;\n \t\t\tkey = prepare_entry(o, n);\n-\t\t\tmatch = hashmap_get(hm, key, o);\n+\t\t\tmatch = hashmap_get(hm, key, NULL);\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tcase DIFF_SYMBOL_MINUS:\n \t\t\thm = add_lines;\n \t\t\tkey = prepare_entry(o, n);\n-\t\t\tmatch = hashmap_get(hm, key, o);\n+\t\t\tmatch = hashmap_get(hm, key, NULL);\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tdefault:\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351282","messageId":"20180629001958.85143-6-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 5/9] diff.c: adjust hash function signature to match hashmap expectation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:54Z","receivedAt":"2018-06-29T00:20:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This makes the follow up patch easier.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ce7bedc1b92..d1bae900cdc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -707,11 +707,15 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n-static int moved_entry_cmp(const struct diff_options *diffopt,\n-\t\t\t const struct moved_entry *a,\n-\t\t\t const struct moved_entry *b,\n+static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n+\t\t\t const void *entry,\n+\t\t\t const void *entry_or_key,\n \t\t\t const void *keydata)\n {\n+\tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n+\tconst struct moved_entry *a = entry;\n+\tconst struct moved_entry *b = entry_or_key;\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@@ -5534,10 +5538,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\tif (o->color_moved) {\n \t\t\tstruct hashmap add_lines, del_lines;\n \n-\t\t\thashmap_init(&del_lines,\n-\t\t\t\t (hashmap_cmp_fn)moved_entry_cmp, o, 0);\n-\t\t\thashmap_init(&add_lines,\n-\t\t\t\t (hashmap_cmp_fn)moved_entry_cmp, o, 0);\n+\t\t\thashmap_init(&del_lines, moved_entry_cmp, o, 0);\n+\t\t\thashmap_init(&add_lines, moved_entry_cmp, o, 0);\n \n \t\t\tadd_lines_to_move_detection(o, &add_lines, &del_lines);\n \t\t\tmark_color_as_moved(o, &add_lines, &del_lines);\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351283","messageId":"20180629001958.85143-7-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 6/9] diff.c: add a blocks mode for moved code detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:55Z","receivedAt":"2018-06-29T00:20:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The new \"blocks\" mode provides a middle ground between plain and zebra.\nIt is as intuitive (few colors) as plain, but still has the requirement\nfor a minimum of lines/characters to count a block as moved.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \n (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-options.txt | 8 ++++--\n diff.c | 6 +++--\n diff.h | 5 ++--\n t/t4015-diff-whitespace.sh | 49 ++++++++++++++++++++++++++++++++--\n 4 files changed, 60 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex e3a44f03cdc..ba56169de31 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -276,10 +276,14 @@ plain::\n \tthat are added somewhere else in the diff. This mode picks up any\n \tmoved line, but it is not very useful in a review to determine\n \tif a block of code was moved without permutation.\n-zebra::\n+blocks::\n \tBlocks of moved text of at least 20 alphanumeric characters\n \tare detected greedily. The detected blocks are\n-\tpainted using either the 'color.diff.{old,new}Moved' color or\n+\tpainted using either the 'color.diff.{old,new}Moved' color.\n+\tAdjacent blocks cannot be told apart.\n+zebra::\n+\tBlocks of moved text are detected as in 'blocks' mode. The blocks\n+\tare painted using either the 'color.diff.{old,new}Moved' color or\n \t'color.diff.{old,new}MovedAlternative'. The change between\n \tthe two colors indicates that a new block was detected.\n dimmed_zebra::\ndiff --git a/diff.c b/diff.c\nindex d1bae900cdc..95c51c0b7df 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -271,6 +271,8 @@ static int parse_color_moved(const char *arg)\n \t\treturn COLOR_MOVED_NO;\n \telse if (!strcmp(arg, \"plain\"))\n \t\treturn COLOR_MOVED_PLAIN;\n+\telse if (!strcmp(arg, \"blocks\"))\n+\t\treturn COLOR_MOVED_BLOCKS;\n \telse if (!strcmp(arg, \"zebra\"))\n \t\treturn COLOR_MOVED_ZEBRA;\n \telse if (!strcmp(arg, \"default\"))\n@@ -278,7 +280,7 @@ static int parse_color_moved(const char *arg)\n \telse if (!strcmp(arg, \"dimmed_zebra\"))\n \t\treturn COLOR_MOVED_ZEBRA_DIM;\n \telse\n-\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'\"));\n+\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n }\n \n int git_diff_ui_config(const char *var, const char *value, void *cb)\n@@ -903,7 +905,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \n \t\tblock_length++;\n \n-\t\tif (flipped_block)\n+\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n \t}\n \tadjust_last_block(o, n, block_length);\ndiff --git a/diff.h b/diff.h\nindex d29560f822c..7bd4f182c33 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -208,8 +208,9 @@ struct diff_options {\n \tenum {\n \t\tCOLOR_MOVED_NO = 0,\n \t\tCOLOR_MOVED_PLAIN = 1,\n-\t\tCOLOR_MOVED_ZEBRA = 2,\n-\t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n+\t\tCOLOR_MOVED_BLOCKS = 2,\n+\t\tCOLOR_MOVED_ZEBRA = 3,\n+\t\tCOLOR_MOVED_ZEBRA_DIM = 4,\n \t} color_moved;\n \t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex ddbc3901385..e54529f026d 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n+test_expect_success 'detect blocks of moved code' '\n \tgit reset --hard &&\n \tcat <<-\\EOF >lines.txt &&\n \t\tlong line 1\n@@ -1271,6 +1271,50 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n \ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n+\tgit diff HEAD --no-renames --color-moved=blocks --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,16 +1,16 @@\n+\t-long line 1\n+\t-long line 2\n+\t-long line 3\n+\t line 4\n+\t line 5\n+\t line 6\n+\t line 7\n+\t line 8\n+\t line 9\n+\t+long line 1\n+\t+long line 2\n+\t+long line 3\n+\t+long line 14\n+\t+long line 15\n+\t+long line 16\n+\t line 10\n+\t line 11\n+\t line 12\n+\t line 13\n+\t-long line 14\n+\t-long line 15\n+\t-long line 16\n+\tEOF\n+\ttest_cmp expected actual\n+\n+'\n+\n+test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n+\t# reuse setup from test before!\n+\ttest_config color.diff.oldMoved \"magenta\" &&\n+\ttest_config color.diff.newMoved \"cyan\" &&\n+\ttest_config color.diff.oldMovedAlternative \"blue\" &&\n+\ttest_config color.diff.newMovedAlternative \"yellow\" &&\n+\ttest_config color.diff.oldMovedDimmed \"normal magenta\" &&\n+\ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n+\ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n+\ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n \tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color >actual.raw &&\n \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1669,7 +1713,8 @@ test_expect_success '--color-moved treats adjacent blocks as separate for MIN_AL\n \t7charsA\n \tEOF\n \n-\tgit diff HEAD --color-moved=zebra --color --no-renames | grep -v \"index\" | test_decode_color >actual &&\n+\tgit diff HEAD --color-moved=zebra --color --no-renames >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat >expected <<-\\EOF &&\n \tdiff --git a/bar b/bar\n \t--- a/bar\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351284","messageId":"20180629001958.85143-8-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 7/9] diff.c: decouple white space treatment from move detection algorithm","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:56Z","receivedAt":"2018-06-29T00:20:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In the original implementation of the move detection logic the choice for\nignoring white space changes is the same for the move detection as it is\nfor the regular diff. Some cases came up where different treatment would\nhave been nice.\n\nAllow the user to specify that white space should be ignored differently\nduring detection of moved lines than during generation of added and removed\nlines. This is done by providing analogs to the --ignore-space-at-eol,\n-b, and -w options by introducing the option --color-moved-ws=\nwith the modes named \"ignore-space-at-eol\", \"ignore-space-change\" and\n\"ignore-all-space\", which is used only during the move detection phase.\n\nAs we change the default, we'll adjust the tests.\n\nFor now we do not infer any options to treat white spaces in the move\ndetection from the generic white space options given to diff.\nThis can be tuned later to reasonable default.\n\nAs we plan on adding more white space related options in a later patch,\nthat interferes with the current white space options, use a flag field\nand clamp it down to XDF_WHITESPACE_FLAGS, as that (a) allows to easily\ncheck at parse time if we give invalid combinations and (b) can reuse\nparts of this patch.\n\nBy having the white space treatment in its own option, we'll also\nmake it easier for a later patch to have an config option for\nspaces in the move detection.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-options.txt | 17 +++++++++\n diff.c | 39 +++++++++++++++++++--\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 64 +++++++++++++++++++++++++++++++---\n 4 files changed, 115 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex ba56169de31..80e29e39854 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -292,6 +292,23 @@ dimmed_zebra::\n \tblocks are considered interesting, the rest is uninteresting.\n --\n \n+--color-moved-ws=::\n+\tThis configures how white spaces are ignored when performing the\n+\tmove detection for `--color-moved`. These modes can be given\n+\tas a comma separated list:\n++\n+--\n+ignore-space-at-eol::\n+\tIgnore changes in whitespace at EOL.\n+ignore-space-change::\n+\tIgnore changes in amount of whitespace. This ignores whitespace\n+\tat line end, and considers all other sequences of one or\n+\tmore whitespace characters to be equivalent.\n+ignore-all-space::\n+\tIgnore whitespace when comparing lines. This ignores differences\n+\teven if one line has whitespace where the other line has none.\n+--\n+\n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\n \tBy default, words are delimited by whitespace; see\ndiff --git a/diff.c b/diff.c\nindex 95c51c0b7df..70eeb40c5fd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -283,6 +283,36 @@ static int parse_color_moved(const char *arg)\n \t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n }\n \n+static int parse_color_moved_ws(const char *arg)\n+{\n+\tint ret = 0;\n+\tstruct string_list l = STRING_LIST_INIT_DUP;\n+\tstruct string_list_item *i;\n+\n+\tstring_list_split(&l, arg, ',', -1);\n+\n+\tfor_each_string_list_item(i, &l) {\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sb, i->string);\n+\t\tstrbuf_trim(&sb);\n+\n+\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n+\t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n+\t\t\tret |= XDF_IGNORE_WHITESPACE;\n+\t\telse\n+\t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n+\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+\tstring_list_clear(&l, 0);\n+\n+\treturn ret;\n+}\n+\n int git_diff_ui_config(const char *var, const char *value, void *cb)\n {\n \tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n@@ -717,10 +747,12 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n \tconst struct moved_entry *a = entry;\n \tconst struct moved_entry *b = entry_or_key;\n+\tunsigned flags = diffopt->color_moved_ws_handling\n+\t\t\t & XDF_WHITESPACE_FLAGS;\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 flags);\n }\n \n static struct moved_entry *prepare_entry(struct diff_options *o,\n@@ -728,8 +760,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n {\n \tstruct moved_entry *ret = xmalloc(sizeof(*ret));\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];\n+\tunsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;\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, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n \n@@ -4710,6 +4743,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n+\t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n+\t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\ndiff --git a/diff.h b/diff.h\nindex 7bd4f182c33..de5dc680051 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -214,6 +214,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_ws_handling;\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 e54529f026d..0c737a47cf8 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1460,7 +1460,8 @@ 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 >actual.raw &&\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-ws=ignore-all-space >actual.raw &&\n \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n@@ -1522,7 +1523,8 @@ 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 >actual.raw &&\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-ws=ignore-space-change >actual.raw &&\n \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n@@ -1587,7 +1589,8 @@ 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 >actual.raw &&\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-ws=ignore-space-at-eol >actual.raw &&\n \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n@@ -1757,7 +1760,60 @@ 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 'only move detection ignores white spaces' '\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\t\ta long line to exceed per-line minimum\n+\t\tanother long line to exceed per-line minimum\n+\t\toriginal file\n+\tEOF\n+\tgit add text.txt &&\n+\tgit commit -m \"add text\" &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\t\tQa long line to exceed per-line minimum\n+\t\tQanother long line to exceed per-line minimum\n+\t\tnew file\n+\tEOF\n+\n+\t# Make sure we get a different diff using -w\n+\tgit diff --color --color-moved -w |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,3 +1,3 @@\n+\t Qa long line to exceed per-line minimum\n+\t Qanother long line to exceed per-line minimum\n+\t-original file\n+\t+new file\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-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,3 +1,3 @@\n+\t-a long line to exceed per-line minimum\n+\t-another long line to exceed per-line minimum\n+\t-original file\n+\t+Qa long line to exceed per-line minimum\n+\t+Qanother long line to exceed per-line minimum\n+\t+new file\n+\tEOF\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351285","messageId":"20180629001958.85143-4-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 3/9] t4015: avoid git as a pipe input","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:52Z","receivedAt":"2018-06-29T00:20:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In t4015 we have a pattern of\n\n git diff [] |\n grep -v \"index\" |\n test_decode_color >actual &&\n\nto produce output that we want to test against. This pattern was introduced\nin 86b452e2769 (diff.c: add dimming to moved line detection, 2017-06-30)\nas then the focus on getting the colors right. However the pattern used\nis not best practice as we do care about the exit code of Git. So let's\nnot have Git as the upstream of a pipe. Piping the output of grep to\nsome function is fine as we assume grep to be un-flawed in our test suite.\n\nSigned-off-by: Stefan Beller \n---\n t/t4015-diff-whitespace.sh | 50 +++++++++++++++-----------------------\n 1 file changed, 20 insertions(+), 30 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 17df491a3ab..ddbc3901385 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1271,9 +1271,8 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n \ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n-\tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --color-moved=dimmed_zebra --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1315,9 +1314,8 @@ test_expect_success 'cmd option assumes configured colored-moved' '\n \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n \ttest_config diff.colorMoved zebra &&\n-\tgit diff HEAD --no-renames --color-moved --color |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1395,9 +1393,8 @@ 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-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1419,9 +1416,8 @@ 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-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames -w --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1459,9 +1455,8 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \tline 5\n \tEOF\n \n-\tgit diff HEAD --no-renames --color-moved --color |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1483,9 +1478,8 @@ 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-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames -b --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1526,9 +1520,8 @@ 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-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1550,9 +1543,8 @@ 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-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --no-renames --ignore-space-at-eol --color-moved --color >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n \tdiff --git a/lines.txt b/lines.txt\n \t--- a/lines.txt\n@@ -1597,9 +1589,8 @@ test_expect_success '--color-moved block at end of diff output respects MIN_ALNU\n \tirrelevant_line\n \tEOF\n \n-\tgit diff HEAD --color-moved=zebra --color --no-renames |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --color-moved=zebra --color --no-renames >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat >expected <<-\\EOF &&\n \tdiff --git a/bar b/bar\n \t--- a/bar\n@@ -1636,9 +1627,8 @@ test_expect_success '--color-moved respects MIN_ALNUM_COUNT' '\n \tnineteen chars 456789\n \tEOF\n \n-\tgit diff HEAD --color-moved=zebra --color --no-renames |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff HEAD --color-moved=zebra --color --no-renames >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tcat >expected <<-\\EOF &&\n \tdiff --git a/bar b/bar\n \t--- a/bar\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351286","messageId":"20180629001958.85143-9-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 8/9] diff.c: factor advance_or_nullify out of mark_color_as_moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:57Z","receivedAt":"2018-06-29T00:20:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This moves the part of code that checks if we're still in a block\ninto its own function. We'll need a different approach on advancing\nthe blocks in a later patch, so having it as a separate function will\nprove useful.\n\nWhile at it rename the variable `p` to `prev` to indicate that it refers\nto the previous line. This is as pmb[i] was assigned in the last iteration\nof the outmost for loop.\n\nFurther rename `pnext` to `cur` to indicate that this should match up with\nthe current line of the outmost for loop.\n\nAlso replace the advancement of pmb[i] to reuse `cur` instead of\nusing `p->next` (which is how the name for pnext could be explained.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n diff.c | 32 ++++++++++++++++++++------------\n 1 file changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 70eeb40c5fd..4963819e530 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -801,6 +801,25 @@ static void add_lines_to_move_detection(struct diff_options *o,\n \t}\n }\n \n+static void pmb_advance_or_null(struct diff_options *o,\n+\t\t\t\tstruct moved_entry *match,\n+\t\t\t\tstruct hashmap *hm,\n+\t\t\t\tstruct moved_entry **pmb,\n+\t\t\t\tint pmb_nr)\n+{\n+\tint i;\n+\tfor (i = 0; i < pmb_nr; i++) {\n+\t\tstruct moved_entry *prev = pmb[i];\n+\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n+\t\t\t\tprev->next_line : NULL;\n+\t\tif (cur && !hm->cmpfn(o, cur, match, NULL)) {\n+\t\t\tpmb[i] = cur;\n+\t\t} else {\n+\t\t\tpmb[i] = NULL;\n+\t\t}\n+\t}\n+}\n+\n static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \t\t\t\t\t int pmb_nr)\n {\n@@ -875,7 +894,6 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tstruct moved_entry *key;\n \t\tstruct moved_entry *match = NULL;\n \t\tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\t\tint i;\n \n \t\tswitch (l->s) {\n \t\tcase DIFF_SYMBOL_PLUS:\n@@ -906,17 +924,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\n \n-\t\t/* Check any potential block runs, advance each or nullify */\n-\t\tfor (i = 0; i < pmb_nr; i++) {\n-\t\t\tstruct moved_entry *p = pmb[i];\n-\t\t\tstruct moved_entry *pnext = (p && p->next_line) ?\n-\t\t\t\t\tp->next_line : NULL;\n-\t\t\tif (pnext && !hm->cmpfn(o, pnext, match, NULL)) {\n-\t\t\t\tpmb[i] = p->next_line;\n-\t\t\t} else {\n-\t\t\t\tpmb[i] = NULL;\n-\t\t\t}\n-\t\t}\n+\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n \n \t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n \n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351287","messageId":"20180629001958.85143-10-sbeller@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-1-sbeller@google.com","subject":"[PATCH v4 9/9] diff.c: add white space mode to move detection that allows indent changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-06-29T00:19:58Z","receivedAt":"2018-06-29T00:20:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The option of --color-moved has proven to be useful as observed on the\nmailing list. However when refactoring sometimes the indentation changes,\nfor example when partitioning a functions into smaller helper functions\nthe code usually mostly moved around except for a decrease in indentation.\n\nTo just review the moved code ignoring the change in indentation, a mode\nto ignore spaces in the move detection as implemented in a previous patch\nwould be enough. However the whole move coloring as motivated in commit\n2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought\nup the notion of the reviewer being able to trust the move of a \"block\".\n\nAs there are languages such as python, which depend on proper relative\nindentation for the control flow of the program, ignoring any white space\nchange in a block would not uphold the promises of 2e2d5ac that allows\nreviewers to pay less attention to the inside of a block, as inside\nthe reviewer wants to assume the same program flow.\n\nThis new mode of white space ignorance will take this into account and will\nonly allow the same white space changes per line in each block. This patch\neven allows only for the same change at the beginning of the lines.\n\nAs this is a white space mode, it is made exclusive to other white space\nmodes in the move detection.\n\nThis patch brings some challenges, related to the detection of blocks.\nWe need a white net the catch the possible moved lines, but then need to\nnarrow down to check if the blocks are still in tact. Consider this\nexample (ignoring block sizes):\n\n - A\n - B\n - C\n + A\n + B\n + C\n\nAt the beginning of a block when checking if there is a counterpart\nfor A, we have to ignore all space changes. However at the following\nlines we have to check if the indent change stayed the same.\n\nChecking if the indentation change did stay the same, is done by computing\nthe indentation change by the difference in line length, and then assume\nthe change is only in the beginning of the longer line, the common tail\nis the same. That is why the test contains lines like:\n\n - A\n ...\n + A \n ...\n\nAs the first line starting a block is caught using a compare function that\nignores white spaces unlike the rest of the block, where the white space\ndelta is taken into account for the comparison, we also have to think about\nthe following situation:\n\n - A\n - B\n - A\n - B\n + A\n + B\n + A\n + B\n\nWhen checking if the first A (both in the + and - lines) is a start of\na block, we have to check all 'A' and record all the white space deltas\nsuch that we can find the example above to be just one block that is\nindented.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \n---\n Documentation/diff-options.txt | 5 ++\n diff.c | 158 ++++++++++++++++++++++++++++++++-\n diff.h | 3 +\n t/t4015-diff-whitespace.sh | 98 ++++++++++++++++++--\n 4 files changed, 256 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 80e29e39854..143acd9417e 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -307,6 +307,11 @@ ignore-space-change::\n ignore-all-space::\n \tIgnore whitespace when comparing lines. This ignores differences\n \teven if one line has whitespace where the other line has none.\n+allow-indentation-change::\n+\tInitially ignore any white spaces in the move detection, then\n+\tgroup the moved code blocks only into a block if the change in\n+\twhitespace is the same per line. This is incompatible with the\n+\tother modes.\n --\n \n --word-diff[=]::\ndiff --git a/diff.c b/diff.c\nindex 4963819e530..f51f0ac32f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -302,12 +302,18 @@ static int parse_color_moved_ws(const char *arg)\n \t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n \t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE;\n+\t\telse if (!strcmp(sb.buf, \"allow-indentation-change\"))\n+\t\t\tret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n \t\telse\n \t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n \n \t\tstrbuf_release(&sb);\n \t}\n \n+\tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n+\t (ret & XDF_WHITESPACE_FLAGS))\n+\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n+\n \tstring_list_clear(&l, 0);\n \n \treturn ret;\n@@ -737,7 +743,91 @@ struct moved_entry {\n \tstruct hashmap_entry ent;\n \tconst struct emitted_diff_symbol *es;\n \tstruct moved_entry *next_line;\n+\tstruct ws_delta *wsd;\n+};\n+\n+/**\n+ * The struct ws_delta holds white space differences between moved lines, i.e.\n+ * between '+' and '-' lines that have been detected to be a move.\n+ * The string contains the difference in leading white spaces, before the\n+ * rest of the line is compared using the white space config for move\n+ * coloring. The current_longer indicates if the first string in the\n+ * comparision is longer than the second.\n+ */\n+struct ws_delta {\n+\tchar *string;\n+\tunsigned int current_longer : 1;\n };\n+#define WS_DELTA_INIT { NULL, 0 }\n+\n+static int compute_ws_delta(const struct emitted_diff_symbol *a,\n+\t\t\t const struct emitted_diff_symbol *b,\n+\t\t\t struct ws_delta *out)\n+{\n+\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n+\tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n+\tint d = longer->len - shorter->len;\n+\n+\tout->string = xmemdupz(longer->line, d);\n+\tout->current_longer = (a == longer);\n+\n+\treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n+}\n+\n+static int cmp_in_block_with_wsd(const struct diff_options *o,\n+\t\t\t\t const struct moved_entry *cur,\n+\t\t\t\t const struct moved_entry *match,\n+\t\t\t\t struct moved_entry *pmb,\n+\t\t\t\t int n)\n+{\n+\tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n+\tint al = cur->es->len, cl = l->len;\n+\tconst char *a = cur->es->line,\n+\t\t *b = match->es->line,\n+\t\t *c = l->line;\n+\n+\tint wslen;\n+\n+\t/*\n+\t * We need to check if 'cur' is equal to 'match'.\n+\t * As those are from the same (+/-) side, we do not need to adjust for\n+\t * indent changes. However these were found using fuzzy matching\n+\t * so we do have to check if they are equal.\n+\t */\n+\tif (strcmp(a, b))\n+\t\treturn 1;\n+\n+\tif (!pmb->wsd)\n+\t\t/*\n+\t\t * No white space delta was carried forward? This can happen\n+\t\t * when we exit early in this function and do not carry\n+\t\t * forward ws.\n+\t\t */\n+\t\treturn 1;\n+\n+\t/*\n+\t * The indent changes of the block are known and carried forward in\n+\t * pmb->wsd; however we need to check if the indent changes of the\n+\t * current line are still the same as before.\n+\t *\n+\t * To do so we need to compare 'l' to 'cur', adjusting the\n+\t * one of them for the white spaces, depending which was longer.\n+\t */\n+\n+\twslen = strlen(pmb->wsd->string);\n+\tif (pmb->wsd->current_longer) {\n+\t\tc += wslen;\n+\t\tcl -= wslen;\n+\t} else {\n+\t\ta += wslen;\n+\t\tal -= wslen;\n+\t}\n+\n+\tif (strcmp(a, c))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n \n static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \t\t\t const void *entry,\n@@ -750,6 +840,16 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \tunsigned flags = diffopt->color_moved_ws_handling\n \t\t\t & XDF_WHITESPACE_FLAGS;\n \n+\tif (diffopt->color_moved_ws_handling &\n+\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t/*\n+\t\t * As there is not specific white space config given,\n+\t\t * we'd need to check for a new block, so ignore all\n+\t\t * white space. The setup of the white space\n+\t\t * configuration for the next block is done else where\n+\t\t */\n+\t\tflags |= XDF_IGNORE_WHITESPACE;\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 flags);\n@@ -765,6 +865,7 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n \tret->ent.hash = xdiff_hash_string(l->line, l->len, flags);\n \tret->es = l;\n \tret->next_line = NULL;\n+\tret->wsd = NULL;\n \n \treturn ret;\n }\n@@ -820,6 +921,37 @@ static void pmb_advance_or_null(struct diff_options *o,\n \t}\n }\n \n+static void pmb_advance_or_null_multi_match(struct diff_options *o,\n+\t\t\t\t\t struct moved_entry *match,\n+\t\t\t\t\t struct hashmap *hm,\n+\t\t\t\t\t struct moved_entry **pmb,\n+\t\t\t\t\t int pmb_nr, int n)\n+{\n+\tint i;\n+\tchar *got_match = xcalloc(1, pmb_nr);\n+\n+\tfor (; match; match = hashmap_get_next(hm, match)) {\n+\t\tfor (i = 0; i < pmb_nr; i++) {\n+\t\t\tstruct moved_entry *prev = pmb[i];\n+\t\t\tstruct moved_entry *cur = (prev && prev->next_line) ?\n+\t\t\t\t\tprev->next_line : NULL;\n+\t\t\tif (!cur)\n+\t\t\t\tcontinue;\n+\t\t\tif (!cmp_in_block_with_wsd(o, cur, match, pmb[i], n))\n+\t\t\t\tgot_match[i] |= 1;\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i next_line->wsd = pmb[i]->wsd;\n+\t\t\tpmb[i] = pmb[i]->next_line;\n+\t\t} else\n+\t\t\tpmb[i] = NULL;\n+\t}\n+}\n+\n static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \t\t\t\t\t int pmb_nr)\n {\n@@ -837,6 +969,10 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n+\t\t\tif (pmb[rp]->wsd) {\n+\t\t\t\tfree(pmb[rp]->wsd->string);\n+\t\t\t\tFREE_AND_NULL(pmb[rp]->wsd);\n+\t\t\t}\n \t\t\tpmb[rp] = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n@@ -924,7 +1060,11 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\n \n-\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n+\t\tif (o->color_moved_ws_handling &\n+\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\tpmb_advance_or_null_multi_match(o, match, hm, pmb, pmb_nr, n);\n+\t\telse\n+\t\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n \n \t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n \n@@ -935,7 +1075,17 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t */\n \t\t\tfor (; match; match = hashmap_get_next(hm, match)) {\n \t\t\t\tALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc);\n-\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\tif (o->color_moved_ws_handling &\n+\t\t\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n+\t\t\t\t\tstruct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n+\t\t\t\t\tif (compute_ws_delta(l, match->es, wsd)) {\n+\t\t\t\t\t\tmatch->wsd = wsd;\n+\t\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t\t} else\n+\t\t\t\t\t\tfree(wsd);\n+\t\t\t\t} else {\n+\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tflipped_block = (flipped_block + 1) % 2;\n@@ -5583,6 +5733,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\tif (o->color_moved) {\n \t\t\tstruct hashmap add_lines, del_lines;\n \n+\t\t\tif (o->color_moved_ws_handling &\n+\t\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\t\to->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n+\n \t\t\thashmap_init(&del_lines, moved_entry_cmp, o, 0);\n \t\t\thashmap_init(&add_lines, moved_entry_cmp, o, 0);\n \ndiff --git a/diff.h b/diff.h\nindex de5dc680051..5e6bcf09260 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -214,6 +214,9 @@ 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+\n+\t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n+\t#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)\n \tint color_moved_ws_handling;\n };\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 0c737a47cf8..41facf7abf9 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1781,9 +1781,8 @@ test_expect_success 'only move detection ignores white spaces' '\n \tEOF\n \n \t# Make sure we get a different diff using -w\n-\tgit diff --color --color-moved -w |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\tgit diff --color --color-moved -w >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tq_to_tab <<-\\EOF >expected &&\n \tdiff --git a/text.txt b/text.txt\n \t--- a/text.txt\n@@ -1798,9 +1797,8 @@ test_expect_success 'only move detection ignores white spaces' '\n \n \t# And now ignoring white space only in the move detection\n \tgit diff --color --color-moved \\\n-\t\t--color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n-\t\tgrep -v \"index\" |\n-\t\ttest_decode_color >actual &&\n+\t\t--color-moved-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n \tq_to_tab <<-\\EOF >expected &&\n \tdiff --git a/text.txt b/text.txt\n \t--- a/text.txt\n@@ -1816,4 +1814,92 @@ test_expect_success 'only move detection ignores white spaces' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'compare whitespace delta across moved blocks' '\n+\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\tQIndented\n+\tQText across\n+\tQsome lines\n+\tQBut! <- this stands out\n+\tQAdjusting with\n+\tQQdifferent starting\n+\tQwhite spaces\n+\tQAnother outlier\n+\tQQQIndented\n+\tQQQText across\n+\tQQQfive lines\n+\tQQQthat has similar lines\n+\tQQQto previous blocks, but with different indent\n+\tQQQYetQAnotherQoutlierQ\n+\tEOF\n+\n+\tgit add text.txt &&\n+\tgit commit -m \"add text.txt\" &&\n+\n+\tq_to_tab <<-\\EOF >text.txt &&\n+\tQQIndented\n+\tQQText across\n+\tQQsome lines\n+\tQQQBut! <- this stands out\n+\tAdjusting with\n+\tQdifferent starting\n+\twhite spaces\n+\tAnotherQoutlier\n+\tQQIndented\n+\tQQText across\n+\tQQfive lines\n+\tQQthat has similar lines\n+\tQQto previous blocks, but with different indent\n+\tQQYetQAnotherQoutlier\n+\tEOF\n+\n+\tgit diff --color --color-moved --color-moved-ws=allow-indentation-change >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n+\n+\tq_to_tab <<-\\EOF >expected &&\n+\t\tdiff --git a/text.txt b/text.txt\n+\t\t--- a/text.txt\n+\t\t+++ b/text.txt\n+\t\t@@ -1,14 +1,14 @@\n+\t\t-QIndented\n+\t\t-QText across\n+\t\t-Qsome lines\n+\t\t-QBut! <- this stands out\n+\t\t-QAdjusting with\n+\t\t-QQdifferent starting\n+\t\t-Qwhite spaces\n+\t\t-QAnother outlier\n+\t\t-QQQIndented\n+\t\t-QQQText across\n+\t\t-QQQfive lines\n+\t\t-QQQthat has similar lines\n+\t\t-QQQto previous blocks, but with different indent\n+\t\t-QQQYetQAnotherQoutlierQ\n+\t\t+QQIndented\n+\t\t+QQText across\n+\t\t+QQsome lines\n+\t\t+QQQBut! <- this stands out\n+\t\t+Adjusting with\n+\t\t+Qdifferent starting\n+\t\t+white spaces\n+\t\t+AnotherQoutlier\n+\t\t+QQIndented\n+\t\t+QQText across\n+\t\t+QQfive lines\n+\t\t+QQthat has similar lines\n+\t\t+QQto previous blocks, but with different indent\n+\t\t+QQYetQAnotherQoutlier\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'compare whitespace delta incompatible with other space options' '\n+\ttest_must_fail git diff \\\n+\t\t--color-moved-ws=allow-indentation-change,ignore-all-space \\\n+\t\t2>err &&\n+\ttest_i18ngrep allow-indentation-change err\n+'\n+\n test_done\n-- \n2.18.0.399.gad0ab374a1-goog\n\n"},{"id":"351531","messageId":"20180702171801.GB246956@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-7-sbeller@google.com","subject":"Re: [PATCH v4 6/9] diff.c: add a blocks mode for moved code detection","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-02T17:18:01Z","receivedAt":"2018-07-02T17:18:08Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/28, Stefan Beller wrote:\n> The new \"blocks\" mode provides a middle ground between plain and zebra.\n> It is as intuitive (few colors) as plain, but still has the requirement\n> for a minimum of lines/characters to count a block as moved.\n> \n> Suggested-by: Ævar Arnfjörð Bjarmason \n> (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/diff-options.txt | 8 ++++--\n> diff.c | 6 +++--\n> diff.h | 5 ++--\n> t/t4015-diff-whitespace.sh | 49 ++++++++++++++++++++++++++++++++--\n> 4 files changed, 60 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index e3a44f03cdc..ba56169de31 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -276,10 +276,14 @@ plain::\n> \tthat are added somewhere else in the diff. This mode picks up any\n> \tmoved line, but it is not very useful in a review to determine\n> \tif a block of code was moved without permutation.\n> -zebra::\n> +blocks::\n> \tBlocks of moved text of at least 20 alphanumeric characters\n> \tare detected greedily. The detected blocks are\n> -\tpainted using either the 'color.diff.{old,new}Moved' color or\n> +\tpainted using either the 'color.diff.{old,new}Moved' color.\n> +\tAdjacent blocks cannot be told apart.\n> +zebra::\n> +\tBlocks of moved text are detected as in 'blocks' mode. The blocks\n> +\tare painted using either the 'color.diff.{old,new}Moved' color or\n> \t'color.diff.{old,new}MovedAlternative'. The change between\n> \tthe two colors indicates that a new block was detected.\n> dimmed_zebra::\n> diff --git a/diff.c b/diff.c\n> index d1bae900cdc..95c51c0b7df 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -271,6 +271,8 @@ static int parse_color_moved(const char *arg)\n> \t\treturn COLOR_MOVED_NO;\n> \telse if (!strcmp(arg, \"plain\"))\n> \t\treturn COLOR_MOVED_PLAIN;\n> +\telse if (!strcmp(arg, \"blocks\"))\n> +\t\treturn COLOR_MOVED_BLOCKS;\n> \telse if (!strcmp(arg, \"zebra\"))\n> \t\treturn COLOR_MOVED_ZEBRA;\n> \telse if (!strcmp(arg, \"default\"))\n> @@ -278,7 +280,7 @@ static int parse_color_moved(const char *arg)\n> \telse if (!strcmp(arg, \"dimmed_zebra\"))\n> \t\treturn COLOR_MOVED_ZEBRA_DIM;\n> \telse\n> -\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'\"));\n> +\t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n> }\n> \n> int git_diff_ui_config(const char *var, const char *value, void *cb)\n> @@ -903,7 +905,7 @@ static void mark_color_as_moved(struct diff_options *o,\n> \n> \t\tblock_length++;\n> \n> -\t\tif (flipped_block)\n> +\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n> \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n> \t}\n> \tadjust_last_block(o, n, block_length);\n> diff --git a/diff.h b/diff.h\n> index d29560f822c..7bd4f182c33 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -208,8 +208,9 @@ struct diff_options {\n> \tenum {\n> \t\tCOLOR_MOVED_NO = 0,\n> \t\tCOLOR_MOVED_PLAIN = 1,\n> -\t\tCOLOR_MOVED_ZEBRA = 2,\n> -\t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n> +\t\tCOLOR_MOVED_BLOCKS = 2,\n> +\t\tCOLOR_MOVED_ZEBRA = 3,\n> +\t\tCOLOR_MOVED_ZEBRA_DIM = 4,\n> \t} color_moved;\n> \t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n> \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index ddbc3901385..e54529f026d 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '\n> \ttest_cmp expected actual\n> '\n> \n> -test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n> +test_expect_success 'detect blocks of moved code' '\n> \tgit reset --hard &&\n> \tcat <<-\\EOF >lines.txt &&\n> \t\tlong line 1\n> @@ -1271,6 +1271,50 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n> \ttest_config color.diff.newMovedDimmed \"normal cyan\" &&\n> \ttest_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n> \ttest_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n> +\tgit diff HEAD --no-renames --color-moved=blocks --color >actual.raw &&\n> +\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n> +\tcat <<-\\EOF >expected &&\n> +\tdiff --git a/lines.txt b/lines.txt\n> +\t--- a/lines.txt\n> +\t+++ b/lines.txt\n> +\t@@ -1,16 +1,16 @@\n> +\t-long line 1\n> +\t-long line 2\n> +\t-long line 3\n> +\t line 4\n> +\t line 5\n> +\t line 6\n> +\t line 7\n> +\t line 8\n> +\t line 9\n> +\t+long line 1\n> +\t+long line 2\n> +\t+long line 3\n> +\t+long line 14\n> +\t+long line 15\n> +\t+long line 16\n> +\t line 10\n> +\t line 11\n> +\t line 12\n> +\t line 13\n> +\t-long line 14\n> +\t-long line 15\n> +\t-long line 16\n> +\tEOF\n> +\ttest_cmp expected actual\n> +\n\nTheres an empty line here. Not worth fixing if its the only issue\nthough.\n\n-- \nBrandon Williams\n"},{"id":"351532","messageId":"20180702172224.GC246956@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-8-sbeller@google.com","subject":"Re: [PATCH v4 7/9] diff.c: decouple white space treatment from move detection algorithm","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-02T17:22:24Z","receivedAt":"2018-07-02T17:22:30Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/28, Stefan Beller wrote:\n> In the original implementation of the move detection logic the choice for\n> ignoring white space changes is the same for the move detection as it is\n> for the regular diff. Some cases came up where different treatment would\n> have been nice.\n> \n> Allow the user to specify that white space should be ignored differently\n> during detection of moved lines than during generation of added and removed\n> lines. This is done by providing analogs to the --ignore-space-at-eol,\n> -b, and -w options by introducing the option --color-moved-ws=\n> with the modes named \"ignore-space-at-eol\", \"ignore-space-change\" and\n> \"ignore-all-space\", which is used only during the move detection phase.\n> \n> As we change the default, we'll adjust the tests.\n> \n> For now we do not infer any options to treat white spaces in the move\n> detection from the generic white space options given to diff.\n> This can be tuned later to reasonable default.\n> \n> As we plan on adding more white space related options in a later patch,\n> that interferes with the current white space options, use a flag field\n> and clamp it down to XDF_WHITESPACE_FLAGS, as that (a) allows to easily\n> check at parse time if we give invalid combinations and (b) can reuse\n> parts of this patch.\n> \n> By having the white space treatment in its own option, we'll also\n> make it easier for a later patch to have an config option for\n> spaces in the move detection.\n> \n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> Documentation/diff-options.txt | 17 +++++++++\n> diff.c | 39 +++++++++++++++++++--\n> diff.h | 1 +\n> t/t4015-diff-whitespace.sh | 64 +++++++++++++++++++++++++++++++---\n> 4 files changed, 115 insertions(+), 6 deletions(-)\n> \n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index ba56169de31..80e29e39854 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -292,6 +292,23 @@ dimmed_zebra::\n> \tblocks are considered interesting, the rest is uninteresting.\n> --\n> \n> +--color-moved-ws=::\n> +\tThis configures how white spaces are ignored when performing the\n> +\tmove detection for `--color-moved`. These modes can be given\n> +\tas a comma separated list:\n> ++\n> +--\n> +ignore-space-at-eol::\n> +\tIgnore changes in whitespace at EOL.\n> +ignore-space-change::\n> +\tIgnore changes in amount of whitespace. This ignores whitespace\n> +\tat line end, and considers all other sequences of one or\n> +\tmore whitespace characters to be equivalent.\n> +ignore-all-space::\n> +\tIgnore whitespace when comparing lines. This ignores differences\n> +\teven if one line has whitespace where the other line has none.\n> +--\n> +\n> --word-diff[=]::\n> \tShow a word diff, using the to delimit changed words.\n> \tBy default, words are delimited by whitespace; see\n> diff --git a/diff.c b/diff.c\n> index 95c51c0b7df..70eeb40c5fd 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -283,6 +283,36 @@ static int parse_color_moved(const char *arg)\n> \t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'\"));\n> }\n> \n> +static int parse_color_moved_ws(const char *arg)\n> +{\n> +\tint ret = 0;\n> +\tstruct string_list l = STRING_LIST_INIT_DUP;\n> +\tstruct string_list_item *i;\n> +\n> +\tstring_list_split(&l, arg, ',', -1);\n> +\n> +\tfor_each_string_list_item(i, &l) {\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\tstrbuf_addstr(&sb, i->string);\n> +\t\tstrbuf_trim(&sb);\n> +\n> +\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n> +\t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n> +\t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n> +\t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n> +\t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n> +\t\t\tret |= XDF_IGNORE_WHITESPACE;\n> +\t\telse\n> +\t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n> +\n> +\t\tstrbuf_release(&sb);\n> +\t}\n> +\n> +\tstring_list_clear(&l, 0);\n> +\n> +\treturn ret;\n> +}\n> +\n> int git_diff_ui_config(const char *var, const char *value, void *cb)\n> {\n> \tif (!strcmp(var, \"diff.color\") || !strcmp(var, \"color.diff\")) {\n> @@ -717,10 +747,12 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n> \tconst struct diff_options *diffopt = hashmap_cmp_fn_data;\n> \tconst struct moved_entry *a = entry;\n> \tconst struct moved_entry *b = entry_or_key;\n> +\tunsigned flags = diffopt->color_moved_ws_handling\n> +\t\t\t & XDF_WHITESPACE_FLAGS;\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 flags);\n> }\n> \n> static struct moved_entry *prepare_entry(struct diff_options *o,\n> @@ -728,8 +760,9 @@ static struct moved_entry *prepare_entry(struct diff_options *o,\n> {\n> \tstruct moved_entry *ret = xmalloc(sizeof(*ret));\n> \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[line_no];\n> +\tunsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;\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, flags);\n> \tret->es = l;\n> \tret->next_line = NULL;\n> \n> @@ -4710,6 +4743,8 @@ int diff_opt_parse(struct diff_options *options,\n> \t\tif (cm < 0)\n> \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n> \t\toptions->color_moved = cm;\n> +\t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n> +\t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n> \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n> \t\toptions->use_color = 1;\n> \t\toptions->word_diff = DIFF_WORDS_COLOR;\n> diff --git a/diff.h b/diff.h\n> index 7bd4f182c33..de5dc680051 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -214,6 +214,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_ws_handling;\n> };\n> \n> void diff_emit_submodule_del(struct diff_options *o, const char *line);\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index e54529f026d..0c737a47cf8 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1460,7 +1460,8 @@ 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 >actual.raw &&\n> +\tgit diff HEAD --no-renames --color-moved --color \\\n> +\t\t--color-moved-ws=ignore-all-space >actual.raw &&\n> \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n> \tcat <<-\\EOF >expected &&\n> \tdiff --git a/lines.txt b/lines.txt\n> @@ -1522,7 +1523,8 @@ 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 >actual.raw &&\n> +\tgit diff HEAD --no-renames --color-moved --color \\\n> +\t\t--color-moved-ws=ignore-space-change >actual.raw &&\n> \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n> \tcat <<-\\EOF >expected &&\n> \tdiff --git a/lines.txt b/lines.txt\n> @@ -1587,7 +1589,8 @@ 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 >actual.raw &&\n> +\tgit diff HEAD --no-renames --color-moved --color \\\n> +\t\t--color-moved-ws=ignore-space-at-eol >actual.raw &&\n> \tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n> \tcat <<-\\EOF >expected &&\n> \tdiff --git a/lines.txt b/lines.txt\n> @@ -1757,7 +1760,60 @@ 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\nMaybe you want to use a test_when_finished call for this instead of\ndoing this at the end of the test? I guess this comes up as a point of\ndebate: Do you have each test clean up after itself or do you ensure\nthat subsequent tests makes sure its env is ready before testing.\nAnyway this is just a suggestion.\n\n> +'\n> +\n> +test_expect_success 'only move detection ignores white spaces' '\n> +\tgit reset --hard &&\n> +\tq_to_tab <<-\\EOF >text.txt &&\n> +\t\ta long line to exceed per-line minimum\n> +\t\tanother long line to exceed per-line minimum\n> +\t\toriginal file\n> +\tEOF\n> +\tgit add text.txt &&\n> +\tgit commit -m \"add text\" &&\n> +\tq_to_tab <<-\\EOF >text.txt &&\n> +\t\tQa long line to exceed per-line minimum\n> +\t\tQanother long line to exceed per-line minimum\n> +\t\tnew file\n> +\tEOF\n> +\n> +\t# Make sure we get a different diff using -w\n> +\tgit diff --color --color-moved -w |\n> +\t\tgrep -v \"index\" |\n> +\t\ttest_decode_color >actual &&\n> +\tq_to_tab <<-\\EOF >expected &&\n> +\tdiff --git a/text.txt b/text.txt\n> +\t--- a/text.txt\n> +\t+++ b/text.txt\n> +\t@@ -1,3 +1,3 @@\n> +\t Qa long line to exceed per-line minimum\n> +\t Qanother long line to exceed per-line minimum\n> +\t-original file\n> +\t+new file\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-ws=ignore-all-space,ignore-space-change,ignore-space-at-eol |\n> +\t\tgrep -v \"index\" |\n> +\t\ttest_decode_color >actual &&\n> +\tq_to_tab <<-\\EOF >expected &&\n> +\tdiff --git a/text.txt b/text.txt\n> +\t--- a/text.txt\n> +\t+++ b/text.txt\n> +\t@@ -1,3 +1,3 @@\n> +\t-a long line to exceed per-line minimum\n> +\t-another long line to exceed per-line minimum\n> +\t-original file\n> +\t+Qa long line to exceed per-line minimum\n> +\t+Qanother long line to exceed per-line minimum\n> +\t+new file\n> +\tEOF\n> +\ttest_cmp expected actual\n> '\n> \n> test_done\n> -- \n> 2.18.0.399.gad0ab374a1-goog\n> \n\n-- \nBrandon Williams\n"},{"id":"351534","messageId":"20180702173614.GD246956@google.com","threadId":"48758","inReplyTo":"20180629001958.85143-10-sbeller@google.com","subject":"Re: [PATCH v4 9/9] diff.c: add white space mode to move detection that allows indent changes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-07-02T17:36:14Z","receivedAt":"2018-07-02T17:36:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 06/28, Stefan Beller wrote:\n> The option of --color-moved has proven to be useful as observed on the\n> mailing list. However when refactoring sometimes the indentation changes,\n> for example when partitioning a functions into smaller helper functions\n> the code usually mostly moved around except for a decrease in indentation.\n> \n> To just review the moved code ignoring the change in indentation, a mode\n> to ignore spaces in the move detection as implemented in a previous patch\n> would be enough. However the whole move coloring as motivated in commit\n> 2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought\n> up the notion of the reviewer being able to trust the move of a \"block\".\n> \n> As there are languages such as python, which depend on proper relative\n> indentation for the control flow of the program, ignoring any white space\n> change in a block would not uphold the promises of 2e2d5ac that allows\n> reviewers to pay less attention to the inside of a block, as inside\n> the reviewer wants to assume the same program flow.\n> \n> This new mode of white space ignorance will take this into account and will\n> only allow the same white space changes per line in each block. This patch\n> even allows only for the same change at the beginning of the lines.\n> \n> As this is a white space mode, it is made exclusive to other white space\n> modes in the move detection.\n> \n> This patch brings some challenges, related to the detection of blocks.\n> We need a white net the catch the possible moved lines, but then need to\n\ns/white/wide/\n\n> +\n> +/**\n> + * The struct ws_delta holds white space differences between moved lines, i.e.\n> + * between '+' and '-' lines that have been detected to be a move.\n> + * The string contains the difference in leading white spaces, before the\n> + * rest of the line is compared using the white space config for move\n> + * coloring. The current_longer indicates if the first string in the\n> + * comparision is longer than the second.\n> + */\n> +struct ws_delta {\n> +\tchar *string;\n> +\tunsigned int current_longer : 1;\n> };\n> +#define WS_DELTA_INIT { NULL, 0 }\n> +\n> +static int compute_ws_delta(const struct emitted_diff_symbol *a,\n> +\t\t\t const struct emitted_diff_symbol *b,\n> +\t\t\t struct ws_delta *out)\n> +{\n> +\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n> +\tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n> +\tint d = longer->len - shorter->len;\n> +\n> +\tout->string = xmemdupz(longer->line, d);\n> +\tout->current_longer = (a == longer);\n> +\n> +\treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n> +}\n\nI'm having a harder time understanding this block. This is used to fill\na ws_delta struct by calculating the whitespace delta between two lines.\nIf that is the case then why doesn't this function verify that the first\n'd' characters in the longer line are indeed whitespace? Also, maybe\nthis is just because I'm not as familiar with the move detection code,\nbut how would the whitespace detection handle a line being moved from\nbeing indented with tabs to spaces or vice versa? Is this something\nalready handled and not an issue?\n\n-- \nBrandon Williams\n"},{"id":"351543","messageId":"CAGZ79kasAqE+=7ciVrdjoRdu0UFjVBr8Ma502nw+3hZL=ebXYQ@mail.gmail.com","threadId":"48758","inReplyTo":"20180702173614.GD246956@google.com","subject":"Re: [PATCH v4 9/9] diff.c: add white space mode to move detection that allows indent changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-07-02T18:59:06Z","receivedAt":"2018-07-02T19:07:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 2, 2018 at 10:36 AM Brandon Williams wrote:\n>\n> On 06/28, Stefan Beller wrote:\n> > The option of --color-moved has proven to be useful as observed on the\n> > mailing list. However when refactoring sometimes the indentation changes,\n> > for example when partitioning a functions into smaller helper functions\n> > the code usually mostly moved around except for a decrease in indentation.\n> >\n> > To just review the moved code ignoring the change in indentation, a mode\n> > to ignore spaces in the move detection as implemented in a previous patch\n> > would be enough. However the whole move coloring as motivated in commit\n> > 2e2d5ac (diff.c: color moved lines differently, 2017-06-30), brought\n> > up the notion of the reviewer being able to trust the move of a \"block\".\n> >\n> > As there are languages such as python, which depend on proper relative\n> > indentation for the control flow of the program, ignoring any white space\n> > change in a block would not uphold the promises of 2e2d5ac that allows\n> > reviewers to pay less attention to the inside of a block, as inside\n> > the reviewer wants to assume the same program flow.\n> >\n> > This new mode of white space ignorance will take this into account and will\n> > only allow the same white space changes per line in each block. This patch\n> > even allows only for the same change at the beginning of the lines.\n> >\n> > As this is a white space mode, it is made exclusive to other white space\n> > modes in the move detection.\n> >\n> > This patch brings some challenges, related to the detection of blocks.\n> > We need a white net the catch the possible moved lines, but then need to\n>\n> s/white/wide/\n\noops. will fix.\n\n>\n> > +\n> > +/**\n> > + * The struct ws_delta holds white space differences between moved lines, i.e.\n> > + * between '+' and '-' lines that have been detected to be a move.\n> > + * The string contains the difference in leading white spaces, before the\n> > + * rest of the line is compared using the white space config for move\n> > + * coloring. The current_longer indicates if the first string in the\n> > + * comparision is longer than the second.\n> > + */\n> > +struct ws_delta {\n> > + char *string;\n> > + unsigned int current_longer : 1;\n> > };\n> > +#define WS_DELTA_INIT { NULL, 0 }\n> > +\n> > +static int compute_ws_delta(const struct emitted_diff_symbol *a,\n> > + const struct emitted_diff_symbol *b,\n> > + struct ws_delta *out)\n> > +{\n> > + const struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n> > + const struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n> > + int d = longer->len - shorter->len;\n> > +\n> > + out->string = xmemdupz(longer->line, d);\n> > + out->current_longer = (a == longer);\n> > +\n> > + return !strncmp(longer->line + d, shorter->line, shorter->len);\n> > +}\n>\n> I'm having a harder time understanding this block. This is used to fill\n> a ws_delta struct by calculating the whitespace delta between two lines.\n\nyes.\n\n> If that is the case then why doesn't this function verify that the first\n> 'd' characters in the longer line are indeed whitespace?\n\nThis was done implicitly before as compute_ws_delta is called only\non two lines that are \"equal with XDF_IGNORE_WHITESPACE\".\n\n> Also, maybe\n> this is just because I'm not as familiar with the move detection code,\n> but how would the whitespace detection handle a line being moved from\n> being indented with tabs to spaces or vice versa? Is this something\n> already handled and not an issue?\n\nThe ws_delta stores the string (of whitespaces) that the 'shorter string'\nneeds to be prefixed with to create the 'longer string.\n(I chose 'shorter' and 'longer' as any of them can be '+' or '-' lines)\n\nThen later when we compare the strings (the current line in\nconsideration and strings that we put in hashmaps that we\nknow are moved lines) in cmp_in_block_with_wsd,\nwe (should) take this white space delta into account.\n\nI just realize that there we do not check for the tab/space replacement\nas there we would need to compare the ws_delta string to the beginning\nof the 'longer' string there.\n\nI'll add a test for the space/tab replacement as well.\n\nThanks,\nStefan\n"}]}