{"thread":{"id":"49410","subject":"[RFC PATCH 1/3] xdiff-interface: make xdl_blankline() available","startedAt":"2018-09-24T10:06:25Z","lastAt":"2019-01-10T18:39:20Z","messageCount":44,"participants":["Phillip Wood","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"358745","messageId":"20180924100604.32208-2-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"[RFC PATCH 1/3] xdiff-interface: make xdl_blankline() available","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-24T10:06:02Z","receivedAt":"2018-09-24T10:06:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis will be used by the move detection code.\n\nSigned-off-by: Phillip Wood \n---\n xdiff-interface.c | 5 +++++\n xdiff-interface.h | 5 +++++\n 2 files changed, 10 insertions(+)\n\ndiff --git a/xdiff-interface.c b/xdiff-interface.c\nindex 9315bc0ede..eceabfa72d 100644\n--- a/xdiff-interface.c\n+++ b/xdiff-interface.c\n@@ -308,6 +308,11 @@ int xdiff_compare_lines(const char *l1, long s1,\n \treturn xdl_recmatch(l1, s1, l2, s2, flags);\n }\n \n+int xdiff_is_blankline(const char *l1, long s1, long flags)\n+{\n+\treturn xdl_blankline(l1, s1, flags);\n+}\n+\n int git_xmerge_style = -1;\n \n int git_xmerge_config(const char *var, const char *value, void *cb)\ndiff --git a/xdiff-interface.h b/xdiff-interface.h\nindex 135fc05d72..d0008b016f 100644\n--- a/xdiff-interface.h\n+++ b/xdiff-interface.h\n@@ -45,4 +45,9 @@ extern int xdiff_compare_lines(const char *l1, long s1,\n */\n extern unsigned long xdiff_hash_string(const char *s, size_t len, long flags);\n \n+/*\n+ * Returns 1 if the line is blank, taking XDF_WHITESPACE_FLAGS into account\n+ */\n+extern int xdiff_is_blankline(const char *s, long len, long flags);\n+\n #endif\n-- \n2.19.0\n\n"},{"id":"358746","messageId":"20180924100604.32208-3-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"[RFC PATCH 2/3] diff.c: remove unused variables","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-24T10:06:03Z","receivedAt":"2018-09-24T10:06:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThe string lengths are not used in cmp_in_block_with_wsd() so lets\nremove them.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9393993e33..0a652e28d4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -789,7 +789,6 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\t\t\t int n)\n {\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\tint al = cur->es->len, cl = l->len;\n \tconst char *a = cur->es->line,\n \t\t *b = match->es->line,\n \t\t *c = l->line;\n@@ -823,13 +822,10 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t */\n \n \twslen = strlen(pmb->wsd->string);\n-\tif (pmb->wsd->current_longer) {\n+\tif (pmb->wsd->current_longer)\n \t\tc += wslen;\n-\t\tcl -= wslen;\n-\t} else {\n+\telse\n \t\ta += wslen;\n-\t\tal -= wslen;\n-\t}\n \n \tif (strcmp(a, c))\n \t\treturn 1;\n-- \n2.19.0\n\n"},{"id":"358747","messageId":"20180924100604.32208-1-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":null,"subject":"[RFC PATCH 0/3] diff --color-moved-ws: allow mixed spaces and tabs in indentation change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-24T10:06:01Z","receivedAt":"2018-09-24T10:06:25Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen trying out the new --color-moved-ws=allow-indentation-change I\nwas disappointed to discover it did not work if the indentation\ncontains a mix of spaces and tabs. This series adds a new option that\ndoes. It's and RFC as there are some open questions about how to\nproceed, see the last patch for details. Once there's a clearer way\nforward I'll reroll with some documentation changes.\n\nPhillip Wood (3):\n xdiff-interface: make xdl_blankline() available\n diff.c: remove unused variables\n diff: add --color-moved-ws=allow-mixed-indentation-change\n\n diff.c | 124 ++++++++++++++++++++++++++++++++-----\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 89 ++++++++++++++++++++++++++\n xdiff-interface.c | 5 ++\n xdiff-interface.h | 5 ++\n 5 files changed, 208 insertions(+), 16 deletions(-)\n\n-- \n2.19.0\n\n"},{"id":"358748","messageId":"20180924100604.32208-4-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"[RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-24T10:06:04Z","receivedAt":"2018-09-24T10:06:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThis adds another mode for highlighting lines that have moved with an\nindentation change. Unlike the existing\n--color-moved-ws=allow-indentation-change setting this mode uses the\nvisible change in the indentation to group lines, rather than the\nindentation string. This means it works with files that use a mix of\ntabs and spaces for indentation and can cope with whitespace errors\nwhere there is a space before a tab (it's the job of\n--ws-error-highlight to deal with those errors, it should affect the\nmove detection). It will also group the lines either\nside of a blank line if their indentation change matches so short\nlines followed by a blank line followed by more lines with the same\nindentation change will be correctly highlighted.\n\nThis is a RFC as there are a number of questions about how to proceed\nfrom here:\n 1) Do we need a second option or should this implementation replace\n --color-moved-ws=allow-indentation-change. I'm unclear if that mode\n has any advantages for some people. There seems to have been an\n intention [1] to get it working with mixes of tabs and spaces but\n nothing ever came of it.\n 2) If we keep two options what should this option be called, the name\n is long and ambiguous at the moment - mixed could refer to mixed\n indentation length rather than a mix of tabs and spaces.\n 3) Should we support whitespace flags with this mode?\n --ignore-space-at-eol and --ignore-cr-at eol would be fairly simple\n to support and I can see a use for them, --ignore-all-space and\n --ignore-space-change would need some changes to xdiff to allow them\n to apply only after the indentation. I think --ignore-blank-lines\n would need a bit of work to get it working as well. (Note the\n existing mode does not support any of these flags either)\n\n[1] https://public-inbox.org/git/CAGZ79kasAqE+=7ciVrdjoRdu0UFjVBr8Ma502nw+3hZL=ebXYQ@mail.gmail.com/\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 122 +++++++++++++++++++++++++++++++++----\n diff.h | 1 +\n t/t4015-diff-whitespace.sh | 89 +++++++++++++++++++++++++++\n 3 files changed, 199 insertions(+), 13 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 0a652e28d4..45f33daa60 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -304,7 +304,11 @@ static int parse_color_moved_ws(const char *arg)\n \t\telse if (!strcmp(sb.buf, \"ignore-all-space\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE;\n \t\telse if (!strcmp(sb.buf, \"allow-indentation-change\"))\n-\t\t\tret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n+\t\t\tret = COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n+\t\t\t (ret & ~COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE);\n+\t\telse if (!strcmp(sb.buf, \"allow-mixed-indentation-change\"))\n+\t\t\tret = COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE |\n+\t\t\t (ret & ~COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE);\n \t\telse\n \t\t\terror(_(\"ignoring unknown color-moved-ws mode '%s'\"), sb.buf);\n \n@@ -314,6 +318,9 @@ static int parse_color_moved_ws(const char *arg)\n \tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n \t (ret & XDF_WHITESPACE_FLAGS))\n \t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n+\telse if ((ret & COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) &&\n+\t\t (ret & XDF_WHITESPACE_FLAGS))\n+\t\tdie(_(\"color-moved-ws: allow-mixed-indentation-change cannot be combined with other white space modes\"));\n \n \tstring_list_clear(&l, 0);\n \n@@ -763,11 +770,65 @@ struct moved_entry {\n * comparision is longer than the second.\n */\n struct ws_delta {\n-\tchar *string;\n+\tunion {\n+\t\tint delta;\n+\t\tchar *string;\n+\t};\n \tunsigned int current_longer : 1;\n+\tunsigned int have_string : 1;\n };\n #define WS_DELTA_INIT { NULL, 0 }\n \n+static int compute_mixed_ws_delta(const struct emitted_diff_symbol *a,\n+\t\t\t\t const struct emitted_diff_symbol *b,\n+\t\t\t\t int *delta)\n+{\n+\tunsigned int i = 0, j = 0;\n+\tint la, lb;\n+\tint ta = a->flags & WS_TAB_WIDTH_MASK;\n+\tint tb = b->flags & WS_TAB_WIDTH_MASK;\n+\tconst char *sa = a->line;\n+\tconst char *sb = b->line;\n+\n+\tif (xdiff_is_blankline(sa, a->len, 0) &&\n+\t xdiff_is_blankline(sb, b->len, 0)) {\n+\t\t*delta = INT_MIN;\n+\t\treturn 1;\n+\t}\n+\n+\t/* skip any \\v \\f \\r at start of indentation */\n+\twhile (sa[i] == '\\f' || sa[i] == '\\v' ||\n+\t (sa[i] == '\\r' && i < a->len - 1))\n+\t\ti++;\n+\twhile (sb[j] == '\\f' || sb[j] == '\\v' ||\n+\t (sb[j] == '\\r' && j < b->len - 1))\n+\t\tj++;\n+\n+\tfor (la = 0; ; i++) {\n+\t\tif (sa[i] == ' ')\n+\t\t\tla++;\n+\t\telse if (sa[i] == '\\t')\n+\t\t\tla = ((la + ta) / ta) * ta;\n+\t\telse\n+\t\t\tbreak;\n+\t}\n+\tfor (lb = 0; ; j++) {\n+\t\tif (sb[j] == ' ')\n+\t\t\tlb++;\n+\t\telse if (sb[j] == '\\t')\n+\t\t\tlb = ((lb + tb) / tb) * tb;\n+\t\telse\n+\t\t\tbreak;\n+\t}\n+\tif (a->s == DIFF_SYMBOL_PLUS)\n+\t\t*delta = la - lb;\n+\telse\n+\t\t*delta = lb - la;\n+\n+\treturn (a->len - i == b->len - j) &&\n+\t\t!memcmp(sa + i, sb + j, a->len - i);\n+}\n+\n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \t\t\t const struct emitted_diff_symbol *b,\n \t\t\t struct ws_delta *out)\n@@ -778,6 +839,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \n \tout->string = xmemdupz(longer->line, d);\n \tout->current_longer = (a == longer);\n+\tout->have_string = 1;\n \n \treturn !strncmp(longer->line + d, shorter->line, shorter->len);\n }\n@@ -820,15 +882,34 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t * To do so we need to compare 'l' to 'cur', adjusting the\n \t * one of them for the white spaces, depending which was longer.\n \t */\n+\tif (o->color_moved_ws_handling &\n+\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n+\t\twslen = strlen(pmb->wsd->string);\n+\t\tif (pmb->wsd->current_longer)\n+\t\t\tc += wslen;\n+\t\telse\n+\t\t\ta += wslen;\n \n-\twslen = strlen(pmb->wsd->string);\n-\tif (pmb->wsd->current_longer)\n-\t\tc += wslen;\n-\telse\n-\t\ta += wslen;\n+\t\tif (strcmp(a, c))\n+\t\t\treturn 1;\n \n-\tif (strcmp(a, c))\n-\t\treturn 1;\n+\t\treturn 0;\n+\t} else if (o->color_moved_ws_handling &\n+\t\t COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n+\t\tint delta;\n+\n+\t\tif (!compute_mixed_ws_delta(cur->es, l, &delta))\n+\t\t return 1;\n+\n+\t\tif (pmb->wsd->delta == INT_MIN) {\n+\t\t\tpmb->wsd->delta = delta;\n+\t\t\treturn 0;\n+\t\t}\n+\n+\t\treturn !(delta == pmb->wsd->delta || delta == INT_MIN);\n+\t} else {\n+\t\tBUG(\"no color_moved_ws_allow_indentation_change set\");\n+\t}\n \n \treturn 0;\n }\n@@ -845,7 +926,8 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n \t\t\t & XDF_WHITESPACE_FLAGS;\n \n \tif (diffopt->color_moved_ws_handling &\n-\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n+\t COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n \t\t/*\n \t\t * As there is not specific white space config given,\n \t\t * we'd need to check for a new block, so ignore all\n@@ -953,7 +1035,8 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n \t\t\tpmb[i] = pmb[i]->next_line;\n \t\t} else {\n \t\t\tif (pmb[i]->wsd) {\n-\t\t\t\tfree(pmb[i]->wsd->string);\n+\t\t\t\tif (pmb[i]->wsd->have_string)\n+\t\t\t\t\tfree(pmb[i]->wsd->string);\n \t\t\t\tFREE_AND_NULL(pmb[i]->wsd);\n \t\t\t}\n \t\t\tpmb[i] = NULL;\n@@ -1066,7 +1149,8 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tcontinue;\n \n \t\tif (o->color_moved_ws_handling &\n-\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n+\t\t COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n \t\t\tpmb_advance_or_null_multi_match(o, match, hm, pmb, pmb_nr, n);\n \t\telse\n \t\t\tpmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n@@ -1088,6 +1172,17 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\tpmb[pmb_nr++] = match;\n \t\t\t\t\t} else\n \t\t\t\t\t\tfree(wsd);\n+\t\t\t\t} else if (o->color_moved_ws_handling &\n+\t\t\t\t\t COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n+\t\t\t\t\tint delta;\n+\n+\t\t\t\t\tif (compute_mixed_ws_delta(l, match->es, &delta)) {\n+\t\t\t\t\t\tstruct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n+\t\t\t\t\t\twsd->delta = delta;\n+\t\t\t\t\t\twsd->have_string = 0;\n+\t\t\t\t\t\tmatch->wsd = wsd;\n+\t\t\t\t\t\tpmb[pmb_nr++] = match;\n+\t\t\t\t\t}\n \t\t\t\t} else {\n \t\t\t\t\tpmb[pmb_nr++] = match;\n \t\t\t\t}\n@@ -5740,7 +5835,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t\t\tstruct hashmap add_lines, del_lines;\n \n \t\t\tif (o->color_moved_ws_handling &\n-\t\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\t (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n+\t\t\t COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n \t\t\t\to->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n \n \t\t\thashmap_init(&del_lines, moved_entry_cmp, o, 0);\ndiff --git a/diff.h b/diff.h\nindex 5e6bcf0926..03628cda45 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -217,6 +217,7 @@ struct diff_options {\n \n \t/* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n \t#define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)\n+\t#define COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE (1<<6)\n \tint color_moved_ws_handling;\n };\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 41facf7abf..737dbd4a42 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1902,4 +1902,93 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n \ttest_i18ngrep allow-indentation-change err\n '\n \n+NUL=''\n+test_expect_success 'compare mixed whitespace delta across moved blocks' '\n+\n+\tgit reset --hard &&\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\t${NUL}\n+\t____too short without\n+\t${NUL}\n+\t____being grouped across blank line\n+\t${NUL}\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n+\t____Indented text to\n+\t_Q____be further indented by four spaces across\n+\t____Qseveral lines\n+\tQQ____These two lines have had their\n+\t____indentation reduced by four spaces\n+\tQdifferent indentation change\n+\t____too short\n+\tEOF\n+\n+\tgit add text.txt &&\n+\tgit commit -m \"add text.txt\" &&\n+\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n+\tQIndented text to\n+\tQQbe further indented by four spaces across\n+\tQ____several lines\n+\t${NUL}\n+\tQQtoo short without\n+\t${NUL}\n+\tQQbeing grouped across blank line\n+\t${NUL}\n+\tQ_QThese two lines have had their\n+\tindentation reduced by four spaces\n+\tQQdifferent indentation change\n+\t__Qtoo short\n+\tEOF\n+\n+\tgit -c color.diff.whitespace=\"normal red\" \\\n+\t\t-c core.whitespace=space-before-tab \\\n+\t\tdiff --color --color-moved --ws-error-highlight=all \\\n+\t\t--color-moved-ws=allow-mixed-indentation-change >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n+\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,16 +1,16 @@\n+\t-\n+\t- too short without\n+\t-\n+\t- being grouped across blank line\n+\t-\n+\t context\n+\t lines\n+\t to\n+\t anchor\n+\t- Indented text to\n+\t- \t be further indented by four spaces across\n+\t- \tseveral lines\n+\t-\t\t These two lines have had their\n+\t- indentation reduced by four spaces\n+\t-\tdifferent indentation change\n+\t- too short\n+\t+\tIndented text to\n+\t+\t\tbe further indented by four spaces across\n+\t+\t several lines\n+\t+\n+\t+\t\ttoo short without\n+\t+\n+\t+\t\tbeing grouped across blank line\n+\t+\n+\t+\t \tThese two lines have had their\n+\t+indentation reduced by four spaces\n+\t+\t\tdifferent indentation change\n+\t+ \ttoo short\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.19.0\n\n"},{"id":"358751","messageId":"c5edcdcb-0c15-0c03-2dd0-4c7c8c0289ec@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH 0/3] diff --color-moved-ws: allow mixed spaces and tabs in indentation change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-09-24T11:03:20Z","receivedAt":"2018-09-24T11:03:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/09/2018 11:06, Phillip Wood wrote:\n> From: Phillip Wood \n> \n> When trying out the new --color-moved-ws=allow-indentation-change I\n> was disappointed to discover it did not work if the indentation\n> contains a mix of spaces and tabs. This series adds a new option that\n> does. It's and RFC as there are some open questions about how to\n> proceed, see the last patch for details. Once there's a clearer way\n> forward I'll reroll with some documentation changes.\n> \n\nI should have said that this series is based on top of fab01ec52e\n(\"diff: fix --color-moved-ws=allow-indentation-change\", 2018-09-04), the\nresult merges into current master without conflicts.\n\nBest Wishes\n\nPhillip\n\n> Phillip Wood (3):\n> xdiff-interface: make xdl_blankline() available\n> diff.c: remove unused variables\n> diff: add --color-moved-ws=allow-mixed-indentation-change\n> \n> diff.c | 124 ++++++++++++++++++++++++++++++++-----\n> diff.h | 1 +\n> t/t4015-diff-whitespace.sh | 89 ++++++++++++++++++++++++++\n> xdiff-interface.c | 5 ++\n> xdiff-interface.h | 5 ++\n> 5 files changed, 208 insertions(+), 16 deletions(-)\n> \n\n"},{"id":"358807","messageId":"CAGZ79kbFwfFBT9auh-KYwyHVnVX9hyBO3h7=P7-GcmE-4JOA4w@mail.gmail.com","threadId":"49410","inReplyTo":"20180924100604.32208-2-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH 1/3] xdiff-interface: make xdl_blankline() available","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-24T23:19:25Z","receivedAt":"2018-09-24T23:19:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 24, 2018 at 3:06 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> This will be used by the move detection code.\n>\n> Signed-off-by: Phillip Wood \n> ---\n> xdiff-interface.c | 5 +++++\n> xdiff-interface.h | 5 +++++\n> 2 files changed, 10 insertions(+)\n>\n> diff --git a/xdiff-interface.c b/xdiff-interface.c\n> index 9315bc0ede..eceabfa72d 100644\n> --- a/xdiff-interface.c\n> +++ b/xdiff-interface.c\n> @@ -308,6 +308,11 @@ int xdiff_compare_lines(const char *l1, long s1,\n> return xdl_recmatch(l1, s1, l2, s2, flags);\n> }\n>\n> +int xdiff_is_blankline(const char *l1, long s1, long flags)\n> +{\n> + return xdl_blankline(l1, s1, flags);\n> +}\n> +\n> int git_xmerge_style = -1;\n>\n> int git_xmerge_config(const char *var, const char *value, void *cb)\n> diff --git a/xdiff-interface.h b/xdiff-interface.h\n> index 135fc05d72..d0008b016f 100644\n> --- a/xdiff-interface.h\n> +++ b/xdiff-interface.h\n> @@ -45,4 +45,9 @@ extern int xdiff_compare_lines(const char *l1, long s1,\n> */\n> extern unsigned long xdiff_hash_string(const char *s, size_t len, long flags);\n>\n> +/*\n> + * Returns 1 if the line is blank, taking XDF_WHITESPACE_FLAGS into account\n\npresumably in the flags field.\n\n> + */\n> +extern int xdiff_is_blankline(const char *s, long len, long flags);\n\nWe also have\n int ws_blank_line(const char *, int, int)\nthat looks very similar, but works slightly differently.\ngrep.c has\n static int is_empty_line(const char *bol, const char *eol)\n {\n while (bol < eol && isspace(*bol))\n bol++;\n return bol == eol;\n }\n\npretty.c also has a is_blank_line() (and some stale comment in\nthat file refers to it as is_empty_line, 77356122443\n(pretty: make the skip_blank_lines() function public,\n2016-06-22)\n\nWould we be able to unify all these down to one or two\nfunctions? (Maybe all can use the new xdiff function?)\nIt seems as if we're reinventing the wheel a couple times\nin our code base.\n\nStefan\n"},{"id":"358811","messageId":"CAGZ79kZjAaLE7G=q9sBeEL_+Q2ufYBTn6p9TDCF8cYFd3k+0oQ@mail.gmail.com","threadId":"49410","inReplyTo":"20180924100604.32208-4-phillip.wood@talktalk.net","subject":"Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-25T01:07:04Z","receivedAt":"2018-09-25T01:07:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 24, 2018 at 3:06 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> This adds another mode for highlighting lines that have moved with an\n> indentation change. Unlike the existing\n> --color-moved-ws=allow-indentation-change setting this mode uses the\n> visible change in the indentation to group lines, rather than the\n> indentation string.\n\nWow! Thanks for putting this RFC out.\nMy original vision was to be useful to python users as well,\nwhich counts 1 tab as 8 spaces IIUC.\n\nThe \"visual\" indentation you mention here sounds like\na tab is counted as \"up to the next position of (n-1) % 8\",\ni.e. stop at positions 8, 16, 24... which would not be pythonic,\nbut useful in e.g. our code base.\n\n> This means it works with files that use a mix of\n> tabs and spaces for indentation and can cope with whitespace errors\n> where there is a space before a tab\n\nCool!\n\n> (it's the job of\n> --ws-error-highlight to deal with those errors, it should affect the\n> move detection).\n\nNot sure I understand this side note. So --ws-error-highlight can\nhighlight them, but the move detection should *not*(?) be affected\nby the highlighted parts, or it should do things differently on\nwhether --ws-error-highlight is given?\n\n> It will also group the lines either\n> side of a blank line if their indentation change matches so short\n> lines followed by a blank line followed by more lines with the same\n> indentation change will be correctly highlighted.\n\nThat sounds very useful (at least for my editor, that strips\nblank lines to be empty lines), but I would think this feature is\nworth its own commit/patch.\n\nI wonder how much this feature is orthogonal to the existing\nproblem of detecting the moved indented blocks (existing\nallow-indentation-change vs the new feature discussed first\nabove)\n\n>\n> This is a RFC as there are a number of questions about how to proceed\n> from here:\n> 1) Do we need a second option or should this implementation replace\n> --color-moved-ws=allow-indentation-change. I'm unclear if that mode\n> has any advantages for some people. There seems to have been an\n> intention [1] to get it working with mixes of tabs and spaces but\n> nothing ever came of it.\n\nOh, yeah, I was working on that, but dropped the ball.\n\nI am not sure what the best end goal is, or if there are many different\nmodes that are useful to different target audiences.\nMy own itch at the time was (de-/)in-dented code from refactoring\npatches for git.git and JGit (so Java, C, shell); and I think not hurting\npython would also be good.\n\nignoring the mixture of ws seems like it would also cater free text or\nother more exotic languages.\n\nWhat is your use case, what kind of content do you process that\nthis patch would help you?\n\nI am not overly attached to the current implementation of\n --color-moved-ws=allow-indentation-change,\nand I think Junio has expressed the fear of \"too many options\"\nalready in this problem space, so if possible I would extend/replace\nthe current option.\n\n> 2) If we keep two options what should this option be called, the name\n> is long and ambiguous at the moment - mixed could refer to mixed\n> indentation length rather than a mix of tabs and spaces.\n\nLet's first read the code to have an opinion, or re-state the question\nfrom above (\"What is this used for?\") as I could imagine one of the\nmodes could be \"ws-pythonic\" and allow for whitespace indentation\nthat would have the whole block count as an indented by the same\namount, (e.g. if you wrap a couple functions in python by a class).\n\n> +++ b/diff.c\n> @@ -304,7 +304,11 @@ static int parse_color_moved_ws(const char *arg)\n> else if (!strcmp(sb.buf, \"ignore-all-space\"))\n> ret |= XDF_IGNORE_WHITESPACE;\n> else if (!strcmp(sb.buf, \"allow-indentation-change\"))\n> - ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n> + ret = COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n> + (ret & ~COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE);\n\nSo this RFC lets \"allow-indentation-change\" override\n\"allow-mixed-indentation-change\" and vice versa. That\nalso solves the issue of configuring one and giving the other\nas a command line option. Nice.\n\n> if ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n> (ret & XDF_WHITESPACE_FLAGS))\n> die(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n> + else if ((ret & COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) &&\n> + (ret & XDF_WHITESPACE_FLAGS))\n> + die(_(\"color-moved-ws: allow-mixed-indentation-change cannot be combined with other white space modes\"));\n\nDo we want to open a bit mask for all indentation change options? e.g.\n#define COLOR_MOVED_WS_INDENTATION_MASK \\\n (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE | \\\n COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE)\n\n> @@ -763,11 +770,65 @@ struct moved_entry {\n> * comparision is longer than the second.\n> */\n> struct ws_delta {\n> - char *string;\n> + union {\n> + int delta;\n> + char *string;\n> + };\n> unsigned int current_longer : 1;\n> + unsigned int have_string : 1;\n> };\n> #define WS_DELTA_INIT { NULL, 0 }\n>\n> +static int compute_mixed_ws_delta(const struct emitted_diff_symbol *a,\n> + const struct emitted_diff_symbol *b,\n> + int *delta)\n> +{\n> + unsigned int i = 0, j = 0;\n> + int la, lb;\n> + int ta = a->flags & WS_TAB_WIDTH_MASK;\n> + int tb = b->flags & WS_TAB_WIDTH_MASK;\n> + const char *sa = a->line;\n> + const char *sb = b->line;\n> +\n> + if (xdiff_is_blankline(sa, a->len, 0) &&\n> + xdiff_is_blankline(sb, b->len, 0)) {\n> + *delta = INT_MIN;\n> + return 1;\n> + }\n> +\n> + /* skip any \\v \\f \\r at start of indentation */\n> + while (sa[i] == '\\f' || sa[i] == '\\v' ||\n> + (sa[i] == '\\r' && i < a->len - 1))\n\nI do not understand the use of parens here.\nI would have expected all comparisons in one\nblock which is then &&'d to the length requirement.\nBut this seems to tread \\r special if not at EOL.\n\n> + i++;\n> + while (sb[j] == '\\f' || sb[j] == '\\v' ||\n> + (sb[j] == '\\r' && j < b->len - 1))\n> + j++;\n> +\n> + for (la = 0; ; i++) {\n> + if (sa[i] == ' ')\n> + la++;\n> + else if (sa[i] == '\\t')\n> + la = ((la + ta) / ta) * ta;\n\nmultiplication/division may be expensive,\nwould something like\n\n la = la - (la % ta) + ta;\n\nwork instead? (the modulo is a hidden division,\nbut at least we do not have another multiplication)\n\nFurther I'd find it slightly easier to understand as it\n\"fills up to the next multiple of \" whereas the\ndivide and re-multiply trick relies on integer logic, but\nthat might be just me. Maybe just add a comment.\n\n> + else\n> + break;\n> + }\n> + for (lb = 0; ; j++) {\n> + if (sb[j] == ' ')\n> + lb++;\n> + else if (sb[j] == '\\t')\n> + lb = ((lb + tb) / tb) * tb;\n> + else\n> + break;\n> + }\n> + if (a->s == DIFF_SYMBOL_PLUS)\n> + *delta = la - lb;\n> + else\n> + *delta = lb - la;\n\nWhen writing the original feature I had reasons\nnot to rely on the symbol, as you could have\nmoved things from + to - (or the other way round)\nand added or removed indentation. That is what the\n`current_longer` is used for. But given that you only\ncount here, we can have negative numbers, so it\nwould work either way for adding or removing indentation.\n\nBut then, why do we need to have a different sign\ndepending on the sign of the line?\n\n> +\n> + return (a->len - i == b->len - j) &&\n> + !memcmp(sa + i, sb + j, a->len - i);\n> +}\n> +\n> static int compute_ws_delta(const struct emitted_diff_symbol *a,\n> const struct emitted_diff_symbol *b,\n> struct ws_delta *out)\n> @@ -778,6 +839,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n>\n> out->string = xmemdupz(longer->line, d);\n> out->current_longer = (a == longer);\n> + out->have_string = 1;\n>\n> return !strncmp(longer->line + d, shorter->line, shorter->len);\n> }\n> @@ -820,15 +882,34 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n> * To do so we need to compare 'l' to 'cur', adjusting the\n> * one of them for the white spaces, depending which was longer.\n> */\n\nThe comment above would only apply to the original mode?\n\n> + if (o->color_moved_ws_handling &\n> + COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n> + wslen = strlen(pmb->wsd->string);\n> + if (pmb->wsd->current_longer)\n> + c += wslen;\n> + else\n> + a += wslen;\n>\n> - wslen = strlen(pmb->wsd->string);\n> - if (pmb->wsd->current_longer)\n> - c += wslen;\n> - else\n> - a += wslen;\n> + if (strcmp(a, c))\n> + return 1;\n\nThis could be \"return strcmp\" instead of falling\nthrough to the last line in the function in case of 0. But this\nis just indenting code that is already there.\n\n>\n> - if (strcmp(a, c))\n> - return 1;\n> + return 0;\n> + } else if (o->color_moved_ws_handling &\n> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n> + int delta;\n> +\n> + if (!compute_mixed_ws_delta(cur->es, l, &delta))\n> + return 1;\n> +\n> + if (pmb->wsd->delta == INT_MIN) {\n> + pmb->wsd->delta = delta;\n> + return 0;\n> + }\n> +\n> + return !(delta == pmb->wsd->delta || delta == INT_MIN);\n\nMost of the code here deals with jumping over empty lines, and the new\nmode is just comparing the two numbers.\n\n\n> + } else {\n> + BUG(\"no color_moved_ws_allow_indentation_change set\");\n\nInstead of the BUG here could we have a switch/case (or if/else)\ncovering the complete space of delta->have_string instead?\nThen we would not leave a lingering bug in the code base.\n\n> + }\n>\n> return 0;\n> }\n> @@ -845,7 +926,8 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n> & XDF_WHITESPACE_FLAGS;\n>\n> if (diffopt->color_moved_ws_handling &\n> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\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> @@ -953,7 +1035,8 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n> pmb[i] = pmb[i]->next_line;\n> } else {\n> if (pmb[i]->wsd) {\n> - free(pmb[i]->wsd->string);\n> + if (pmb[i]->wsd->have_string)\n> + free(pmb[i]->wsd->string);\n> FREE_AND_NULL(pmb[i]->wsd);\n> }\n> pmb[i] = NULL;\n> @@ -1066,7 +1149,8 @@ static void mark_color_as_moved(struct diff_options *o,\n> continue;\n>\n> if (o->color_moved_ws_handling &\n> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n> pmb_advance_or_null_multi_match(o, match, hm, pmb, pmb_nr, n);\n> else\n> pmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n> @@ -1088,6 +1172,17 @@ static void mark_color_as_moved(struct diff_options *o,\n> pmb[pmb_nr++] = match;\n> } else\n> free(wsd);\n> + } else if (o->color_moved_ws_handling &\n> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n> + int delta;\n> +\n> + if (compute_mixed_ws_delta(l, match->es, &delta)) {\n> + struct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n> + wsd->delta = delta;\n> + wsd->have_string = 0;\n> + match->wsd = wsd;\n> + pmb[pmb_nr++] = match;\n\nI would want to keep mark_color_as_moved and friends smaller, and instead\nmove the complexity to compute_ws_delta which would check for the mode\nin `o` instead of repeating the modes in all these function.\nJust like cmp_in_block_with_wsd takes both modes into account\n\n\n> + }\n> } else {\n> pmb[pmb_nr++] = match;\n> }\n> @@ -5740,7 +5835,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n> struct hashmap add_lines, del_lines;\n>\n> if (o->color_moved_ws_handling &\n> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n> o->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n>\n> hashmap_init(&del_lines, moved_entry_cmp, o, 0);\n> diff --git a/diff.h b/diff.h\n> index 5e6bcf0926..03628cda45 100644\n> --- a/diff.h\n> +++ b/diff.h\n> @@ -217,6 +217,7 @@ struct diff_options {\n>\n> /* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n> #define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)\n> + #define COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE (1<<6)\n> int color_moved_ws_handling;\n> };\n>\n> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n> index 41facf7abf..737dbd4a42 100755\n> --- a/t/t4015-diff-whitespace.sh\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1902,4 +1902,93 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n> test_i18ngrep allow-indentation-change err\n> '\n>\n> +NUL=''\n> +test_expect_success 'compare mixed whitespace delta across moved blocks' '\n> +\n> + git reset --hard &&\n> + tr Q_ \"\\t \" <<-EOF >text.txt &&\n\nSo this is the extended version of q_to_tab, as it also\ntranslates _ to blank.\n\n> + ${NUL}\n\nis an empty line? So maybe s/NUL/EMPTY/ ?\n\nI think the following test cases may be useful:\n\n (3x_) too short without\n $EMPTY\n (4x_) being grouped across blank line\n\nthat will be indented to\n\n (3x_+n) too short without\n $EMPTY\n (4x_+n) being grouped across blank line\n\ni.e. the current test of grouping across an empty line\nalways has the same indentation before and after, but we\nonly care about the change in indentation, such that\nwe should be able to have different indentation levels\nbefore and after an empty line in the code, and\nstill count it as a block when they are indented the\nsame amount.\n\n\nIs it possible for a block to start with an empty line?\nHow do we handle multiple adjacent empty lines?\n\nDo we need tests for such special cases?\n\n-\n\nI hope this helps, as I gave the feedback above\nmostly unstructured.\n\nI'm excited about the skip blank lines mode, but\nI am not quite sure about the \"just count\" mode,\nas that is what I had originally IIRC but Jonathan\nseemed to not be fond of it. Maybe he remembers\nwhy.\n\nThanks,\nStefan\n"},{"id":"359913","messageId":"b3d29d34-616d-5d12-bb86-19ea488a766d@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79kZjAaLE7G=q9sBeEL_+Q2ufYBTn6p9TDCF8cYFd3k+0oQ@mail.gmail.com","subject":"[RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-09T09:50:55Z","receivedAt":"2018-10-09T09:51:02Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nThanks for all your comments on this, they've been really helpful.\n\nOn 25/09/2018 02:07, Stefan Beller wrote:\n> On Mon, Sep 24, 2018 at 3:06 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> This adds another mode for highlighting lines that have moved with an\n>> indentation change. Unlike the existing\n>> --color-moved-ws=allow-indentation-change setting this mode uses the\n>> visible change in the indentation to group lines, rather than the\n>> indentation string.\n> \n> Wow! Thanks for putting this RFC out.\n> My original vision was to be useful to python users as well,\n> which counts 1 tab as 8 spaces IIUC.\n> \n> The \"visual\" indentation you mention here sounds like\n> a tab is counted as \"up to the next position of (n-1) % 8\",\n> i.e. stop at positions 8, 16, 24... which would not be pythonic,\n> but useful in e.g. our code base.\n\nThe docs for python2 state[1]\n\n Leading whitespace (spaces and tabs) at the beginning of a logical\n line is used to compute the indentation level of the line, which in\n turn is used to determine the grouping of statements.\n\n First, tabs are replaced (from left to right) by one to eight spaces\n such that the total number of characters up to and including the\n replacement is a multiple of eight (this is intended to be the same\n rule as used by Unix). The total number of spaces preceding the\n first non-blank character then determines the line’s\n indentation. Indentation cannot be split over multiple physical\n lines using backslashes; the whitespace up to the first backslash\n determines the indentation.\n\nAs I understand it that fits with the \"visual\" indentation implemented\nby this patch.\n\nFor python3 adds a third paragraph[2]\n\n Indentation is rejected as inconsistent if a source file mixes tabs\n and spaces in a way that makes the meaning dependent on the worth of\n a tab in spaces; a TabError is raised in that case.\n\nMy impression is that people generally avoid mixing tabs and spaces in\npython3 code, in which case I wonder if the \"visual\" indentation\ncombined with a suitable setting for core.whitespace to highlight\nerroneous tabs/spaces would be enough. (I'm not a python programmer so I\ncould be completely wrong on that)\n\nIn any case the more I think about it the more convinced I am that\nhaving a move detection mode for \"pythonic\" indentation is a mistake. If\na line is added with dodgy indentation then it is a problem whether or\nnot it has been moved so I think this should be handled by the\nwhitespace error highlighting. This would allow a single mode for move\ndetection with an indentation change.\n\n[1] https://docs.python.org/2.7/reference/lexical_analysis.html#indentation\n[2] https://docs.python.org/3.7/reference/lexical_analysis.html#indentation\n\n>> This means it works with files that use a mix of\n>> tabs and spaces for indentation and can cope with whitespace errors\n>> where there is a space before a tab\n> \n> Cool!\n> \n>> (it's the job of\n>> --ws-error-highlight to deal with those errors, it should affect the\n>> move detection).\n> \n> Not sure I understand this side note. So --ws-error-highlight can\n> highlight them, but the move detection should *not*(?) be affected\n> by the highlighted parts, or it should do things differently on\n> whether --ws-error-highlight is given?\n\nI just meant that the move detection should pretend the whitespace\nerrors do not exist.\n\n>> It will also group the lines either\n>> side of a blank line if their indentation change matches so short\n>> lines followed by a blank line followed by more lines with the same\n>> indentation change will be correctly highlighted.\n> \n> That sounds very useful (at least for my editor, that strips\n> blank lines to be empty lines), but I would think this feature is\n> worth its own commit/patch.\n> \n> I wonder how much this feature is orthogonal to the existing\n> problem of detecting the moved indented blocks (existing\n> allow-indentation-change vs the new feature discussed first\n> above)\n\nIt only works if the blank lines get moved with the non-blank lines\naround it, then it matches the normal moved behavior I think. I'd like\nto have it include blank context lines where the lines either side have\nthe same indentation change but that is trickier to implement.\n\n>>\n>> This is a RFC as there are a number of questions about how to proceed\n>> from here:\n>> 1) Do we need a second option or should this implementation replace\n>> --color-moved-ws=allow-indentation-change. I'm unclear if that mode\n>> has any advantages for some people. There seems to have been an\n>> intention [1] to get it working with mixes of tabs and spaces but\n>> nothing ever came of it.\n> \n> Oh, yeah, I was working on that, but dropped the ball.\n> \n> I am not sure what the best end goal is, or if there are many different\n> modes that are useful to different target audiences.\n> My own itch at the time was (de-/)in-dented code from refactoring\n> patches for git.git and JGit (so Java, C, shell); and I think not hurting\n> python would also be good.\n\nAs I said above I've more or less come to the view that the correctness\nof pythonic indentation is orthogonal to move detection as it affects\nall additions, not just those that correspond to moved lines.\n\n> ignoring the mixture of ws seems like it would also cater free text or\n> other more exotic languages.\n> \n> What is your use case, what kind of content do you process that\n> this patch would help you?\n\nI wrote this because I was re-factoring some shell code than was using a\nindentation step of four spaces but with tabs in the leading indentation\nwhich the current mode does not handle.\n\n> I am not overly attached to the current implementation of\n> --color-moved-ws=allow-indentation-change,\n> and I think Junio has expressed the fear of \"too many options\"\n> already in this problem space, so if possible I would extend/replace\n> the current option.\n> \n>> 2) If we keep two options what should this option be called, the name\n>> is long and ambiguous at the moment - mixed could refer to mixed\n>> indentation length rather than a mix of tabs and spaces.\n> \n> Let's first read the code to have an opinion, or re-state the question\n> from above (\"What is this used for?\") as I could imagine one of the\n> modes could be \"ws-pythonic\" and allow for whitespace indentation\n> that would have the whole block count as an indented by the same\n> amount, (e.g. if you wrap a couple functions in python by a class).\n> \n>> +++ b/diff.c\n>> @@ -304,7 +304,11 @@ static int parse_color_moved_ws(const char *arg)\n>> else if (!strcmp(sb.buf, \"ignore-all-space\"))\n>> ret |= XDF_IGNORE_WHITESPACE;\n>> else if (!strcmp(sb.buf, \"allow-indentation-change\"))\n>> - ret |= COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE;\n>> + ret = COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n>> + (ret & ~COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE);\n> \n> So this RFC lets \"allow-indentation-change\" override\n> \"allow-mixed-indentation-change\" and vice versa. That\n> also solves the issue of configuring one and giving the other\n> as a command line option. Nice.\n> \n>> if ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n>> (ret & XDF_WHITESPACE_FLAGS))\n>> die(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n>> + else if ((ret & COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) &&\n>> + (ret & XDF_WHITESPACE_FLAGS))\n>> + die(_(\"color-moved-ws: allow-mixed-indentation-change cannot be combined with other white space modes\"));\n> \n> Do we want to open a bit mask for all indentation change options? e.g.\n> #define COLOR_MOVED_WS_INDENTATION_MASK \\\n> (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE | \\\n> COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE)\n\nThat's a good idea if we retain two separate modes\n\n> \n>> @@ -763,11 +770,65 @@ struct moved_entry {\n>> * comparision is longer than the second.\n>> */\n>> struct ws_delta {\n>> - char *string;\n>> + union {\n>> + int delta;\n>> + char *string;\n>> + };\n>> unsigned int current_longer : 1;\n>> + unsigned int have_string : 1;\n>> };\n>> #define WS_DELTA_INIT { NULL, 0 }\n>>\n>> +static int compute_mixed_ws_delta(const struct emitted_diff_symbol *a,\n>> + const struct emitted_diff_symbol *b,\n>> + int *delta)\n>> +{\n>> + unsigned int i = 0, j = 0;\n>> + int la, lb;\n>> + int ta = a->flags & WS_TAB_WIDTH_MASK;\n>> + int tb = b->flags & WS_TAB_WIDTH_MASK;\n>> + const char *sa = a->line;\n>> + const char *sb = b->line;\n>> +\n>> + if (xdiff_is_blankline(sa, a->len, 0) &&\n>> + xdiff_is_blankline(sb, b->len, 0)) {\n>> + *delta = INT_MIN;\n>> + return 1;\n>> + }\n>> +\n>> + /* skip any \\v \\f \\r at start of indentation */\n>> + while (sa[i] == '\\f' || sa[i] == '\\v' ||\n>> + (sa[i] == '\\r' && i < a->len - 1))\n> \n> I do not understand the use of parens here.\n> I would have expected all comparisons in one\n> block which is then &&'d to the length requirement.\n> But this seems to tread \\r special if not at EOL.\n\nI only want to skip '\\r' if it isn't part of \"\\r\\n\" at the end of the\nline. (similar to way --ignore-cr-at-eol does not ignore a trailing '\\r'\non an incomplete line)\n\n>> + i++;\n>> + while (sb[j] == '\\f' || sb[j] == '\\v' ||\n>> + (sb[j] == '\\r' && j < b->len - 1))\n>> + j++;\n>> +\n>> + for (la = 0; ; i++) {\n>> + if (sa[i] == ' ')\n>> + la++;\n>> + else if (sa[i] == '\\t')\n>> + la = ((la + ta) / ta) * ta;\n> \n> multiplication/division may be expensive,\n> would something like\n> \n> la = la - (la % ta) + ta;\n> \n> work instead? (the modulo is a hidden division,\n> but at least we do not have another multiplication)\n> \n> Further I'd find it slightly easier to understand as it\n> \"fills up to the next multiple of \" whereas the\n> divide and re-multiply trick relies on integer logic, but\n> that might be just me. Maybe just add a comment.\n\nI agree your version is clearer and it is marginally (~1%) faster\n\n>> + else\n>> + break;\n>> + }\n>> + for (lb = 0; ; j++) {\n>> + if (sb[j] == ' ')\n>> + lb++;\n>> + else if (sb[j] == '\\t')\n>> + lb = ((lb + tb) / tb) * tb;\n>> + else\n>> + break;\n>> + }\n>> + if (a->s == DIFF_SYMBOL_PLUS)\n>> + *delta = la - lb;\n>> + else\n>> + *delta = lb - la;\n> \n> When writing the original feature I had reasons\n> not to rely on the symbol, as you could have\n> moved things from + to - (or the other way round)\n> and added or removed indentation. That is what the\n> `current_longer` is used for. But given that you only\n> count here, we can have negative numbers, so it\n> would work either way for adding or removing indentation.\n> \n> But then, why do we need to have a different sign\n> depending on the sign of the line?\n\nThe check means that we get the same delta whichever way round the lines\nare compared. I think I added this because without it the highlighting\ngets broken if there is increase in indentation followed by an identical\ndecrease on the next line.\n\n>> +\n>> + return (a->len - i == b->len - j) &&\n>> + !memcmp(sa + i, sb + j, a->len - i);\n>> +}\n>> +\n>> static int compute_ws_delta(const struct emitted_diff_symbol *a,\n>> const struct emitted_diff_symbol *b,\n>> struct ws_delta *out)\n>> @@ -778,6 +839,7 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n>>\n>> out->string = xmemdupz(longer->line, d);\n>> out->current_longer = (a == longer);\n>> + out->have_string = 1;\n>>\n>> return !strncmp(longer->line + d, shorter->line, shorter->len);\n>> }\n>> @@ -820,15 +882,34 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n>> * To do so we need to compare 'l' to 'cur', adjusting the\n>> * one of them for the white spaces, depending which was longer.\n>> */\n> \n> The comment above would only apply to the original mode?\n\nYes that should be changed/moved\n\n>> + if (o->color_moved_ws_handling &\n>> + COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) {\n>> + wslen = strlen(pmb->wsd->string);\n>> + if (pmb->wsd->current_longer)\n>> + c += wslen;\n>> + else\n>> + a += wslen;\n>>\n>> - wslen = strlen(pmb->wsd->string);\n>> - if (pmb->wsd->current_longer)\n>> - c += wslen;\n>> - else\n>> - a += wslen;\n>> + if (strcmp(a, c))\n>> + return 1;\n> \n> This could be \"return strcmp\" instead of falling\n> through to the last line in the function in case of 0. But this\n> is just indenting code that is already there.\n\nAs you say it's keeping the code the same, also while it does not matter\nto the caller at the moment I was wary of potentially changing the sign\nof the return value.\n\n>> - if (strcmp(a, c))\n>> - return 1;\n>> + return 0;\n>> + } else if (o->color_moved_ws_handling &\n>> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n>> + int delta;\n>> +\n>> + if (!compute_mixed_ws_delta(cur->es, l, &delta))\n>> + return 1;\n>> +\n>> + if (pmb->wsd->delta == INT_MIN) {\n>> + pmb->wsd->delta = delta;\n>> + return 0;\n>> + }\n>> +\n>> + return !(delta == pmb->wsd->delta || delta == INT_MIN);\n> \n> Most of the code here deals with jumping over empty lines, and the new\n> mode is just comparing the two numbers.\n> \n>> + } else {\n>> + BUG(\"no color_moved_ws_allow_indentation_change set\");\n> \n> Instead of the BUG here could we have a switch/case (or if/else)\n> covering the complete space of delta->have_string instead?\n> Then we would not leave a lingering bug in the code base.\n\nI'm not sure what you mean, we cover all the existing\ncolor_moved_ws_handling values, I added the BUG() call to pick up future\nomissions if another mode is added. (If we go for a single mode none of\nthis matters)\n\n>> + }\n>>\n>> return 0;\n>> }\n>> @@ -845,7 +926,8 @@ static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n>> & XDF_WHITESPACE_FLAGS;\n>>\n>> if (diffopt->color_moved_ws_handling &\n>> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n>> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n>> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\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>> @@ -953,7 +1035,8 @@ static void pmb_advance_or_null_multi_match(struct diff_options *o,\n>> pmb[i] = pmb[i]->next_line;\n>> } else {\n>> if (pmb[i]->wsd) {\n>> - free(pmb[i]->wsd->string);\n>> + if (pmb[i]->wsd->have_string)\n>> + free(pmb[i]->wsd->string);\n>> FREE_AND_NULL(pmb[i]->wsd);\n>> }\n>> pmb[i] = NULL;\n>> @@ -1066,7 +1149,8 @@ static void mark_color_as_moved(struct diff_options *o,\n>> continue;\n>>\n>> if (o->color_moved_ws_handling &\n>> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n>> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n>> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n>> pmb_advance_or_null_multi_match(o, match, hm, pmb, pmb_nr, n);\n>> else\n>> pmb_advance_or_null(o, match, hm, pmb, pmb_nr);\n>> @@ -1088,6 +1172,17 @@ static void mark_color_as_moved(struct diff_options *o,\n>> pmb[pmb_nr++] = match;\n>> } else\n>> free(wsd);\n>> + } else if (o->color_moved_ws_handling &\n>> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE) {\n>> + int delta;\n>> +\n>> + if (compute_mixed_ws_delta(l, match->es, &delta)) {\n>> + struct ws_delta *wsd = xmalloc(sizeof(*match->wsd));\n>> + wsd->delta = delta;\n>> + wsd->have_string = 0;\n>> + match->wsd = wsd;\n>> + pmb[pmb_nr++] = match;\n> \n> I would want to keep mark_color_as_moved and friends smaller, and instead\n> move the complexity to compute_ws_delta which would check for the mode\n> in `o` instead of repeating the modes in all these function.\n> Just like cmp_in_block_with_wsd takes both modes into account\n\nThat makes sense, I'll fix it.\n\n>> + }\n>> } else {\n>> pmb[pmb_nr++] = match;\n>> }\n>> @@ -5740,7 +5835,8 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n>> struct hashmap add_lines, del_lines;\n>>\n>> if (o->color_moved_ws_handling &\n>> - COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n>> + (COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE |\n>> + COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE))\n>> o->color_moved_ws_handling |= XDF_IGNORE_WHITESPACE;\n>>\n>> hashmap_init(&del_lines, moved_entry_cmp, o, 0);\n>> diff --git a/diff.h b/diff.h\n>> index 5e6bcf0926..03628cda45 100644\n>> --- a/diff.h\n>> +++ b/diff.h\n>> @@ -217,6 +217,7 @@ struct diff_options {\n>>\n>> /* XDF_WHITESPACE_FLAGS regarding block detection are set at 2, 3, 4 */\n>> #define COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE (1<<5)\n>> + #define COLOR_MOVED_WS_ALLOW_MIXED_INDENTATION_CHANGE (1<<6)\n>> int color_moved_ws_handling;\n>> };\n>>\n>> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\n>> index 41facf7abf..737dbd4a42 100755\n>> --- a/t/t4015-diff-whitespace.sh\n>> +++ b/t/t4015-diff-whitespace.sh\n>> @@ -1902,4 +1902,93 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n>> test_i18ngrep allow-indentation-change err\n>> '\n>>\n>> +NUL=''\n>> +test_expect_success 'compare mixed whitespace delta across moved blocks' '\n>> +\n>> + git reset --hard &&\n>> + tr Q_ \"\\t \" <<-EOF >text.txt &&\n> \n> So this is the extended version of q_to_tab, as it also\n> translates _ to blank.\n\nExactly\n\n>> + ${NUL}\n> \n> is an empty line? So maybe s/NUL/EMPTY/ ?\n\nThat might be clearer\n\n> I think the following test cases may be useful:\n> \n> (3x_) too short without\n> $EMPTY\n> (4x_) being grouped across blank line\n> \n> that will be indented to\n> \n> (3x_+n) too short without\n> $EMPTY\n> (4x_+n) being grouped across blank line\n> \n> i.e. the current test of grouping across an empty line\n> always has the same indentation before and after, but we\n> only care about the change in indentation, such that\n> we should be able to have different indentation levels\n> before and after an empty line in the code, and\n> still count it as a block when they are indented the\n> same amount.\n\nThat's a good idea, thanks\n\n> Is it possible for a block to start with an empty line?\n\nYes, the block in the test starts with an empty line\n\n> How do we handle multiple adjacent empty lines?\n\nWe group them all with the moved lines. This is slightly different to\n--ignore-blank-lines which has a threshold on how may blank lines it\nwill ignore.\n\n> Do we need tests for such special cases?\n\nI would probably be best, picking up changes to the behavior of unusual\ncorner cases is best done with a test.\n\n> I hope this helps, as I gave the feedback above\n> mostly unstructured.\n\nIt's been really useful, thank for taking the time to look through the\npatch so carefully.\n\nBest Wishes\n\nPhillip\n\n> I'm excited about the skip blank lines mode, but\n> I am not quite sure about the \"just count\" mode,\n> as that is what I had originally IIRC but Jonathan\n> seemed to not be fond of it. Maybe he remembers\n> why.\n> \n> Thanks,\n> Stefan\n"},{"id":"359950","messageId":"CAGZ79kYjeqME-tt89Fp=Wt0hAW0FVAyZ00ftN5XTOkFSn7Kq9A@mail.gmail.com","threadId":"49410","inReplyTo":"b3d29d34-616d-5d12-bb86-19ea488a766d@talktalk.net","subject":"Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-09T21:10:56Z","receivedAt":"2018-10-09T21:11:11Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> As I said above I've more or less come to the view that the correctness\n> of pythonic indentation is orthogonal to move detection as it affects\n> all additions, not just those that correspond to moved lines.\n\nMakes sense.\n\n> > What is your use case, what kind of content do you process that\n> > this patch would help you?\n>\n> I wrote this because I was re-factoring some shell code than was using a\n> indentation step of four spaces but with tabs in the leading indentation\n> which the current mode does not handle.\n\nAh that is good to know.\n\nI was thinking whether we want to generalize the move detection into a more\ngeneric \"detect and fade out uninteresting things\" and not just focus on white\nspaces (but these are most often the uninteresting things).\n\nOver the last year we had quite a couple of large refactorings, that\nwould have helped by that:\n* For example the hash transition plan had a lot of patches that\n were basically s/char *sha1/struct object oid/ or some variation thereof.\n* Introducing struct repository\n\nI used the word diff to look at those patches, which helped a lot, but\nmaybe a mode that would allow me to mark this specific replacement\nuninteresting would be even better.\nMaybe this can be done as a piggyback on top of the move detection as\na \"move in place, but with uninteresting pattern\". The problem of this\nis that the pattern needs to be accounted for when hashing the entries\ninto the hashmaps, which is easy when doing white spaces only.\n\n\n> >> + if (a->s == DIFF_SYMBOL_PLUS)\n> >> + *delta = la - lb;\n> >> + else\n> >> + *delta = lb - la;\n> >\n> > When writing the original feature I had reasons\n> > not to rely on the symbol, as you could have\n> > moved things from + to - (or the other way round)\n> > and added or removed indentation. That is what the\n> > `current_longer` is used for. But given that you only\n> > count here, we can have negative numbers, so it\n> > would work either way for adding or removing indentation.\n> >\n> > But then, why do we need to have a different sign\n> > depending on the sign of the line?\n>\n> The check means that we get the same delta whichever way round the lines\n> are compared. I think I added this because without it the highlighting\n> gets broken if there is increase in indentation followed by an identical\n> decrease on the next line.\n\nBut wouldn't we want to get that highlighted?\nI do not quite understand the scenario, yet. Are both indented\nand dedented part of the same block?\n\n\n> >\n> >> + } else {\n> >> + BUG(\"no color_moved_ws_allow_indentation_change set\");\n> >\n> > Instead of the BUG here could we have a switch/case (or if/else)\n> > covering the complete space of delta->have_string instead?\n> > Then we would not leave a lingering bug in the code base.\n>\n> I'm not sure what you mean, we cover all the existing\n> color_moved_ws_handling values, I added the BUG() call to pick up future\n> omissions if another mode is added. (If we go for a single mode none of\n> this matters)\n\nAh, makes sense!\n"},{"id":"360037","messageId":"fb500556-adb0-fbd8-0119-443455915eab@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79kYjeqME-tt89Fp=Wt0hAW0FVAyZ00ftN5XTOkFSn7Kq9A@mail.gmail.com","subject":"Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-10-10T15:26:54Z","receivedAt":"2018-10-10T15:26:58Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/10/2018 22:10, Stefan Beller wrote:\n>> As I said above I've more or less come to the view that the correctness\n>> of pythonic indentation is orthogonal to move detection as it affects\n>> all additions, not just those that correspond to moved lines.\n> \n> Makes sense.\n\nRight so are you happy for we to re-roll with a single \nallow-indentation-change mode based on my RFC?\n\n> \n>>> What is your use case, what kind of content do you process that\n>>> this patch would help you?\n>>\n>> I wrote this because I was re-factoring some shell code than was using a\n>> indentation step of four spaces but with tabs in the leading indentation\n>> which the current mode does not handle.\n> \n> Ah that is good to know.\n> \n> I was thinking whether we want to generalize the move detection into a more\n> generic \"detect and fade out uninteresting things\" and not just focus on white\n> spaces (but these are most often the uninteresting things).\n> \n> Over the last year we had quite a couple of large refactorings, that\n> would have helped by that:\n> * For example the hash transition plan had a lot of patches that\n> were basically s/char *sha1/struct object oid/ or some variation thereof.\n> * Introducing struct repository\n> \n> I used the word diff to look at those patches, which helped a lot, but\n> maybe a mode that would allow me to mark this specific replacement\n> uninteresting would be even better.\n> Maybe this can be done as a piggyback on top of the move detection as\n> a \"move in place, but with uninteresting pattern\". The problem of this\n> is that the pattern needs to be accounted for when hashing the entries\n> into the hashmaps, which is easy when doing white spaces only.\n\nYes the I like the idea. Yesterday I was looking at Alban's patches to \nrefactor the todo list handling for rebase -i and there are a lot of '.' \nto '->' changes which weren't particularly interesting though at least \ndiff-highlight made it clear if that was the only change on a line. \nIncidentally --color-moved was very useful for looking at that series.\n\n>>>> + if (a->s == DIFF_SYMBOL_PLUS)\n>>>> + *delta = la - lb;\n>>>> + else\n>>>> + *delta = lb - la;\n>>>\n>>> When writing the original feature I had reasons\n>>> not to rely on the symbol, as you could have\n>>> moved things from + to - (or the other way round)\n>>> and added or removed indentation. That is what the\n>>> `current_longer` is used for. But given that you only\n>>> count here, we can have negative numbers, so it\n>>> would work either way for adding or removing indentation.\n>>>\n>>> But then, why do we need to have a different sign\n>>> depending on the sign of the line?\n>>\n>> The check means that we get the same delta whichever way round the lines\n>> are compared. I think I added this because without it the highlighting\n>> gets broken if there is increase in indentation followed by an identical\n>> decrease on the next line.\n> \n> But wouldn't we want to get that highlighted?\n> I do not quite understand the scenario, yet. Are both indented\n> and dedented part of the same block?\n\nWith --color-moved=zebra the indented lines and the de-indented lines \nshould be different colors, without the test they both ended up in the \nsame block.\n\nBest Wishes\n\nPhillip\n>>>\n>>>> + } else {\n>>>> + BUG(\"no color_moved_ws_allow_indentation_change set\");\n>>>\n>>> Instead of the BUG here could we have a switch/case (or if/else)\n>>> covering the complete space of delta->have_string instead?\n>>> Then we would not leave a lingering bug in the code base.\n>>\n>> I'm not sure what you mean, we cover all the existing\n>> color_moved_ws_handling values, I added the BUG() call to pick up future\n>> omissions if another mode is added. (If we go for a single mode none of\n>> this matters)\n> \n> Ah, makes sense!\n> \n\n"},{"id":"360050","messageId":"CAGZ79kYeKer_yYxRkRugWVjvwrYk7dT4EO3NyVJkqR3j_XCdRw@mail.gmail.com","threadId":"49410","inReplyTo":"fb500556-adb0-fbd8-0119-443455915eab@talktalk.net","subject":"Re: [RFC PATCH 3/3] diff: add --color-moved-ws=allow-mixed-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-10T18:05:04Z","receivedAt":"2018-10-10T18:05:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Oct 10, 2018 at 8:26 AM Phillip Wood wrote:\n>\n> On 09/10/2018 22:10, Stefan Beller wrote:\n> >> As I said above I've more or less come to the view that the correctness\n> >> of pythonic indentation is orthogonal to move detection as it affects\n> >> all additions, not just those that correspond to moved lines.\n> >\n> > Makes sense.\n>\n> Right so are you happy for we to re-roll with a single\n> allow-indentation-change mode based on my RFC?\n\nI am happy with that.\n"},{"id":"363498","messageId":"20181116110356.12311-2-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 1/9] diff: document --no-color-moved","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:48Z","receivedAt":"2018-11-16T11:04:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAdd documentation for --no-color-moved.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 0378cd574e..151690f814 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -293,6 +293,10 @@ dimmed-zebra::\n \t`dimmed_zebra` is a deprecated synonym.\n --\n \n+--no-color-moved::\n+\tTurn off move detection. This can be used to override configuration\n+\tsettings. It is the same as `--color-moved=no`.\n+\n --color-moved-ws=::\n \tThis configures how white spaces are ignored when performing the\n \tmove detection for `--color-moved`.\n-- \n2.19.1\n\n"},{"id":"363499","messageId":"20181116110356.12311-8-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 7/9] diff --color-moved-ws: optimize allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:54Z","receivedAt":"2018-11-16T11:04:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen running\n\n git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\n\ncmp_in_block_with_wsd() is called 694908327 times. Of those 42.7%\nreturn after comparing a and b. By comparing the lengths first we can\nreturn early in all but 0.03% of those cases without dereferencing the\nstring pointers. The comparison between a and c fails in 6.8% of\ncalls, by comparing the lengths first we reject all the failing calls\nwithout dereferencing the string pointers.\n\nThis reduces the time to run the command above by by 42% from 14.6s to\n8.5s. This is still much slower than the normal --color-moved which\ntakes ~0.6-0.7s to run but is a significant improvement.\n\nThe next commits will replace the current implementation with one that\nworks with mixed tabs and spaces in the indentation. I think it is\nworth optimizing the current implementation first to enable a fair\ncomparison between the two implementations.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8c08dd68df..c378ce3daf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -829,20 +829,23 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\t\t\t int n)\n {\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\tint al = cur->es->len, cl = l->len;\n+\tint al = cur->es->len, bl = match->es->len, cl = l->len;\n \tconst char *a = cur->es->line,\n \t\t *b = match->es->line,\n \t\t *c = l->line;\n-\n+\tconst char *orig_a = a;\n \tint wslen;\n \n \t/*\n-\t * We need to check if 'cur' is equal to 'match'.\n-\t * As those are from the same (+/-) side, we do not need to adjust for\n-\t * indent changes. However these were found using fuzzy matching\n-\t * so we do have to check if they are equal.\n+\t * We need to check if 'cur' is equal to 'match'. As those\n+\t * are from the same (+/-) side, we do not need to adjust for\n+\t * indent changes. However these were found using fuzzy\n+\t * matching so we do have to check if they are equal. Here we\n+\t * just check the lengths. We delay calling memcmp() to check\n+\t * the contents until later as if the length comparison for a\n+\t * and c fails we can avoid the call all together.\n \t */\n-\tif (strcmp(a, b))\n+\tif (al != bl)\n \t\treturn 1;\n \n \tif (!pmb->wsd.string)\n@@ -870,7 +873,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\tal -= wslen;\n \t}\n \n-\tif (al != cl || memcmp(a, c, al))\n+\tif (al != cl || memcmp(orig_a, b, bl) || memcmp(a, c, al))\n \t\treturn 1;\n \n \treturn 0;\n-- \n2.19.1\n\n"},{"id":"363500","messageId":"20181116110356.12311-5-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 4/9] diff --color-moved-ws: demonstrate false positives","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:51Z","receivedAt":"2018-11-16T11:04:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n'diff --color-moved-ws=allow-indentation-change' can highlight lines\nthat have internal whitespace changes rather than indentation\nchanges. For example in commit 1a07e59c3e (\"Update messages in\npreparation for i18n\", 2018-07-21) the lines\n\n- die (_(\"must end with a color\"));\n+ die(_(\"must end with a color\"));\n\nare highlighted as moved when they should not be. Modify an existing\ntest to show the problem that will be fixed in the next commit.\n\nSigned-off-by: Phillip Wood \n---\n t/t4015-diff-whitespace.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex a9fb226c5a..eee81a1987 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1809,7 +1809,7 @@ test_expect_success 'only move detection ignores white spaces' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'compare whitespace delta across moved blocks' '\n+test_expect_failure 'compare whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \tq_to_tab <<-\\EOF >text.txt &&\n@@ -1827,6 +1827,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \tQQQthat has similar lines\n \tQQQto previous blocks, but with different indent\n \tQQQYetQAnotherQoutlierQ\n+\tQLine with internal w h i t e s p a c e change\n \tEOF\n \n \tgit add text.txt &&\n@@ -1847,6 +1848,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \tQQthat has similar lines\n \tQQto previous blocks, but with different indent\n \tQQYetQAnotherQoutlier\n+\tQLine with internal whitespace change\n \tEOF\n \n \tgit diff --color --color-moved --color-moved-ws=allow-indentation-change >actual.raw &&\n@@ -1856,7 +1858,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\tdiff --git a/text.txt b/text.txt\n \t\t--- a/text.txt\n \t\t+++ b/text.txt\n-\t\t@@ -1,14 +1,14 @@\n+\t\t@@ -1,15 +1,15 @@\n \t\t-QIndented\n \t\t-QText across\n \t\t-Qsome lines\n@@ -1871,6 +1873,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\t-QQQthat has similar lines\n \t\t-QQQto previous blocks, but with different indent\n \t\t-QQQYetQAnotherQoutlierQ\n+\t\t-QLine with internal w h i t e s p a c e change\n \t\t+QQIndented\n \t\t+QQText across\n \t\t+QQsome lines\n@@ -1885,6 +1888,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\t+QQthat has similar lines\n \t\t+QQto previous blocks, but with different indent\n \t\t+QQYetQAnotherQoutlier\n+\t\t+QLine with internal whitespace change\n \tEOF\n \n \ttest_cmp expected actual\n-- \n2.19.1\n\n"},{"id":"363501","messageId":"20181116110356.12311-9-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 8/9] diff --color-moved-ws: modify allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:55Z","receivedAt":"2018-11-16T11:04:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCurrently diff --color-moved-ws=allow-indentation-change does not\nsupport indentation that contains a mix of tabs and spaces. For\nexample in commit 546f70f377 (\"convert.h: drop 'extern' from function\ndeclaration\", 2018-06-30) the function parameters in the following\nlines are not colored as moved [1].\n\n-extern int stream_filter(struct stream_filter *,\n- const char *input, size_t *isize_p,\n- char *output, size_t *osize_p);\n+int stream_filter(struct stream_filter *,\n+ const char *input, size_t *isize_p,\n+ char *output, size_t *osize_p);\n\nThis commit changes the way the indentation is handled to track the\nvisual size of the indentation rather than the characters in the\nindentation. This has they benefit that any whitespace errors do not\ninterfer with the move detection (the whitespace errors will still be\nhighlighted according to --ws-error-highlight). During the discussion\nof this feature there were concerns about the correct detection of\nindentation for python. However those concerns apply whether or not\nwe're detecting moved lines so no attempt is made to determine if the\nindentation is 'pythonic'.\n\n[1] Note that before the commit to fix the erroneous coloring of moved\n lines each line was colored as a different block, since that commit\n they are uncolored.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n Changes since rfc:\n - It now replaces the existing implementation rather than adding a new\n mode.\n - The indentation deltas are now calculated once for each line and\n cached.\n - Optimized the whitespace delta comparison to compare string lengths\n before comparing the actual strings.\n - Modified the calculation of tabs as suggested by Stefan.\n - Split out the blank line handling into a separate commit as suggest\n by Stefan.\n - Fixed some comments pointed out by Stefan.\n\n diff.c | 130 +++++++++++++++++++++----------------\n t/t4015-diff-whitespace.sh | 56 ++++++++++++++++\n 2 files changed, 129 insertions(+), 57 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c378ce3daf..89559293e7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -750,6 +750,8 @@ struct emitted_diff_symbol {\n \tconst char *line;\n \tint len;\n \tint flags;\n+\tint indent_off;\n+\tint indent_width;\n \tenum diff_symbol s;\n };\n #define EMITTED_DIFF_SYMBOL_INIT {NULL}\n@@ -780,44 +782,68 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n-/**\n- * The struct ws_delta holds white space differences between moved lines, i.e.\n- * between '+' and '-' lines that have been detected to be a move.\n- * The string contains the difference in leading white spaces, before the\n- * rest of the line is compared using the white space config for move\n- * coloring. The current_longer indicates if the first string in the\n- * comparision is longer than the second.\n- */\n-struct ws_delta {\n-\tchar *string;\n-\tunsigned int current_longer : 1;\n-};\n-#define WS_DELTA_INIT { NULL, 0 }\n-\n struct moved_block {\n \tstruct moved_entry *match;\n-\tstruct ws_delta wsd;\n+\tint wsd; /* The whitespace delta of this block */\n };\n \n static void moved_block_clear(struct moved_block *b)\n {\n-\tFREE_AND_NULL(b->wsd.string);\n-\tb->match = NULL;\n+\tmemset(b, 0, sizeof(*b));\n+}\n+\n+static void fill_es_indent_data(struct emitted_diff_symbol *es)\n+{\n+\tunsigned int off = 0;\n+\tint width = 0, tab_width = es->flags & WS_TAB_WIDTH_MASK;\n+\tconst char *s = es->line;\n+\tconst int len = es->len;\n+\n+\t/* skip any \\v \\f \\r at start of indentation */\n+\twhile (s[off] == '\\f' || s[off] == '\\v' ||\n+\t (s[off] == '\\r' && off < len - 1))\n+\t\toff++;\n+\n+\t/* calculate the visual width of indentation */\n+\twhile(1) {\n+\t\tif (s[off] == ' ') {\n+\t\t\twidth++;\n+\t\t\toff++;\n+\t\t} else if (s[off] == '\\t') {\n+\t\t\twidth += tab_width - (width % tab_width);\n+\t\t\twhile (s[++off] == '\\t')\n+\t\t\t\twidth += tab_width;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tes->indent_off = off;\n+\tes->indent_width = width;\n }\n \n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n-\t\t\t const struct emitted_diff_symbol *b,\n-\t\t\t struct ws_delta *out)\n+\t\t\t const struct emitted_diff_symbol *b,\n+\t\t\t int *out)\n {\n-\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n-\tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n-\tint d = longer->len - shorter->len;\n+\tint a_len = a->len,\n+\t b_len = b->len,\n+\t a_off = a->indent_off,\n+\t a_width = a->indent_width,\n+\t b_off = b->indent_off,\n+\t b_width = b->indent_width;\n+\tint delta;\n \n-\tif (strncmp(longer->line + d, shorter->line, shorter->len))\n+\tif (a->s == DIFF_SYMBOL_PLUS)\n+\t\tdelta = a_width - b_width;\n+\telse\n+\t\tdelta = b_width - a_width;\n+\n+\tif (a_len - a_off != b_len - b_off ||\n+\t memcmp(a->line + a_off, b->line + b_off, a_len - a_off))\n \t\treturn 0;\n \n-\tout->string = xmemdupz(longer->line, d);\n-\tout->current_longer = (a == longer);\n+\t*out = delta;\n \n \treturn 1;\n }\n@@ -833,8 +859,11 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tconst char *a = cur->es->line,\n \t\t *b = match->es->line,\n \t\t *c = l->line;\n-\tconst char *orig_a = a;\n-\tint wslen;\n+\tint a_off = cur->es->indent_off,\n+\t a_width = cur->es->indent_width,\n+\t c_off = l->indent_off,\n+\t c_width = l->indent_width;\n+\tint delta;\n \n \t/*\n \t * We need to check if 'cur' is equal to 'match'. As those\n@@ -848,35 +877,20 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (al != bl)\n \t\treturn 1;\n \n-\tif (!pmb->wsd.string)\n-\t\t/*\n-\t\t * The white space delta is not active? This can happen\n-\t\t * when we exit early in this function.\n-\t\t */\n-\t\treturn 1;\n-\n \t/*\n-\t * The indent changes of the block are known and stored in\n-\t * pmb->wsd; however we need to check if the indent changes of the\n-\t * current line are still the same as before.\n-\t *\n-\t * To do so we need to compare 'l' to 'cur', adjusting the\n-\t * one of them for the white spaces, depending which was longer.\n+\t * The indent changes of the block are known and stored in pmb->wsd;\n+\t * however we need to check if the indent changes of the current line\n+\t * match those of the current block and that the text of 'l' and 'cur'\n+\t * after the indentation match.\n \t */\n+\tif (cur->es->s == DIFF_SYMBOL_PLUS)\n+\t\tdelta = a_width - c_width;\n+\telse\n+\t\tdelta = c_width - a_width;\n \n-\twslen = strlen(pmb->wsd.string);\n-\tif (pmb->wsd.current_longer) {\n-\t\tc += wslen;\n-\t\tcl -= wslen;\n-\t} else {\n-\t\ta += wslen;\n-\t\tal -= wslen;\n-\t}\n-\n-\tif (al != cl || memcmp(orig_a, b, bl) || memcmp(a, c, al))\n-\t\treturn 1;\n-\n-\treturn 0;\n+\treturn !(delta == pmb->wsd && al - a_off == cl - c_off &&\n+\t\t !memcmp(a, b, al) && !\n+\t\t memcmp(a + a_off, c + c_off, al - a_off));\n }\n \n static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n@@ -942,6 +956,9 @@ static void add_lines_to_move_detection(struct diff_options *o,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (o->color_moved_ws_handling &\n+\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\tfill_es_indent_data(&o->emitted_symbols->buf[n]);\n \t\tkey = prepare_entry(o, n);\n \t\tif (prev_line && prev_line->es->s == o->emitted_symbols->buf[n].s)\n \t\t\tprev_line->next_line = key;\n@@ -1020,8 +1037,7 @@ static int shrink_potential_moved_blocks(struct moved_block *pmb,\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n-\t\t\tpmb[rp].match = NULL;\n-\t\t\tpmb[rp].wsd.string = NULL;\n+\t\t\tmemset(&pmb[rp], 0, sizeof(pmb[rp]));\n \t\t\trp--;\n \t\t\tlp++;\n \t\t}\n@@ -1141,7 +1157,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb[pmb_nr].wsd))\n \t\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t} else {\n-\t\t\t\t\tpmb[pmb_nr].wsd.string = NULL;\n+\t\t\t\t\tpmb[pmb_nr].wsd = 0;\n \t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t}\n \t\t\t}\n@@ -1507,7 +1523,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n \t\t\t const char *line, int len, unsigned flags)\n {\n-\tstruct emitted_diff_symbol e = {line, len, flags, s};\n+\tstruct emitted_diff_symbol e = {line, len, flags, 0, 0, s};\n \n \tif (o->emitted_symbols)\n \t\tappend_emitted_diff_symbol(o, &e);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex fe8a2ab06e..e023839ba6 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1901,4 +1901,60 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n \ttest_i18ngrep allow-indentation-change err\n '\n \n+test_expect_success 'compare mixed whitespace delta across moved blocks' '\n+\n+\tgit reset --hard &&\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\t____Indented text to\n+\t_Q____be further indented by four spaces across\n+\t____Qseveral lines\n+\tQQ____These two lines have had their\n+\t____indentation reduced by four spaces\n+\tQdifferent indentation change\n+\t____too short\n+\tEOF\n+\n+\tgit add text.txt &&\n+\tgit commit -m \"add text.txt\" &&\n+\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\tQIndented text to\n+\tQQbe further indented by four spaces across\n+\tQ____several lines\n+\tQ_QThese two lines have had their\n+\tindentation reduced by four spaces\n+\tQQdifferent indentation change\n+\t__Qtoo short\n+\tEOF\n+\n+\tgit -c color.diff.whitespace=\"normal red\" \\\n+\t\t-c core.whitespace=space-before-tab \\\n+\t\tdiff --color --color-moved --ws-error-highlight=all \\\n+\t\t--color-moved-ws=allow-indentation-change >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n+\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,7 +1,7 @@\n+\t- Indented text to\n+\t- \t be further indented by four spaces across\n+\t- \tseveral lines\n+\t-\t\t These two lines have had their\n+\t- indentation reduced by four spaces\n+\t-\tdifferent indentation change\n+\t- too short\n+\t+\tIndented text to\n+\t+\t\tbe further indented by four spaces across\n+\t+\t several lines\n+\t+\t \tThese two lines have had their\n+\t+indentation reduced by four spaces\n+\t+\t\tdifferent indentation change\n+\t+ \ttoo short\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.19.1\n\n"},{"id":"363502","messageId":"20181116110356.12311-7-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 6/9] diff --color-moved=zebra: be stricter with color alternation","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:53Z","receivedAt":"2018-11-16T11:04:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCurrently when using --color-moved=zebra the color of moved blocks\ndepends on the number of lines separating them. This means that adding\nan odd number of unmoved lines between blocks that are already separated\nby one or more unmoved lines will change the color of subsequent moved\nblocks. This does not make much sense as the blocks were already\nseparated by unmoved lines and causes problems when adding lines to test\ncases.\n\nFix this by only using the alternate colors for adjacent moved blocks.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n An alternative would be to always alternate the color of blocks whether\n are not they are adjacent to each other.\n\n diff.c | 27 +++++++++++++++++++--------\n t/t4015-diff-whitespace.sh | 6 +++---\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 53a7ab5aca..8c08dd68df 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1038,26 +1038,30 @@ static int shrink_potential_moved_blocks(struct moved_block *pmb,\n * The last block consists of the (n - block_length)'th line up to but not\n * including the nth line.\n *\n+ * Returns 0 if the last block is empty or is unset by this function, non zero\n+ * otherwise.\n+ *\n * NEEDSWORK: This uses the same heuristic as blame_entry_score() in blame.c.\n * Think of a way to unify them.\n */\n-static void adjust_last_block(struct diff_options *o, int n, int block_length)\n+static int 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-\t\treturn;\n+\t\treturn block_length;\n \tfor (i = 1; i < block_length + 1; i++) {\n \t\tconst char *c = o->emitted_symbols->buf[n - i].line;\n \t\tfor (; *c; c++) {\n \t\t\tif (!isalnum(*c))\n \t\t\t\tcontinue;\n \t\t\talnum_count++;\n \t\t\tif (alnum_count >= COLOR_MOVED_MIN_ALNUM_COUNT)\n-\t\t\t\treturn;\n+\t\t\t\treturn 1;\n \t\t}\n \t}\n \tfor (i = 1; i < block_length + 1; i++)\n \t\to->emitted_symbols->buf[n - i].flags &= ~DIFF_SYMBOL_MOVED_LINE;\n+\treturn 0;\n }\n \n /* Find blocks of moved code, delegate actual coloring decision to helper */\n@@ -1067,14 +1071,15 @@ static void mark_color_as_moved(struct diff_options *o,\n {\n \tstruct moved_block *pmb = NULL; /* potentially moved blocks */\n \tint pmb_nr = 0, pmb_alloc = 0;\n-\tint n, flipped_block = 1, block_length = 0;\n+\tint n, flipped_block = 0, block_length = 0;\n \n \n \tfor (n = 0; n < o->emitted_symbols->nr; n++) {\n \t\tstruct hashmap *hm = NULL;\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\tenum diff_symbol last_symbol = 0;\n \n \t\tswitch (l->s) {\n \t\tcase DIFF_SYMBOL_PLUS:\n@@ -1090,7 +1095,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\tflipped_block = 1;\n+\t\t\tflipped_block = 0;\n \t\t}\n \n \t\tif (!match) {\n@@ -1101,10 +1106,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tmoved_block_clear(&pmb[i]);\n \t\t\tpmb_nr = 0;\n \t\t\tblock_length = 0;\n+\t\t\tflipped_block = 0;\n+\t\t\tlast_symbol = l->s;\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (o->color_moved == COLOR_MOVED_PLAIN) {\n+\t\t\tlast_symbol = l->s;\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tcontinue;\n \t\t}\n@@ -1135,19 +1143,22 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t}\n \t\t\t}\n \n-\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\tif (adjust_last_block(o, n, block_length) &&\n+\t\t\t pmb_nr && last_symbol != l->s)\n+\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\telse\n+\t\t\t\tflipped_block = 0;\n \n-\t\t\tadjust_last_block(o, n, block_length);\n \t\t\tblock_length = 0;\n \t\t}\n \n \t\tif (pmb_nr) {\n \t\t\tblock_length++;\n-\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n \t\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n \t\t}\n+\t\tlast_symbol = l->s;\n \t}\n \tadjust_last_block(o, n, block_length);\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex eee81a1987..fe8a2ab06e 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1802,14 +1802,14 @@ test_expect_success 'only move detection ignores white spaces' '\n \t-a long line to exceed per-line minimum\n \t-another long line to exceed per-line minimum\n \t-original file\n-\t+Qa long line to exceed per-line minimum\n-\t+Qanother long line to exceed per-line minimum\n+\t+Qa long line to exceed per-line minimum\n+\t+Qanother long line to exceed per-line minimum\n \t+new file\n \tEOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'compare whitespace delta across moved blocks' '\n+test_expect_success 'compare whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \tq_to_tab <<-\\EOF >text.txt &&\n-- \n2.19.1\n\n"},{"id":"363503","messageId":"20181116110356.12311-10-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 9/9] diff --color-moved-ws: handle blank lines","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:56Z","receivedAt":"2018-11-16T11:04:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen using --color-moved-ws=allow-indentation-change allow lines with\nthe same indentation change to be grouped across blank lines. For now\nthis only works if the blank lines have been moved as well, not for\nblocks that have just had their indentation changed.\n\nThis completes the changes to the implementation of\n--color-moved=allow-indentation-change. Running\n\n git diff --color-moved=allow-indentation-change v2.18.0 v2.19.0\n\nnow takes 5.0s. This is a saving of 41% from 8.5s for the optimized\nversion of the previous implementation and 66% from the original which\ntook 14.6s.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n Changes since rfc:\n - Split these changes into a separate commit.\n - Detect blank lines when processing the indentation rather than\n parsing each line twice.\n - Tweaked the test to make it harder as suggested by Stefan.\n - Added timing data to the commit message.\n\n diff.c | 34 ++++++++++++++++++++++++++++---\n t/t4015-diff-whitespace.sh | 41 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 68 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 89559293e7..072b5bced6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -792,9 +792,11 @@ static void moved_block_clear(struct moved_block *b)\n \tmemset(b, 0, sizeof(*b));\n }\n \n+#define INDENT_BLANKLINE INT_MIN\n+\n static void fill_es_indent_data(struct emitted_diff_symbol *es)\n {\n-\tunsigned int off = 0;\n+\tunsigned int off = 0, i;\n \tint width = 0, tab_width = es->flags & WS_TAB_WIDTH_MASK;\n \tconst char *s = es->line;\n \tconst int len = es->len;\n@@ -818,8 +820,18 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n \t\t}\n \t}\n \n-\tes->indent_off = off;\n-\tes->indent_width = width;\n+\t/* check if this line is blank */\n+\tfor (i = off; i < len; i++)\n+\t\tif (!isspace(s[i]))\n+\t\t break;\n+\n+\tif (i == len) {\n+\t\tes->indent_width = INDENT_BLANKLINE;\n+\t\tes->indent_off = len;\n+\t} else {\n+\t\tes->indent_off = off;\n+\t\tes->indent_width = width;\n+\t}\n }\n \n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n@@ -834,6 +846,11 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \t b_width = b->indent_width;\n \tint delta;\n \n+\tif (a_width == INDENT_BLANKLINE && b_width == INDENT_BLANKLINE) {\n+\t\t*out = INDENT_BLANKLINE;\n+\t\treturn 1;\n+\t}\n+\n \tif (a->s == DIFF_SYMBOL_PLUS)\n \t\tdelta = a_width - b_width;\n \telse\n@@ -877,6 +894,10 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (al != bl)\n \t\treturn 1;\n \n+\t/* If 'l' and 'cur' are both blank then they match. */\n+\tif (a_width == INDENT_BLANKLINE && c_width == INDENT_BLANKLINE)\n+\t\treturn 0;\n+\n \t/*\n \t * The indent changes of the block are known and stored in pmb->wsd;\n \t * however we need to check if the indent changes of the current line\n@@ -888,6 +909,13 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \telse\n \t\tdelta = c_width - a_width;\n \n+\t/*\n+\t * If the previous lines of this block were all blank then set its\n+\t * whitespace delta.\n+\t */\n+\tif (pmb->wsd == INDENT_BLANKLINE)\n+\t\tpmb->wsd = delta;\n+\n \treturn !(delta == pmb->wsd && al - a_off == cl - c_off &&\n \t\t !memcmp(a, b, al) && !\n \t\t memcmp(a + a_off, c + c_off, al - a_off));\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex e023839ba6..9d6f88b07f 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1901,10 +1901,20 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n \ttest_i18ngrep allow-indentation-change err\n '\n \n+EMPTY=''\n test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\t${EMPTY}\n+\t____too short without\n+\t${EMPTY}\n+\t___being grouped across blank line\n+\t${EMPTY}\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n \t____Indented text to\n \t_Q____be further indented by four spaces across\n \t____Qseveral lines\n@@ -1918,9 +1928,18 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \tgit commit -m \"add text.txt\" &&\n \n \ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n \tQIndented text to\n \tQQbe further indented by four spaces across\n \tQ____several lines\n+\t${EMPTY}\n+\tQQtoo short without\n+\t${EMPTY}\n+\tQ_______being grouped across blank line\n+\t${EMPTY}\n \tQ_QThese two lines have had their\n \tindentation reduced by four spaces\n \tQQdifferent indentation change\n@@ -1937,7 +1956,16 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \tdiff --git a/text.txt b/text.txt\n \t--- a/text.txt\n \t+++ b/text.txt\n-\t@@ -1,7 +1,7 @@\n+\t@@ -1,16 +1,16 @@\n+\t-\n+\t- too short without\n+\t-\n+\t- being grouped across blank line\n+\t-\n+\t context\n+\t lines\n+\t to\n+\t anchor\n \t- Indented text to\n \t- \t be further indented by four spaces across\n \t- \tseveral lines\n@@ -1948,9 +1976,14 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \t+\tIndented text to\n \t+\t\tbe further indented by four spaces across\n \t+\t several lines\n-\t+\t \tThese two lines have had their\n-\t+indentation reduced by four spaces\n-\t+\t\tdifferent indentation change\n+\t+\n+\t+\t\ttoo short without\n+\t+\n+\t+\t being grouped across blank line\n+\t+\n+\t+\t \tThese two lines have had their\n+\t+indentation reduced by four spaces\n+\t+\t\tdifferent indentation change\n \t+ \ttoo short\n \tEOF\n \n-- \n2.19.1\n\n"},{"id":"363504","messageId":"20181116110356.12311-1-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"[PATCH v1 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:47Z","receivedAt":"2018-11-16T11:04:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen trying out the new --color-moved-ws=allow-indentation-change I\nwas disappointed to discover it did not work if the indentation\ncontains a mix of spaces and tabs. This series reworks it so that it\ndoes.\n\nSince the rfc this series has grown a few fixes at the beginning. The\nimplementation has been reworked, the last two patches correspond to a\nheavily reworked version the last patch of the rfc version, all the\nother patches are new.\n\nPhillip Wood (9):\n diff: document --no-color-moved\n diff: use whitespace consistently\n diff: allow --no-color-moved-ws\n diff --color-moved-ws: demonstrate false positives\n diff --color-moved-ws: fix false positives\n diff --color-moved=zebra: be stricter with color alternation\n diff --color-moved-ws: optimize allow-indentation-change\n diff --color-moved-ws: modify allow-indentation-change\n diff --color-moved-ws: handle blank lines\n\n Documentation/diff-options.txt | 15 ++-\n diff.c | 219 +++++++++++++++++++++------------\n t/t4015-diff-whitespace.sh | 99 ++++++++++++++-\n 3 files changed, 251 insertions(+), 82 deletions(-)\n\n-- \n2.19.1\n\n"},{"id":"363505","messageId":"20181116110356.12311-6-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 5/9] diff --color-moved-ws: fix false positives","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:52Z","receivedAt":"2018-11-16T11:04:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n'diff --color-moved-ws=allow-indentation-change' can color lines as\nmoved when they are in fact different. For example in commit\n1a07e59c3e (\"Update messages in preparation for i18n\", 2018-07-21) the\nlines\n\n- die (_(\"must end with a color\"));\n+ die(_(\"must end with a color\"));\n\nare colored as moved even though they are different.\n\nThis is because if there is a fuzzy match for the first line of\na potential moved block the line is marked as moved before the\npotential match is checked to see if it actually matches. The fix is\nto delay marking the line as moved until after we have checked that\nthere really is at least one matching potential moved block.\n\nNote that the test modified in the last commit still fails because\nadding an unmoved line between two moved blocks that are already\nseparated by unmoved lines changes the color of the block following the\naddition. This should not be the case and will be fixed in the next\ncommit.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9b9811988b..53a7ab5aca 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1104,10 +1104,10 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tcontinue;\n \t\t}\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_PLAIN) {\n+\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (o->color_moved_ws_handling &\n \t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n@@ -1141,10 +1141,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tblock_length = 0;\n \t\t}\n \n-\t\tblock_length++;\n+\t\tif (pmb_nr) {\n+\t\t\tblock_length++;\n \n-\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n-\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n+\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n+\t\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n+\t\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n+\t\t}\n \t}\n \tadjust_last_block(o, n, block_length);\n \n-- \n2.19.1\n\n"},{"id":"363506","messageId":"20181116110356.12311-4-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 3/9] diff: allow --no-color-moved-ws","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:50Z","receivedAt":"2018-11-16T11:04:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAllow --no-color-moved-ws and --color-moved-ws=no to cancel any previous\n--color-moved-ws option.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 7 +++++++\n diff.c | 6 +++++-\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 57a2f4cb7a..e1744fa80d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -306,6 +306,8 @@ endif::git-diff[]\n \tThese modes can be given as a comma separated list:\n +\n --\n+no::\n+\tDo not ignore whitespace when performing move detection.\n ignore-space-at-eol::\n \tIgnore changes in whitespace at EOL.\n ignore-space-change::\n@@ -322,6 +324,11 @@ allow-indentation-change::\n \tother modes.\n --\n \n+--no-color-moved-ws::\n+\tDo not ignore whitespace when performing move detection. This can be\n+\tused to override configuration settings. It is the same as\n+\t`--color-moved-ws=no`.\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 78cd3958f4..9b9811988b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -304,7 +304,9 @@ static int parse_color_moved_ws(const char *arg)\n \t\tstrbuf_addstr(&sb, i->string);\n \t\tstrbuf_trim(&sb);\n \n-\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n+\t\tif (!strcmp(sb.buf, \"no\"))\n+\t\t\tret = 0;\n+\t\telse if (!strcmp(sb.buf, \"ignore-space-change\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n \t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n@@ -5008,6 +5010,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n+\t} else if (!strcmp(arg, \"--no-color-moved-ws\")) {\n+\t\toptions->color_moved_ws_handling = 0;\n \t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n \t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n-- \n2.19.1\n\n"},{"id":"363507","messageId":"20181116110356.12311-3-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181116110356.12311-1-phillip.wood@talktalk.net","subject":"[PATCH v1 2/9] diff: use whitespace consistently","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-16T11:03:49Z","receivedAt":"2018-11-16T11:04:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nMost of the documentation uses 'whitespace' rather than 'white space'\nor 'white spaces' convert to latter two to the former for consistency.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 4 ++--\n diff.c | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 151690f814..57a2f4cb7a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -298,7 +298,7 @@ dimmed-zebra::\n \tsettings. It is the same as `--color-moved=no`.\n \n --color-moved-ws=::\n-\tThis configures how white spaces are ignored when performing the\n+\tThis configures how whitespace is ignored when performing the\n \tmove detection for `--color-moved`.\n ifdef::git-diff[]\n \tIt can be set by the `diff.colorMovedWS` configuration setting.\n@@ -316,7 +316,7 @@ ignore-all-space::\n \tIgnore whitespace when comparing lines. This ignores differences\n \teven if one line has whitespace where the other line has none.\n allow-indentation-change::\n-\tInitially ignore any white spaces in the move detection, then\n+\tInitially ignore any whitespace in the move detection, then\n \tgroup the moved code blocks only into a block if the change in\n \twhitespace is the same per line. This is incompatible with the\n \tother modes.\ndiff --git a/diff.c b/diff.c\nindex c29b1cce14..78cd3958f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -320,7 +320,7 @@ static int parse_color_moved_ws(const char *arg)\n \n \tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n \t (ret & XDF_WHITESPACE_FLAGS))\n-\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n+\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other whitespace modes\"));\n \n \tstring_list_clear(&l, 0);\n \n-- \n2.19.1\n\n"},{"id":"363522","messageId":"CAGZ79kad66BzrWotXKQULG9qX11Yf1zx6_2YU9H4wF8VOhvHDA@mail.gmail.com","threadId":"49410","inReplyTo":"20181116110356.12311-3-phillip.wood@talktalk.net","subject":"Re: [PATCH v1 2/9] diff: use whitespace consistently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-16T18:29:48Z","receivedAt":"2018-11-16T18:30:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Most of the documentation uses 'whitespace' rather than 'white space'\n> or 'white spaces' convert to latter two to the former for consistency.\n\nMakes sense; this doesn't touch docs, but also code.\n$ git grep \"white space\" yields some other places\nas well (Documentation/git-cat-file.txt and lots in t/)\nBut I guess we keep it to this feature for now instead\nof a tree wide cleanup.\n\nStefan\n"},{"id":"363534","messageId":"CAGZ79ka4mHxtcwjfu3taipakUHtDXg6DjQu=nJun8Nm+snyo0g@mail.gmail.com","threadId":"49410","inReplyTo":"20181116110356.12311-8-phillip.wood@talktalk.net","subject":"Re: [PATCH v1 7/9] diff --color-moved-ws: optimize allow-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-16T20:40:34Z","receivedAt":"2018-11-16T20:40:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> When running\n>\n> git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\n>\n> cmp_in_block_with_wsd() is called 694908327 times. Of those 42.7%\n> return after comparing a and b. By comparing the lengths first we can\n> return early in all but 0.03% of those cases without dereferencing the\n> string pointers. The comparison between a and c fails in 6.8% of\n> calls, by comparing the lengths first we reject all the failing calls\n> without dereferencing the string pointers.\n>\n> This reduces the time to run the command above by by 42% from 14.6s to\n> 8.5s. This is still much slower than the normal --color-moved which\n> takes ~0.6-0.7s to run but is a significant improvement.\n>\n> The next commits will replace the current implementation with one that\n> works with mixed tabs and spaces in the indentation. I think it is\n> worth optimizing the current implementation first to enable a fair\n> comparison between the two implementations.\n\nUp to here the series looks good and I think we could take it\nas a preparatory self-standing series.\n\nI'll read on.\nThanks,\nStefan\n"},{"id":"363536","messageId":"CAGZ79kb2fY+6xg=B+t=gSEBQ+u-wKAff++z5A=KiN2u0yYFF6g@mail.gmail.com","threadId":"49410","inReplyTo":"20181116110356.12311-9-phillip.wood@talktalk.net","subject":"Re: [PATCH v1 8/9] diff --color-moved-ws: modify allow-indentation-change","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-16T21:47:03Z","receivedAt":"2018-11-16T21:47:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Currently diff --color-moved-ws=allow-indentation-change does not\n> support indentation that contains a mix of tabs and spaces. For\n> example in commit 546f70f377 (\"convert.h: drop 'extern' from function\n> declaration\", 2018-06-30) the function parameters in the following\n> lines are not colored as moved [1].\n>\n> -extern int stream_filter(struct stream_filter *,\n> - const char *input, size_t *isize_p,\n> - char *output, size_t *osize_p);\n> +int stream_filter(struct stream_filter *,\n> + const char *input, size_t *isize_p,\n> + char *output, size_t *osize_p);\n>\n> This commit changes the way the indentation is handled to track the\n> visual size of the indentation rather than the characters in the\n> indentation. This has they benefit that any whitespace errors do not\n\ns/they/the/\n\n> interfer with the move detection (the whitespace errors will still be\n> highlighted according to --ws-error-highlight). During the discussion\n> of this feature there were concerns about the correct detection of\n> indentation for python. However those concerns apply whether or not\n> we're detecting moved lines so no attempt is made to determine if the\n> indentation is 'pythonic'.\n>\n> [1] Note that before the commit to fix the erroneous coloring of moved\n> lines each line was colored as a different block, since that commit\n> they are uncolored.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> Notes:\n> Changes since rfc:\n> - It now replaces the existing implementation rather than adding a new\n> mode.\n> - The indentation deltas are now calculated once for each line and\n> cached.\n> - Optimized the whitespace delta comparison to compare string lengths\n> before comparing the actual strings.\n> - Modified the calculation of tabs as suggested by Stefan.\n> - Split out the blank line handling into a separate commit as suggest\n> by Stefan.\n> - Fixed some comments pointed out by Stefan.\n>\n> diff.c | 130 +++++++++++++++++++++----------------\n> t/t4015-diff-whitespace.sh | 56 ++++++++++++++++\n> 2 files changed, 129 insertions(+), 57 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index c378ce3daf..89559293e7 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -750,6 +750,8 @@ struct emitted_diff_symbol {\n> const char *line;\n> int len;\n> int flags;\n> + int indent_off;\n> + int indent_width;\n\nSo this is the trick how we compute the ws related\ndata only once per line. :-)\n\nOn the other hand, we do not save memory by disabling\nthe ws detection, but I guess that is not a problem for now.\n\nWould it make sense to have the new variables be\nunsigned? (Also a comment on what they are, I\nneeded to read the code to understand off to be\noffset into the line, where the content starts, and\nwidth to be the visual width, as I did not recall\nthe RFC.)\n\n> +static void fill_es_indent_data(struct emitted_diff_symbol *es)\n> [...]\n\n> + if (o->color_moved_ws_handling &\n> + COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n> + fill_es_indent_data(&o->emitted_symbols->buf[n]);\n\nNice.\n\nBy reducing the information kept around to ints, we also do not need\nto alloc/free\nmemory for each line.\n\n> +++ b/t/t4015-diff-whitespace.sh\n> @@ -1901,4 +1901,60 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n> test_i18ngrep allow-indentation-change err\n> '\n>\n> +test_expect_success 'compare mixed whitespace delta across moved blocks' '\n\nLooks good,\n\nThanks!\nStefan\n"},{"id":"363556","messageId":"a7ca6eef-937c-ae97-bb79-5859a2849e64@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79ka4mHxtcwjfu3taipakUHtDXg6DjQu=nJun8Nm+snyo0g@mail.gmail.com","subject":"Re: [PATCH v1 7/9] diff --color-moved-ws: optimize allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-17T14:52:01Z","receivedAt":"2018-11-17T14:52:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 16/11/2018 20:40, Stefan Beller wrote:\n> On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> When running\n>>\n>> git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\n>>\n>> cmp_in_block_with_wsd() is called 694908327 times. Of those 42.7%\n>> return after comparing a and b. By comparing the lengths first we can\n>> return early in all but 0.03% of those cases without dereferencing the\n>> string pointers. The comparison between a and c fails in 6.8% of\n>> calls, by comparing the lengths first we reject all the failing calls\n>> without dereferencing the string pointers.\n>>\n>> This reduces the time to run the command above by by 42% from 14.6s to\n>> 8.5s. This is still much slower than the normal --color-moved which\n>> takes ~0.6-0.7s to run but is a significant improvement.\n>>\n>> The next commits will replace the current implementation with one that\n>> works with mixed tabs and spaces in the indentation. I think it is\n>> worth optimizing the current implementation first to enable a fair\n>> comparison between the two implementations.\n> \n> Up to here the series looks good and I think we could take it\n> as a preparatory self-standing series.\n\nThanks for looking at these, I think it makes sense to split the series \nhere, the commit message for this patch may want tweaking slightly if we \ndo. (I did wonder about splitting it in two when I submitted it but took \nthe easy way out.)\n\nBest Wishes\n\nPhillip\n> \n> I'll read on.\n> Thanks,\n> Stefan\n> \n\n"},{"id":"363557","messageId":"42818e32-8774-722d-b46d-9a23f4097315@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79kb2fY+6xg=B+t=gSEBQ+u-wKAff++z5A=KiN2u0yYFF6g@mail.gmail.com","subject":"Re: [PATCH v1 8/9] diff --color-moved-ws: modify allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-17T14:59:50Z","receivedAt":"2018-11-17T14:59:55Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nOn 16/11/2018 21:47, Stefan Beller wrote:\n> On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> Currently diff --color-moved-ws=allow-indentation-change does not\n>> support indentation that contains a mix of tabs and spaces. For\n>> example in commit 546f70f377 (\"convert.h: drop 'extern' from function\n>> declaration\", 2018-06-30) the function parameters in the following\n>> lines are not colored as moved [1].\n>>\n>> -extern int stream_filter(struct stream_filter *,\n>> - const char *input, size_t *isize_p,\n>> - char *output, size_t *osize_p);\n>> +int stream_filter(struct stream_filter *,\n>> + const char *input, size_t *isize_p,\n>> + char *output, size_t *osize_p);\n>>\n>> This commit changes the way the indentation is handled to track the\n>> visual size of the indentation rather than the characters in the\n>> indentation. This has they benefit that any whitespace errors do not\n> \n> s/they/the/\n\nThanks, well spotted\n\n> \n>> interfer with the move detection (the whitespace errors will still be\n>> highlighted according to --ws-error-highlight). During the discussion\n>> of this feature there were concerns about the correct detection of\n>> indentation for python. However those concerns apply whether or not\n>> we're detecting moved lines so no attempt is made to determine if the\n>> indentation is 'pythonic'.\n>>\n>> [1] Note that before the commit to fix the erroneous coloring of moved\n>> lines each line was colored as a different block, since that commit\n>> they are uncolored.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>>\n>> Notes:\n>> Changes since rfc:\n>> - It now replaces the existing implementation rather than adding a new\n>> mode.\n>> - The indentation deltas are now calculated once for each line and\n>> cached.\n>> - Optimized the whitespace delta comparison to compare string lengths\n>> before comparing the actual strings.\n>> - Modified the calculation of tabs as suggested by Stefan.\n>> - Split out the blank line handling into a separate commit as suggest\n>> by Stefan.\n>> - Fixed some comments pointed out by Stefan.\n>>\n>> diff.c | 130 +++++++++++++++++++++----------------\n>> t/t4015-diff-whitespace.sh | 56 ++++++++++++++++\n>> 2 files changed, 129 insertions(+), 57 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index c378ce3daf..89559293e7 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -750,6 +750,8 @@ struct emitted_diff_symbol {\n>> const char *line;\n>> int len;\n>> int flags;\n>> + int indent_off;\n>> + int indent_width;\n> \n> So this is the trick how we compute the ws related\n> data only once per line. :-)\n> \n> On the other hand, we do not save memory by disabling\n> the ws detection, but I guess that is not a problem for now.\n\nI did wonder about that, but decided the increase was small compared to \nall the strings that are copied when creating the emitted_diff_symbols. \nIf we want to save memory then we should stop struct \nemitted_diff_symbol() from carrying a copy of all the strings.\n\n> Would it make sense to have the new variables be\n> unsigned? (Also a comment on what they are, I\n> needed to read the code to understand off to be\n> offset into the line, where the content starts, and\n> width to be the visual width, as I did not recall\n> the RFC.)\n\nYes a comment would make sense. I don't think I have a strong preference \nfor signed/unsigned, I can change it if you want.\n\nThanks for looking at these so promptly\n\nBest Wishes\n\nPhillip\n>> +static void fill_es_indent_data(struct emitted_diff_symbol *es)\n>> [...]\n> \n>> + if (o->color_moved_ws_handling &\n>> + COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n>> + fill_es_indent_data(&o->emitted_symbols->buf[n]);\n> \n> Nice.\n> \n> By reducing the information kept around to ints, we also do not need\n> to alloc/free\n> memory for each line.\n> \n>> +++ b/t/t4015-diff-whitespace.sh\n>> @@ -1901,4 +1901,60 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n>> test_i18ngrep allow-indentation-change err\n>> '\n>>\n>> +test_expect_success 'compare mixed whitespace delta across moved blocks' '\n> \n> Looks good,\n> \n> Thanks!\n> Stefan\n> \n\n"},{"id":"363761","messageId":"CAGZ79kYm-uNWi-3=0fG=PfA3HbT7tKwER=r8fm6UFiy3P=JEmA@mail.gmail.com","threadId":"49410","inReplyTo":"20181116110356.12311-10-phillip.wood@talktalk.net","subject":"Re: [PATCH v1 9/9] diff --color-moved-ws: handle blank lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-20T18:05:29Z","receivedAt":"2018-11-20T18:05:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> When using --color-moved-ws=allow-indentation-change allow lines with\n> the same indentation change to be grouped across blank lines. For now\n> this only works if the blank lines have been moved as well, not for\n> blocks that have just had their indentation changed.\n>\n> This completes the changes to the implementation of\n> --color-moved=allow-indentation-change. Running\n>\n> git diff --color-moved=allow-indentation-change v2.18.0 v2.19.0\n>\n> now takes 5.0s. This is a saving of 41% from 8.5s for the optimized\n> version of the previous implementation and 66% from the original which\n> took 14.6s.\n>\n> Signed-off-by: Phillip Wood \n> ---\n>\n> Notes:\n> Changes since rfc:\n> - Split these changes into a separate commit.\n> - Detect blank lines when processing the indentation rather than\n> parsing each line twice.\n> - Tweaked the test to make it harder as suggested by Stefan.\n> - Added timing data to the commit message.\n>\n> diff.c | 34 ++++++++++++++++++++++++++++---\n> t/t4015-diff-whitespace.sh | 41 ++++++++++++++++++++++++++++++++++----\n> 2 files changed, 68 insertions(+), 7 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 89559293e7..072b5bced6 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -792,9 +792,11 @@ static void moved_block_clear(struct moved_block *b)\n> memset(b, 0, sizeof(*b));\n> }\n>\n> +#define INDENT_BLANKLINE INT_MIN\n\nAnswering my question from the previous patch:\nThis is why we need to keep the indents signed.\n\nThis patch looks quite nice to read along.\n\nThe whole series looks good to me.\nDo we need to update the docs in any way?\n\nThanks,\nStefan\n"},{"id":"363839","messageId":"ef49bb0e-cf03-a707-f562-595564bd70d8@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79kYm-uNWi-3=0fG=PfA3HbT7tKwER=r8fm6UFiy3P=JEmA@mail.gmail.com","subject":"Re: [PATCH v1 9/9] diff --color-moved-ws: handle blank lines","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-21T15:49:56Z","receivedAt":"2018-11-21T15:51:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 20/11/2018 18:05, Stefan Beller wrote:\n> On Fri, Nov 16, 2018 at 3:04 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> When using --color-moved-ws=allow-indentation-change allow lines with\n>> the same indentation change to be grouped across blank lines. For now\n>> this only works if the blank lines have been moved as well, not for\n>> blocks that have just had their indentation changed.\n>>\n>> This completes the changes to the implementation of\n>> --color-moved=allow-indentation-change. Running\n>>\n>> git diff --color-moved=allow-indentation-change v2.18.0 v2.19.0\n>>\n>> now takes 5.0s. This is a saving of 41% from 8.5s for the optimized\n>> version of the previous implementation and 66% from the original which\n>> took 14.6s.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n>>\n>> Notes:\n>> Changes since rfc:\n>> - Split these changes into a separate commit.\n>> - Detect blank lines when processing the indentation rather than\n>> parsing each line twice.\n>> - Tweaked the test to make it harder as suggested by Stefan.\n>> - Added timing data to the commit message.\n>>\n>> diff.c | 34 ++++++++++++++++++++++++++++---\n>> t/t4015-diff-whitespace.sh | 41 ++++++++++++++++++++++++++++++++++----\n>> 2 files changed, 68 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 89559293e7..072b5bced6 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -792,9 +792,11 @@ static void moved_block_clear(struct moved_block *b)\n>> memset(b, 0, sizeof(*b));\n>> }\n>>\n>> +#define INDENT_BLANKLINE INT_MIN\n> \n> Answering my question from the previous patch:\n> This is why we need to keep the indents signed.\n> \n> This patch looks quite nice to read along.\n> \n> The whole series looks good to me.\n\nThanks\n\n> Do we need to update the docs in any way?\n\nI'm not sure, at the moment it does not make any promises about the \nexact behavior of --color-moved-ws=allow-indentation-change, we could \nchange it to be more explicit but I'm not sure it's worth it.\n\nThanks for looking over these patches, I'll post a reroll soon based on \nyour comments.\n\nPhillip\n\n> Thanks,\n> Stefan\n> \n\n"},{"id":"363961","messageId":"20181123111658.30342-2-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 1/9] diff: document --no-color-moved","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:50Z","receivedAt":"2018-11-23T11:17:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAdd documentation for --no-color-moved.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 0378cd574e..151690f814 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -293,6 +293,10 @@ dimmed-zebra::\n \t`dimmed_zebra` is a deprecated synonym.\n --\n \n+--no-color-moved::\n+\tTurn off move detection. This can be used to override configuration\n+\tsettings. It is the same as `--color-moved=no`.\n+\n --color-moved-ws=::\n \tThis configures how white spaces are ignored when performing the\n \tmove detection for `--color-moved`.\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363962","messageId":"20181123111658.30342-1-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20180924100604.32208-1-phillip.wood@talktalk.net","subject":"[PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:49Z","receivedAt":"2018-11-23T11:17:16Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nThanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in\nresponse to those comments - see the range-diff below for details (the\npatch numbers are off by one in the range diff, I think because the\nfirst patch is unchanged and so it was used as the merge base by\n--range-diff=. For some reason the range-diff also includes\nthe notes even though I did not give --notes to format-patch)\n\nWhen trying out the new --color-moved-ws=allow-indentation-change I\nwas disappointed to discover it did not work if the indentation\ncontains a mix of spaces and tabs. This series reworks it so that it\ndoes.\n\n\nPhillip Wood (9):\n diff: document --no-color-moved\n Use \"whitespace\" consistently\n diff: allow --no-color-moved-ws\n diff --color-moved-ws: demonstrate false positives\n diff --color-moved-ws: fix false positives\n diff --color-moved=zebra: be stricter with color alternation\n diff --color-moved-ws: optimize allow-indentation-change\n diff --color-moved-ws: modify allow-indentation-change\n diff --color-moved-ws: handle blank lines\n\n Documentation/diff-options.txt | 15 ++-\n Documentation/git-cat-file.txt | 8 +-\n diff.c | 219 +++++++++++++++++++++------------\n t/t4015-diff-whitespace.sh | 99 ++++++++++++++-\n 4 files changed, 255 insertions(+), 86 deletions(-)\n\nRange-diff against v1:\n1: ae58ae4f29 ! 1: 4939ee371d diff: use whitespace consistently\n @@ -1,9 +1,10 @@\n Author: Phillip Wood \n \n - diff: use whitespace consistently\n + Use \"whitespace\" consistently\n \n - Most of the documentation uses 'whitespace' rather than 'white space'\n - or 'white spaces' convert to latter two to the former for consistency.\n + Most of the messages and documentation use 'whitespace' rather than\n + 'white space' or 'white spaces' convert to latter two to the former for\n + consistency.\n \n Signed-off-by: Phillip Wood \n \n @@ -29,6 +30,39 @@\n \twhitespace is the same per line. This is incompatible with the\n \tother modes.\n \n + diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n + --- a/Documentation/git-cat-file.txt\n + +++ b/Documentation/git-cat-file.txt\n +@@\n + stdin, and the SHA-1, type, and size of each object is printed on stdout. The\n + output format can be overridden using the optional `` argument. If\n + either `--textconv` or `--filters` was specified, the input is expected to\n +-list the object names followed by the path name, separated by a single white\n +-space, so that the appropriate drivers can be determined.\n ++list the object names followed by the path name, separated by a single\n ++whitespace, so that the appropriate drivers can be determined.\n + \n + OPTIONS\n + -------\n +@@\n + \tPrint object information and contents for each object provided\n + \ton stdin. May not be combined with any other options or arguments\n + \texcept `--textconv` or `--filters`, in which case the input lines\n +-\talso need to specify the path, separated by white space. See the\n ++\talso need to specify the path, separated by whitespace. See the\n + \tsection `BATCH OUTPUT` below for details.\n + \n + --batch-check::\n + --batch-check=::\n + \tPrint object information for each object provided on stdin. May\n + \tnot be combined with any other options or arguments except\n + \t`--textconv` or `--filters`, in which case the input lines also\n +-\tneed to specify the path, separated by white space. See the\n ++\tneed to specify the path, separated by whitespace. See the\n + \tsection `BATCH OUTPUT` below for details.\n + \n + --batch-all-objects::\n +\n diff --git a/diff.c b/diff.c\n --- a/diff.c\n +++ b/diff.c\n2: 7072bc6211 = 2: 204c7fea9d diff: allow --no-color-moved-ws\n3: ce3ad19eea = 3: 542b79b215 diff --color-moved-ws: demonstrate false positives\n4: 700e0b61e7 = 4: 4ffb5c4122 diff --color-moved-ws: fix false positives\n5: 9ecd8159a7 = 5: a3a84f90c5 diff --color-moved=zebra: be stricter with color alternation\n6: 1b1158b1ca = 6: f94f2e0bae diff --color-moved-ws: optimize allow-indentation-change\n7: d8a362be6a ! 7: fe8eb9cdbc diff --color-moved-ws: modify allow-indentation-change\n @@ -17,7 +17,7 @@\n \n This commit changes the way the indentation is handled to track the\n visual size of the indentation rather than the characters in the\n - indentation. This has they benefit that any whitespace errors do not\n + indentation. This has the benefit that any whitespace errors do not\n interfer with the move detection (the whitespace errors will still be\n highlighted according to --ws-error-highlight). During the discussion\n of this feature there were concerns about the correct detection of\n @@ -30,7 +30,7 @@\n they are uncolored.\n \n Signed-off-by: Phillip Wood \n - Changes since rfc:\n + changes since rfc:\n - It now replaces the existing implementation rather than adding a new\n mode.\n - The indentation deltas are now calculated once for each line and\n @@ -49,8 +49,8 @@\n \tconst char *line;\n \tint len;\n \tint flags;\n -+\tint indent_off;\n -+\tint indent_width;\n ++\tint indent_off; /* Offset to first non-whitespace character */\n ++\tint indent_width; /* The visual width of the indentation */\n \tenum diff_symbol s;\n };\n #define EMITTED_DIFF_SYMBOL_INIT {NULL}\n8: 1f7e99d45c = 8: e600f8247c diff --color-moved-ws: handle blank lines\n \n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363963","messageId":"20181123111658.30342-6-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 5/9] diff --color-moved-ws: fix false positives","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:54Z","receivedAt":"2018-11-23T11:17:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n'diff --color-moved-ws=allow-indentation-change' can color lines as\nmoved when they are in fact different. For example in commit\n1a07e59c3e (\"Update messages in preparation for i18n\", 2018-07-21) the\nlines\n\n- die (_(\"must end with a color\"));\n+ die(_(\"must end with a color\"));\n\nare colored as moved even though they are different.\n\nThis is because if there is a fuzzy match for the first line of\na potential moved block the line is marked as moved before the\npotential match is checked to see if it actually matches. The fix is\nto delay marking the line as moved until after we have checked that\nthere really is at least one matching potential moved block.\n\nNote that the test modified in the last commit still fails because\nadding an unmoved line between two moved blocks that are already\nseparated by unmoved lines changes the color of the block following the\naddition. This should not be the case and will be fixed in the next\ncommit.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9b9811988b..53a7ab5aca 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1104,10 +1104,10 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tcontinue;\n \t\t}\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_PLAIN) {\n+\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tcontinue;\n+\t\t}\n \n \t\tif (o->color_moved_ws_handling &\n \t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n@@ -1141,10 +1141,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tblock_length = 0;\n \t\t}\n \n-\t\tblock_length++;\n+\t\tif (pmb_nr) {\n+\t\t\tblock_length++;\n \n-\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n-\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n+\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n+\t\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n+\t\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n+\t\t}\n \t}\n \tadjust_last_block(o, n, block_length);\n \n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363964","messageId":"20181123111658.30342-5-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 4/9] diff --color-moved-ws: demonstrate false positives","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:53Z","receivedAt":"2018-11-23T11:17:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\n'diff --color-moved-ws=allow-indentation-change' can highlight lines\nthat have internal whitespace changes rather than indentation\nchanges. For example in commit 1a07e59c3e (\"Update messages in\npreparation for i18n\", 2018-07-21) the lines\n\n- die (_(\"must end with a color\"));\n+ die(_(\"must end with a color\"));\n\nare highlighted as moved when they should not be. Modify an existing\ntest to show the problem that will be fixed in the next commit.\n\nSigned-off-by: Phillip Wood \n---\n t/t4015-diff-whitespace.sh | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex a9fb226c5a..eee81a1987 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1809,7 +1809,7 @@ test_expect_success 'only move detection ignores white spaces' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'compare whitespace delta across moved blocks' '\n+test_expect_failure 'compare whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \tq_to_tab <<-\\EOF >text.txt &&\n@@ -1827,6 +1827,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \tQQQthat has similar lines\n \tQQQto previous blocks, but with different indent\n \tQQQYetQAnotherQoutlierQ\n+\tQLine with internal w h i t e s p a c e change\n \tEOF\n \n \tgit add text.txt &&\n@@ -1847,6 +1848,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \tQQthat has similar lines\n \tQQto previous blocks, but with different indent\n \tQQYetQAnotherQoutlier\n+\tQLine with internal whitespace change\n \tEOF\n \n \tgit diff --color --color-moved --color-moved-ws=allow-indentation-change >actual.raw &&\n@@ -1856,7 +1858,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\tdiff --git a/text.txt b/text.txt\n \t\t--- a/text.txt\n \t\t+++ b/text.txt\n-\t\t@@ -1,14 +1,14 @@\n+\t\t@@ -1,15 +1,15 @@\n \t\t-QIndented\n \t\t-QText across\n \t\t-Qsome lines\n@@ -1871,6 +1873,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\t-QQQthat has similar lines\n \t\t-QQQto previous blocks, but with different indent\n \t\t-QQQYetQAnotherQoutlierQ\n+\t\t-QLine with internal w h i t e s p a c e change\n \t\t+QQIndented\n \t\t+QQText across\n \t\t+QQsome lines\n@@ -1885,6 +1888,7 @@ test_expect_success 'compare whitespace delta across moved blocks' '\n \t\t+QQthat has similar lines\n \t\t+QQto previous blocks, but with different indent\n \t\t+QQYetQAnotherQoutlier\n+\t\t+QLine with internal whitespace change\n \tEOF\n \n \ttest_cmp expected actual\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363965","messageId":"20181123111658.30342-7-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 6/9] diff --color-moved=zebra: be stricter with color alternation","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:55Z","receivedAt":"2018-11-23T11:17:19Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCurrently when using --color-moved=zebra the color of moved blocks\ndepends on the number of lines separating them. This means that adding\nan odd number of unmoved lines between blocks that are already separated\nby one or more unmoved lines will change the color of subsequent moved\nblocks. This does not make much sense as the blocks were already\nseparated by unmoved lines and causes problems when adding lines to test\ncases.\n\nFix this by only using the alternate colors for adjacent moved blocks.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 27 +++++++++++++++++++--------\n t/t4015-diff-whitespace.sh | 6 +++---\n 2 files changed, 22 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 53a7ab5aca..8c08dd68df 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1038,26 +1038,30 @@ static int shrink_potential_moved_blocks(struct moved_block *pmb,\n * The last block consists of the (n - block_length)'th line up to but not\n * including the nth line.\n *\n+ * Returns 0 if the last block is empty or is unset by this function, non zero\n+ * otherwise.\n+ *\n * NEEDSWORK: This uses the same heuristic as blame_entry_score() in blame.c.\n * Think of a way to unify them.\n */\n-static void adjust_last_block(struct diff_options *o, int n, int block_length)\n+static int 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-\t\treturn;\n+\t\treturn block_length;\n \tfor (i = 1; i < block_length + 1; i++) {\n \t\tconst char *c = o->emitted_symbols->buf[n - i].line;\n \t\tfor (; *c; c++) {\n \t\t\tif (!isalnum(*c))\n \t\t\t\tcontinue;\n \t\t\talnum_count++;\n \t\t\tif (alnum_count >= COLOR_MOVED_MIN_ALNUM_COUNT)\n-\t\t\t\treturn;\n+\t\t\t\treturn 1;\n \t\t}\n \t}\n \tfor (i = 1; i < block_length + 1; i++)\n \t\to->emitted_symbols->buf[n - i].flags &= ~DIFF_SYMBOL_MOVED_LINE;\n+\treturn 0;\n }\n \n /* Find blocks of moved code, delegate actual coloring decision to helper */\n@@ -1067,14 +1071,15 @@ static void mark_color_as_moved(struct diff_options *o,\n {\n \tstruct moved_block *pmb = NULL; /* potentially moved blocks */\n \tint pmb_nr = 0, pmb_alloc = 0;\n-\tint n, flipped_block = 1, block_length = 0;\n+\tint n, flipped_block = 0, block_length = 0;\n \n \n \tfor (n = 0; n < o->emitted_symbols->nr; n++) {\n \t\tstruct hashmap *hm = NULL;\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\tenum diff_symbol last_symbol = 0;\n \n \t\tswitch (l->s) {\n \t\tcase DIFF_SYMBOL_PLUS:\n@@ -1090,7 +1095,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\tfree(key);\n \t\t\tbreak;\n \t\tdefault:\n-\t\t\tflipped_block = 1;\n+\t\t\tflipped_block = 0;\n \t\t}\n \n \t\tif (!match) {\n@@ -1101,10 +1106,13 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\tmoved_block_clear(&pmb[i]);\n \t\t\tpmb_nr = 0;\n \t\t\tblock_length = 0;\n+\t\t\tflipped_block = 0;\n+\t\t\tlast_symbol = l->s;\n \t\t\tcontinue;\n \t\t}\n \n \t\tif (o->color_moved == COLOR_MOVED_PLAIN) {\n+\t\t\tlast_symbol = l->s;\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tcontinue;\n \t\t}\n@@ -1135,19 +1143,22 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t}\n \t\t\t}\n \n-\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\tif (adjust_last_block(o, n, block_length) &&\n+\t\t\t pmb_nr && last_symbol != l->s)\n+\t\t\t\tflipped_block = (flipped_block + 1) % 2;\n+\t\t\telse\n+\t\t\t\tflipped_block = 0;\n \n-\t\t\tadjust_last_block(o, n, block_length);\n \t\t\tblock_length = 0;\n \t\t}\n \n \t\tif (pmb_nr) {\n \t\t\tblock_length++;\n-\n \t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \t\t\tif (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)\n \t\t\t\tl->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;\n \t\t}\n+\t\tlast_symbol = l->s;\n \t}\n \tadjust_last_block(o, n, block_length);\n \ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex eee81a1987..fe8a2ab06e 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1802,14 +1802,14 @@ test_expect_success 'only move detection ignores white spaces' '\n \t-a long line to exceed per-line minimum\n \t-another long line to exceed per-line minimum\n \t-original file\n-\t+Qa long line to exceed per-line minimum\n-\t+Qanother long line to exceed per-line minimum\n+\t+Qa long line to exceed per-line minimum\n+\t+Qanother long line to exceed per-line minimum\n \t+new file\n \tEOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'compare whitespace delta across moved blocks' '\n+test_expect_success 'compare whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \tq_to_tab <<-\\EOF >text.txt &&\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363966","messageId":"20181123111658.30342-8-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 7/9] diff --color-moved-ws: optimize allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:56Z","receivedAt":"2018-11-23T11:17:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen running\n\n git diff --color-moved-ws=allow-indentation-change v2.18.0 v2.19.0\n\ncmp_in_block_with_wsd() is called 694908327 times. Of those 42.7%\nreturn after comparing a and b. By comparing the lengths first we can\nreturn early in all but 0.03% of those cases without dereferencing the\nstring pointers. The comparison between a and c fails in 6.8% of\ncalls, by comparing the lengths first we reject all the failing calls\nwithout dereferencing the string pointers.\n\nThis reduces the time to run the command above by by 42% from 14.6s to\n8.5s. This is still much slower than the normal --color-moved which\ntakes ~0.6-0.7s to run but is a significant improvement.\n\nThe next commits will replace the current implementation with one that\nworks with mixed tabs and spaces in the indentation. I think it is\nworth optimizing the current implementation first to enable a fair\ncomparison between the two implementations.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 19 +++++++++++--------\n 1 file changed, 11 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8c08dd68df..c378ce3daf 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -829,20 +829,23 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\t\t\t int n)\n {\n \tstruct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];\n-\tint al = cur->es->len, cl = l->len;\n+\tint al = cur->es->len, bl = match->es->len, cl = l->len;\n \tconst char *a = cur->es->line,\n \t\t *b = match->es->line,\n \t\t *c = l->line;\n-\n+\tconst char *orig_a = a;\n \tint wslen;\n \n \t/*\n-\t * We need to check if 'cur' is equal to 'match'.\n-\t * As those are from the same (+/-) side, we do not need to adjust for\n-\t * indent changes. However these were found using fuzzy matching\n-\t * so we do have to check if they are equal.\n+\t * We need to check if 'cur' is equal to 'match'. As those\n+\t * are from the same (+/-) side, we do not need to adjust for\n+\t * indent changes. However these were found using fuzzy\n+\t * matching so we do have to check if they are equal. Here we\n+\t * just check the lengths. We delay calling memcmp() to check\n+\t * the contents until later as if the length comparison for a\n+\t * and c fails we can avoid the call all together.\n \t */\n-\tif (strcmp(a, b))\n+\tif (al != bl)\n \t\treturn 1;\n \n \tif (!pmb->wsd.string)\n@@ -870,7 +873,7 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \t\tal -= wslen;\n \t}\n \n-\tif (al != cl || memcmp(a, c, al))\n+\tif (al != cl || memcmp(orig_a, b, bl) || memcmp(a, c, al))\n \t\treturn 1;\n \n \treturn 0;\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363967","messageId":"20181123111658.30342-9-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 8/9] diff --color-moved-ws: modify allow-indentation-change","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:57Z","receivedAt":"2018-11-23T11:17:20Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nCurrently diff --color-moved-ws=allow-indentation-change does not\nsupport indentation that contains a mix of tabs and spaces. For\nexample in commit 546f70f377 (\"convert.h: drop 'extern' from function\ndeclaration\", 2018-06-30) the function parameters in the following\nlines are not colored as moved [1].\n\n-extern int stream_filter(struct stream_filter *,\n- const char *input, size_t *isize_p,\n- char *output, size_t *osize_p);\n+int stream_filter(struct stream_filter *,\n+ const char *input, size_t *isize_p,\n+ char *output, size_t *osize_p);\n\nThis commit changes the way the indentation is handled to track the\nvisual size of the indentation rather than the characters in the\nindentation. This has the benefit that any whitespace errors do not\ninterfer with the move detection (the whitespace errors will still be\nhighlighted according to --ws-error-highlight). During the discussion\nof this feature there were concerns about the correct detection of\nindentation for python. However those concerns apply whether or not\nwe're detecting moved lines so no attempt is made to determine if the\nindentation is 'pythonic'.\n\n[1] Note that before the commit to fix the erroneous coloring of moved\n lines each line was colored as a different block, since that commit\n they are uncolored.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 130 +++++++++++++++++++++----------------\n t/t4015-diff-whitespace.sh | 56 ++++++++++++++++\n 2 files changed, 129 insertions(+), 57 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex c378ce3daf..148503e49c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -750,6 +750,8 @@ struct emitted_diff_symbol {\n \tconst char *line;\n \tint len;\n \tint flags;\n+\tint indent_off; /* Offset to first non-whitespace character */\n+\tint indent_width; /* The visual width of the indentation */\n \tenum diff_symbol s;\n };\n #define EMITTED_DIFF_SYMBOL_INIT {NULL}\n@@ -780,44 +782,68 @@ struct moved_entry {\n \tstruct moved_entry *next_line;\n };\n \n-/**\n- * The struct ws_delta holds white space differences between moved lines, i.e.\n- * between '+' and '-' lines that have been detected to be a move.\n- * The string contains the difference in leading white spaces, before the\n- * rest of the line is compared using the white space config for move\n- * coloring. The current_longer indicates if the first string in the\n- * comparision is longer than the second.\n- */\n-struct ws_delta {\n-\tchar *string;\n-\tunsigned int current_longer : 1;\n-};\n-#define WS_DELTA_INIT { NULL, 0 }\n-\n struct moved_block {\n \tstruct moved_entry *match;\n-\tstruct ws_delta wsd;\n+\tint wsd; /* The whitespace delta of this block */\n };\n \n static void moved_block_clear(struct moved_block *b)\n {\n-\tFREE_AND_NULL(b->wsd.string);\n-\tb->match = NULL;\n+\tmemset(b, 0, sizeof(*b));\n+}\n+\n+static void fill_es_indent_data(struct emitted_diff_symbol *es)\n+{\n+\tunsigned int off = 0;\n+\tint width = 0, tab_width = es->flags & WS_TAB_WIDTH_MASK;\n+\tconst char *s = es->line;\n+\tconst int len = es->len;\n+\n+\t/* skip any \\v \\f \\r at start of indentation */\n+\twhile (s[off] == '\\f' || s[off] == '\\v' ||\n+\t (s[off] == '\\r' && off < len - 1))\n+\t\toff++;\n+\n+\t/* calculate the visual width of indentation */\n+\twhile(1) {\n+\t\tif (s[off] == ' ') {\n+\t\t\twidth++;\n+\t\t\toff++;\n+\t\t} else if (s[off] == '\\t') {\n+\t\t\twidth += tab_width - (width % tab_width);\n+\t\t\twhile (s[++off] == '\\t')\n+\t\t\t\twidth += tab_width;\n+\t\t} else {\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tes->indent_off = off;\n+\tes->indent_width = width;\n }\n \n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n-\t\t\t const struct emitted_diff_symbol *b,\n-\t\t\t struct ws_delta *out)\n+\t\t\t const struct emitted_diff_symbol *b,\n+\t\t\t int *out)\n {\n-\tconst struct emitted_diff_symbol *longer = a->len > b->len ? a : b;\n-\tconst struct emitted_diff_symbol *shorter = a->len > b->len ? b : a;\n-\tint d = longer->len - shorter->len;\n+\tint a_len = a->len,\n+\t b_len = b->len,\n+\t a_off = a->indent_off,\n+\t a_width = a->indent_width,\n+\t b_off = b->indent_off,\n+\t b_width = b->indent_width;\n+\tint delta;\n \n-\tif (strncmp(longer->line + d, shorter->line, shorter->len))\n+\tif (a->s == DIFF_SYMBOL_PLUS)\n+\t\tdelta = a_width - b_width;\n+\telse\n+\t\tdelta = b_width - a_width;\n+\n+\tif (a_len - a_off != b_len - b_off ||\n+\t memcmp(a->line + a_off, b->line + b_off, a_len - a_off))\n \t\treturn 0;\n \n-\tout->string = xmemdupz(longer->line, d);\n-\tout->current_longer = (a == longer);\n+\t*out = delta;\n \n \treturn 1;\n }\n@@ -833,8 +859,11 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tconst char *a = cur->es->line,\n \t\t *b = match->es->line,\n \t\t *c = l->line;\n-\tconst char *orig_a = a;\n-\tint wslen;\n+\tint a_off = cur->es->indent_off,\n+\t a_width = cur->es->indent_width,\n+\t c_off = l->indent_off,\n+\t c_width = l->indent_width;\n+\tint delta;\n \n \t/*\n \t * We need to check if 'cur' is equal to 'match'. As those\n@@ -848,35 +877,20 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (al != bl)\n \t\treturn 1;\n \n-\tif (!pmb->wsd.string)\n-\t\t/*\n-\t\t * The white space delta is not active? This can happen\n-\t\t * when we exit early in this function.\n-\t\t */\n-\t\treturn 1;\n-\n \t/*\n-\t * The indent changes of the block are known and stored in\n-\t * pmb->wsd; however we need to check if the indent changes of the\n-\t * current line are still the same as before.\n-\t *\n-\t * To do so we need to compare 'l' to 'cur', adjusting the\n-\t * one of them for the white spaces, depending which was longer.\n+\t * The indent changes of the block are known and stored in pmb->wsd;\n+\t * however we need to check if the indent changes of the current line\n+\t * match those of the current block and that the text of 'l' and 'cur'\n+\t * after the indentation match.\n \t */\n+\tif (cur->es->s == DIFF_SYMBOL_PLUS)\n+\t\tdelta = a_width - c_width;\n+\telse\n+\t\tdelta = c_width - a_width;\n \n-\twslen = strlen(pmb->wsd.string);\n-\tif (pmb->wsd.current_longer) {\n-\t\tc += wslen;\n-\t\tcl -= wslen;\n-\t} else {\n-\t\ta += wslen;\n-\t\tal -= wslen;\n-\t}\n-\n-\tif (al != cl || memcmp(orig_a, b, bl) || memcmp(a, c, al))\n-\t\treturn 1;\n-\n-\treturn 0;\n+\treturn !(delta == pmb->wsd && al - a_off == cl - c_off &&\n+\t\t !memcmp(a, b, al) && !\n+\t\t memcmp(a + a_off, c + c_off, al - a_off));\n }\n \n static int moved_entry_cmp(const void *hashmap_cmp_fn_data,\n@@ -942,6 +956,9 @@ static void add_lines_to_move_detection(struct diff_options *o,\n \t\t\tcontinue;\n \t\t}\n \n+\t\tif (o->color_moved_ws_handling &\n+\t\t COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE)\n+\t\t\tfill_es_indent_data(&o->emitted_symbols->buf[n]);\n \t\tkey = prepare_entry(o, n);\n \t\tif (prev_line && prev_line->es->s == o->emitted_symbols->buf[n].s)\n \t\t\tprev_line->next_line = key;\n@@ -1020,8 +1037,7 @@ static int shrink_potential_moved_blocks(struct moved_block *pmb,\n \n \t\tif (lp < pmb_nr && rp > -1 && lp < rp) {\n \t\t\tpmb[lp] = pmb[rp];\n-\t\t\tpmb[rp].match = NULL;\n-\t\t\tpmb[rp].wsd.string = NULL;\n+\t\t\tmemset(&pmb[rp], 0, sizeof(pmb[rp]));\n \t\t\trp--;\n \t\t\tlp++;\n \t\t}\n@@ -1141,7 +1157,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \t\t\t\t\t\t\t &pmb[pmb_nr].wsd))\n \t\t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t} else {\n-\t\t\t\t\tpmb[pmb_nr].wsd.string = NULL;\n+\t\t\t\t\tpmb[pmb_nr].wsd = 0;\n \t\t\t\t\tpmb[pmb_nr++].match = match;\n \t\t\t\t}\n \t\t\t}\n@@ -1507,7 +1523,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,\n \t\t\t const char *line, int len, unsigned flags)\n {\n-\tstruct emitted_diff_symbol e = {line, len, flags, s};\n+\tstruct emitted_diff_symbol e = {line, len, flags, 0, 0, s};\n \n \tif (o->emitted_symbols)\n \t\tappend_emitted_diff_symbol(o, &e);\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex fe8a2ab06e..e023839ba6 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1901,4 +1901,60 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n \ttest_i18ngrep allow-indentation-change err\n '\n \n+test_expect_success 'compare mixed whitespace delta across moved blocks' '\n+\n+\tgit reset --hard &&\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\t____Indented text to\n+\t_Q____be further indented by four spaces across\n+\t____Qseveral lines\n+\tQQ____These two lines have had their\n+\t____indentation reduced by four spaces\n+\tQdifferent indentation change\n+\t____too short\n+\tEOF\n+\n+\tgit add text.txt &&\n+\tgit commit -m \"add text.txt\" &&\n+\n+\ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\tQIndented text to\n+\tQQbe further indented by four spaces across\n+\tQ____several lines\n+\tQ_QThese two lines have had their\n+\tindentation reduced by four spaces\n+\tQQdifferent indentation change\n+\t__Qtoo short\n+\tEOF\n+\n+\tgit -c color.diff.whitespace=\"normal red\" \\\n+\t\t-c core.whitespace=space-before-tab \\\n+\t\tdiff --color --color-moved --ws-error-highlight=all \\\n+\t\t--color-moved-ws=allow-indentation-change >actual.raw &&\n+\tgrep -v \"index\" actual.raw | test_decode_color >actual &&\n+\n+\tcat <<-\\EOF >expected &&\n+\tdiff --git a/text.txt b/text.txt\n+\t--- a/text.txt\n+\t+++ b/text.txt\n+\t@@ -1,7 +1,7 @@\n+\t- Indented text to\n+\t- \t be further indented by four spaces across\n+\t- \tseveral lines\n+\t-\t\t These two lines have had their\n+\t- indentation reduced by four spaces\n+\t-\tdifferent indentation change\n+\t- too short\n+\t+\tIndented text to\n+\t+\t\tbe further indented by four spaces across\n+\t+\t several lines\n+\t+\t \tThese two lines have had their\n+\t+indentation reduced by four spaces\n+\t+\t\tdifferent indentation change\n+\t+ \ttoo short\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363968","messageId":"20181123111658.30342-10-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 9/9] diff --color-moved-ws: handle blank lines","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:58Z","receivedAt":"2018-11-23T11:17:22Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen using --color-moved-ws=allow-indentation-change allow lines with\nthe same indentation change to be grouped across blank lines. For now\nthis only works if the blank lines have been moved as well, not for\nblocks that have just had their indentation changed.\n\nThis completes the changes to the implementation of\n--color-moved=allow-indentation-change. Running\n\n git diff --color-moved=allow-indentation-change v2.18.0 v2.19.0\n\nnow takes 5.0s. This is a saving of 41% from 8.5s for the optimized\nversion of the previous implementation and 66% from the original which\ntook 14.6s.\n\nSigned-off-by: Phillip Wood \n---\n diff.c | 34 ++++++++++++++++++++++++++++---\n t/t4015-diff-whitespace.sh | 41 ++++++++++++++++++++++++++++++++++----\n 2 files changed, 68 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 148503e49c..ef5b8c78d7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -792,9 +792,11 @@ static void moved_block_clear(struct moved_block *b)\n \tmemset(b, 0, sizeof(*b));\n }\n \n+#define INDENT_BLANKLINE INT_MIN\n+\n static void fill_es_indent_data(struct emitted_diff_symbol *es)\n {\n-\tunsigned int off = 0;\n+\tunsigned int off = 0, i;\n \tint width = 0, tab_width = es->flags & WS_TAB_WIDTH_MASK;\n \tconst char *s = es->line;\n \tconst int len = es->len;\n@@ -818,8 +820,18 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)\n \t\t}\n \t}\n \n-\tes->indent_off = off;\n-\tes->indent_width = width;\n+\t/* check if this line is blank */\n+\tfor (i = off; i < len; i++)\n+\t\tif (!isspace(s[i]))\n+\t\t break;\n+\n+\tif (i == len) {\n+\t\tes->indent_width = INDENT_BLANKLINE;\n+\t\tes->indent_off = len;\n+\t} else {\n+\t\tes->indent_off = off;\n+\t\tes->indent_width = width;\n+\t}\n }\n \n static int compute_ws_delta(const struct emitted_diff_symbol *a,\n@@ -834,6 +846,11 @@ static int compute_ws_delta(const struct emitted_diff_symbol *a,\n \t b_width = b->indent_width;\n \tint delta;\n \n+\tif (a_width == INDENT_BLANKLINE && b_width == INDENT_BLANKLINE) {\n+\t\t*out = INDENT_BLANKLINE;\n+\t\treturn 1;\n+\t}\n+\n \tif (a->s == DIFF_SYMBOL_PLUS)\n \t\tdelta = a_width - b_width;\n \telse\n@@ -877,6 +894,10 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \tif (al != bl)\n \t\treturn 1;\n \n+\t/* If 'l' and 'cur' are both blank then they match. */\n+\tif (a_width == INDENT_BLANKLINE && c_width == INDENT_BLANKLINE)\n+\t\treturn 0;\n+\n \t/*\n \t * The indent changes of the block are known and stored in pmb->wsd;\n \t * however we need to check if the indent changes of the current line\n@@ -888,6 +909,13 @@ static int cmp_in_block_with_wsd(const struct diff_options *o,\n \telse\n \t\tdelta = c_width - a_width;\n \n+\t/*\n+\t * If the previous lines of this block were all blank then set its\n+\t * whitespace delta.\n+\t */\n+\tif (pmb->wsd == INDENT_BLANKLINE)\n+\t\tpmb->wsd = delta;\n+\n \treturn !(delta == pmb->wsd && al - a_off == cl - c_off &&\n \t\t !memcmp(a, b, al) && !\n \t\t memcmp(a + a_off, c + c_off, al - a_off));\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex e023839ba6..9d6f88b07f 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -1901,10 +1901,20 @@ test_expect_success 'compare whitespace delta incompatible with other space opti\n \ttest_i18ngrep allow-indentation-change err\n '\n \n+EMPTY=''\n test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \n \tgit reset --hard &&\n \ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\t${EMPTY}\n+\t____too short without\n+\t${EMPTY}\n+\t___being grouped across blank line\n+\t${EMPTY}\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n \t____Indented text to\n \t_Q____be further indented by four spaces across\n \t____Qseveral lines\n@@ -1918,9 +1928,18 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \tgit commit -m \"add text.txt\" &&\n \n \ttr Q_ \"\\t \" <<-EOF >text.txt &&\n+\tcontext\n+\tlines\n+\tto\n+\tanchor\n \tQIndented text to\n \tQQbe further indented by four spaces across\n \tQ____several lines\n+\t${EMPTY}\n+\tQQtoo short without\n+\t${EMPTY}\n+\tQ_______being grouped across blank line\n+\t${EMPTY}\n \tQ_QThese two lines have had their\n \tindentation reduced by four spaces\n \tQQdifferent indentation change\n@@ -1937,7 +1956,16 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \tdiff --git a/text.txt b/text.txt\n \t--- a/text.txt\n \t+++ b/text.txt\n-\t@@ -1,7 +1,7 @@\n+\t@@ -1,16 +1,16 @@\n+\t-\n+\t- too short without\n+\t-\n+\t- being grouped across blank line\n+\t-\n+\t context\n+\t lines\n+\t to\n+\t anchor\n \t- Indented text to\n \t- \t be further indented by four spaces across\n \t- \tseveral lines\n@@ -1948,9 +1976,14 @@ test_expect_success 'compare mixed whitespace delta across moved blocks' '\n \t+\tIndented text to\n \t+\t\tbe further indented by four spaces across\n \t+\t several lines\n-\t+\t \tThese two lines have had their\n-\t+indentation reduced by four spaces\n-\t+\t\tdifferent indentation change\n+\t+\n+\t+\t\ttoo short without\n+\t+\n+\t+\t being grouped across blank line\n+\t+\n+\t+\t \tThese two lines have had their\n+\t+indentation reduced by four spaces\n+\t+\t\tdifferent indentation change\n \t+ \ttoo short\n \tEOF\n \n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363969","messageId":"20181123111658.30342-4-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 3/9] diff: allow --no-color-moved-ws","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:52Z","receivedAt":"2018-11-23T11:17:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nAllow --no-color-moved-ws and --color-moved-ws=no to cancel any previous\n--color-moved-ws option.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 7 +++++++\n diff.c | 6 +++++-\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 57a2f4cb7a..e1744fa80d 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -306,6 +306,8 @@ endif::git-diff[]\n \tThese modes can be given as a comma separated list:\n +\n --\n+no::\n+\tDo not ignore whitespace when performing move detection.\n ignore-space-at-eol::\n \tIgnore changes in whitespace at EOL.\n ignore-space-change::\n@@ -322,6 +324,11 @@ allow-indentation-change::\n \tother modes.\n --\n \n+--no-color-moved-ws::\n+\tDo not ignore whitespace when performing move detection. This can be\n+\tused to override configuration settings. It is the same as\n+\t`--color-moved-ws=no`.\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 78cd3958f4..9b9811988b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -304,7 +304,9 @@ static int parse_color_moved_ws(const char *arg)\n \t\tstrbuf_addstr(&sb, i->string);\n \t\tstrbuf_trim(&sb);\n \n-\t\tif (!strcmp(sb.buf, \"ignore-space-change\"))\n+\t\tif (!strcmp(sb.buf, \"no\"))\n+\t\t\tret = 0;\n+\t\telse if (!strcmp(sb.buf, \"ignore-space-change\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE_CHANGE;\n \t\telse if (!strcmp(sb.buf, \"ignore-space-at-eol\"))\n \t\t\tret |= XDF_IGNORE_WHITESPACE_AT_EOL;\n@@ -5008,6 +5010,8 @@ int diff_opt_parse(struct diff_options *options,\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n \t\toptions->color_moved = cm;\n+\t} else if (!strcmp(arg, \"--no-color-moved-ws\")) {\n+\t\toptions->color_moved_ws_handling = 0;\n \t} else if (skip_prefix(arg, \"--color-moved-ws=\", &arg)) {\n \t\toptions->color_moved_ws_handling = parse_color_moved_ws(arg);\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"363970","messageId":"20181123111658.30342-3-phillip.wood@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"[PATCH v2 2/9] Use \"whitespace\" consistently","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-23T11:16:51Z","receivedAt":"2018-11-23T11:17:27Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nMost of the messages and documentation use 'whitespace' rather than\n'white space' or 'white spaces' convert to latter two to the former for\nconsistency.\n\nSigned-off-by: Phillip Wood \n---\n Documentation/diff-options.txt | 4 ++--\n Documentation/git-cat-file.txt | 8 ++++----\n diff.c | 2 +-\n 3 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 151690f814..57a2f4cb7a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -298,7 +298,7 @@ dimmed-zebra::\n \tsettings. It is the same as `--color-moved=no`.\n \n --color-moved-ws=::\n-\tThis configures how white spaces are ignored when performing the\n+\tThis configures how whitespace is ignored when performing the\n \tmove detection for `--color-moved`.\n ifdef::git-diff[]\n \tIt can be set by the `diff.colorMovedWS` configuration setting.\n@@ -316,7 +316,7 @@ ignore-all-space::\n \tIgnore whitespace when comparing lines. This ignores differences\n \teven if one line has whitespace where the other line has none.\n allow-indentation-change::\n-\tInitially ignore any white spaces in the move detection, then\n+\tInitially ignore any whitespace in the move detection, then\n \tgroup the moved code blocks only into a block if the change in\n \twhitespace is the same per line. This is incompatible with the\n \tother modes.\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 74013335a1..9a2e9cdafb 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -23,8 +23,8 @@ In the second form, a list of objects (separated by linefeeds) is provided on\n stdin, and the SHA-1, type, and size of each object is printed on stdout. The\n output format can be overridden using the optional `` argument. If\n either `--textconv` or `--filters` was specified, the input is expected to\n-list the object names followed by the path name, separated by a single white\n-space, so that the appropriate drivers can be determined.\n+list the object names followed by the path name, separated by a single\n+whitespace, so that the appropriate drivers can be determined.\n \n OPTIONS\n -------\n@@ -79,15 +79,15 @@ OPTIONS\n \tPrint object information and contents for each object provided\n \ton stdin. May not be combined with any other options or arguments\n \texcept `--textconv` or `--filters`, in which case the input lines\n-\talso need to specify the path, separated by white space. See the\n+\talso need to specify the path, separated by whitespace. See the\n \tsection `BATCH OUTPUT` below for details.\n \n --batch-check::\n --batch-check=::\n \tPrint object information for each object provided on stdin. May\n \tnot be combined with any other options or arguments except\n \t`--textconv` or `--filters`, in which case the input lines also\n-\tneed to specify the path, separated by white space. See the\n+\tneed to specify the path, separated by whitespace. See the\n \tsection `BATCH OUTPUT` below for details.\n \n --batch-all-objects::\ndiff --git a/diff.c b/diff.c\nindex c29b1cce14..78cd3958f4 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -320,7 +320,7 @@ static int parse_color_moved_ws(const char *arg)\n \n \tif ((ret & COLOR_MOVED_WS_ALLOW_INDENTATION_CHANGE) &&\n \t (ret & XDF_WHITESPACE_FLAGS))\n-\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other white space modes\"));\n+\t\tdie(_(\"color-moved-ws: allow-indentation-change cannot be combined with other whitespace modes\"));\n \n \tstring_list_clear(&l, 0);\n \n-- \n2.19.1.1690.g258b440b18\n\n"},{"id":"364099","messageId":"CAGZ79kZXW3YoptBzG_Bhjpnh6-7AYTWwT5tcrow2SDwNoF65ZA@mail.gmail.com","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-26T21:20:44Z","receivedAt":"2018-11-26T21:20:59Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 23, 2018 at 3:17 AM Phillip Wood wrote:\n>\n> From: Phillip Wood \n>\n> Thanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in\n> response to those comments - see the range-diff below for details (the\n> patch numbers are off by one in the range diff, I think because the\n> first patch is unchanged and so it was used as the merge base by\n> --range-diff=.\n\n`git range-diff` accepts a three dotted \"range\" OLD...NEW\nas an easy abbreviation for the arguments\n\"COMMON..OLD COMMON..NEW\" and the common element is\ncomputed as the last common element. It doesn't have knowledge\nabout where you started your topic branch.\n\n\n> For some reason the range-diff also includes\n> the notes even though I did not give --notes to format-patch)\n\nThis is interesting.\nThe existence of notes.rewrite. seems to work well\nwith the range-diff then, as the config would trigger the copy-over\nof notes and then range-diff would diff the original notes to the new\nnotes.\n\n>\n> When trying out the new --color-moved-ws=allow-indentation-change I\n> was disappointed to discover it did not work if the indentation\n> contains a mix of spaces and tabs. This series reworks it so that it\n> does.\n>\n\nThe range-diff looks good to me.\n\nThanks,\nStefan\n"},{"id":"364164","messageId":"c69d55b6-a4c6-86b6-bea0-0b11c3c5b8e8@talktalk.net","threadId":"49410","inReplyTo":"CAGZ79kZXW3YoptBzG_Bhjpnh6-7AYTWwT5tcrow2SDwNoF65ZA@mail.gmail.com","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2018-11-27T20:52:27Z","receivedAt":"2018-11-27T20:52:33Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Stefan\n\nOn 26/11/2018 21:20, Stefan Beller wrote:\n> On Fri, Nov 23, 2018 at 3:17 AM Phillip Wood wrote:\n>>\n>> From: Phillip Wood \n>>\n>> Thanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in\n>> response to those comments - see the range-diff below for details (the\n>> patch numbers are off by one in the range diff, I think because the\n>> first patch is unchanged and so it was used as the merge base by\n>> --range-diff=.\n> \n> `git range-diff` accepts a three dotted \"range\" OLD...NEW\n> as an easy abbreviation for the arguments\n> \"COMMON..OLD COMMON..NEW\" and the common element is\n> computed as the last common element. It doesn't have knowledge\n> about where you started your topic branch.\n\nI was using the new --range-diff option to format-patch, I think I \nshould have given --range-diff=@{u}...\n\n>> For some reason the range-diff also includes\n>> the notes even though I did not give --notes to format-patch)\n> \n> This is interesting.\n> The existence of notes.rewrite. seems to work well\n> with the range-diff then, as the config would trigger the copy-over\n> of notes and then range-diff would diff the original notes to the new\n> notes.\n\nYes, but I think with format-patch it should only diff the notes when \n--notes is given.\n\n>> When trying out the new --color-moved-ws=allow-indentation-change I\n>> was disappointed to discover it did not work if the indentation\n>> contains a mix of spaces and tabs. This series reworks it so that it\n>> does.\n>>\n> \n> The range-diff looks good to me.\n\nThat's good, thanks for your comments on the previous iterations.\n\nBest Wishes\n\nPhillip\n> Thanks,\n> Stefan\n> \n\n"},{"id":"366334","messageId":"402b9c01-cd7c-79f3-9fde-55907f03c406@talktalk.net","threadId":"49410","inReplyTo":"20181123111658.30342-1-phillip.wood@talktalk.net","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2019-01-08T16:22:20Z","receivedAt":"2019-01-08T16:22:26Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nI just wanted to check that these patches are on your radar as they \nhaven't made it into pu yet.\n\nBest Wishes for the New Year\n\nPhillip\n\nOn 23/11/2018 11:16, Phillip Wood wrote:\n> From: Phillip Wood \n> \n> Thanks to Stefan for his feedback on v1. I've updated patches 2 & 8 in\n> response to those comments - see the range-diff below for details (the\n> patch numbers are off by one in the range diff, I think because the\n> first patch is unchanged and so it was used as the merge base by\n> --range-diff=. For some reason the range-diff also includes\n> the notes even though I did not give --notes to format-patch)\n> \n> When trying out the new --color-moved-ws=allow-indentation-change I\n> was disappointed to discover it did not work if the indentation\n> contains a mix of spaces and tabs. This series reworks it so that it\n> does.\n> \n> \n> Phillip Wood (9):\n> diff: document --no-color-moved\n> Use \"whitespace\" consistently\n> diff: allow --no-color-moved-ws\n> diff --color-moved-ws: demonstrate false positives\n> diff --color-moved-ws: fix false positives\n> diff --color-moved=zebra: be stricter with color alternation\n> diff --color-moved-ws: optimize allow-indentation-change\n> diff --color-moved-ws: modify allow-indentation-change\n> diff --color-moved-ws: handle blank lines\n> \n> Documentation/diff-options.txt | 15 ++-\n> Documentation/git-cat-file.txt | 8 +-\n> diff.c | 219 +++++++++++++++++++++------------\n> t/t4015-diff-whitespace.sh | 99 ++++++++++++++-\n> 4 files changed, 255 insertions(+), 86 deletions(-)\n> \n> Range-diff against v1:\n> 1: ae58ae4f29 ! 1: 4939ee371d diff: use whitespace consistently\n> @@ -1,9 +1,10 @@\n> Author: Phillip Wood \n> \n> - diff: use whitespace consistently\n> + Use \"whitespace\" consistently\n> \n> - Most of the documentation uses 'whitespace' rather than 'white space'\n> - or 'white spaces' convert to latter two to the former for consistency.\n> + Most of the messages and documentation use 'whitespace' rather than\n> + 'white space' or 'white spaces' convert to latter two to the former for\n> + consistency.\n> \n> Signed-off-by: Phillip Wood \n> \n> @@ -29,6 +30,39 @@\n> \twhitespace is the same per line. This is incompatible with the\n> \tother modes.\n> \n> + diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> + --- a/Documentation/git-cat-file.txt\n> + +++ b/Documentation/git-cat-file.txt\n> +@@\n> + stdin, and the SHA-1, type, and size of each object is printed on stdout. The\n> + output format can be overridden using the optional `` argument. If\n> + either `--textconv` or `--filters` was specified, the input is expected to\n> +-list the object names followed by the path name, separated by a single white\n> +-space, so that the appropriate drivers can be determined.\n> ++list the object names followed by the path name, separated by a single\n> ++whitespace, so that the appropriate drivers can be determined.\n> +\n> + OPTIONS\n> + -------\n> +@@\n> + \tPrint object information and contents for each object provided\n> + \ton stdin. May not be combined with any other options or arguments\n> + \texcept `--textconv` or `--filters`, in which case the input lines\n> +-\talso need to specify the path, separated by white space. See the\n> ++\talso need to specify the path, separated by whitespace. See the\n> + \tsection `BATCH OUTPUT` below for details.\n> +\n> + --batch-check::\n> + --batch-check=::\n> + \tPrint object information for each object provided on stdin. May\n> + \tnot be combined with any other options or arguments except\n> + \t`--textconv` or `--filters`, in which case the input lines also\n> +-\tneed to specify the path, separated by white space. See the\n> ++\tneed to specify the path, separated by whitespace. See the\n> + \tsection `BATCH OUTPUT` below for details.\n> +\n> + --batch-all-objects::\n> +\n> diff --git a/diff.c b/diff.c\n> --- a/diff.c\n> +++ b/diff.c\n> 2: 7072bc6211 = 2: 204c7fea9d diff: allow --no-color-moved-ws\n> 3: ce3ad19eea = 3: 542b79b215 diff --color-moved-ws: demonstrate false positives\n> 4: 700e0b61e7 = 4: 4ffb5c4122 diff --color-moved-ws: fix false positives\n> 5: 9ecd8159a7 = 5: a3a84f90c5 diff --color-moved=zebra: be stricter with color alternation\n> 6: 1b1158b1ca = 6: f94f2e0bae diff --color-moved-ws: optimize allow-indentation-change\n> 7: d8a362be6a ! 7: fe8eb9cdbc diff --color-moved-ws: modify allow-indentation-change\n> @@ -17,7 +17,7 @@\n> \n> This commit changes the way the indentation is handled to track the\n> visual size of the indentation rather than the characters in the\n> - indentation. This has they benefit that any whitespace errors do not\n> + indentation. This has the benefit that any whitespace errors do not\n> interfer with the move detection (the whitespace errors will still be\n> highlighted according to --ws-error-highlight). During the discussion\n> of this feature there were concerns about the correct detection of\n> @@ -30,7 +30,7 @@\n> they are uncolored.\n> \n> Signed-off-by: Phillip Wood \n> - Changes since rfc:\n> + changes since rfc:\n> - It now replaces the existing implementation rather than adding a new\n> mode.\n> - The indentation deltas are now calculated once for each line and\n> @@ -49,8 +49,8 @@\n> \tconst char *line;\n> \tint len;\n> \tint flags;\n> -+\tint indent_off;\n> -+\tint indent_width;\n> ++\tint indent_off; /* Offset to first non-whitespace character */\n> ++\tint indent_width; /* The visual width of the indentation */\n> \tenum diff_symbol s;\n> };\n> #define EMITTED_DIFF_SYMBOL_INIT {NULL}\n> 8: 1f7e99d45c = 8: e600f8247c diff --color-moved-ws: handle blank lines\n> \n> \n\n"},{"id":"366354","messageId":"xmqqh8ej57d1.fsf@gitster-ct.c.googlers.com","threadId":"49410","inReplyTo":"402b9c01-cd7c-79f3-9fde-55907f03c406@talktalk.net","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-08T18:31:22Z","receivedAt":"2019-01-08T18:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I just wanted to check that these patches are on your radar as they\n> haven't made it into pu yet.\n\nSorry, but they were not on my radar. I was waiting for comments to\ncome in on them before doing anything, and now it is more than a\nmonth ago X-<.\n"},{"id":"366463","messageId":"CAGZ79kZBfkKc6L5o4rCJoSw63q49YZwn7QRedNmFr=-nd=GbMw@mail.gmail.com","threadId":"49410","inReplyTo":"xmqqh8ej57d1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-10T00:37:27Z","receivedAt":"2019-01-10T00:37:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 8, 2019 at 10:31 AM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > I just wanted to check that these patches are on your radar as they\n> > haven't made it into pu yet.\n>\n> Sorry, but they were not on my radar. I was waiting for comments to\n> come in on them before doing anything, and now it is more than a\n> month ago X-<.\n\nI have reviewed the whole series again, and still have\nno comment on them, i.e. the series as-is in v2 is\n Reviewed-by: Stefan Beller \nif that helps.\n"},{"id":"366516","messageId":"xmqq5zuw2w8c.fsf@gitster-ct.c.googlers.com","threadId":"49410","inReplyTo":"CAGZ79kZBfkKc6L5o4rCJoSw63q49YZwn7QRedNmFr=-nd=GbMw@mail.gmail.com","subject":"Re: [PATCH v2 0/9] diff --color-moved-ws fixes and enhancment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-10T18:39:15Z","receivedAt":"2019-01-10T18:39:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Jan 8, 2019 at 10:31 AM Junio C Hamano wrote:\n>>\n>> Phillip Wood writes:\n>>\n>> > I just wanted to check that these patches are on your radar as they\n>> > haven't made it into pu yet.\n>>\n>> Sorry, but they were not on my radar. I was waiting for comments to\n>> come in on them before doing anything, and now it is more than a\n>> month ago X-<.\n>\n> I have reviewed the whole series again, and still have\n> no comment on them, i.e. the series as-is in v2 is\n> Reviewed-by: Stefan Beller \n> if that helps.\n\nThanks.\n"}]}