{"thread":{"id":"48205","subject":"[RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","startedAt":"2018-04-02T22:49:11Z","lastAt":"2018-04-06T22:27:43Z","messageCount":26,"participants":["Stefan Beller","Jonathan Tan","Jacob Keller","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"343634","messageId":"20180402224854.86922-1-sbeller@google.com","threadId":"48205","inReplyTo":null,"subject":"[RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-02T22:48:47Z","receivedAt":"2018-04-02T22:49:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This 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: refactor internal representation for coloring moved code\n diff.c: decouple white space treatment for move detection from generic\n option\n diff.c: add --color-moved-ignore-space-delta option\n\n Documentation/diff-options.txt | 13 ++\n diff.c | 155 ++++++++++++---\n diff.h | 18 +-\n t/t4015-diff-whitespace.sh | 341 ++++++++++++++++++++++++++++++++-\n xdiff/xdiff.h | 8 -\n xdiff/xdiffi.c | 17 --\n 6 files changed, 483 insertions(+), 69 deletions(-)\n\n-- \n2.17.0.484.g0c8726318c-goog\n\n"},{"id":"343635","messageId":"20180402224854.86922-3-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-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-02T22:48:49Z","receivedAt":"2018-04-02T22:49:14Z","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.484.g0c8726318c-goog\n\n"},{"id":"343636","messageId":"20180402224854.86922-2-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-1-sbeller@google.com","subject":"[PATCH 1/7] xdiff/xdiff.h: remove unused flags","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-02T22:48:48Z","receivedAt":"2018-04-02T22:49:16Z","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.484.g0c8726318c-goog\n\n"},{"id":"343637","messageId":"20180402224854.86922-6-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-1-sbeller@google.com","subject":"[PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-02T22:48:52Z","receivedAt":"2018-04-02T22:49:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"At the time the move coloring was implemented we thought an enum of modes\nis the best to configure this feature. However as we want to tack on new\nfeatures, the enum would grow exponentially.\n\nRefactor the code such that features are enabled via bits. Currently we can\n* activate the move detection,\n* enable the block detection on top, and\n* enable the dimming inside a block, though this could be done without\n block detection as well (mode \"plain, dimmed\")\n\nChoose the flags to not be at bit position 2,3,4 as the next patch\nwill occupy these.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 27 ++++++++++++++-------------\n diff.h | 17 ++++++++++-------\n 2 files changed, 24 insertions(+), 20 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 879b8a5d9d..2052a43f7a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -260,7 +260,7 @@ static int parse_color_moved(const char *arg)\n {\n \tswitch (git_parse_maybe_bool(arg)) {\n \tcase 0:\n-\t\treturn COLOR_MOVED_NO;\n+\t\treturn 0;\n \tcase 1:\n \t\treturn COLOR_MOVED_DEFAULT;\n \tdefault:\n@@ -268,15 +268,17 @@ static int parse_color_moved(const char *arg)\n \t}\n \n \tif (!strcmp(arg, \"no\"))\n-\t\treturn COLOR_MOVED_NO;\n+\t\treturn 0;\n \telse if (!strcmp(arg, \"plain\"))\n-\t\treturn COLOR_MOVED_PLAIN;\n+\t\treturn COLOR_MOVED_ENABLED;\n \telse if (!strcmp(arg, \"zebra\"))\n-\t\treturn COLOR_MOVED_ZEBRA;\n+\t\treturn COLOR_MOVED_ENABLED | COLOR_MOVED_FIND_BLOCKS;\n \telse if (!strcmp(arg, \"default\"))\n \t\treturn COLOR_MOVED_DEFAULT;\n \telse if (!strcmp(arg, \"dimmed_zebra\"))\n-\t\treturn COLOR_MOVED_ZEBRA_DIM;\n+\t\treturn COLOR_MOVED_ENABLED |\n+\t\t COLOR_MOVED_FIND_BLOCKS |\n+\t\t COLOR_MOVED_DIMMED_BLOCKS;\n \telse\n \t\treturn error(_(\"color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'\"));\n }\n@@ -794,7 +796,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n }\n \n /*\n- * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.\n+ * If o->color_moved is not set to find blocks, this function does nothing.\n *\n * Otherwise, if the last block has fewer alphanumeric characters than\n * COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in\n@@ -809,7 +811,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n static void adjust_last_block(struct diff_options *o, int n, int block_length)\n {\n \tint i, alnum_count = 0;\n-\tif (o->color_moved == COLOR_MOVED_PLAIN)\n+\tif (!(o->color_moved & COLOR_MOVED_FIND_BLOCKS))\n \t\treturn;\n \tfor (i = 1; i < block_length + 1; i++) {\n \t\tconst char *c = o->emitted_symbols->buf[n - i].line;\n@@ -868,7 +870,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \n \t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \n-\t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n+\t\tif (!(o->color_moved & COLOR_MOVED_FIND_BLOCKS))\n \t\t\tcontinue;\n \n \t\t/* Check any potential block runs, advance each or nullify */\n@@ -4697,12 +4699,11 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\toptions->use_color = 0;\n \telse if (!strcmp(arg, \"--color-moved\")) {\n-\t\tif (diff_color_moved_default)\n-\t\t\toptions->color_moved = diff_color_moved_default;\n-\t\tif (options->color_moved == COLOR_MOVED_NO)\n+\t\toptions->color_moved = diff_color_moved_default;\n+\t\tif (!options->color_moved)\n \t\t\toptions->color_moved = COLOR_MOVED_DEFAULT;\n \t} else if (!strcmp(arg, \"--no-color-moved\"))\n-\t\toptions->color_moved = COLOR_MOVED_NO;\n+\t\toptions->color_moved = 0;\n \telse if (skip_prefix(arg, \"--color-moved=\", &arg)) {\n \t\tint cm = parse_color_moved(arg);\n \t\tif (cm < 0)\n@@ -5543,7 +5544,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\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)\n+\t\t\tif (o->color_moved & COLOR_MOVED_DIMMED_BLOCKS)\n \t\t\t\tdim_moved_lines(o);\n \n \t\t\thashmap_free(&add_lines, 0);\ndiff --git a/diff.h b/diff.h\nindex d29560f822..9542017986 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -205,13 +205,16 @@ struct diff_options {\n \tint diff_path_counter;\n \n \tstruct emitted_diff_symbols *emitted_symbols;\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} color_moved;\n-\t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n+\tunsigned color_moved;\n+\n+\t#define COLOR_MOVED_ENABLED\t\t(1 << 0)\n+\n+\t#define COLOR_MOVED_FIND_BLOCKS\t\t(1 << 1)\n+\t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n+\n+\t#define COLOR_MOVED_DIMMED_BLOCKS\t(1 << 23)\n+\n+\t#define COLOR_MOVED_DEFAULT (COLOR_MOVED_ENABLED | COLOR_MOVED_FIND_BLOCKS)\n \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\n };\n \n-- \n2.17.0.484.g0c8726318c-goog\n\n"},{"id":"343638","messageId":"20180402224854.86922-7-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-1-sbeller@google.com","subject":"[PATCH 6/7] diff.c: decouple white space treatment for move detection from generic option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-02T22:48:53Z","receivedAt":"2018-04-02T22:49:22Z","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 we assumed that\nthe choice for ignoring white space changes is the same for the move\ndetection as it is for the generic diff. It turns out this is wrong.\n\nThere are a couple of things where the user wants to input their\ndecision into the diff machinery:\n\n* Whether or not to ignore white space for the general diff detection.\n This is covered with the -w, -b options already.\n* Whether the move detection ought to pay attention to white spaces\n in general. And if so, how are white spaces handled for the block\n detection.\n\nOne example would be reviewing a current patch under discussion, that moves\ncode around. In that case, you may want to have the general diff machinery\nnot ignore the white spaces (i.e. exact matching), as that will show you\nthe diff as the patch sent. The code moved however may have another\nindentation level, such that you want to ignore white spaces for the move\ndetection. The code may be in python, such that spaces matter for program\nflow, though. That is why you'd want each block to have the same change\nin white space.\n\nThis patch decouples white space treatment in the general diff machinery\nfrom the white space treatment in the move detection.\n\nThis adds all the the options for ignoring white space that the regular\ndiff machinery has to the move detection, such that we can cover all the\ncases that were introduced in 7a55427094 (Merge branch\n'jk/diff-color-moved-fix', 2017-11-06).\n\nThe example from above (different lines in the diff with -w for the regular\ndiff) is covered in a test. Convert the existing tests to be more explicit\non their choice of white space behavior in the move detection as the tests\nshould not fail if the default is changed.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 13 +++\n diff.c | 17 +++-\n t/t4015-diff-whitespace.sh | 150 +++++++++++++++++++++++++++++++--\n 3 files changed, 171 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex e3a44f03cd..4ca09c1977 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -288,6 +288,19 @@ dimmed_zebra::\n \tblocks are considered interesting, the rest is uninteresting.\n --\n \n+--color-moved-[no-]ignore-all-space::\n+\tIgnore whitespace when comparing lines when performing the move\n+\tdetection for --color-moved. This ignores differences even if\n+\tone line has whitespace where the other line has none.\n+--color-moved-[no-]ignore-space-change::\n+\tIgnore changes in amount of whitespace when performing the move\n+\tdetection for --color-moved. This ignores whitespace\n+\tat line end, and considers all other sequences of one or\n+\tmore whitespace characters to be equivalent.\n+--color-moved-[no-]ignore-space-at-eol::\n+\tIgnore changes in whitespace at EOL when performing the move\n+\tdetection for --color-moved.\n+\n --word-diff[=]::\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 2052a43f7a..5fe2930dca 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -720,7 +720,7 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \n \treturn !xdiff_compare_lines(a->es->line, a->es->len,\n \t\t\t\t b->es->line, b->es->len,\n-\t\t\t\t diffopt->xdl_opts);\n+\t\t\t\t diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n }\n \n static struct moved_entry *prepare_entry(struct diff_options *o,\n@@ -728,8 +728,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 & 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 +4639,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 &= ~XDF_IGNORE_WHITESPACE;\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-change\"))\n+\t\toptions->color_moved &= ~XDF_IGNORE_WHITESPACE_CHANGE;\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-at-eol\"))\n+\t\toptions->color_moved &= ~XDF_IGNORE_WHITESPACE_AT_EOL;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-all-space\"))\n+\t\toptions->color_moved |= XDF_IGNORE_WHITESPACE;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-space-change\"))\n+\t\toptions->color_moved |= XDF_IGNORE_WHITESPACE_CHANGE;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-space-at-eol\"))\n+\t\toptions->color_moved |= 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/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 17df491a3a..38aaf4c46c 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1395,7 +1395,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@@ -1419,7 +1422,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@@ -1459,7 +1465,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@@ -1483,7 +1492,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@@ -1526,7 +1538,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@@ -1550,7 +1565,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@@ -1722,7 +1740,125 @@ test_expect_success 'move detection with submodules' '\n \n \t# nor did we mess with it another way\n \tgit diff --submodule=diff --color | test_decode_color >expect &&\n-\ttest_cmp expect decoded_actual\n+\ttest_cmp expect decoded_actual &&\n+\trm -rf bananas &&\n+\tgit submodule deinit bananas\n+'\n+\n+test_expect_success 'move detection only ignores white spaces' '\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >function.c &&\n+\tint func()\n+\t{\n+\tQif (foo) {\n+\tQQ// this part of the function\n+\tQQ// function will be very long\n+\tQQ// indeed. We must exceed both\n+\tQQ// per-line and number of line\n+\tQQ// minimums\n+\tQQ;\n+\tQ}\n+\tQbaz();\n+\tQbar();\n+\tQ// more unrelated stuff\n+\t}\n+\tEOF\n+\tgit add function.c &&\n+\tgit commit -m \"add function.c\" &&\n+\tq_to_tab <<-\\EOF >function.c &&\n+\tint do_foo()\n+\t{\n+\tQ// this part of the function\n+\tQ// function will be very long\n+\tQ// indeed. We must exceed both\n+\tQ// per-line and number of line\n+\tQ// minimums\n+\tQ;\n+\t}\n+\n+\tint func()\n+\t{\n+\tQif (foo)\n+\tQQdo_foo();\n+\tQbaz();\n+\tQbar();\n+\tQ// more unrelated stuff\n+\t}\n+\tEOF\n+\n+\t# Make sure we get a different diff using -w (\"moved function header\")\n+\tgit diff --color --color-moved -w \\\n+\t\t--color-moved-no-ignore-all-space \\\n+\t\t--color-moved-no-ignore-space-change \\\n+\t\t--color-moved-no-ignore-space-at-eol |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/function.c b/function.c\n+\t--- a/function.c\n+\t+++ b/function.c\n+\t@@ -1,6 +1,5 @@\n+\t-int func()\n+\t+int do_foo()\n+\t {\n+\t-\tif (foo) {\n+\t Q// this part of the function\n+\t Q// function will be very long\n+\t Q// indeed. We must exceed both\n+\t@@ -8,6 +7,11 @@ int func()\n+\t Q// minimums\n+\t Q;\n+\t }\n+\t+\n+\t+int func()\n+\t+{\n+\t+Qif (foo)\n+\t+QQdo_foo();\n+\t Qbaz();\n+\t Qbar();\n+\t Q// more unrelated stuff\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/function.c b/function.c\n+\t--- a/function.c\n+\t+++ b/function.c\n+\t@@ -1,13 +1,17 @@\n+\t+int do_foo()\n+\t+{\n+\t+Q// this part of the function\n+\t+Q// function will be very long\n+\t+Q// indeed. We must exceed both\n+\t+Q// per-line and number of line\n+\t+Q// minimums\n+\t+Q;\n+\t+}\n+\t+\n+\t int func()\n+\t {\n+\t-Qif (foo) {\n+\t-QQ// this part of the function\n+\t-QQ// function will be very long\n+\t-QQ// indeed. We must exceed both\n+\t-QQ// per-line and number of line\n+\t-QQ// minimums\n+\t-QQ;\n+\t-Q}\n+\t+Qif (foo)\n+\t+QQdo_foo();\n+\t Qbaz();\n+\t Qbar();\n+\t Q// more unrelated stuff\n+\tEOF\n+\ttest_cmp expected actual\n '\n \n test_done\n-- \n2.17.0.484.g0c8726318c-goog\n\n"},{"id":"343639","messageId":"20180402224854.86922-8-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-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-02T22:48:54Z","receivedAt":"2018-04-02T22:49:25Z","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 diff.c | 93 ++++++++++++++++--\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 191 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 278 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 5fe2930dca..9f969be588 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -709,6 +709,41 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n+struct ws_delta {\n+\tint deltachars;\n+\tchar firstchar;\n+};\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+\tint i;\n+\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n+\n+\n+\tout->deltachars = 0;\n+\tout->firstchar = 0;\n+\n+\tif (longer->len > 0 && isspace(longer->line[0]))\n+\t\tout->firstchar = longer->line[0];\n+\telse\n+\t\treturn;\n+\n+\tfor (i = 0; i < a->len; i++)\n+\t\tif (a->line[i] == out->firstchar)\n+\t\t\tout->deltachars ++;\n+\n+\tfor (i = 0; i < b->len; i++)\n+\t\tif (b->line[i] == out->firstchar)\n+\t\t\tout->deltachars --;\n+}\n+\n+static int compare_ws_delta(const struct ws_delta *a, const struct ws_delta *b)\n+{\n+\treturn a->firstchar == b->firstchar && a->deltachars == b->deltachars;\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@@ -717,10 +752,20 @@ 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 & XDF_WHITESPACE_FLAGS;\n+\n+\tif (diffopt->color_moved & 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 diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n+\t\t\t\t flags);\n }\n \n static struct moved_entry *prepare_entry(struct diff_options *o,\n@@ -770,7 +815,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@@ -786,6 +832,8 @@ 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\t(*wsd)[lp] = (*wsd)[rp];\n \t\t\tpmb[rp] = NULL;\n \t\t\trp--;\n \t\t\tlp++;\n@@ -835,8 +883,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@@ -879,14 +930,30 @@ 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 & COLOR_MOVED_DELTA_WHITESPACES) {\n+\t\t\t\tstruct ws_delta out;\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} 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@@ -895,6 +962,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 & 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@@ -912,6 +983,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@@ -4645,12 +4717,16 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->color_moved &= ~XDF_IGNORE_WHITESPACE_CHANGE;\n \telse if (!strcmp(arg, \"--color-moved-no-ignore-space-at-eol\"))\n \t\toptions->color_moved &= ~XDF_IGNORE_WHITESPACE_AT_EOL;\n+\telse if (!strcmp(arg, \"--color-moved-no-ignore-space-delta\"))\n+\t\toptions->color_moved &= ~COLOR_MOVED_DELTA_WHITESPACES;\n \telse if (!strcmp(arg, \"--color-moved-ignore-all-space\"))\n \t\toptions->color_moved |= XDF_IGNORE_WHITESPACE;\n \telse if (!strcmp(arg, \"--color-moved-ignore-space-change\"))\n \t\toptions->color_moved |= XDF_IGNORE_WHITESPACE_CHANGE;\n \telse if (!strcmp(arg, \"--color-moved-ignore-space-at-eol\"))\n \t\toptions->color_moved |= XDF_IGNORE_WHITESPACE_AT_EOL;\n+\telse if (!strcmp(arg, \"--color-moved-ignore-space-delta\"))\n+\t\toptions->color_moved |= 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@@ -5555,6 +5631,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 & COLOR_MOVED_DELTA_WHITESPACES)\n+\t\t\t\to->color_moved |= 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_DIMMED_BLOCKS)\ndiff --git a/diff.h b/diff.h\nindex 9542017986..b8c0cf1232 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -212,6 +212,7 @@ struct diff_options {\n \t#define COLOR_MOVED_FIND_BLOCKS\t\t(1 << 1)\n \t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n \n+\t#define COLOR_MOVED_DELTA_WHITESPACES\t(1 << 22)\n \t#define COLOR_MOVED_DIMMED_BLOCKS\t(1 << 23)\n \n \t#define COLOR_MOVED_DEFAULT (COLOR_MOVED_ENABLED | COLOR_MOVED_FIND_BLOCKS)\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 38aaf4c46c..246ccadf9b 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1861,4 +1861,195 @@ test_expect_success 'move detection only 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\n+\tQacross\n+\tQfive\n+\tQlines\n+\tQBut!\n+\tQthis\n+\tQQone\n+\tQline\n+\tQQdid\n+\tQnot\n+\tQQadjust\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\n+\tQQacross\n+\tQQfive\n+\tQQlines\n+\tQQQBut!\n+\tQQthis\n+\tQQQone\n+\tQQline\n+\tQQQdid\n+\tQQnot\n+\tQQQadjust\n+\tEOF\n+\n+\tgit diff --color --color-moved-ignore-space-delta |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\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,12 +1,12 @@\n+\t-QIndented\n+\t-QText\n+\t-Qacross\n+\t-Qfive\n+\t-Qlines\n+\t-QBut!\n+\t-Qthis\n+\t-QQone\n+\t-Qline\n+\t-QQdid\n+\t-Qnot\n+\t-QQadjust\n+\t+QQIndented\n+\t+QQText\n+\t+QQacross\n+\t+QQfive\n+\t+QQlines\n+\t+QQQBut!\n+\t+QQthis\n+\t+QQQone\n+\t+QQline\n+\t+QQQdid\n+\t+QQnot\n+\t+QQQadjust\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'compare whitespace delta across moved blocks with multiple indentation levels' '\n+\t# alternative: \"python programmers would love the move detection, too\"\n+\n+\tgit reset --hard &&\n+\tq_to_tab <<-\\EOF >test.py &&\n+\tclass test:\n+\tQdef f(x):\n+\tQQ\"\"\"\n+\tQQA simple python function\n+\tQQthat returns the square of a number\n+\tQQAlthough it may not be pythonic\n+\tQQ\"\"\"\n+\tQQdef g(x):\n+\tQQQ\"\"\"\n+\tQQQNested function that returns the same number\n+\tQQQWe just multiply by 1.0\n+\tQQQso we can write a comment about it\n+\tQQQas we need longer blocks\n+\tQQQ\"\"\"\n+\tQQQreturn 1.0 * x\n+\tQQ# Another comment for f(x)\n+\tQQ# also spanning multiple lines\n+\tQQ# to make a block\n+\tQQreturn g(x) * g(x)\n+\tQdef h(x):\n+\tQQ# Another function unrelated to the previous\n+\tQQ# but building a block,\n+\tQQ# long enough to call it a block\n+\tQQreturn x * 1.0\n+\tEOF\n+\n+\tgit add test.py &&\n+\tgit commit -m \"add test.py\" &&\n+\n+\tq_to_tab <<-\\EOF >test.py &&\n+\tclass test:\n+\tQdef h(x):\n+\tQQ# Another function unrelated to the previous\n+\tQQ# but building a block,\n+\tQQ# long enough to call it a block\n+\tQQreturn x * 1.0\n+\tdef f(x):\n+\tQ\"\"\"\n+\tQA simple python function\n+\tQthat returns the square of a number\n+\tQAlthough it may not be pythonic\n+\tQ\"\"\"\n+\tQdef g(x):\n+\tQQ\"\"\"\n+\tQQNested function that returns the same number\n+\tQQWe just multiply by 1.0\n+\tQQso we can write a comment about it\n+\tQQas we need longer blocks\n+\tQQ\"\"\"\n+\tQQreturn 1.0 * x\n+\tQ# Another comment for f(x)\n+\tQ# also spanning multiple lines\n+\tQ# to make a block\n+\tQreturn g(x) * g(x)\n+\tEOF\n+\n+\tgit diff --color --color-moved-ignore-space-delta |\n+\t\tgrep -v \"index\" |\n+\t\ttest_decode_color >actual &&\n+\n+\tq_to_tab <<-\\EOF >expected &&\n+\tdiff --git a/test.py b/test.py\n+\t--- a/test.py\n+\t+++ b/test.py\n+\t@@ -1,24 +1,24 @@\n+\t class test:\n+\t-Qdef f(x):\n+\t-QQ\"\"\"\n+\t-QQA simple python function\n+\t-QQthat returns the square of a number\n+\t-QQAlthough it may not be pythonic\n+\t-QQ\"\"\"\n+\t-QQdef g(x):\n+\t-QQQ\"\"\"\n+\t-QQQNested function that returns the same number\n+\t-QQQWe just multiply by 1.0\n+\t-QQQso we can write a comment about it\n+\t-QQQas we need longer blocks\n+\t-QQQ\"\"\"\n+\t-QQQreturn 1.0 * x\n+\t-QQ# Another comment for f(x)\n+\t-QQ# also spanning multiple lines\n+\t-QQ# to make a block\n+\t-QQreturn g(x) * g(x)\n+\t Qdef h(x):\n+\t QQ# Another function unrelated to the previous\n+\t QQ# but building a block,\n+\t QQ# long enough to call it a block\n+\t QQreturn x * 1.0\n+\t+def f(x):\n+\t+Q\"\"\"\n+\t+QA simple python function\n+\t+Qthat returns the square of a number\n+\t+QAlthough it may not be pythonic\n+\t+Q\"\"\"\n+\t+Qdef g(x):\n+\t+QQ\"\"\"\n+\t+QQNested function that returns the same number\n+\t+QQWe just multiply by 1.0\n+\t+QQso we can write a comment about it\n+\t+QQas we need longer blocks\n+\t+QQ\"\"\"\n+\t+QQreturn 1.0 * x\n+\t+Q# Another comment for f(x)\n+\t+Q# also spanning multiple lines\n+\t+Q# to make a block\n+\t+Qreturn g(x) * g(x)\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.17.0.484.g0c8726318c-goog\n\n"},{"id":"343640","messageId":"20180402224854.86922-4-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-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-02T22:48:50Z","receivedAt":"2018-04-02T22:49:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The diff options are passed to the compare function as\n'hashmap_cmp_fn_data', which are given when the hashmaps\nare initialized.\n\nA later patch will make use of the keydata to signal\ndifferent settings for comparision.\n\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 4c59f5f5d3..52f6e02130 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.484.g0c8726318c-goog\n\n"},{"id":"343641","messageId":"20180402224854.86922-5-sbeller@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-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-02T22:48:51Z","receivedAt":"2018-04-02T22:49:31Z","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 52f6e02130..879b8a5d9d 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.484.g0c8726318c-goog\n\n"},{"id":"343647","messageId":"20180402164742.11d4c5507d6ee737c9935d73@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-1-sbeller@google.com","subject":"Re: [RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-02T23:47:42Z","receivedAt":"2018-04-02T23:47:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 2 Apr 2018 15:48:47 -0700\nStefan Beller wrote:\n\n> This is a re-attempt of [1], which allows the moved code detection to\n> ignore blanks in various modes.\n> \n> patches 1-5 are refactoring, patch 6 adds all existing white space options\n> of regular diff to the move detection. (I am unsure about this patch,\n> as I presume we want to keep the option space at a minimum if possible).\n\nMy preference is to not do this until a need has been demonstrated, but\nthis sounds like it could be useful one day. I'll review the patches\nfrom the viewpoint that we do want this feature.\n\n> The fun is in the last patch, which allows white space sensitive\n> languages to trust the move detection, too. Each block that is marked as\n> moved will have the same delta in {in-, de-}dentation.\n> I would think this mode might be a reasonable default eventually.\n\nThis sounds like a good idea. \"Trust\" is probably too strong a word, but\nI can see this being useful even in non-whitespace-sensitive languages\nwith nested blocks (like C).\n"},{"id":"343648","messageId":"20180402165144.73e701d7816ddfcf00006d95@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-6-sbeller@google.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-02T23:51:44Z","receivedAt":"2018-04-02T23:51:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 2 Apr 2018 15:48:52 -0700\nStefan Beller wrote:\n\n> At the time the move coloring was implemented we thought an enum of modes\n> is the best to configure this feature. However as we want to tack on new\n> features, the enum would grow exponentially.\n> \n> Refactor the code such that features are enabled via bits. Currently we can\n> * activate the move detection,\n> * enable the block detection on top, and\n> * enable the dimming inside a block, though this could be done without\n> block detection as well (mode \"plain, dimmed\")\n\nFirstly, patches 1-4 are obviously correct.\n\nAs for this patch, I don't think that using flags is the right way to do\nthis. We are not under any size pressure for struct diff_options, and\nthe additional options that we plan to add (color-moved-whitespace-flags\nand ignore-space-delta) can easily be additional fields instead.\n"},{"id":"343649","messageId":"CA+P7+xpDfKSvQZ+YktsSSBPTc3JXzc9dR36tohLctfDf04iaOg@mail.gmail.com","threadId":"48205","inReplyTo":"20180402164742.11d4c5507d6ee737c9935d73@google.com","subject":"Re: [RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-04-03T00:03:23Z","receivedAt":"2018-04-03T00:03:49Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Apr 2, 2018 at 4:47 PM, Jonathan Tan wrote:\n> On Mon, 2 Apr 2018 15:48:47 -0700\n> Stefan Beller wrote:\n>\n>> This is a re-attempt of [1], which allows the moved code detection to\n>> ignore blanks in various modes.\n>>\n>> patches 1-5 are refactoring, patch 6 adds all existing white space options\n>> of regular diff to the move detection. (I am unsure about this patch,\n>> as I presume we want to keep the option space at a minimum if possible).\n>\n> My preference is to not do this until a need has been demonstrated, but\n> this sounds like it could be useful one day. I'll review the patches\n> from the viewpoint that we do want this feature.\n>\n>> The fun is in the last patch, which allows white space sensitive\n>> languages to trust the move detection, too. Each block that is marked as\n>> moved will have the same delta in {in-, de-}dentation.\n>> I would think this mode might be a reasonable default eventually.\n>\n> This sounds like a good idea. \"Trust\" is probably too strong a word, but\n> I can see this being useful even in non-whitespace-sensitive languages\n> with nested blocks (like C).\n\nThe ability to detect moved code despite whitespace changes would be\ngood, even while showing diffs with the whitespace intact.\n\nWe may not need *all* the options available, but allowing the internal\nsettings to enable this but have the user-visible controls be limited\nshould be totally fine.\n\nThanks,\nJake\n"},{"id":"343650","messageId":"20180402173135.a6ea2bc1154781abac4dc21b@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-7-sbeller@google.com","subject":"Re: [PATCH 6/7] diff.c: decouple white space treatment for move detection from generic option","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-03T00:31:35Z","receivedAt":"2018-04-03T00:31:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 2 Apr 2018 15:48:53 -0700\nStefan Beller wrote:\n\n> In the original implementation of the move detection logic we assumed that\n> the choice for ignoring white space changes is the same for the move\n> detection as it is for the generic diff. It turns out this is wrong.\n\nI don't think we can say that this is wrong - just that we decided that\nit would be useful to allow different whitespace handling methods when\ncalculating the diff and when detecting moves.\n\n> There are a couple of things where the user wants to input their\n> decision into the diff machinery:\n> \n> * Whether or not to ignore white space for the general diff detection.\n> This is covered with the -w, -b options already.\n> * Whether the move detection ought to pay attention to white spaces\n> in general. And if so, how are white spaces handled for the block\n> detection.\n> \n> One example would be reviewing a current patch under discussion, that moves\n> code around. In that case, you may want to have the general diff machinery\n> not ignore the white spaces (i.e. exact matching), as that will show you\n> the diff as the patch sent. The code moved however may have another\n> indentation level, such that you want to ignore white spaces for the move\n> detection. The code may be in python, such that spaces matter for program\n> flow, though. That is why you'd want each block to have the same change\n> in white space.\n> \n> This patch decouples white space treatment in the general diff machinery\n> from the white space treatment in the move detection.\n> \n> This adds all the the options for ignoring white space that the regular\n> diff machinery has to the move detection, such that we can cover all the\n> cases that were introduced in 7a55427094 (Merge branch\n> 'jk/diff-color-moved-fix', 2017-11-06).\n\nI would just write the above as follows:\n\n Allow the user to specify that whitespace should be ignored\n differently during detection of moved lines than during generation of\n added and removed lines. This is done by providing analogs to the\n --ignore-space-at-eol, -b, and -w options (namely,\n --color-moved-[no-]ignore-space-at-eol ) that affect\n only the color of the output, and making the existing\n --ignore-space-at-eol, -b, and -w options no longer affect the color\n of the output.\n\n(And if the above is not clear enough that this is a\nbackwards-incompatible change, we might need to call it out.)\n\n> The example from above (different lines in the diff with -w for the regular\n> diff) is covered in a test. Convert the existing tests to be more explicit\n> on their choice of white space behavior in the move detection as the tests\n> should not fail if the default is changed.\n\nThis statement is confusing to me - of course the tests should fail,\nsince we changed the defaults. I think it is sufficient to just mention\nthat whitespace handling has to be explicitly specified for the move\ndetection part.\n\n> +--color-moved-[no-]ignore-all-space::\n> +\tIgnore whitespace when comparing lines when performing the move\n> +\tdetection for --color-moved. This ignores differences even if\n> +\tone line has whitespace where the other line has none.\n> +--color-moved-[no-]ignore-space-change::\n> +\tIgnore changes in amount of whitespace when performing the move\n> +\tdetection for --color-moved. This ignores whitespace\n> +\tat line end, and considers all other sequences of one or\n> +\tmore whitespace characters to be equivalent.\n> +--color-moved-[no-]ignore-space-at-eol::\n> +\tIgnore changes in whitespace at EOL when performing the move\n> +\tdetection for --color-moved.\n\nOptional: I would reorder this to be in the same order as the analogous\noptions (--ignore-space-at-eol first, then -b, then -w).\n\n> @@ -720,7 +720,7 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n> \n> \treturn !xdiff_compare_lines(a->es->line, a->es->len,\n> \t\t\t\t b->es->line, b->es->len,\n> -\t\t\t\t diffopt->xdl_opts);\n> +\t\t\t\t diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n> }\n> \n> static struct moved_entry *prepare_entry(struct diff_options *o,\n> @@ -728,8 +728,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 & 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\nThese 2 changes looked suspicious to me at first, but then I saw that\nmoved_entry_cmp() and prepare_entry() are only used during move\ndetection.\n\n> @@ -1419,7 +1422,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\nThis change (removal of -w) looked suspicious to me at first, but then I\nsaw that the \"-w\" was only used to trigger whitespace insensitivity\nduring the move detection phase. (The line in question was moved, so \"-\"\nand \"+\" lines would have been generated in the diff anyway regardless of\nwhether whitespace was ignored.) Same as to the other test\nmodifications.\n\n> +test_expect_success 'move detection only ignores white spaces' '\n\nThis seems to be \"only move detection ignores white spaces\", not as\nwritten. I would title this \"--color-moved-ignore-all-space only ignores\nwhitespace during move detection\".\n\n> +\tq_to_tab <<-\\EOF >function.c &&\n> +\tint func()\n> +\t{\n> +\tQif (foo) {\n> +\tQQ// this part of the function\n> +\tQQ// function will be very long\n> +\tQQ// indeed. We must exceed both\n> +\tQQ// per-line and number of line\n> +\tQQ// minimums\n> +\tQQ;\n> +\tQ}\n> +\tQbaz();\n> +\tQbar();\n> +\tQ// more unrelated stuff\n> +\t}\n> +\tEOF\n\nCould the sample data be more concise? In particular, a few lines should\nbe sufficient for before:\n\n a long line to exceed per-line minimum\n another long line to exceed per-line minimum\n original file\n\nand after:\n\n Qa long line to exceed per-line minimum\n Qanother long line to exceed per-line minimum\n new file\n\nThen the ordinary diff (with -w) will only have \"-\" and \"+\" for\n\"original file\" and \"new file\", and the one with only\n--color-moved-ignore-all-space would have \"-\" and \"+\" for all lines.\n\n> +\t# Make sure we get a different diff using -w (\"moved function header\")\n\n-w is not \"moved function header\", maybe reword.\n"},{"id":"343651","messageId":"20180402174118.d204ec0d4b9d2fa7ebd77739@google.com","threadId":"48205","inReplyTo":"20180402224854.86922-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-03T00:41:18Z","receivedAt":"2018-04-03T00:41:28Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Mon, 2 Apr 2018 15:48:54 -0700\nStefan Beller wrote:\n\n> +struct ws_delta {\n> +\tint deltachars;\n> +\tchar firstchar;\n> +};\n\nI'll just make some overall design comments.\n\nShouldn't this be a string of characters (or a char* and len) and\nwhether it was added or removed? If you're only checking the first\ncharacter, this would not work if the other characters were different.\n\n> @@ -717,10 +752,20 @@ 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 & XDF_WHITESPACE_FLAGS;\n> +\n> +\tif (diffopt->color_moved & 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 diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n> +\t\t\t\t flags);\n> }\n\nI think we should just prohibit combining this with any of the\nwhitespace ignoring flags except for the space-at-eol one. They seem to\ncontradict anyway.\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\n> +\tQacross\n> +\tQfive\n> +\tQlines\n> +\tQBut!\n> +\tQthis\n> +\tQQone\n> +\tQline\n> +\tQQdid\n> +\tQnot\n> +\tQQadjust\n> +\tEOF\n\nDo we need 5 lines? I thought 2 would suffice. (It's the ALNUM_COUNT\nthat matters, as far as I know.) This makes it hard to see that the\n\"But!\" line is the one that counts.\n\n> +test_expect_success 'compare whitespace delta across moved blocks with multiple indentation levels' '\n> +\t# alternative: \"python programmers would love the move detection, too\"\n> +\n> +\tgit reset --hard &&\n> +\tq_to_tab <<-\\EOF >test.py &&\n> +\tclass test:\n> +\tQdef f(x):\n> +\tQQ\"\"\"\n> +\tQQA simple python function\n> +\tQQthat returns the square of a number\n> +\tQQAlthough it may not be pythonic\n> +\tQQ\"\"\"\n> +\tQQdef g(x):\n> +\tQQQ\"\"\"\n> +\tQQQNested function that returns the same number\n> +\tQQQWe just multiply by 1.0\n> +\tQQQso we can write a comment about it\n> +\tQQQas we need longer blocks\n> +\tQQQ\"\"\"\n> +\tQQQreturn 1.0 * x\n> +\tQQ# Another comment for f(x)\n> +\tQQ# also spanning multiple lines\n> +\tQQ# to make a block\n> +\tQQreturn g(x) * g(x)\n> +\tQdef h(x):\n> +\tQQ# Another function unrelated to the previous\n> +\tQQ# but building a block,\n> +\tQQ# long enough to call it a block\n> +\tQQreturn x * 1.0\n> +\tEOF\n\nIf the objective it just to show that the functions f and g are treated\nas one unit despite their lines being of multiple indentation levels,\nthe test file could be much shorter.\n"},{"id":"343752","messageId":"CAGZ79kawikdHRK_HpB53R8c-B3K1JEVSmfR9V27X1=1OuXbmhw@mail.gmail.com","threadId":"48205","inReplyTo":"20180402165144.73e701d7816ddfcf00006d95@google.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-03T18:59:40Z","receivedAt":"2018-04-03T18:59:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 2, 2018 at 4:51 PM, Jonathan Tan wrote:\n> On Mon, 2 Apr 2018 15:48:52 -0700\n> Stefan Beller wrote:\n>\n>> At the time the move coloring was implemented we thought an enum of modes\n>> is the best to configure this feature. However as we want to tack on new\n>> features, the enum would grow exponentially.\n>>\n>> Refactor the code such that features are enabled via bits. Currently we can\n>> * activate the move detection,\n>> * enable the block detection on top, and\n>> * enable the dimming inside a block, though this could be done without\n>> block detection as well (mode \"plain, dimmed\")\n>\n> Firstly, patches 1-4 are obviously correct.\n>\n> As for this patch, I don't think that using flags is the right way to do\n> this. We are not under any size pressure for struct diff_options, and\n> the additional options that we plan to add (color-moved-whitespace-flags\n> and ignore-space-delta) can easily be additional fields instead.\n\nGah! I'll give it a try.\n\nWhen I came to the conclusion that the features of this series is\northogonal to the existing code, bit fields came first to mind.\n\nLet's see if the alternative is easier to read.\n"},{"id":"343754","messageId":"CAGZ79kZ-k7Ho-_Cg3OLLypfjoEn8pTDN7dWpX0=8o+o2BvUcqA@mail.gmail.com","threadId":"48205","inReplyTo":"CA+P7+xpDfKSvQZ+YktsSSBPTc3JXzc9dR36tohLctfDf04iaOg@mail.gmail.com","subject":"Re: [RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-03T19:00:31Z","receivedAt":"2018-04-03T19:00:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">>> The fun is in the last patch, which allows white space sensitive\n>>> languages to trust the move detection, too. Each block that is marked as\n>>> moved will have the same delta in {in-, de-}dentation.\n>>> I would think this mode might be a reasonable default eventually.\n>>\n>> This sounds like a good idea. \"Trust\" is probably too strong a word, but\n>> I can see this being useful even in non-whitespace-sensitive languages\n>> with nested blocks (like C).\n>\n> The ability to detect moved code despite whitespace changes would be\n> good, even while showing diffs with the whitespace intact.\n\nThat is what the last patch is about.\n"},{"id":"343758","messageId":"CAGZ79kYPx=OB92yWFw8W=zJDoJT5BjBd3Q+gt_WS_KuUw299vQ@mail.gmail.com","threadId":"48205","inReplyTo":"20180402174118.d204ec0d4b9d2fa7ebd77739@google.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-03T19:22:32Z","receivedAt":"2018-04-03T19:22:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 2, 2018 at 5:41 PM, Jonathan Tan wrote:\n> On Mon, 2 Apr 2018 15:48:54 -0700\n> Stefan Beller wrote:\n>\n>> +struct ws_delta {\n>> + int deltachars;\n>> + char firstchar;\n>> +};\n>\n> I'll just make some overall design comments.\n>\n> Shouldn't this be a string of characters (or a char* and len) and\n> whether it was added or removed? If you're only checking the first\n> character, this would not work if the other characters were different.\n\nI considered diving into this, but it seemed to be too complicated for\n>95 % of the use cases, which can be approximated in change of the\nfirst character.\n\nBecause if we take a string of characters, we'd also need to take care of\ntricky conversions (e.g. Are 8 white spaces equal to a tab, and if so do\nwe break blocks if one line converts 8 ws to a tab?)\n\nSo I would definitely pursue the string instead of change of first\ncharacter, but what are all the heuristics to put in?\n\nJust to be clear: The string would contain only the change in\nwhite space up front, or would we also somehow store white space\nin other parts?\n\n- # This is a sample comment\n- # across multiple lines\n- # maybe even a license header\n+ # This is a sample comment\n+ # across multiple lines\n+ # maybe even a license header\n\nHow about this?\n\n\n>\n>> @@ -717,10 +752,20 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n>> const struct diff_options *diffopt = hashmap_cmp_fn_data;\n>> const struct moved_entry *a = entry;\n>> const struct moved_entry *b = entry_or_key;\n>> + unsigned flags = diffopt->color_moved & XDF_WHITESPACE_FLAGS;\n>> +\n>> + if (diffopt->color_moved & 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>> - diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n>> + flags);\n>> }\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 to\n> contradict anyway.\n\nok, we can narrow this one down to ignore all white space.\n\n>\n>> +test_expect_success 'compare whitespace delta across moved blocks' '\n>> +\n>> + git reset --hard &&\n>> + q_to_tab <<-\\EOF >text.txt &&\n>> + QIndented\n>> + QText\n>> + Qacross\n>> + Qfive\n>> + Qlines\n>> + QBut!\n>> + Qthis\n>> + QQone\n>> + Qline\n>> + QQdid\n>> + Qnot\n>> + QQadjust\n>> + EOF\n>\n> Do we need 5 lines? I thought 2 would suffice. (It's the ALNUM_COUNT\n> that matters, as far as I know.) This makes it hard to see that the\n> \"But!\" line is the one that counts.\n\nI did not want to go with the bare minimum as then adjusting the minimum\nwould be a pain as these unrelated (to the minimum) test cases would\nbreak.\n\n>\n>> +test_expect_success 'compare whitespace delta across moved blocks with multiple indentation levels' '\n\n>> + EOF\n>\n> If the objective it just to show that the functions f and g are treated\n> as one unit despite their lines being of multiple indentation levels,\n> the test file could be much shorter.\n\nyeah, I noticed that we already test that in the test above where we\nhave that test after the \"But!\", where lines ziggy-zag. Will drop this test.\n"},{"id":"343760","messageId":"87o9j0uljo.fsf@evledraar.gmail.com","threadId":"48205","inReplyTo":"20180402224854.86922-6-sbeller@google.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-03T19:39:23Z","receivedAt":"2018-04-03T19:39:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Apr 02 2018, Stefan Beller wrote:\n\n> At the time the move coloring was implemented we thought an enum of modes\n> is the best to configure this feature. However as we want to tack on new\n> features, the enum would grow exponentially.\n>\n> Refactor the code such that features are enabled via bits. Currently we can\n> * activate the move detection,\n> * enable the block detection on top, and\n> * enable the dimming inside a block, though this could be done without\n> block detection as well (mode \"plain, dimmed\")\n>\n> Choose the flags to not be at bit position 2,3,4 as the next patch\n> will occupy these.\n\nWhen I've been playing with colorMoved the thing I've found really\nconfusing is that the current config has confused two completely\nunrelated things (at least, from a user perspective), what underlying\nalgorithm you use, and how the colors look.\n\nI was helping someone at work the other day where they were trying:\n\n git -c color.diff.new=\"green bold\" \\\n -c color.diff.old=\"red bold\" \\\n -c color.diff.newMoved=\"green\" \\\n -c color.diff.oldMoved=\"red\" \\\n -c diff.colorMoved=plain show \n\nBut what gave better results was:\n\n git -c color.diff.new=\"green bold\" \\\n -c color.diff.old=\"red bold\" \\\n -c color.diff.newMoved=\"green\" \\\n -c color.diff.oldMoved=\"red\" \\\n -c diff.colorMoved=zebra \\\n -c color.diff.oldMovedAlternative=red \\\n -c color.diff.newMovedAlternative=green show \n\nI don't have a public test commit to share (sorry), but I have an\ninternal example where \"plain\" will consider a thing as falling under\ncolor.diff.old OR color.diff.oldMoved, but zebra will consider that\nwhole part only color.diff.old.\n\nI see now that that might be since only the \"zebra\" supports the\n*Alternative that it ends up \"stealing\" chunks from something that would\nhave otherwise been classified differently, so I have no idea if there's\nan easy \"solution\", or if it's even a problem.\n\nSorry about being vague, I just dug this up from some old notes now\nafter this patch jolted my memory about it.\n"},{"id":"343761","messageId":"CAGZ79kaQV-F0by52fbv3fmOH_ZVMU6u=KOkJFxALyHHaH2Enfw@mail.gmail.com","threadId":"48205","inReplyTo":"87o9j0uljo.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-03T19:49:54Z","receivedAt":"2018-04-03T19:50:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Apr 3, 2018 at 12:39 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Mon, Apr 02 2018, Stefan Beller wrote:\n>\n>> At the time the move coloring was implemented we thought an enum of modes\n>> is the best to configure this feature. However as we want to tack on new\n>> features, the enum would grow exponentially.\n>>\n>> Refactor the code such that features are enabled via bits. Currently we can\n>> * activate the move detection,\n>> * enable the block detection on top, and\n>> * enable the dimming inside a block, though this could be done without\n>> block detection as well (mode \"plain, dimmed\")\n>>\n>> Choose the flags to not be at bit position 2,3,4 as the next patch\n>> will occupy these.\n>\n> When I've been playing with colorMoved the thing I've found really\n> confusing is that the current config has confused two completely\n> unrelated things (at least, from a user perspective), what underlying\n> algorithm you use, and how the colors look.\n\nNot sure I follow. The colors are in color.diff.X and the algorithm is in\ndiff.colorMoved, whereas some colors are reused for different algorithms?\n\n>\n> I was helping someone at work the other day where they were trying:\n>\n> git -c color.diff.new=\"green bold\" \\\n> -c color.diff.old=\"red bold\" \\\n> -c color.diff.newMoved=\"green\" \\\n> -c color.diff.oldMoved=\"red\" \\\n> -c diff.colorMoved=plain show \n>\n> But what gave better results was:\n>\n> git -c color.diff.new=\"green bold\" \\\n> -c color.diff.old=\"red bold\" \\\n> -c color.diff.newMoved=\"green\" \\\n> -c color.diff.oldMoved=\"red\" \\\n> -c diff.colorMoved=zebra \\\n> -c color.diff.oldMovedAlternative=red \\\n> -c color.diff.newMovedAlternative=green show \n>\n> I don't have a public test commit to share (sorry), but I have an\n> internal example where \"plain\" will consider a thing as falling under\n> color.diff.old OR color.diff.oldMoved, but zebra will consider that\n> whole part only color.diff.old.\n\nWhat do you mean by \"OR\" ?\nIs the hunk present multiple times and colored one or the other way?\nIs it colored differently in different invocations of Git?\nIs one hunk mixing up both colors?\n\nIs the hunk \"small\" ?\nsmall hunks are un-colored, to avoid showing empty lines\nor closing braces as moved. But plain mode ignores this heuristic.\n\n> I see now that that might be since only the \"zebra\" supports the\n> *Alternative that it ends up \"stealing\" chunks from something that would\n> have otherwise been classified differently, so I have no idea if there's\n> an easy \"solution\", or if it's even a problem.\n\nCan you describe the issue more to see if it is a problem?\n(It sounds like a problem in the documentation/UX to me already\nas the docs could not tell you what to expect)\n\n> Sorry about being vague, I just dug this up from some old notes now\n> after this patch jolted my memory about it.\n\nok.\n"},{"id":"343762","messageId":"CA+P7+xpiTL4jpjFjxu-wStiGado60P2jkBs0XrhkRXfzVp6UqQ@mail.gmail.com","threadId":"48205","inReplyTo":"CAGZ79kZ-k7Ho-_Cg3OLLypfjoEn8pTDN7dWpX0=8o+o2BvUcqA@mail.gmail.com","subject":"Re: [RFC PATCH 0/7] Moved code detection: ignore space on uniform indentation","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2018-04-03T19:55:40Z","receivedAt":"2018-04-03T19:56:08Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Apr 3, 2018 at 12:00 PM, Stefan Beller wrote:\n>>>> The fun is in the last patch, which allows white space sensitive\n>>>> languages to trust the move detection, too. Each block that is marked as\n>>>> moved will have the same delta in {in-, de-}dentation.\n>>>> I would think this mode might be a reasonable default eventually.\n>>>\n>>> This sounds like a good idea. \"Trust\" is probably too strong a word, but\n>>> I can see this being useful even in non-whitespace-sensitive languages\n>>> with nested blocks (like C).\n>>\n>> The ability to detect moved code despite whitespace changes would be\n>> good, even while showing diffs with the whitespace intact.\n>\n> That is what the last patch is about.\n\nRight. I was trying to say \"I agree with the goal, even if we don't\nnecessarily allow every possible white-space + color-moved lines\ncombination\" (i.e. to avoid polluting the option space too much)\n\nThanks,\nJake\n"},{"id":"343766","messageId":"20180403133814.4845c4cc8cc2c9c749e2ada5@google.com","threadId":"48205","inReplyTo":"CAGZ79kYPx=OB92yWFw8W=zJDoJT5BjBd3Q+gt_WS_KuUw299vQ@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-03T20:38:14Z","receivedAt":"2018-04-03T20:38:25Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Tue, 3 Apr 2018 12:22:32 -0700\nStefan Beller wrote:\n\n> On Mon, Apr 2, 2018 at 5:41 PM, Jonathan Tan wrote:\n> > On Mon, 2 Apr 2018 15:48:54 -0700\n> > Stefan Beller wrote:\n> >\n> >> +struct ws_delta {\n> >> + int deltachars;\n> >> + char firstchar;\n> >> +};\n> >\n> > I'll just make some overall design comments.\n> >\n> > Shouldn't this be a string of characters (or a char* and len) and\n> > whether it was added or removed? If you're only checking the first\n> > character, this would not work if the other characters were different.\n> \n> I considered diving into this, but it seemed to be too complicated for\n> >95 % of the use cases, which can be approximated in change of the\n> first character.\n\nIt's true that most use cases can be approximated this way, but I don't\nthink that it's worth the approximation.\n\n> Because if we take a string of characters, we'd also need to take care of\n> tricky conversions (e.g. Are 8 white spaces equal to a tab, and if so do\n> we break blocks if one line converts 8 ws to a tab?)\n\nNo conversions - spaces are spaces and tabs are tabs.\n\n> So I would definitely pursue the string instead of change of first\n> character, but what are all the heuristics to put in?\n\nNo heuristics - a few lines make a block if the same prefix (which\nconsists of all whitespace) was added or removed.\n\n> Just to be clear: The string would contain only the change in\n> white space up front, or would we also somehow store white space\n> in other parts?\n\nOnly change in white space at the start of the line - this option only\nhandles space at the start of the line, right?\n\n> - # This is a sample comment\n> - # across multiple lines\n> - # maybe even a license header\n> + # This is a sample comment\n> + # across multiple lines\n> + # maybe even a license header\n> \n> How about this?\n\nMy understanding is that this patch does not handle this case.\n\n> >> @@ -717,10 +752,20 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n> >> const struct diff_options *diffopt = hashmap_cmp_fn_data;\n> >> const struct moved_entry *a = entry;\n> >> const struct moved_entry *b = entry_or_key;\n> >> + unsigned flags = diffopt->color_moved & XDF_WHITESPACE_FLAGS;\n> >> +\n> >> + if (diffopt->color_moved & 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> >> - diffopt->color_moved & XDF_WHITESPACE_FLAGS);\n> >> + flags);\n> >> }\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 to\n> > contradict anyway.\n> \n> ok, we can narrow this one down to ignore all white space.\n\nWhat do you mean? The rationale for my comment is that I saw that you\nneed to specify a special flag to xdiff_compare_lines if\nCOLOR_MOVED_DELTA_WHITESPACES is set, which could conflict with other\nflags that the user has explicitly set. So avoiding that case entirely\nseems like a good idea, especially since it is logical to do so.\n\n> >> +test_expect_success 'compare whitespace delta across moved blocks' '\n> >> +\n> >> + git reset --hard &&\n> >> + q_to_tab <<-\\EOF >text.txt &&\n> >> + QIndented\n> >> + QText\n> >> + Qacross\n> >> + Qfive\n> >> + Qlines\n> >> + QBut!\n> >> + Qthis\n> >> + QQone\n> >> + Qline\n> >> + QQdid\n> >> + Qnot\n> >> + QQadjust\n> >> + EOF\n> >\n> > Do we need 5 lines? I thought 2 would suffice. (It's the ALNUM_COUNT\n> > that matters, as far as I know.) This makes it hard to see that the\n> > \"But!\" line is the one that counts.\n> \n> I did not want to go with the bare minimum as then adjusting the minimum\n> would be a pain as these unrelated (to the minimum) test cases would\n> break.\n\nThat is true, but it makes the test case harder to read now. If you're\nworried about bumping into the minimum if we do adjust the minimum,\nmaking the lines longer should be sufficient.\n\n> >> +test_expect_success 'compare whitespace delta across moved blocks with multiple indentation levels' '\n> \n> >> + EOF\n> >\n> > If the objective it just to show that the functions f and g are treated\n> > as one unit despite their lines being of multiple indentation levels,\n> > the test file could be much shorter.\n> \n> yeah, I noticed that we already test that in the test above where we\n> have that test after the \"But!\", where lines ziggy-zag. Will drop this test.\n\nOK, sounds good.\n"},{"id":"343767","messageId":"87muykuij7.fsf@evledraar.gmail.com","threadId":"48205","inReplyTo":"CAGZ79kaQV-F0by52fbv3fmOH_ZVMU6u=KOkJFxALyHHaH2Enfw@mail.gmail.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-04-03T20:44:28Z","receivedAt":"2018-04-03T20:44:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Apr 03 2018, Stefan Beller wrote:\n\n> On Tue, Apr 3, 2018 at 12:39 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> On Mon, Apr 02 2018, Stefan Beller wrote:\n>>\n>>> At the time the move coloring was implemented we thought an enum of modes\n>>> is the best to configure this feature. However as we want to tack on new\n>>> features, the enum would grow exponentially.\n>>>\n>>> Refactor the code such that features are enabled via bits. Currently we can\n>>> * activate the move detection,\n>>> * enable the block detection on top, and\n>>> * enable the dimming inside a block, though this could be done without\n>>> block detection as well (mode \"plain, dimmed\")\n>>>\n>>> Choose the flags to not be at bit position 2,3,4 as the next patch\n>>> will occupy these.\n>>\n>> When I've been playing with colorMoved the thing I've found really\n>> confusing is that the current config has confused two completely\n>> unrelated things (at least, from a user perspective), what underlying\n>> algorithm you use, and how the colors look.\n>\n> Not sure I follow. The colors are in color.diff.X and the algorithm is in\n> diff.colorMoved, whereas some colors are reused for different algorithms?\n>\n>>\n>> I was helping someone at work the other day where they were trying:\n>>\n>> git -c color.diff.new=\"green bold\" \\\n>> -c color.diff.old=\"red bold\" \\\n>> -c color.diff.newMoved=\"green\" \\\n>> -c color.diff.oldMoved=\"red\" \\\n>> -c diff.colorMoved=plain show \n>>\n>> But what gave better results was:\n>>\n>> git -c color.diff.new=\"green bold\" \\\n>> -c color.diff.old=\"red bold\" \\\n>> -c color.diff.newMoved=\"green\" \\\n>> -c color.diff.oldMoved=\"red\" \\\n>> -c diff.colorMoved=zebra \\\n>> -c color.diff.oldMovedAlternative=red \\\n>> -c color.diff.newMovedAlternative=green show \n>>\n>> I don't have a public test commit to share (sorry), but I have an\n>> internal example where \"plain\" will consider a thing as falling under\n>> color.diff.old OR color.diff.oldMoved, but zebra will consider that\n>> whole part only color.diff.old.\n>\n> What do you mean by \"OR\" ?\n> Is the hunk present multiple times and colored one or the other way?\n> Is it colored differently in different invocations of Git?\n> Is one hunk mixing up both colors?\n>\n> Is the hunk \"small\" ?\n> small hunks are un-colored, to avoid showing empty lines\n> or closing braces as moved. But plain mode ignores this heuristic.\n>\n>> I see now that that might be since only the \"zebra\" supports the\n>> *Alternative that it ends up \"stealing\" chunks from something that would\n>> have otherwise been classified differently, so I have no idea if there's\n>> an easy \"solution\", or if it's even a problem.\n>\n> Can you describe the issue more to see if it is a problem?\n> (It sounds like a problem in the documentation/UX to me already\n> as the docs could not tell you what to expect)\n>\n>> Sorry about being vague, I just dug this up from some old notes now\n>> after this patch jolted my memory about it.\n\nForget about what I said so far, sorry, that was a really shitty\nreport. I dug into this a bit more and here's a better one.\n\nI still can't share the actual diff I have in front of me (internal\ncode).\n\nCurrently we have plain, zebra & dimmed_zebra, and zebra is the\ndefault.\n\nI got an internal report from someone who had, because zebra looked\ncrappy in his terminal, moved to \"plain\", and was reporting getting\nworse moved diffs as a result.\n\nI found that there's essentially a missing setting between \"plain\" and\n\"zebra\", in git command terms:\n\n # The \"plain\" setting\n git -c diff.colorMoved=true \\\n -c diff.colorMoved=plain \\\n show \n\n # We don't have this, it's \"plain\" but with \"zebra\" heuristics,\n # plain_zebra?\n git -c diff.colorMoved=true \\\n -c color.diff.oldMovedAlternative=\"bold magenta\" \\\n -c color.diff.newMovedAlternative=\"bold yellow\" \\\n -c diff.colorMoved=zebra \\\n show \n\n # The \"zebra\" setting.\n git -c diff.colorMoved=true \\\n -c diff.colorMoved=zebra \\\n show \n\nWhich is what I mean by the current config conflating two (to me)\nunrelated things. One is how we, via any method, detect what's moved or\nnot, and the other is what color/format we use to present this to the\nuser.\n\nYou can feed that plain_zebra invocation input where it'll color-wise\nproduce something that looks *almost* like \"plain\", but will differ (and\nusually be better) in what lines it decides to show as moved, which of\ncourse is due to *MovedAlternative.\n"},{"id":"343768","messageId":"20180403210536.33798-1-sbeller@google.com","threadId":"48205","inReplyTo":"87muykuij7.fsf@evledraar.gmail.com","subject":"[PATCH] diff: add a blocks mode for moved code detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-03T21:05:36Z","receivedAt":"2018-04-03T21:05:57Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Currently we have plain, zebra & dimmed_zebra, and zebra is the\n> default.\n>\n> I got an internal report from someone who had, because zebra looked\n> crappy in his terminal, moved to \"plain\", and was reporting getting\n> worse moved diffs as a result.\n>\n> I found that there's essentially a missing setting between \"plain\" and\n> \"zebra\", in git command terms:\n>\n> # The \"plain\" setting\n> git -c diff.colorMoved=true \\\n> -c diff.colorMoved=plain \\\n> show \n>\n> # We don't have this, it's \"plain\" but with \"zebra\" heuristics,\n> # plain_zebra?\n> git -c diff.colorMoved=true \\\n> -c color.diff.oldMovedAlternative=\"bold magenta\" \\\n> -c color.diff.newMovedAlternative=\"bold yellow\" \\\n> -c diff.colorMoved=zebra \\\n> show \n>\n> # The \"zebra\" setting.\n> git -c diff.colorMoved=true \\\n> -c diff.colorMoved=zebra \\\n> show \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\nOh I see.\n\nReading the docs again, maybe we want to have a \"blocks\" mode,\nthat is zebra with the same color for any block?\n\n> You can feed that plain_zebra invocation input where it'll color-wise\n> produce something that looks *almost* like \"plain\", but will differ (and\n> usually be better) in what lines it decides to show as moved, which of\n> course is due to *MovedAlternative.\n\nI would think this is close to what you want (module implementation errors,\nI did not run/test this code).\n\nOne could also argue that this is *too* weak, as when there are\nmultiple blocks of say 15 chars adjacent, they might be one large block.\n\n---8<----\n\nFrom dde04f6afa35a313fac3575100fe83b554ec2b59 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Tue, 3 Apr 2018 14:03:01 -0700\nSubject: [PATCH] diff: add a blocks mode for moved code detection\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 5 +++++\n diff.c | 4 +++-\n diff.h | 5 +++--\n 3 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex c330c01ff0..abce5142d2 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -268,6 +268,11 @@ 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+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.\n+\tAdjacent blocks cannot be told apart.\n zebra::\n \tBlocks of moved text of at least 20 alphanumeric characters\n \tare detected greedily. The detected blocks are\ndiff --git a/diff.c b/diff.c\nindex 21c3838b25..80dd8cbd9a 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@@ -899,7 +901,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 6bd278aac1..3a228861d9 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -207,8 +207,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-- \n2.17.0.484.g0c8726318c-goog\n\n"},{"id":"344000","messageId":"20180406200443.GE11450@sigill.intra.peff.net","threadId":"48205","inReplyTo":"20180402224854.86922-4-sbeller@google.com","subject":"Re: [PATCH 3/7] diff.c: do not pass diff options as keydata to hashmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-04-06T20:04:43Z","receivedAt":"2018-04-06T20:04:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 02, 2018 at 03:48:50PM -0700, Stefan Beller wrote:\n\n> The diff options are passed to the compare function as\n> 'hashmap_cmp_fn_data', which are given when the hashmaps\n> are initialized.\n> \n> A later patch will make use of the keydata to signal\n> different settings for comparision.\n\nI had to scratch my head here for a moment. Don't we use those options\nas part of the comparison?\n\nI took the \"which\" to mean \"the compare function\", but I think you mean\n\"we pass these diff options already when the hashmap is initialized\".\n\nMaybe something like this would be more clear:\n\n When we initialize the hashmap, we give it a pointer to the\n diff_options, which it then passes along to each call of the\n hashmap_cmp_fn function. There's no need to pass it a second time as\n the \"keydata\" parameter, and our comparison functions never look at\n keydata.\n\n This was a mistake left over from an earlier round of 2e2d5ac184\n (diff.c: color moved lines differently, 2017-06-30), before hashmap\n learned to pass the data pointer for us.\n\n(I'm just guessing on the second paragraph based on a quick look at\ngit-blame and my recollection from the time).\n\n-Peff\n"},{"id":"344006","messageId":"CAGZ79kY+Eke-nZ2R5ULVi1za4e8hvFKzuMQiEzXQF8GNA9=adw@mail.gmail.com","threadId":"48205","inReplyTo":"20180406200443.GE11450@sigill.intra.peff.net","subject":"Re: [PATCH 3/7] diff.c: do not pass diff options as keydata to hashmap","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-06T20:41:53Z","receivedAt":"2018-04-06T20:42:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Hi Jeff,\n\nOn Fri, Apr 6, 2018 at 1:04 PM, Jeff King wrote:\n> On Mon, Apr 02, 2018 at 03:48:50PM -0700, Stefan Beller wrote:\n>\n>> The diff options are passed to the compare function as\n>> 'hashmap_cmp_fn_data', which are given when the hashmaps\n>> are initialized.\n>>\n>> A later patch will make use of the keydata to signal\n>> different settings for comparision.\n>\n> I had to scratch my head here for a moment. Don't we use those options\n> as part of the comparison?\n\nYes we do, but not as passed in here.\n\nStepping back a bit: There are 2 void pointers passed to the cmp function:\n\ntypedef int (*hashmap_cmp_fn)(const void *hashmap_cmp_fn_data,\n const void *entry, const void *entry_or_key,\n const void *keydata);\n\nThe hashmap_cmp_fn_data is the same pointer as we pass as\nthe equals_function data in\n\nextern void hashmap_init(struct hashmap *map,\n hashmap_cmp_fn equals_function,\n const void *equals_function_data,\n size_t initial_size);\n\nwhereas the keydata is passed in either directly when calling cmp directly\nor in hashmap_get_from_hash:\n\nstatic inline void *hashmap_get_from_hash(const struct hashmap *map,\n unsigned int hash,\n const void *keydata)\n{\n struct hashmap_entry key;\n hashmap_entry_init(&key, hash);\n return hashmap_get(map, &key, keydata);\n}\n\nIt turns out we are passing the struct diff_options *o into the cmp\nfunction twice, once via the inits equals_function_data, as well as\nkeydata directly. Omit the direct pass in.\n\nThis is mostly a cleanup as of now, as it turns out I do not need to\nreuse the keydata field anyway.\n\n> I took the \"which\" to mean \"the compare function\", but I think you mean\n> \"we pass these diff options already when the hashmap is initialized\".\n\nOh, yes.\n\n> Maybe something like this would be more clear:\n>\n> When we initialize the hashmap, we give it a pointer to the\n> diff_options, which it then passes along to each call of the\n> hashmap_cmp_fn function. There's no need to pass it a second time as\n> the \"keydata\" parameter, and our comparison functions never look at\n> keydata.\n>\n\nThanks for clearing this up, will take as-is.\n\n> This was a mistake left over from an earlier round of 2e2d5ac184\n> (diff.c: color moved lines differently, 2017-06-30), before hashmap\n> learned to pass the data pointer for us.\n\nThat sounds about right.\n\n>\n> (I'm just guessing on the second paragraph based on a quick look at\n> git-blame and my recollection from the time).\n>\n> -Peff\n\nThanks,\nStefan\n"},{"id":"344011","messageId":"CAGZ79kZe1SEy2PkYGe200KwZHZVieAJJaiWYLD7GadOCCcmeOg@mail.gmail.com","threadId":"48205","inReplyTo":"20180402165144.73e701d7816ddfcf00006d95@google.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-04-06T21:28:40Z","receivedAt":"2018-04-06T21:28:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Apr 2, 2018 at 4:51 PM, Jonathan Tan wrote:\n> On Mon, 2 Apr 2018 15:48:52 -0700\n> Stefan Beller wrote:\n>\n>> At the time the move coloring was implemented we thought an enum of modes\n>> is the best to configure this feature. However as we want to tack on new\n>> features, the enum would grow exponentially.\n>>\n>> Refactor the code such that features are enabled via bits. Currently we can\n>> * activate the move detection,\n>> * enable the block detection on top, and\n>> * enable the dimming inside a block, though this could be done without\n>> block detection as well (mode \"plain, dimmed\")\n>\n> Firstly, patches 1-4 are obviously correct.\n>\n> As for this patch, I don't think that using flags is the right way to do\n> this.\n\nNow that I redid it another way[1], I have the impression that this was the\nright approach, because it allows for a short\n if (o->color_moved)\ncondition. If we treat white spaces separately, then we'd have to\nhave implications such as:\n\n if (some white space option)\n the enum = default if not given explicitely.\n\nwhich we do not need in case of a flags field.\n\n[1] Keeping the enum around and having an extra variable for the\nwhite space related configuration.\n\n> We are not under any size pressure for struct diff_options, and\n> the additional options that we plan to add (color-moved-whitespace-flags\n> and ignore-space-delta) can easily be additional fields instead.\n\nThe traditional white space flags would want to be a field and occupy\nthe same bits in that field for ease of implementation, and the new option\nwould just fit in by picking a new place in the bit field.\n\nThanks,\nStefan\n"},{"id":"344021","messageId":"20180406152734.b36d1301fb49fb6db7307b51@google.com","threadId":"48205","inReplyTo":"CAGZ79kZe1SEy2PkYGe200KwZHZVieAJJaiWYLD7GadOCCcmeOg@mail.gmail.com","subject":"Re: [PATCH 5/7] diff.c: refactor internal representation for coloring moved code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-04-06T22:27:34Z","receivedAt":"2018-04-06T22:27:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Fri, 6 Apr 2018 14:28:40 -0700\nStefan Beller wrote:\n\n> Now that I redid it another way[1], I have the impression that this was the\n> right approach, because it allows for a short\n> if (o->color_moved)\n> condition. If we treat white spaces separately, then we'd have to\n> have implications such as:\n> \n> if (some white space option)\n> the enum = default if not given explicitely.\n> \n> which we do not need in case of a flags field.\n> \n> [1] Keeping the enum around and having an extra variable for the\n> white space related configuration.\n\nAh, I think I see what you mean. In your way, move detection can be\nactivated either by explicitly setting a color-move pattern (e.g.\nzebra), or by setting a move-detection whitespace option, both of which\nwill set bits in the uint32.\n\nAs opposed to my proposed way, where you either have to set the default\nexplicitly (like you describe), or write \"if (o->color_moved ||\no->color_moved_whitespace_handling)\" instead of \"if (o->color_moved)\".\n\nI don't think such an implicit dependence (whitespace option to\ncolor-move pattern) is reason enough to use a bitfield, and I think the\nopposite actually - this dependence should be in fact explicit, not\nimplicit. But I'm open to opinions from others.\n\n> > We are not under any size pressure for struct diff_options, and\n> > the additional options that we plan to add (color-moved-whitespace-flags\n> > and ignore-space-delta) can easily be additional fields instead.\n> \n> The traditional white space flags would want to be a field and occupy\n> the same bits in that field for ease of implementation, and the new option\n> would just fit in by picking a new place in the bit field.\n\nIf we were to use bitfields, this would be the way, yes.\n"}]}