{"thread":{"id":"46275","subject":"[PATCH 0/6] Fixing up sb/diff-color-moved","startedAt":"2017-06-28T00:57:03Z","lastAt":"2017-06-29T23:51:44Z","messageCount":18,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"323401","messageId":"20170628005651.8110-1-sbeller@google.com","threadId":"46275","inReplyTo":null,"subject":"[PATCH 0/6] Fixing up sb/diff-color-moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:45Z","receivedAt":"2017-06-28T00:57:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This goes on top of sb/diff-color-moved.\n\nEach patch is written as if it can go in as a normal patch,\nbut I intend to squash them into the series appropriately if\nthere is more feedback on that series in general.\n\nStefan Beller (6):\n diff.c: factor out shrinking of potential moved line blocks\n diff.c: change the default for move coloring to zebra\n diff.c: better reporting on color.moved bogus configuration\n Documentation/diff: reword color moved\n diff.c: omit uninteresting moved lines\n diff.c: detect blocks despite whitespace changes\n\n Documentation/config.txt | 6 ++--\n Documentation/diff-options.txt | 10 ++++--\n diff.c | 79 ++++++++++++++++++++++++++++--------------\n diff.h | 2 ++\n t/t4015-diff-whitespace.sh | 65 ++++++++++++++++++++++++++++++++++\n 5 files changed, 131 insertions(+), 31 deletions(-)\n\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323402","messageId":"20170628005651.8110-2-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 1/6] diff.c: factor out shrinking of potential moved line blocks","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:46Z","receivedAt":"2017-06-28T00:57:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is cleaner and keeps the rather large function\nthat performs the move detection smaller.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 50 +++++++++++++++++++++++++++++---------------------\n 1 file changed, 29 insertions(+), 21 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 82ace48c38..5311dcf133 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -808,6 +808,33 @@ static void add_lines_to_move_detection(struct diff_options *o,\n \t}\n }\n \n+static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n+\t\t\t\t\t int pmb_nr)\n+{\n+\tint lp, rp;\n+\n+\t/* Shrink the set of potential block to the remaining running */\n+\tfor (lp = 0, rp = pmb_nr - 1; lp <= rp;) {\n+\t\twhile (lp < pmb_nr && pmb[lp])\n+\t\t\tlp++;\n+\t\t/* lp points at the first NULL now */\n+\n+\t\twhile (rp > -1 && !pmb[rp])\n+\t\t\trp--;\n+\t\t/* rp points at the last non-NULL */\n+\n+\t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n+\t\t\tpmb[lp] = pmb[rp];\n+\t\t\tpmb[rp] = NULL;\n+\t\t\trp--;\n+\t\t\tlp++;\n+\t\t}\n+\t}\n+\n+\t/* Remember the number of running sets */\n+\treturn rp + 1;\n+}\n+\n /* Find blocks of moved code, delegate actual coloring decision to helper */\n static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tstruct hashmap *add_lines,\n@@ -822,7 +849,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\tstruct moved_entry *key;\n \t\tstruct moved_entry *match = NULL;\n \t\tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\t\tint i, lp, rp;\n+\t\tint i;\n \n \t\tswitch (l->s) {\n \t\tcase DIFF_SYMBOL_PLUS:\n@@ -864,26 +891,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t}\n \t\t}\n \n-\t\t/* Shrink the set of potential block to the remaining running */\n-\t\tfor (lp = 0, rp = pmb_nr - 1; lp <= rp;) {\n-\t\t\twhile (lp < pmb_nr && pmb[lp])\n-\t\t\t\tlp++;\n-\t\t\t/* lp points at the first NULL now */\n-\n-\t\t\twhile (rp > -1 && !pmb[rp])\n-\t\t\t\trp--;\n-\t\t\t/* rp points at the last non-NULL */\n-\n-\t\t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n-\t\t\t\tpmb[lp] = pmb[rp];\n-\t\t\t\tpmb[rp] = NULL;\n-\t\t\t\trp--;\n-\t\t\t\tlp++;\n-\t\t\t}\n-\t\t}\n-\n-\t\t/* Remember the number of running sets */\n-\t\tpmb_nr = rp + 1;\n+\t\tpmb_nr = shrink_potential_moved_blocks(pmb, pmb_nr);\n \n \t\tif (pmb_nr == 0) {\n \t\t\t/*\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323403","messageId":"20170628005651.8110-3-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 2/6] diff.c: change the default for move coloring to zebra","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:47Z","receivedAt":"2017-06-28T00:57:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Introduce a new mode COLOR_MOVED_DEFAULT, which is the same as\nCOLOR_MOVED_ZEBRA. But having two different symbols allows us to\ndifferentiate them in the code.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/diff-options.txt | 3 +++\n diff.c | 13 ++++++++++++-\n diff.h | 1 +\n 3 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 058c8014ed..d2c6a60af2 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -243,6 +243,9 @@ endif::git-diff[]\n --\n no::\n \tMoved lines are not highlighted.\n+default::\n+\tIs a synonym for `zebra`. This may change to more sensible modes\n+\tin the future.\n plain::\n \tAny line that is added in one location and was removed\n \tin another location will be colored with 'color.diff.newMoved'.\ndiff --git a/diff.c b/diff.c\nindex 5311dcf133..31cdec05ac 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -256,12 +256,23 @@ int git_diff_heuristic_config(const char *var, const char *value, void *cb)\n \n static int parse_color_moved(const char *arg)\n {\n+\tint v = git_parse_maybe_bool(arg);\n+\n+\tif (v != -1) {\n+\t\tif (v == 0)\n+\t\t\treturn COLOR_MOVED_NO;\n+\t\telse if (v == 1)\n+\t\t\treturn COLOR_MOVED_DEFAULT;\n+\t}\n+\n \tif (!strcmp(arg, \"no\"))\n \t\treturn COLOR_MOVED_NO;\n \telse if (!strcmp(arg, \"plain\"))\n \t\treturn COLOR_MOVED_PLAIN;\n \telse if (!strcmp(arg, \"zebra\"))\n \t\treturn COLOR_MOVED_ZEBRA;\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 \telse\n@@ -4654,7 +4665,7 @@ int diff_opt_parse(struct diff_options *options,\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\t\toptions->color_moved = COLOR_MOVED_ZEBRA_DIM;\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 \telse if (skip_prefix(arg, \"--color-moved=\", &arg)) {\ndiff --git a/diff.h b/diff.h\nindex 98abd75521..9298d211d7 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -192,6 +192,7 @@ struct diff_options {\n \t\tCOLOR_MOVED_NO = 0,\n \t\tCOLOR_MOVED_PLAIN = 1,\n \t\tCOLOR_MOVED_ZEBRA = 2,\n+\t\tCOLOR_MOVED_DEFAULT = 2,\n \t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n \t} color_moved;\n };\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323404","messageId":"20170628005651.8110-4-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 3/6] diff.c: better reporting on color.moved bogus configuration","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:48Z","receivedAt":"2017-06-28T00:57:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n diff.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 31cdec05ac..015c854530 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -276,7 +276,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 -1;\n+\t\treturn error(_(\"color moved setting must be one of 'default', 'plain', 'zebra', 'dimmed_zebra'\"));\n }\n \n int git_diff_ui_config(const char *var, const char *value, void *cb)\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323405","messageId":"20170628005651.8110-5-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 4/6] Documentation/diff: reword color moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:49Z","receivedAt":"2017-06-28T00:57:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is easier for the casual reader.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/config.txt | 6 ++++--\n Documentation/diff-options.txt | 7 ++++---\n 2 files changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 29e0b9fa69..3d89be2d84 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1052,8 +1052,10 @@ This does not affect linkgit:git-format-patch[1] or the\n command line with the `--color[=]` option.\n \n diff.colorMoved::\n-\tIf set moved lines in a diff are colored differently,\n-\tfor details see '--color-moved' in linkgit:git-diff[1].\n+\tIf set to either a valid `` or a true value, moved lines\n+\tin a diff are colored differently, for details of valid modes\n+\tsee '--color-moved' in linkgit:git-diff[1]. If simply set to\n+\ttrue the default color mode will be used.\n \n color.diff.::\n \tUse customized color for diff colorization. `` specifies\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex d2c6a60af2..d4dc46ee2f 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -252,9 +252,10 @@ plain::\n \tSimilarly 'color.diff.oldMoved' will be used for removed lines\n \tthat are added somewhere else in the diff.\n zebra::\n-\tBlocks of moved code are detected. The detected blocks are\n-\tpainted using the 'color.diff.{old,new}Moved' alternating with\n-\t'color.diff.{old,new}MovedAlternative'.\n+\tBlocks of moved code are detected greedily. The detected blocks are\n+\tpainted using either the 'color.diff.{old,new}Moved' color or\n+\t'color.diff.{old,new}MovedAlternative'. The change between\n+\tthe two colors indicates that a new block was detected.\n dimmed_zebra::\n \tSimilar to 'zebra', but additional dimming of uninteresting parts\n \tof moved code is performed. The bordering lines of two adjacent\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323406","messageId":"20170628005651.8110-6-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 5/6] diff.c: omit uninteresting moved lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:50Z","receivedAt":"2017-06-28T00:57:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"It is useful to have moved lines colored, but there are annoying corner\ncases, such as a single line moved, that is very common. For example\nin a typical patch of C code, we have closing braces that end statement\nblocks or functions.\n\nWhile it is technically true that these lines are moved as they show up\nelsewhere, it is harmful for the review as the reviewers attention is\ndrawn to such a minor side annoyance.\n\nOne of the first solutions considered, started off by these hypothesis':\n (a) The more blocks of the same code we have, the less interesting it is.\n (b) The shorter a block of moved code is the less need of markup there\n is for review.\n\n Introduce a heuristic which drops any potential moved blocks if their\n length is shorter than the number of potential moved blocks.\n\n This heuristic was chosen as it is agnostic of the content (in other\n languages or contents to manage, we may have longer lines, e.g. in\n shell the closing of a condition is already 2 characters. Thinking\n about Latex documents tracked in Git, there can also be some\n boilerplate code with lots of characters) while taking both\n hypothesis' into account. An alternative considered was the number\n of non-whitespace characters in a line for example.\n\nThinking further about this, a linear relation between number of moved\nblocks and number of lines of code seems like a bad idea to start with.\nSo let's start with a simpler solution of hardcoding the number of lines\nto 3.\n\nNote, that the length is applied across all blocks to find the 'lonely'\nblocks that pollute new code, but do not interfere with a permutated\nblock where each permutation has less lines than 3.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 11 ++++++++++-\n diff.h | 1 +\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 015c854530..1d93e98e3a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -853,7 +853,8 @@ 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;\n+\tint n, flipped_block = 1, block_length = 0;\n+\n \n \tfor (n = 0; n < o->emitted_symbols->nr; n++) {\n \t\tstruct hashmap *hm = NULL;\n@@ -880,11 +881,19 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t}\n \n \t\tif (!match) {\n+\t\t\tif (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH) {\n+\t\t\t\tfor (i = 0; i < block_length + 1; i++) {\n+\t\t\t\t\tl = &o->emitted_symbols->buf[n - i];\n+\t\t\t\t\tl->flags &= ~DIFF_SYMBOL_MOVED_LINE;\n+\t\t\t\t}\n+\t\t\t}\n \t\t\tpmb_nr = 0;\n+\t\t\tblock_length = 0;\n \t\t\tcontinue;\n \t\t}\n \n \t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n+\t\tblock_length++;\n \n \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\ndiff --git a/diff.h b/diff.h\nindex 9298d211d7..cc1224a93b 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -195,6 +195,7 @@ struct diff_options {\n \t\tCOLOR_MOVED_DEFAULT = 2,\n \t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n \t} color_moved;\n+\t#define COLOR_MOVED_MIN_BLOCK_LENGTH 3\n };\n \n void diff_emit_submodule_del(struct diff_options *o, const char *line);\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323407","messageId":"20170628005651.8110-7-sbeller@google.com","threadId":"46275","inReplyTo":"20170628005651.8110-1-sbeller@google.com","subject":"[PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T00:56:51Z","receivedAt":"2017-06-28T00:57:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Reuse the compare function from the hash map instead of calling the\ncompare function directly. Then we pick the correct compare function\nwhen told to compare ignoring white space.\n\nSigned-off-by: Stefan Beller \n---\n diff.c | 3 +--\n t/t4015-diff-whitespace.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 2 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1d93e98e3a..4bcf938e3a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -903,8 +903,7 @@ 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 &&\n-\t\t\t !emitted_symbol_cmp(pnext->es, l, o)) {\n+\t\t\tif (pnext && !hm->cmpfn(pnext, match, NULL)) {\n \t\t\t\tpmb[i] = p->next_line;\n \t\t\t} else {\n \t\t\t\tpmb[i] = NULL;\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex ae8c686f3c..c3b697411a 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1317,6 +1317,71 @@ test_expect_success 'no effect from --color-moved with --word-diff' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'move detection ignoring whitespace ' '\n+\tgit reset --hard &&\n+\tcat <<\\EOF >lines.txt &&\n+line 1\n+line 2\n+line 3\n+line 4\n+line 5\n+line 6\n+line 7\n+EOF\n+\tgit add lines.txt &&\n+\tgit commit -m \"add poetry\" &&\n+\tcat <<\\EOF >lines.txt &&\n+\tline 5\n+\tline 6\n+\tline 7\n+line 1\n+line 2\n+line 3\n+line 4\n+EOF\n+\ttest_config color.diff.oldMoved \"magenta\" &&\n+\ttest_config color.diff.newMoved \"cyan\" &&\n+\tgit diff HEAD --no-renames --color-moved| test_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\tindex 734156d..eb89ead 100644\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,7 +1,7 @@\n+\t+\tline 5\n+\t+\tline 6\n+\t+\tline 7\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t-line 5\n+\t-line 6\n+\t-line 7\n+\tEOF\n+\ttest_cmp expected actual &&\n+\n+\tgit diff HEAD --no-renames -w --color-moved| test_decode_color >actual &&\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/lines.txt b/lines.txt\n+\tindex 734156d..eb89ead 100644\n+\t--- a/lines.txt\n+\t+++ b/lines.txt\n+\t@@ -1,7 +1,7 @@\n+\t+\tline 5\n+\t+\tline 6\n+\t+\tline 7\n+\t line 1\n+\t line 2\n+\t line 3\n+\t line 4\n+\t-line 5\n+\t-line 6\n+\t-line 7\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'move detection with submodules' '\n \ttest_create_repo bananas &&\n \techo ripe >bananas/recipe &&\n-- \n2.13.0.31.g9b732c453e\n\n"},{"id":"323409","messageId":"xmqq60fgztud.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"20170628005651.8110-3-sbeller@google.com","subject":"Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T03:14:50Z","receivedAt":"2017-06-28T03:15:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Introduce a new mode COLOR_MOVED_DEFAULT, which is the same as\n> COLOR_MOVED_ZEBRA. But having two different symbols allows us to\n> differentiate them in the code.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/diff-options.txt | 3 +++\n> diff.c | 13 ++++++++++++-\n> diff.h | 1 +\n> 3 files changed, 16 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 058c8014ed..d2c6a60af2 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -243,6 +243,9 @@ endif::git-diff[]\n> --\n> no::\n> \tMoved lines are not highlighted.\n> +default::\n> +\tIs a synonym for `zebra`. This may change to more sensible modes\n> +\tin the future.\n\n\"to a more sensible mode\"?\n\nThis is part of the choice for\n\n\t--color-moved[=]::\n\nand the current text does not exactly say what happens when =\nis omitted. \n\nI am guessing that the intent is to behave as if \"=default\" is given\nwhen it happens; this new entry would be a good place to mention it.\n\n\tdefault::\n\t\tUses a sensible default mode (currently `zebra`).\n\t\tGiving the `--color-moved` option without an\n\t\texplicit `=` also behaves like this.\n\nor something like that, perhaps.\n\nThe \"diff.colorMoved\" configuration is now a bool-or-string; does it\nneed to be documented as such in Documentation/config.txt?\n\n\tdiff.colorMoved::\n\t\tWhen set to `false`, moved lines are not treated any\n\t\tdifferently. When set to any one of the valid\n\t\t`` for `--color-moved=` option for `git\n\t\tdiff` familly of commands, they behave as if\n\t\t`--color-moved=` option was given from the\n\t\tcommand line. Setting it to `true` has the same\n\t\teffect as setting it to `default`.\n\nAs the configuration can express everything without the optional\nboolness, it may not be worth describing it. I dunno.\n\n> diff --git a/diff.c b/diff.c\n> index 5311dcf133..31cdec05ac 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -256,12 +256,23 @@ int git_diff_heuristic_config(const char *var, const char *value, void *cb)\n> \n> static int parse_color_moved(const char *arg)\n> {\n> +\tint v = git_parse_maybe_bool(arg);\n> +\n> +\tif (v != -1) {\n> +\t\tif (v == 0)\n> +\t\t\treturn COLOR_MOVED_NO;\n> +\t\telse if (v == 1)\n> +\t\t\treturn COLOR_MOVED_DEFAULT;\n> +\t}\n> +\n\nThis is not wrong per se, but\n\n\tswitch (git_parse_maybe_bool(arg)) {\n\tcase 0:\n\t\treturn COLOR_MOVED_NO;\n\tcase 1:\n\t\treturn COLOR_MOVED_DEFAULT;\n\tdefault:\n\t\tbreak;\n\t}\n\nwithout an extra variable \"v\" may be easier to follow.\n\n> @@ -4654,7 +4665,7 @@ int diff_opt_parse(struct diff_options *options,\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\t\toptions->color_moved = COLOR_MOVED_ZEBRA_DIM;\n> +\t\t\toptions->color_moved = COLOR_MOVED_DEFAULT;\n\nThis part made me look at the hunk with wider context. \n\nThis code is reacting to \"--color-moved\" (no arguments) and\ndiff_color_moved_default presumably comes from the configuration.\nWhen the configuration says diff.colorMoved is 'false' by default,\nthe \"--color-moved\" option from the command line needs to trump it,\nbut we do not have any mode given (other than the configuration\nsaying \"no, no, no, we do not want color-moved at all!\"), so we\nchoose the default setting. Which is correct but was a bit tricky\nto reason about.\n\n> diff --git a/diff.h b/diff.h\n> index 98abd75521..9298d211d7 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -192,6 +192,7 @@ struct diff_options {\n> \t\tCOLOR_MOVED_NO = 0,\n> \t\tCOLOR_MOVED_PLAIN = 1,\n> \t\tCOLOR_MOVED_ZEBRA = 2,\n> +\t\tCOLOR_MOVED_DEFAULT = 2,\n> \t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n> \t} color_moved;\n> };\n\nHmph. I would have expected that COLOR_MOVED_DEFAULT would not be\npart of the enum, but a \n\n #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n\nI do not have a strong preference either way; it was just a bit\nunexpected.\n\nThanks.\n\n\n"},{"id":"323410","messageId":"xmqq1sq4ztcq.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"20170628005651.8110-5-sbeller@google.com","subject":"Re: [PATCH 4/6] Documentation/diff: reword color moved","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T03:25:25Z","receivedAt":"2017-06-28T03:25:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This is easier for the casual reader.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> Documentation/config.txt | 6 ++++--\n> Documentation/diff-options.txt | 7 ++++---\n> 2 files changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 29e0b9fa69..3d89be2d84 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1052,8 +1052,10 @@ This does not affect linkgit:git-format-patch[1] or the\n> command line with the `--color[=]` option.\n> \n> diff.colorMoved::\n> -\tIf set moved lines in a diff are colored differently,\n> -\tfor details see '--color-moved' in linkgit:git-diff[1].\n> +\tIf set to either a valid `` or a true value, moved lines\n> +\tin a diff are colored differently, for details of valid modes\n> +\tsee '--color-moved' in linkgit:git-diff[1]. If simply set to\n> +\ttrue the default color mode will be used.\n\nAh, I was ahead of myself on a review on an earlier patch. The\nabove is OK but it may want to say what happens when set to false.\n\n> \n> color.diff.::\n> \tUse customized color for diff colorization. `` specifies\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index d2c6a60af2..d4dc46ee2f 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -252,9 +252,10 @@ plain::\n> \tSimilarly 'color.diff.oldMoved' will be used for removed lines\n> \tthat are added somewhere else in the diff.\n> zebra::\n> -\tBlocks of moved code are detected. The detected blocks are\n> -\tpainted using the 'color.diff.{old,new}Moved' alternating with\n> -\t'color.diff.{old,new}MovedAlternative'.\n> +\tBlocks of moved code are detected greedily. The detected blocks are\n> +\tpainted using either the 'color.diff.{old,new}Moved' color or\n> +\t'color.diff.{old,new}MovedAlternative'. The change between\n> +\tthe two colors indicates that a new block was detected.\n> dimmed_zebra::\n> \tSimilar to 'zebra', but additional dimming of uninteresting parts\n> \tof moved code is performed. The bordering lines of two adjacent\n"},{"id":"323411","messageId":"xmqqwp7wyei3.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"20170628005651.8110-6-sbeller@google.com","subject":"Re: [PATCH 5/6] diff.c: omit uninteresting moved lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T03:31:32Z","receivedAt":"2017-06-28T03:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> It is useful to have moved lines colored, but there are annoying corner\n> cases, such as a single line moved, that is very common. For example\n> in a typical patch of C code, we have closing braces that end statement\n> blocks or functions.\n>\n> While it is technically true that these lines are moved as they show up\n> elsewhere, it is harmful for the review as the reviewers attention is\n> drawn to such a minor side annoyance.\n>\n> One of the first solutions considered, started off by these hypothesis':\n\nHypotheses is the plural form of that word, I think.\n\n> (a) The more blocks of the same code we have, the less interesting it is.\n> (b) The shorter a block of moved code is the less need of markup there\n> is for review.\n>\n> Introduce a heuristic which drops any potential moved blocks if their\n> length is shorter than the number of potential moved blocks.\n>\n> This heuristic was chosen as it is agnostic of the content (in other\n> languages or contents to manage, we may have longer lines, e.g. in\n> shell the closing of a condition is already 2 characters. Thinking\n> about Latex documents tracked in Git, there can also be some\n> boilerplate code with lots of characters) while taking both\n> hypothesis' into account. An alternative considered was the number\n> of non-whitespace characters in a line for example.\n\nIt was puzzling what the above two paragraphs were. I took (a) and\n(b) were the hypotheses, and the two above, and also the next\nparagraphs, were the design that fell out of them. But that is not\nwhat is happening. You changed your mind and settled on the design\nin the next paragraph.\n\nPerhaps we can do without all of the \"I thought about this but it\ndidn't make sense\" that is longer than the solution in the patch?\n\n> Thinking further about this, a linear relation between number of moved\n> blocks and number of lines of code seems like a bad idea to start with.\n> So let's start with a simpler solution of hardcoding the number of lines\n> to 3.\n>\n> Note, that the length is applied across all blocks to find the 'lonely'\n> blocks that pollute new code, but do not interfere with a permutated\n> block where each permutation has less lines than 3.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 11 ++++++++++-\n> diff.h | 1 +\n> 2 files changed, 11 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 015c854530..1d93e98e3a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -853,7 +853,8 @@ 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;\n> +\tint n, flipped_block = 1, block_length = 0;\n> +\n> \n> \tfor (n = 0; n < o->emitted_symbols->nr; n++) {\n> \t\tstruct hashmap *hm = NULL;\n> @@ -880,11 +881,19 @@ static void mark_color_as_moved(struct diff_options *o,\n> \t\t}\n> \n> \t\tif (!match) {\n> +\t\t\tif (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH) {\n> +\t\t\t\tfor (i = 0; i < block_length + 1; i++) {\n> +\t\t\t\t\tl = &o->emitted_symbols->buf[n - i];\n> +\t\t\t\t\tl->flags &= ~DIFF_SYMBOL_MOVED_LINE;\n> +\t\t\t\t}\n> +\t\t\t}\n> \t\t\tpmb_nr = 0;\n> +\t\t\tblock_length = 0;\n> \t\t\tcontinue;\n> \t\t}\n> \n> \t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n> +\t\tblock_length++;\n> \n> \t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n> \t\t\tcontinue;\n> diff --git a/diff.h b/diff.h\n> index 9298d211d7..cc1224a93b 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -195,6 +195,7 @@ struct diff_options {\n> \t\tCOLOR_MOVED_DEFAULT = 2,\n> \t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n> \t} color_moved;\n> +\t#define COLOR_MOVED_MIN_BLOCK_LENGTH 3\n> };\n> \n> void diff_emit_submodule_del(struct diff_options *o, const char *line);\n"},{"id":"323412","messageId":"xmqqshikye0o.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"20170628005651.8110-7-sbeller@google.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T03:41:59Z","receivedAt":"2017-06-28T03:42:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Reuse the compare function from the hash map instead of calling the\n> compare function directly. Then we pick the correct compare function\n> when told to compare ignoring white space.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 3 +--\n> t/t4015-diff-whitespace.sh | 65 ++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 66 insertions(+), 2 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 1d93e98e3a..4bcf938e3a 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -903,8 +903,7 @@ 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 &&\n> -\t\t\t !emitted_symbol_cmp(pnext->es, l, o)) {\n> +\t\t\tif (pnext && !hm->cmpfn(pnext, match, NULL)) {\n> \t\t\t\tpmb[i] = p->next_line;\n> \t\t\t} else {\n> \t\t\t\tpmb[i] = NULL;\n\nI presume that this makes the use of emitted_symbol_cmp() vs\nemitted_symbol_cmp_no_ws() consistent with the remainder of the\nfile.\n\nLooking at the implementation of get_ws_cleaned_string() that is the\nworkhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong\nthings for various \"ignore whitespace\" options (i.e. there is only\none implementation, while \"git diff\" family takes things like\n\"ignore space change\", \"ignore all whitespace\", etc.), though.\n\n"},{"id":"323415","messageId":"xmqq37akya3j.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"xmqqshikye0o.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T05:06:40Z","receivedAt":"2017-06-28T05:06:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Looking at the implementation of get_ws_cleaned_string() that is the\n> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong\n> things for various \"ignore whitespace\" options (i.e. there is only\n> one implementation, while \"git diff\" family takes things like\n> \"ignore space change\", \"ignore all whitespace\", etc.), though.\n\nThis probably deserves a bit more illustration of how I envision the\ncode should evolve.\n\nIn the longer term, I would prefer to see emitted_symbol_cmp_no_ws()\nto go and instead emitted_symbol_cmp() to take the diff options so\nthat it can change the behaviour of the comparison function based on\nthe -w/-b/--ignore-space-at-eol/etc. settings. And compare two strings\nin place.\n\nFor that, you may need a helper function that takes a pointer to a\ncharacter pointer, picks the next byte that matters while advancing\nthe pointer, and returns that byte. The emitted_symbol_cmp(a, b)\nwhich is not used for real comparison (i.e. ordering to see if a\nsorts earlier than b) but for equivalence (i.e. considering various\nwhitespace-ignoring settings, does a and b matfch?) may become\nsomething like:\n\n int\n emitted_symbol_eqv(struct emitted_diff_symbol *a,\n struct emitted_diff_symbol *b,\n const void *keydata) {\n struct diff_options *diffopt = keydata;\n const char *ap = a->line;\n const char *bp = b->line;\n\n while (1) {\n int ca, cb;\n ca = next_byte(&ap, diffopt);\n cb = next_byte(&bp, diffopt);\n if (ca != cb)\n return 1; /* differs */\n if (!ca)\n return 0;\n };\n } \n\nwhere next_byte() may look like:\n\n static int\n next_byte(const char **cp, struct diff_options *diffopt)\n { \n int retval;\n\n again:\n retval = **cp;\n if (!retval)\n return retval; /* end of string */\n if (!isspace(retval)) {\n (*cp)++; /* advance */\n return retval;\n }\n\n switch (ignore_space_type(diffopt)) {\n case NOT_IGNORING:\n (*cp)++; /* advance */\n return retval;\n case IGNORE_SPACE_CHANGE:\n while (**cp && isspace(**cp))\n (*cp)++; /* squash consecutive spaces */\n return ' '; /* normalize spaces with a SP */\n case IGNORE_ALL_SPACE:\n (*cp)++; /* advance */\n goto again;\n ... other cases here ...\n }\n }\n\n\n"},{"id":"323438","messageId":"CAGZ79kY2qhaefcjFJLDAuhFB_ZngGw7bKnzv-hA-5hmCfH3KOA@mail.gmail.com","threadId":"46275","inReplyTo":"xmqq60fgztud.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T19:54:33Z","receivedAt":"2017-06-28T19:54:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 27, 2017 at 8:14 PM, Junio C Hamano wrote:\n>> diff --git a/diff.h b/diff.h\n>> index 98abd75521..9298d211d7 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -192,6 +192,7 @@ struct diff_options {\n>> COLOR_MOVED_NO = 0,\n>> COLOR_MOVED_PLAIN = 1,\n>> COLOR_MOVED_ZEBRA = 2,\n>> + COLOR_MOVED_DEFAULT = 2,\n>> COLOR_MOVED_ZEBRA_DIM = 3,\n>> } color_moved;\n>> };\n>\n> Hmph. I would have expected that COLOR_MOVED_DEFAULT would not be\n> part of the enum, but a\n>\n> #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n>\n> I do not have a strong preference either way; it was just a bit\n> unexpected.\n\nYes it is not as one would expect.\nOriginally I thought it could buy us some more protection via\nsmart static analysis (to be invented yet), but now I am not so\nsure any more. Changed it to the #define.\n\nThanks.\n"},{"id":"323439","messageId":"CAGZ79kYXzMkO0jtAbCS3f57NtXFHqxVtFUVgq=wNC8PDrfa1dA@mail.gmail.com","threadId":"46275","inReplyTo":"xmqqwp7wyei3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 5/6] diff.c: omit uninteresting moved lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T20:00:09Z","receivedAt":"2017-06-28T20:00:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 27, 2017 at 8:31 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> It is useful to have moved lines colored, but there are annoying corner\n>> cases, such as a single line moved, that is very common. For example\n>> in a typical patch of C code, we have closing braces that end statement\n>> blocks or functions.\n>>\n>> While it is technically true that these lines are moved as they show up\n>> elsewhere, it is harmful for the review as the reviewers attention is\n>> drawn to such a minor side annoyance.\n>>\n>> One of the first solutions considered, started off by these hypothesis':\n>\n> Hypotheses is the plural form of that word, I think.\n>\n>> (a) The more blocks of the same code we have, the less interesting it is.\n>> (b) The shorter a block of moved code is the less need of markup there\n>> is for review.\n>>\n>> Introduce a heuristic which drops any potential moved blocks if their\n>> length is shorter than the number of potential moved blocks.\n>>\n>> This heuristic was chosen as it is agnostic of the content (in other\n>> languages or contents to manage, we may have longer lines, e.g. in\n>> shell the closing of a condition is already 2 characters. Thinking\n>> about Latex documents tracked in Git, there can also be some\n>> boilerplate code with lots of characters) while taking both\n>> hypothesis' into account. An alternative considered was the number\n>> of non-whitespace characters in a line for example.\n>\n> It was puzzling what the above two paragraphs were. I took (a) and\n> (b) were the hypotheses, and the two above, and also the next\n> paragraphs, were the design that fell out of them. But that is not\n> what is happening. You changed your mind and settled on the design\n> in the next paragraph.\n\nYes, I somehow want to say:\n\n \"What is implemented in this patch is stupid. And I know it, but I\n know no smarter idea. This is what I thought was smarter, maybe\n someone in the future can be inspired by this, at least.\"\n\n> Perhaps we can do without all of the \"I thought about this but it\n> didn't make sense\" that is longer than the solution in the patch?\n\nAs I do changes based on your responses, I want to squash\nthese patches sent out last night into the original patch, so I'll butcher\nthe commit message to be way smaller\n"},{"id":"323440","messageId":"CAGZ79kbsAi95+ZgjMem+MViKiXby7gZnGyg4Hbua5xw6TaPz_w@mail.gmail.com","threadId":"46275","inReplyTo":"xmqq37akya3j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-28T20:36:56Z","receivedAt":"2017-06-28T20:37:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Looking at the implementation of get_ws_cleaned_string() that is the\n>> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong\n>> things for various \"ignore whitespace\" options (i.e. there is only\n>> one implementation, while \"git diff\" family takes things like\n>> \"ignore space change\", \"ignore all whitespace\", etc.), though.\n>\n> This probably deserves a bit more illustration of how I envision the\n> code should evolve.\n>\n> In the longer term, I would prefer to see emitted_symbol_cmp_no_ws()\n> to go and instead emitted_symbol_cmp() to take the diff options\n> so\n> that it can change the behaviour of the comparison function based on\n> the -w/-b/--ignore-space-at-eol/etc. settings. And compare two strings\n> in place.\n\nok, in-place is no problem. But passing down the diff options into the\ncompare function is a bit hard.\n\nOriginally I wanted to do that, see prep work in [1], but Jeff explained that\nthe additional pointer in the compare function is **not** supposed to be\na additional payload (such as the diff options specifying the white space\noptions.)\n\n[1] https://public-inbox.org/git/20170512200244.25245-1-sbeller@google.com/\n\nHowever as we no settled on the struct emitted_diff_symbol,\nthat has a 'flags' field in there, which ought to contain everything we\nknow about whitespace settings, we should be able to do that from there.\n\n> emitted_symbol_eqv(struct emitted_diff_symbol *a,\n> struct emitted_diff_symbol *b,\n> const void *keydata) {\n> struct diff_options *diffopt = keydata;\n\nThe prep work mentioned, would allow for this, as keydata\nwould be passed through as-is in all calls of the hashmap API,\nsuch that the user can decide if they use it as the actual 'keydata'\nor rather as an additional payload.\n\nThanks for outlining the idea in code, but maybe we need to\nreconsider the hashmap API before that.\n\nBy not considering the change in the hashmap API, the current\nimplementation tried to get away by having different compare functions.\n\nThanks,\nStefan\n"},{"id":"323444","messageId":"xmqqpodnvmmw.fsf@gitster.mtv.corp.google.com","threadId":"46275","inReplyTo":"CAGZ79kbsAi95+ZgjMem+MViKiXby7gZnGyg4Hbua5xw6TaPz_w@mail.gmail.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-28T21:16:23Z","receivedAt":"2017-06-28T21:16:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Originally I wanted to do that, see prep work in [1], but Jeff explained that\n> the additional pointer in the compare function is **not** supposed to be\n> a additional payload (such as the diff options specifying the white space\n> options.)\n>\n> [1] https://public-inbox.org/git/20170512200244.25245-1-sbeller@google.com/\n\nAh, yes, keydata is a wrong thing to use to give additional hint to\nthe eqv function. We do need to fix the function signature of\nhashmap_cmp_fn. It is common for us to want to use a single\nimplementation of an eqv function that can be configured to behave\nslightly differently but the customization will stay the same\nthroughout the lifetime of a hashmap. IOW, hashmap_init() needs to\nbe able to take a pointer to such a configuration data\n(e.g. diff_options), and then the comparison made between two\nelements that hash to the same bucket needs to be given not just the\ntwo elements but also that configuration data to tweak the function.\n\n"},{"id":"323498","messageId":"CAGZ79kaq2ym9aSAcHXRSd=onkqjoO-2_0B8PHed2j4q_9gwvCA@mail.gmail.com","threadId":"46275","inReplyTo":"xmqq37akya3j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-06-29T21:01:44Z","receivedAt":"2017-06-29T21:01:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Looking at the implementation of get_ws_cleaned_string() that is the\n>> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong\n>> things for various \"ignore whitespace\" options (i.e. there is only\n>> one implementation, while \"git diff\" family takes things like\n>> \"ignore space change\", \"ignore all whitespace\", etc.), though.\n>\n> This probably deserves a bit more illustration of how I envision the\n> code should evolve.\n>\n> In the longer term, I would prefer to see emitted_symbol_cmp_no_ws()\n> to go and instead emitted_symbol_cmp() to take the diff options so\n> that it can change the behaviour of the comparison function based on\n> the -w/-b/--ignore-space-at-eol/etc. settings. And compare two strings\n> in place.\n>\n> For that, you may need a helper function that takes a pointer to a\n> character pointer, picks the next byte that matters while advancing\n> the pointer, and returns that byte. The emitted_symbol_cmp(a, b)\n> which is not used for real comparison (i.e. ordering to see if a\n> sorts earlier than b) but for equivalence (i.e. considering various\n> whitespace-ignoring settings, does a and b matfch?) may become\n> something like:\n>\n> int\n> emitted_symbol_eqv(struct emitted_diff_symbol *a,\n> struct emitted_diff_symbol *b,\n> const void *keydata) {\n> struct diff_options *diffopt = keydata;\n> const char *ap = a->line;\n> const char *bp = b->line;\n>\n> while (1) {\n> int ca, cb;\n> ca = next_byte(&ap, diffopt);\n> cb = next_byte(&bp, diffopt);\n> if (ca != cb)\n> return 1; /* differs */\n> if (!ca)\n> return 0;\n> };\n> }\n>\n> where next_byte() may look like:\n>\n> static int\n> next_byte(const char **cp, struct diff_options *diffopt)\n> {\n> int retval;\n>\n> again:\n> retval = **cp;\n> if (!retval)\n> return retval; /* end of string */\n> if (!isspace(retval)) {\n> (*cp)++; /* advance */\n> return retval;\n> }\n>\n> switch (ignore_space_type(diffopt)) {\n> case NOT_IGNORING:\n> (*cp)++; /* advance */\n> return retval;\n> case IGNORE_SPACE_CHANGE:\n> while (**cp && isspace(**cp))\n> (*cp)++; /* squash consecutive spaces */\n> return ' '; /* normalize spaces with a SP */\n> case IGNORE_ALL_SPACE:\n> (*cp)++; /* advance */\n> goto again;\n> ... other cases here ...\n> }\n> }\n>\n>\n\nI just rebased the diff series on top of the hashmap series and now\nI want to implement the compare function based on the new data\nfeature. So I thought I might start off this proposal, as you seem to\nhave good taste for how to approach problems.\n\nIt turns out that the code here is rather a very loose proposal,\nnot to be copied literally, because of these constraints:\n* When dealing with user content, we do not have C-strings,\n but memory + length, such that next_byte also needs to be\n aware of the end pointer.\n* The ignore_space_type() as well as these constants do not exist\n as-is, I think the cleanest for now is to parse diffopt->xdl_opts via\n DIFF_XDL_TST\n\nThanks,\nStefan\n"},{"id":"323512","messageId":"CAPc5daXBprMyWeQmcUem_Yw9bsyTD8-C818+t94cy5K3onw8Og@mail.gmail.com","threadId":"46275","inReplyTo":"CAGZ79kaq2ym9aSAcHXRSd=onkqjoO-2_0B8PHed2j4q_9gwvCA@mail.gmail.com","subject":"Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-06-29T23:51:18Z","receivedAt":"2017-06-29T23:51:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Jun 29, 2017 at 2:01 PM, Stefan Beller wrote:\n> On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>> This probably deserves a bit more illustration of how I envision the\n>> code should evolve.\n>> ...\n> It turns out that the code here is rather a very loose proposal,\n> not to be copied literally,\n\nYeah, whenever I say \"illustration\", do not take it as anything more than\nscribbling on the back of an envelope, whose primary purpose is to show\nsome idea (in this case, the central idea is that a helper function that lets\nyou ask \"what's the next byte that matters? tell me also when you run out\"\nis all you need to do an in-place comparison with various \"ignore\" modes;\nby the way, that applies also to ignore-case comparison if there is a need\nto support it).\n\nThe implementation detail to support that central idea is left\nto the readers. ;-)\n\nThanks.\n"}]}