{"thread":{"id":"48354","subject":"[PATCHv2 0/7] Moved code detection: ignore space on uniform indentation","startedAt":"2018-04-24T21:03:36Z","lastAt":"2018-04-25T00:11:29Z","messageCount":18,"participants":["Stefan Beller","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"345677","messageId":"20180424210330.87861-1-sbeller@google.com","threadId":"48354","inReplyTo":null,"subject":"[PATCHv2 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:23Z","receivedAt":"2018-04-24T21:03:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\nI think I have addressed Jonathans feedback\n* by using a string instead of counting the first character only.\n* refined tests slightly (easier to read)\n* moved white space handling for moved blocks into its own flag field,\n keeping the enum for the actual mode of move detection.\n\nv1:\nThis is a re-attempt of [1], which allows the moved code detection to\nignore blanks in various modes.\n\npatches 1-5 are refactoring, patch 6 adds all existing white space options\nof regular diff to the move detection. (I am unsure about this patch,\nas I presume we want to keep the option space at a minimum if possible).\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\n[1] https://public-inbox.org/git/20171025224620.27657-1-sbeller@google.com/\n\nStefan Beller (7):\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: add --color-moved-ignore-space-delta option\n\n Documentation/diff-options.txt | 25 ++++-\n diff.c | 128 ++++++++++++++++++----\n diff.h | 8 +-\n t/t4015-diff-whitespace.sh | 192 +++++++++++++++++++++++++++++++--\n xdiff/xdiff.h | 8 --\n xdiff/xdiffi.c | 17 ---\n 6 files changed, 322 insertions(+), 56 deletions(-)\n\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345678","messageId":"20180424210330.87861-2-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 1/7] xdiff/xdiff.h: remove unused flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:24Z","receivedAt":"2018-04-24T21:03:39Z","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 \n---\n xdiff/xdiff.h | 8 --------\n 1 file changed, 8 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex c1937a2911..2356da5f78 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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345679","messageId":"20180424210330.87861-3-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 2/7] xdiff/xdiffi.c: remove unneeded function declarations","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:25Z","receivedAt":"2018-04-24T21:03:43Z","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 \n---\n xdiff/xdiffi.c | 17 -----------------\n 1 file changed, 17 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 0de1ef463b..3e8aff92bc 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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345680","messageId":"20180424210330.87861-4-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 3/7] diff.c: do not pass diff options as keydata to hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:26Z","receivedAt":"2018-04-24T21:03:44Z","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 \n---\n diff.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1289df4b1f..ce7bedc1b9 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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345681","messageId":"20180424210330.87861-8-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:30Z","receivedAt":"2018-04-24T21:03:50Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This marks moved code still as blocks when their indentation level\nchanges uniformly.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 4 ++\n diff.c | 83 +++++++++++++++++++++++++++++++---\n diff.h | 2 +\n t/t4015-diff-whitespace.sh | 54 ++++++++++++++++++++++\n 4 files changed, 137 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 7b2527b9a1..facdbc8f95 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -304,6 +304,10 @@ dimmed_zebra::\n \tIgnore whitespace when comparing lines when performing the move\n \tdetection for --color-moved. This ignores differences even if\n \tone line has whitespace where the other line has none.\n+--color-moved-[no-]ignore-space-prefix-delta::\n+\tIgnores whitespace when comparing lines when performing the move\n+\tdetection for --color-moved. This ignores uniform differences\n+\tof white space at the beginning lines in moved blocks.\n \n --word-diff[=]::\n \tShow a word diff, using the to delimit changed words.\ndiff --git a/diff.c b/diff.c\nindex b5819dd538..1227a4d2a8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -709,6 +709,31 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n+struct ws_delta {\n+\tchar *string; /* The prefix delta, which is the same in the block */\n+\tint direction; /* adding or removing the line? */\n+\tint missmatch; /* in the remainder */\n+};\n+#define WS_DELTA_INIT { NULL, 0, 0 }\n+\n+static void 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->missmatch = !memcmp(longer->line + d, shorter->line, shorter->len);\n+\tout->string = xmemdupz(longer->line, d);\n+\tout->direction = (a == longer);\n+}\n+\n+static int compare_ws_delta(const struct ws_delta *a, const struct ws_delta *b)\n+{\n+\treturn a->direction == b->direction && !strcmp(a->string, b->string);\n+}\n+\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@@ -720,6 +745,15 @@ 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 & COLOR_MOVED_DELTA_WHITESPACES)\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@@ -772,7 +806,8 @@ static void add_lines_to_move_detection(struct diff_options *o,\n }\n \n static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n-\t\t\t\t\t int pmb_nr)\n+\t\t\t\t\t int pmb_nr,\n+\t\t\t\t\t struct ws_delta **wsd)\n {\n \tint lp, rp;\n \n@@ -788,6 +823,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 (*wsd) {\n+\t\t\t\tfree((*wsd)[lp].string);\n+\t\t\t\t(*wsd)[lp] = (*wsd)[rp];\n+\t\t\t}\n \t\t\tpmb[rp] = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n@@ -837,8 +876,11 @@ static void mark_color_as_moved(struct diff_options *o,\n {\n \tstruct moved_entry **pmb = NULL; /* potentially moved blocks */\n \tint pmb_nr = 0, pmb_alloc = 0;\n-\tint n, flipped_block = 1, block_length = 0;\n \n+\tstruct ws_delta *wsd = NULL; /* white space deltas between pmb */\n+\tint wsd_alloc = 0;\n+\n+\tint n, flipped_block = 1, block_length = 0;\n \n \tfor (n = 0; n < o->emitted_symbols->nr; n++) {\n \t\tstruct hashmap *hm = NULL;\n@@ -881,14 +923,31 @@ static void mark_color_as_moved(struct diff_options *o,\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+\n+\t\t\tif (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES) {\n+\t\t\t\tstruct ws_delta out = WS_DELTA_INIT;\n+\n+\t\t\t\tif (pnext)\n+\t\t\t\t\tcompute_ws_delta(l, pnext->es, &out);\n+\t\t\t\tif (pnext &&\n+\t\t\t\t !hm->cmpfn(o, pnext, match, NULL) &&\n+\t\t\t\t compare_ws_delta(&out, &wsd[i])) {\n+\t\t\t\t\tpmb[i] = p->next_line;\n+\t\t\t\t\t/* wsd[i] is the same */\n+\t\t\t\t} else {\n+\t\t\t\t\tpmb[i] = NULL;\n+\t\t\t\t}\n+\t\t\t\tfree(out.string);\n \t\t\t} else {\n-\t\t\t\tpmb[i] = NULL;\n+\t\t\t\tif (pnext && !hm->cmpfn(o, pnext, match, NULL)) {\n+\t\t\t\t\tpmb[i] = p->next_line;\n+\t\t\t\t} else {\n+\t\t\t\t\tpmb[i] = NULL;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n \n-\t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n+\t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr, &wsd);\n \n \t\tif (pmb_nr == 0) {\n \t\t\t/*\n@@ -897,6 +956,10 @@ 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\tif (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES) {\n+\t\t\t\t\tALLOC_GROW(wsd, pmb_nr + 1, wsd_alloc);\n+\t\t\t\t\tcompute_ws_delta(l, match->es, &wsd[pmb_nr]);\n+\t\t\t\t}\n \t\t\t\tpmb[pmb_nr++] = match;\n \t\t\t}\n \n@@ -914,6 +977,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \tadjust_last_block(o, n, block_length);\n \n \tfree(pmb);\n+\tfree(wsd);\n }\n \n #define DIFF_SYMBOL_MOVED_LINE_ZEBRA_MASK \\\n@@ -4647,12 +4711,16 @@ int diff_opt_parse(struct diff_options *options,\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-no-ignore-space-prefix-delta\"))\n+\t\toptions->color_moved_ws_handling &= ~COLOR_MOVED_DELTA_WHITESPACES;\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, \"--color-moved-ignore-space-prefix-delta\"))\n+\t\toptions->color_moved_ws_handling |= COLOR_MOVED_DELTA_WHITESPACES;\n \telse if (!strcmp(arg, \"--indent-heuristic\"))\n \t\tDIFF_XDL_SET(options, INDENT_HEURISTIC);\n \telse if (!strcmp(arg, \"--no-indent-heuristic\"))\n@@ -5558,6 +5626,9 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\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\tif (o->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)\n+\t\t\t\to->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\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 \t\t\tif (o->color_moved == COLOR_MOVED_ZEBRA_DIM)\ndiff --git a/diff.h b/diff.h\nindex de5dc68005..b00ea76c08 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -214,6 +214,8 @@ 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+\t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n+\t#define COLOR_MOVED_DELTA_WHITESPACES\t(1 << 22)\n \tint color_moved_ws_handling;\n };\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 751fc478dd..3e94cf051d 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1847,4 +1847,58 @@ 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+\tQthree lines\n+\tQBut! <- this stands out\n+\tQthis one\n+\tQQline did\n+\tQnot adjust\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+\tQQthree lines\n+\tQQQBut! <- this stands out\n+\tthis one\n+\tQline did\n+\tnot adjust\n+\tEOF\n+\n+\tgit diff --color --color-moved-ignore-space-prefix-delta |\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,7 +1,7 @@\n+\t\t-QIndented\n+\t\t-QText across\n+\t\t-Qthree lines\n+\t\t-QBut! <- this stands out\n+\t\t-Qthis one\n+\t\t-QQline did\n+\t\t-Qnot adjust\n+\t\t+QQIndented\n+\t\t+QQText across\n+\t\t+QQthree lines\n+\t\t+QQQBut! <- this stands out\n+\t\t+this one\n+\t\t+Qline did\n+\t\t+not adjust\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345682","messageId":"20180424210330.87861-5-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 4/7] diff.c: adjust hash function signature to match hashmap expectation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:27Z","receivedAt":"2018-04-24T21:03:54Z","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 \n---\n diff.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex ce7bedc1b9..d1bae900cd 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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345683","messageId":"20180424210330.87861-6-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 5/7] diff.c: add a blocks mode for moved code detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:28Z","receivedAt":"2018-04-24T21:03:56Z","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 \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 e3a44f03cd..bb9f1b7cd8 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 d1bae900cd..95c51c0b7d 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 d29560f822..7bd4f182c3 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 17df491a3a..45091abb19 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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345684","messageId":"20180424210330.87861-7-sbeller@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"[PATCH 6/7] diff.c: decouple white space treatment from move detection algorithm","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T21:03:29Z","receivedAt":"2018-04-24T21:03:56Z","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 whitespace 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 (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\noutput, and making the existing --ignore-space-at-eol, -b, and -w options\nno longer affect the color of the output.\n\nAs we change the default, we'll adjust the tests.\n\nFor now we do not infer any options to treat whitespaces in the move\ndetection from the generic white space options given to diff.\nThis can be tuned later to reasonable default.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 13 +++++\n diff.c | 19 ++++++-\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 90 +++++++++++++++++++++++++++++++---\n 4 files changed, 114 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex bb9f1b7cd8..7b2527b9a1 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -292,6 +292,19 @@ dimmed_zebra::\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+\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+\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 95c51c0b7d..b5819dd538 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -717,10 +717,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 +730,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@@ -4638,6 +4641,18 @@ int diff_opt_parse(struct diff_options *options,\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\"))\ndiff --git a/diff.h b/diff.h\nindex 7bd4f182c3..de5dc68005 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 45091abb19..751fc478dd 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1441,7 +1441,10 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tline 4\n \tline 5\n \tEOF\n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1465,7 +1468,10 @@ test_expect_success 'move detection ignoring whitespace ' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames -w --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1505,7 +1511,10 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \tline 5\n \tEOF\n \n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1529,7 +1538,10 @@ test_expect_success 'move detection ignoring whitespace changes' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames -b --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-at-eol \\\n+\t\t--color-moved-ignore-space-change |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1572,7 +1584,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '\n \t# avoid cluttering the output with complaints about our eol whitespace\n \ttest_config core.whitespace -blank-at-eol &&\n \n-\tgit diff HEAD --no-renames --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1596,7 +1611,10 @@ test_expect_success 'move detection ignoring whitespace at eol' '\n \tEOF\n \ttest_cmp expected actual &&\n \n-\tgit diff HEAD --no-renames --ignore-space-at-eol --color-moved --color |\n+\tgit diff HEAD --no-renames --color-moved --color \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-ignore-space-at-eol |\n \t\tgrep -v \"index\" |\n \t\ttest_decode_color >actual &&\n \tcat <<-\\EOF >expected &&\n@@ -1768,7 +1786,65 @@ 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\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\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-ignore-all-space \\\n+\t\t--color-moved-ignore-space-change \\\n+\t\t--color-moved-ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\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.17.0.441.gb46fe60e1d-goog\n\n"},{"id":"345687","messageId":"20180424145050.e0ad29d4f304216441656057@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-6-sbeller@google.com","subject":"Re: [PATCH 5/7] diff.c: add a blocks mode for moved code detection","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-24T21:50:50Z","receivedAt":"2018-04-24T21:50:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 14:03:28 -0700\nStefan Beller wrote:\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\nFirstly, I don't know if this is the right solution- as written\nin the linked e-mail [1], the issue might be more that the config\nconflates 2 unrelated things, not that a certain intersection is\nmissing.\n\n[1] https://public-inbox.org/git/87muykuij7.fsf@evledraar.gmail.com/\n\nOptional: Probably better to put the link inline, instead of in the\ntrailer.\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\nAdd a comment here explaining that these colors do not appear in the\noutput, but merely set to recognizable values to ensure that they do not\nappear in the output.\n\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[snip]\n"},{"id":"345704","messageId":"20180424150036.8e7ca862525bedb91673ae46@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-7-sbeller@google.com","subject":"Re: [PATCH 6/7] diff.c: decouple white space treatment from move detection algorithm","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-24T22:00:36Z","receivedAt":"2018-04-24T22:00:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 14:03:29 -0700\nStefan Beller wrote:\n\n> As we change the default, we'll adjust the tests.\n\nThis statement is probably better written as:\n\n In some existing tests, options like --ignore-space-at-eol were used\n to control the color of the output. They have been updated to use\n options like --color-moved-ignore-space-at-eol instead.\n\n> +\tunsigned flags = diffopt->color_moved_ws_handling\n> +\t\t\t & XDF_WHITESPACE_FLAGS;\n\nNo need for \"& XDF_WHITESPACE_FLAGS\".\n\n> +\tunsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;\n\nSame here.\n\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\nShould the \"int\" be \"unsigned\"? I noticed that the flag-like xdl_opts is\nsigned, but I think it's better for flags to be unsigned. Also, document\nwhat this stores. (And also, I would limit the bits.)\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\t--color-moved-no-ignore-all-space \\\n> +\t\t--color-moved-no-ignore-space-change \\\n> +\t\t--color-moved-no-ignore-space-at-eol |\n> +\t\tgrep -v \"index\" |\n> +\t\ttest_decode_color >actual &&\n> +\tq_to_tab <<-\\EOF >expected &&\n> +\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-ignore-all-space \\\n> +\t\t--color-moved-ignore-space-change \\\n> +\t\t--color-moved-ignore-space-at-eol |\n> +\t\tgrep -v \"index\" |\n> +\t\ttest_decode_color >actual &&\n> +\tq_to_tab <<-\\EOF >expected &&\n> +\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\nI know I suggested \"per-line minimum\", but I don't think there is one -\nI think we only have a per-block minimum. Maybe delete \"per-line\" in\neach of the lines.\n"},{"id":"345705","messageId":"CAGZ79kYPd4KF7H8CiaztouF513gw=nwRuqwGDDKjvBk362e9uw@mail.gmail.com","threadId":"48354","inReplyTo":"20180424150036.8e7ca862525bedb91673ae46@google.com","subject":"Re: [PATCH 6/7] diff.c: decouple white space treatment from move detection algorithm","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T22:19:07Z","receivedAt":"2018-04-24T22:19:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jonathan,\n\nOn Tue, Apr 24, 2018 at 3:00 PM, Jonathan Tan wrote:\n> On Tue, 24 Apr 2018 14:03:29 -0700\n> Stefan Beller wrote:\n>\n>> As we change the default, we'll adjust the tests.\n>\n> This statement is probably better written as:\n>\n> In some existing tests, options like --ignore-space-at-eol were used\n> to control the color of the output. They have been updated to use\n> options like --color-moved-ignore-space-at-eol instead.\n\nI'll adjust that statement; thanks for helping me out with good commit\nmessages (even the \"As we change the defaults, ..\" was proposed by\nyou in a previous round)\n\n>\n>> + unsigned flags = diffopt->color_moved_ws_handling\n>> + & XDF_WHITESPACE_FLAGS;\n>\n> No need for \"& XDF_WHITESPACE_FLAGS\".\n\nThis is in anticipation of the next commit, when\ncolor_moved_ws_handling takes more flags.\nI can move that over to the last commit.\n\n>\n>> + unsigned flags = o->color_moved_ws_handling & XDF_WHITESPACE_FLAGS;\n>\n> Same here.\n\nMaybe I'll just state in the commit message why we keep the masking\nhere.\n\n>\n>> @@ -214,6 +214,7 @@ struct diff_options {\n>> } color_moved;\n>> #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n>> #define COLOR_MOVED_MIN_ALNUM_COUNT 20\n>> + int color_moved_ws_handling;\n>> };\n>\n> Should the \"int\" be \"unsigned\"?\n\nyes.\n\n> I noticed that the flag-like xdl_opts is\n> signed, but I think it's better for flags to be unsigned.\n\nI can change those, too.\n\n> Also, document\n> what this stores.\n\nok, will document.\n\n> (And also, I would limit the bits.)\n\nNot sure I follow. you want to make it e.g.\n\n unsigned color_moved_ws_handling : 6;\n\n?\n\nOh, that would actually work, as XDF_WHITESPACE_FLAGS\nare in second to fifth bits.\n\nBut then we need to document why the XDF_WHITESPACE\nneed to be at these low positions.\n\n>> + q_to_tab <<-\\EOF >text.txt &&\n>> + Qa long line to exceed per-line minimum\n>> + Qanother long line to exceed per-line minimum\n>> + new file\n\n>\n> I know I suggested \"per-line minimum\", but I don't think there is one -\n> I think we only have a per-block minimum. Maybe delete \"per-line\" in\n> each of the lines.\n\nyeah, I guess this heuristic could also make for another setting, though\nas of now I did not desire any other heuristic than you originally came up\nwith. Will reword the text. Thanks!\n\nThanks,\nStefan\n"},{"id":"345706","messageId":"20180424153513.dc2404cd111c44ac78bd8ed8@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-8-sbeller@google.com","subject":"Re: [PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-24T22:35:13Z","receivedAt":"2018-04-24T22:35:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 14:03:30 -0700\nStefan Beller wrote:\n\n> +--color-moved-[no-]ignore-space-prefix-delta::\n> +\tIgnores whitespace when comparing lines when performing the move\n> +\tdetection for --color-moved. This ignores uniform differences\n> +\tof white space at the beginning lines in moved blocks.\n\nSetting this option means that moved lines may be indented or dedented,\nand if they have been indented or dedented by the same amount, they are\nstill considered to be the same block. Maybe call this\n--color-moved-allow-indentation-change.\n\n> +struct ws_delta {\n> +\tchar *string; /* The prefix delta, which is the same in the block */\n\nProbably better described as \"the difference between the '-' line and\nthe '+' line\". This may be the empty string if there is no difference.\n\n> +\tint direction; /* adding or removing the line? */\n\nWhat is the value when \"added\" and what when \"removed\"? Also, it is not\ntruly \"added\" or \"removed\", so a better way might be: 1 if the '-' line\nis longer than the '+' line, and 0 otherwise. (And make sure that the\ndocumentation is correct with respect to equal lines.)\n\n> +\tint missmatch; /* in the remainder */\n\ns/missmatch/mismatch/\nAlso, what is this used for?\n\n> +\tif (diffopt->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)\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\nI wrote in [1]:\n\n I think we should just prohibit combining this with any of the\n whitespace ignoring flags except for the space-at-eol one. They seem\n to contradict anyway.\n\nTo elaborate, adding a specific flag that may interfere with other\nuser-provided flags sounds like we're unnecessarily adding combinations\nthat we must keep track of, and that it's both logical (from a user's\npoint of view) and clearer (as for the code) to just forbid such\ncombinations.\n\n[1] https://public-inbox.org/git/20180402174118.d204ec0d4b9d2fa7ebd77739@google.com/\n\n> \tstruct moved_entry **pmb = NULL; /* potentially moved blocks */\n> \tint pmb_nr = 0, pmb_alloc = 0;\n> -\tint n, flipped_block = 1, block_length = 0;\n> \n> +\tstruct ws_delta *wsd = NULL; /* white space deltas between pmb */\n> +\tint wsd_alloc = 0;\n> +\n> +\tint n, flipped_block = 1, block_length = 0;\n\nIt seems like pmb and wsd are parallel arrays - could each wsd be\nembedded into the corresponding entry of pmb instead?\n\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -214,6 +214,8 @@ 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> +\t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n> +\t#define COLOR_MOVED_DELTA_WHITESPACES\t(1 << 22)\n> \tint color_moved_ws_handling;\n> };\n\nSetting of DELTA_WHITESPACES should be a separate field, not as part of\nws_handling. It's fine for the ws_handling to be a bitset, since that's\nhow it's passed to xdiff_compare_lines(), but we don't need to do the\nsame for DELTA_WHITESPACES.\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> +\tQthree lines\n> +\tQBut! <- this stands out\n> +\tQthis one\n> +\tQQline did\n> +\tQnot adjust\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> +\tQQthree lines\n> +\tQQQBut! <- this stands out\n> +\tthis one\n> +\tQline did\n> +\tnot adjust\n> +\tEOF\n> +\n> +\tgit diff --color --color-moved-ignore-space-prefix-delta |\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,7 +1,7 @@\n> +\t\t-QIndented\n> +\t\t-QText across\n> +\t\t-Qthree lines\n> +\t\t-QBut! <- this stands out\n> +\t\t-Qthis one\n> +\t\t-QQline did\n> +\t\t-Qnot adjust\n> +\t\t+QQIndented\n> +\t\t+QQText across\n> +\t\t+QQthree lines\n> +\t\t+QQQBut! <- this stands out\n> +\t\t+this one\n> +\t\t+Qline did\n> +\t\t+not adjust\n> +\tEOF\n> +\n> +\ttest_cmp expected actual\n> +'\n\nI would have expected every line except the \"this stands out\" line to be\ncolored differently than the usual RED and GREEN. Is this test output\nexpected?\n"},{"id":"345707","messageId":"20180424153736.480b609d0af412ec46311eb6@google.com","threadId":"48354","inReplyTo":"20180424210330.87861-1-sbeller@google.com","subject":"Re: [PATCHv2 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-24T22:37:36Z","receivedAt":"2018-04-24T22:37:41Z","isPatch":false,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 14:03:23 -0700\nStefan Beller wrote:\n\n> v2:\n> I think I have addressed Jonathans feedback\n> * by using a string instead of counting the first character only.\n> * refined tests slightly (easier to read)\n> * moved white space handling for moved blocks into its own flag field,\n> keeping the enum for the actual mode of move detection.\n\nFor reference, v1 is here:\nhttps://public-inbox.org/git/20180402224854.86922-1-sbeller@google.com/\n\n> Stefan Beller (7):\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: add --color-moved-ignore-space-delta option\n\nI'm not sure if we should add a new \"blocks\" mode, or if we should\nmodify the existing plain mode to have the minimum block length instead.\nI reviewed the code as if we want the new \"blocks\" mode.\n"},{"id":"345708","messageId":"CAGZ79ka9Tdk=ijbw6Hyxs9OsBMK9O69_3_cY8oX2j=wRc_F5-g@mail.gmail.com","threadId":"48354","inReplyTo":"20180424145050.e0ad29d4f304216441656057@google.com","subject":"Re: [PATCH 5/7] diff.c: add a blocks mode for moved code detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T22:37:51Z","receivedAt":"2018-04-24T22:37:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 24, 2018 at 2:50 PM, Jonathan Tan wrote:\n> On Tue, 24 Apr 2018 14:03:28 -0700\n> Stefan Beller wrote:\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>\n> Firstly, I don't know if this is the right solution- as written\n> in the linked e-mail [1], the issue might be more that the config\n> conflates 2 unrelated things, not that a certain intersection is\n> missing.\n\nThe \"plain zebra\" or as I call them \"blocks\", has the \"heuristic\nfor a minimum of 20 characters\" and \"few colors\" as its defining\nfeatures, which solves that use case.\n\nStepping back a bit, we have different \"building blocks\"\nat our disposal:\n* move detection by line or block\n* alternating blocks\n* a heuristic to skip over small chunks (20 alnum chars)\n\nThese can be combined independently, so would\nyou expect the user to expect two options for them?\nFor example \"--color-moved=zebra\" could be split\ninto \"--skip-small --alternate-blocks\"\n\nEventually we'll use various colors to inform the user\nwhat these building blocks made of the diff.\n\nÆvar wrote:\n\n> Which is what I mean by the current config conflating two (to me)\n> unrelated things. One is how we, via any method, detect what's moved or\n> not, and the other is what color/format we use to present this to the\n> user.\n\nSo instead of building blocks we rather want to split into algorithms\nand presentation layer?\n\nThe presentation layer would be things like:\n* use a different color for moved things\n* alternate colors for adjacent blocks\n* paint border of a block (dimmed zebra)\n\nThe algorithm side would be\n* detect moves\n* detect moves as blocks\n* skip small heuristic\n\nAm I still missing the big picture?\n\n> [1] https://public-inbox.org/git/87muykuij7.fsf@evledraar.gmail.com/\n>\n> Optional: Probably better to put the link inline, instead of in the\n> trailer.\n\nok.\n\n>\n>> -test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n>> +test_expect_success 'detect blocks of moved code' '\n>> git reset --hard &&\n>> cat <<-\\EOF >lines.txt &&\n>> long line 1\n>> @@ -1271,6 +1271,52 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '\n>> test_config color.diff.newMovedDimmed \"normal cyan\" &&\n>> test_config color.diff.oldMovedAlternativeDimmed \"normal blue\" &&\n>> test_config color.diff.newMovedAlternativeDimmed \"normal yellow\" &&\n>\n> Add a comment here explaining that these colors do not appear in the\n> output, but merely set to recognizable values to ensure that they do not\n> appear in the output.\n\nok.\n"},{"id":"345710","messageId":"CAGZ79kZSMURd_s6yxUCJoDjQzu-NjnFzp+740GFnYZurnw_2yw@mail.gmail.com","threadId":"48354","inReplyTo":"20180424153736.480b609d0af412ec46311eb6@google.com","subject":"Re: [PATCHv2 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T22:58:57Z","receivedAt":"2018-04-24T22:59:04Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 24, 2018 at 3:37 PM, Jonathan Tan wrote:\n> On Tue, 24 Apr 2018 14:03:23 -0700\n> Stefan Beller wrote:\n>\n>> v2:\n>> I think I have addressed Jonathans feedback\n>> * by using a string instead of counting the first character only.\n>> * refined tests slightly (easier to read)\n>> * moved white space handling for moved blocks into its own flag field,\n>> keeping the enum for the actual mode of move detection.\n>\n> For reference, v1 is here:\n> https://public-inbox.org/git/20180402224854.86922-1-sbeller@google.com/\n>\n>> Stefan Beller (7):\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: add --color-moved-ignore-space-delta option\n>\n> I'm not sure if we should add a new \"blocks\" mode, or if we should\n> modify the existing plain mode to have the minimum block length instead.\n> I reviewed the code as if we want the new \"blocks\" mode.\n\nThanks for the review!\n\nI think keeping plain is useful, see 176841f0c9 (diff.c: color\nmoved lines differently, plain mode, 2017-06-30)\n\n diff.c: color moved lines differently, plain mode\n\n Add the 'plain' mode for move detection of code. This omits the checking\n for adjacent blocks, so it is not as useful. If you have a lot of the\n same blocks moved in the same patch, the 'Zebra' would end up slow as it\n is O(n^2) (n is number of same blocks). So this may be useful there and\n is generally easy to add. Instead be very literal at the move detection,\n do not skip over short blocks here.\n\n\nAlthough if we do not care about that use case we can just add heuristics to\nplain.\n\nAs eluded to in Ævars email, we might want to break it up into multiple\noptions as well?\n"},{"id":"345711","messageId":"20180424155948.e37a0d12fdf0e2fdafde153d@google.com","threadId":"48354","inReplyTo":"CAGZ79ka9Tdk=ijbw6Hyxs9OsBMK9O69_3_cY8oX2j=wRc_F5-g@mail.gmail.com","subject":"Re: [PATCH 5/7] diff.c: add a blocks mode for moved code detection","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-24T22:59:48Z","receivedAt":"2018-04-24T22:59:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 15:37:51 -0700\nStefan Beller wrote:\n\n> These can be combined independently, so would\n> you expect the user to expect two options for them?\n> For example \"--color-moved=zebra\" could be split\n> into \"--skip-small --alternate-blocks\"\n\nYes, this is a good explanation. Reusing your terms below, --skip-small\ncontrols the algorithm, and --alternate-blocks controls the presentation\nlayer.\n\n> So instead of building blocks we rather want to split into algorithms\n> and presentation layer?\n> \n> The presentation layer would be things like:\n> * use a different color for moved things\n> * alternate colors for adjacent blocks\n> * paint border of a block (dimmed zebra)\n> \n> The algorithm side would be\n> * detect moves\n> * detect moves as blocks\n> * skip small heuristic\n\nYes.\n\nThis was just brainstorming, though - this might not be the direction we\nwant to take in this patch. (The right solution might just be to always\nuse blocks - thereby simplifying the algorithm aspect.)\n"},{"id":"345712","messageId":"CAGZ79kbQUWq_pfvwLWotqCxc1-Y=ctJ4SqgqR+EvJ5wkJkp5kQ@mail.gmail.com","threadId":"48354","inReplyTo":"CAGZ79kbGkHFSS9K8KKTwNikx3Tw+m+RMLY3RAf8SW_iK9a2OJQ@mail.gmail.com","subject":"Re: [PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-24T23:23:28Z","receivedAt":"2018-04-24T23:23:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Replied to Jonathan only instead of all. My reply is below:\n\nOn Tue, Apr 24, 2018 at 3:55 PM, Stefan Beller wrote:\n> On Tue, Apr 24, 2018 at 3:35 PM, Jonathan Tan wrote:\n>> On Tue, 24 Apr 2018 14:03:30 -0700\n>> Stefan Beller wrote:\n>>\n>>> +--color-moved-[no-]ignore-space-prefix-delta::\n>>> + Ignores whitespace when comparing lines when performing the move\n>>> + detection for --color-moved. This ignores uniform differences\n>>> + of white space at the beginning lines in moved blocks.\n>>\n>> Setting this option means that moved lines may be indented or dedented,\n>> and if they have been indented or dedented by the same amount, they are\n>> still considered to be the same block. Maybe call this\n>> --color-moved-allow-indentation-change.\n>\n> ok, sounds good as well. I tried coming up with a name that refers to\n> the block check as that is the important part.\n>\n>>> +struct ws_delta {\n>>> + char *string; /* The prefix delta, which is the same in the block */\n>>\n>> Probably better described as \"the difference between the '-' line and\n>> the '+' line\". This may be the empty string if there is no difference.\n>\n> Makes sense.\n>\n>>\n>>> + int direction; /* adding or removing the line? */\n>>\n>> What is the value when \"added\" and what when \"removed\"? Also, it is not\n>> truly \"added\" or \"removed\", so a better way might be: 1 if the '-' line\n>> is longer than the '+' line, and 0 otherwise. (And make sure that the\n>> documentation is correct with respect to equal lines.)\n>>\n>>> + int missmatch; /* in the remainder */\n>>\n>> s/missmatch/mismatch/\n>> Also, what is this used for?\n>\n> The mismatch should be used for (thanks for catching!)\n> checking if the remainder of a line is the same, although a boolean\n> may be not the correct choice. We know that the two strings\n> passed into compute_ws_delta come from a complete white space\n> agnostic comparison, so consider:\n>\n> + SP SP more TAB more\n> + SP SP text TAB text\n>\n> - SP more TAB more\n> - SP text TAB text\n>\n> which would mark this as a moved block. This is the feature\n> working as intended, but what about\n>\n> + SP SP more TAB more\n> + SP SP text TAB text\n>\n> - SP more SP more\n> - SP text TAB text\n>\n> Note how the length of the strings is the same, hence the current\n> code of\n>\n> compute_ws_delta(...) {\n> int d = longer->len - shorter->len;\n> out->string = xmemdupz(longer->line, d);\n> }\n>\n> would work fine and not notice the \"change in the remainder\".\n> That ought to be caught by the mismatch variable, that\n> is assigned, but not used.\n>\n> The compare_ws_delta(a, b) needs to be extended to\n>\n> !a->mismatch && !b->mismatch && existing_condition\n>\n> Ideally the example from above would be different depending\n> on whether the other white space flags are given or not.\n>\n>>> + if (diffopt->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)\n>>> + /*\n>>> + * As there is not specific white space config given,\n>>> + * we'd need to check for a new block, so ignore all\n>>> + * white space. The setup of the white space\n>>> + * configuration for the next block is done else where\n>>> + */\n>>> + flags |= XDF_IGNORE_WHITESPACE;\n>>> +\n>>> return !xdiff_compare_lines(a->es->line, a->es->len,\n>>> b->es->line, b->es->len,\n>>> flags);\n>>\n>> I wrote in [1]:\n>>\n>> I think we should just prohibit combining this with any of the\n>> whitespace ignoring flags except for the space-at-eol one. They seem\n>> to contradict anyway.\n>\n> As outlined above, I think there are corner cases in which they do not\n> contradict. So I think the COLOR_MOVED_DELTA_WHITESPACES\n> will go into its own variable, and then we can solve the corner cases\n> correctly.\n>\n>> To elaborate, adding a specific flag that may interfere with other\n>> user-provided flags sounds like we're unnecessarily adding combinations\n>> that we must keep track of, and that it's both logical (from a user's\n>> point of view) and clearer (as for the code) to just forbid such\n>> combinations.\n>\n> Yes, I think you mentioned this before. Thanks for reminding!\n>\n>>> + struct ws_delta *wsd = NULL; /* white space deltas between pmb */\n>>> + int wsd_alloc = 0;\n>>> +\n>>> + int n, flipped_block = 1, block_length = 0;\n>>\n>> It seems like pmb and wsd are parallel arrays - could each wsd be\n>> embedded into the corresponding entry of pmb instead?\n>\n> I'll explore that. It sounds like a good idea for code hygiene.\n> Although if you do not intend to use this feature, then keeping it separate\n> would allow for a smaller footprint in memory.\n>\n>>\n>>> --- a/diff.h\n>>> +++ b/diff.h\n>>> @@ -214,6 +214,8 @@ struct diff_options {\n>>> } color_moved;\n>>> #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n>>> #define COLOR_MOVED_MIN_ALNUM_COUNT 20\n>>> + /* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n>>> + #define COLOR_MOVED_DELTA_WHITESPACES (1 << 22)\n>>> int color_moved_ws_handling;\n>>> };\n>>\n>> Setting of DELTA_WHITESPACES should be a separate field, not as part of\n>> ws_handling. It's fine for the ws_handling to be a bitset, since that's\n>> how it's passed to xdiff_compare_lines(), but we don't need to do the\n>> same for DELTA_WHITESPACES.\n>\n> You are correct. Thanks for your patience in this series!\n>\n>> + git diff --color --color-moved-ignore-space-prefix-delta |\n>> + grep -v \"index\" |\n>> + test_decode_color >actual &&\n>\n>>> + q_to_tab <<-\\EOF >expected &&\n>>> + diff --git a/text.txt b/text.txt\n>>> + --- a/text.txt\n>>> + +++ b/text.txt\n>>> + @@ -1,7 +1,7 @@\n>>> + -QIndented\n>>> + -QText across\n>>> + -Qthree lines\n>>> + -QBut! <- this stands out\n>>> + -Qthis one\n>>> + -QQline did\n>>> + -Qnot adjust\n>>> + +QQIndented\n>>> + +QQText across\n>>> + +QQthree lines\n>>> + +QQQBut! <- this stands out\n>>> + +this one\n>>> + +Qline did\n>>> + +not adjust\n>>> + EOF\n>>> +\n>>> + test_cmp expected actual\n>>> +'\n>>\n>> I would have expected every line except the \"this stands out\" line to be\n>> colored differently than the usual RED and GREEN. Is this test output\n>> expected?\n>\n> It is wrong indeed. I blindly copied the actual file once interactive testing\n> confirmed it worked.\n>\n> The command is missing a --color-moved, as the --color-moved-whitespace-settings\n> do not imply --color-moved, yet(?)\n>\n> Thanks,\n> Stefan\n"},{"id":"345719","messageId":"20180424171123.7092788b94498908c25eccf0@google.com","threadId":"48354","inReplyTo":"CAGZ79kbQUWq_pfvwLWotqCxc1-Y=ctJ4SqgqR+EvJ5wkJkp5kQ@mail.gmail.com","subject":"Re: [PATCH 7/7] diff.c: add --color-moved-ignore-space-delta option","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-25T00:11:23Z","receivedAt":"2018-04-25T00:11:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 24 Apr 2018 16:23:28 -0700\nStefan Beller wrote:\n\n> >> s/missmatch/mismatch/\n> >> Also, what is this used for?\n> >\n> > The mismatch should be used for (thanks for catching!)\n> > checking if the remainder of a line is the same, although a boolean\n> > may be not the correct choice. We know that the two strings\n> > passed into compute_ws_delta come from a complete white space\n> > agnostic comparison, so consider:\n> >\n> > + SP SP more TAB more\n> > + SP SP text TAB text\n> >\n> > - SP more TAB more\n> > - SP text TAB text\n> >\n> > which would mark this as a moved block. This is the feature\n> > working as intended, but what about\n> >\n> > + SP SP more TAB more\n> > + SP SP text TAB text\n> >\n> > - SP more SP more\n> > - SP text TAB text\n> >\n> > Note how the length of the strings is the same, hence the current\n> > code of\n> >\n> > compute_ws_delta(...) {\n> > int d = longer->len - shorter->len;\n> > out->string = xmemdupz(longer->line, d);\n> > }\n> >\n> > would work fine and not notice the \"change in the remainder\".\n> > That ought to be caught by the mismatch variable, that\n> > is assigned, but not used.\n> >\n> > The compare_ws_delta(a, b) needs to be extended to\n> >\n> > !a->mismatch && !b->mismatch && existing_condition\n> >\n> > Ideally the example from above would be different depending\n> > on whether the other white space flags are given or not.\n\nThanks - this gives me food for thought.\n\nI'm starting to think that it is impossible to avoid creating our own\nstring comparison function that:\n - seeks to the first non-whitespace character in both strings\n - checks that both strings, from that first non-whitespace characters,\n are equal for some definition of equal (whether through strcmp or\n xdiff_compare_lines)\n - walks backwards from that first non-whitespace characters to look for\n the first non-matching whitespace character between the 2 strings\n\nThe existing diff whitespace modes (to be passed to xdiff_compare_lines)\ndo not give the exact result we want. For example, if\nXDF_IGNORE_WHITESPACE is used (as is in this patch), lines like \"a b\"\nand \"ab \" would match even though they shouldn't.\n\nThis comparison function can be used both in moved_entry_cmp() and when\nconstructing the ws_delta (in which case it should be made to output\nwhatever information is needed as out parameters or similar).\n\n> >>> + if (diffopt->color_moved_ws_handling & COLOR_MOVED_DELTA_WHITESPACES)\n> >>> + /*\n> >>> + * As there is not specific white space config given,\n> >>> + * we'd need to check for a new block, so ignore all\n> >>> + * white space. The setup of the white space\n> >>> + * configuration for the next block is done else where\n> >>> + */\n> >>> + flags |= XDF_IGNORE_WHITESPACE;\n> >>> +\n> >>> return !xdiff_compare_lines(a->es->line, a->es->len,\n> >>> b->es->line, b->es->len,\n> >>> flags);\n\nWe can't add XDF_IGNORE_WHITESPACE here - as far as I can tell, this\nmeans that more lines will be treated as moved than the user wants (if\nthe user did not set --color-moved-ignore-all-space).\n\n> >> It seems like pmb and wsd are parallel arrays - could each wsd be\n> >> embedded into the corresponding entry of pmb instead?\n> >\n> > I'll explore that. It sounds like a good idea for code hygiene.\n> > Although if you do not intend to use this feature, then keeping it separate\n> > would allow for a smaller footprint in memory.\n\nIf you're worried about memory, wsd can be embedded as a pointer.\n\n> > The command is missing a --color-moved, as the --color-moved-whitespace-settings\n> > do not imply --color-moved, yet(?)\n\nMaybe one should imply the other (or at least a warning).\n"}]}