{"thread":{"id":"44017","subject":"[PATCHv4] diff.c: emit moved lines with a different color","startedAt":"2016-09-06T07:02:02Z","lastAt":"2016-09-07T18:52:27Z","messageCount":6,"participants":["Stefan Beller","Ramsay Jones","Jakub Narębski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"301165","messageId":"20160906070151.15163-1-stefanbeller@gmail.com","threadId":"44017","inReplyTo":null,"subject":"[PATCHv4] diff.c: emit moved lines with a different color","fromName":"Stefan Beller","fromEmail":"stefanbeller@gmail.com","sentAt":"2016-09-06T07:01:51Z","receivedAt":"2016-09-06T07:02:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When we color the diff, we'll mark moved lines with a different color.\n\nThis is achieved by doing a two passes over the diff. The first pass\nwill inspect each line of the diff and store the removed lines and the\nadded lines in its own hash map.\nThe second pass will check for each added line if that is found in the\nset of removed lines. If so it will color the added line differently as\nwith the new `moved-new` color mode. For each removed line we check the\nset of added lines and if found emit that with the new color `moved-old`.\n\nWhen detecting the moved lines, we cannot just rely on a line being equal,\nbut we need to take the context into account to detect when the moves were\nreordered as we do not want to color moved but per-mutated lines.\nTo do that we use the hash of the preceding line.\n\nThis patch was motivated by e.g. reviewing 3b0c4200 (\"apply: move\nlibified code from builtin/apply.c to apply.{c,h}\", 2016-08-08)\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n * moved new data structures into struct diff_options\n * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature\n * color.diff.movedfrom and color.diff.movedto to control the colors\n * added a test\n \n Documentation/config.txt               |  12 +-\n Documentation/diff-options.txt         |   7 ++\n contrib/completion/git-completion.bash |   2 +\n diff.c                                 | 211 +++++++++++++++++++++++++++++++--\n diff.h                                 |  16 ++-\n t/t4015-diff-whitespace.sh             |  44 +++++++\n 6 files changed, 282 insertions(+), 10 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 0bcb679..5daf77a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the\n 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the\n command line with the `--color[=<when>]` option.\n \n+color.moved::\n+\tA boolean value, whether a diff should color moved lines\n+\tdifferently. The moved lines are searched for in the diff only.\n+\tDuplicated lines from somewhere in the project that are not\n+\tpart of the diff are not colored as moved.\n+\tDefaults to true.\n+\n color.diff.<slot>::\n \tUse customized color for diff colorization.  `<slot>` specifies\n \twhich part of the patch to use the specified color, and is one\n \tof `context` (context text - `plain` is a historical synonym),\n \t`meta` (metainformation), `frag`\n \t(hunk header), 'func' (function in hunk header), `old` (removed lines),\n-\t`new` (added lines), `commit` (commit headers), or `whitespace`\n-\t(highlighting whitespace errors).\n+\t`new` (added lines), `commit` (commit headers), `whitespace`\n+\t(highlighting whitespace errors), `movedfrom` (removed lines that\n+\treappear), `movedto` (added lines that were removed elsewhere).\n \n color.decorate.<slot>::\n \tUse customized color for 'git log --decorate' output.  `<slot>` is one\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 705a873..13b6a2a 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -234,6 +234,13 @@ ifdef::git-diff[]\n endif::git-diff[]\n \tIt is the same as `--color=never`.\n \n+--[no-]color-moved::\n+\tShow moved blocks in a different color.\n+ifdef::git-diff[]\n+\tIt can be changed by the `diff.ui` and `color.diff`\n+\tconfiguration settings.\n+endif::git-diff[]\n+\n --word-diff[=<mode>]::\n \tShow a word diff, using the <mode> to delimit changed words.\n \tBy default, words are delimited by whitespace; see\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9c8f738..9827c2e 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2115,6 +2115,8 @@ _git_config ()\n \t\tcolor.diff.old\n \t\tcolor.diff.plain\n \t\tcolor.diff.whitespace\n+\t\tcolor.diff.movedfrom\n+\t\tcolor.diff.movedto\n \t\tcolor.grep\n \t\tcolor.grep.context\n \t\tcolor.grep.filename\ndiff --git a/diff.c b/diff.c\nindex 534c12e..47685f3 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -18,6 +18,7 @@\n #include \"ll-merge.h\"\n #include \"string-list.h\"\n #include \"argv-array.h\"\n+#include \"git-compat-util.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -30,6 +31,7 @@ static int diff_compaction_heuristic; /* experimental */\n static int diff_rename_limit_default = 400;\n static int diff_suppress_blank_empty;\n static int diff_use_color_default = -1;\n+static int diff_color_moved_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n@@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_YELLOW,\t/* COMMIT */\n \tGIT_COLOR_BG_RED,\t/* WHITESPACE */\n \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n+\tGIT_COLOR_BLUE,\t\t/* MOVED FROM */\n+\tGIT_COLOR_MAGENTA,\t/* MOVED TO */\n };\n \n static int parse_diff_color_slot(const char *var)\n@@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)\n \t\treturn DIFF_WHITESPACE;\n \tif (!strcasecmp(var, \"func\"))\n \t\treturn DIFF_FUNCINFO;\n+\tif (!strcasecmp(var, \"movedfrom\"))\n+\t\treturn DIFF_FILE_MOVED_FROM;\n+\tif (!strcasecmp(var, \"movedto\"))\n+\t\treturn DIFF_FILE_MOVED_TO;\n \treturn -1;\n }\n \n@@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_use_color_default = git_config_colorbool(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"color.moved\")) {\n+\t\tdiff_color_moved_default = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n \tif (!strcmp(var, \"diff.context\")) {\n \t\tdiff_context_default = git_config_int(var, value);\n \t\tif (diff_context_default < 0)\n@@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \treturn git_default_config(var, value, cb);\n }\n \n+static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,\n+\t\t\t\t     const struct diff_line_moved_entry *b,\n+\t\t\t\t     const void *unused)\n+{\n+\treturn strcmp(a->line, b->line) &&\n+\t       a->hash_prev_line == b->hash_prev_line;\n+}\n+\n+static struct diff_line_moved_entry *\n+prepare_diff_line_moved_entry(const char *line,\n+\t\t\t      unsigned long len,\n+\t\t\t      int hash_prev_line)\n+{\n+\tstruct diff_line_moved_entry *ret = xmalloc(sizeof(*ret));\n+\tret->ent.hash = memhash(line, len) ^ hash_prev_line;\n+\tret->line = xmemdupz(line, len);\n+\tret->hash_prev_line = hash_prev_line;\n+\treturn ret;\n+}\n+\n static char *quote_two(const char *one, const char *two)\n {\n \tint need_one = quote_c_style(one, NULL, NULL, 1);\n@@ -537,16 +569,42 @@ static void emit_add_line(const char *reset,\n \t\t\t  struct emit_callback *ecbdata,\n \t\t\t  const char *line, int len)\n {\n+\tenum color_diff color = DIFF_FILE_NEW;\n+\tunsigned ws_error_highlight = WSEH_NEW;\n+\n+\tif (ecbdata->opt->color_moved) {\n+\t\tstruct diff_options *o = ecbdata->opt;\n+\t\tint hash = memhash(line, len);\n+\t\tstruct diff_line_moved_entry *keydata =\n+\t\t\tprepare_diff_line_moved_entry(line, len,\n+\t\t\t\t\t\t      o->hash_prev_added);\n+\t\tif (hashmap_get(o->moved_del, keydata, keydata))\n+\t\t\tcolor = DIFF_FILE_MOVED_TO;\n+\t\to->hash_prev_added = hash;\n+\t}\n \temit_line_checked(reset, ecbdata, line, len,\n-\t\t\t  DIFF_FILE_NEW, WSEH_NEW, '+');\n+\t\t\t  color, ws_error_highlight, '+');\n }\n \n static void emit_del_line(const char *reset,\n \t\t\t  struct emit_callback *ecbdata,\n \t\t\t  const char *line, int len)\n {\n+\tenum color_diff color = DIFF_FILE_OLD;\n+\tunsigned ws_error_highlight = WSEH_OLD;\n+\n+\tif (ecbdata->opt->color_moved) {\n+\t\tstruct diff_options *o = ecbdata->opt;\n+\t\tint hash = memhash(line, len);\n+\t\tstruct diff_line_moved_entry *keydata =\n+\t\t\tprepare_diff_line_moved_entry(line, len,\n+\t\t\t\t\t\t      o->hash_prev_removed);\n+\t\tif (hashmap_get(ecbdata->opt->moved_add, keydata, keydata))\n+\t\t\tcolor = DIFF_FILE_MOVED_FROM;\n+\t\to->hash_prev_removed = hash;\n+\t}\n \temit_line_checked(reset, ecbdata, line, len,\n-\t\t\t  DIFF_FILE_OLD, WSEH_OLD, '-');\n+\t\t\t  color, ws_error_highlight, '-');\n }\n \n static void emit_context_line(const char *reset,\n@@ -555,6 +613,11 @@ static void emit_context_line(const char *reset,\n {\n \temit_line_checked(reset, ecbdata, line, len,\n \t\t\t  DIFF_CONTEXT, WSEH_CONTEXT, ' ');\n+\tif (ecbdata->opt->color_moved) {\n+\t\tint h = memhash(line, len);\n+\t\tecbdata->opt->hash_prev_removed = h;\n+\t\tecbdata->opt->hash_prev_added = h;\n+\t}\n }\n \n static void emit_hunk_header(struct emit_callback *ecbdata,\n@@ -1323,6 +1386,47 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t}\n }\n \n+static void fn_prepare_consume(void *priv, char *line, unsigned long len)\n+{\n+\tstruct emit_callback *ecbdata = priv;\n+\tstruct diff_options *o = ecbdata->opt;\n+\tstruct diff_line_moved_entry *d;\n+\tint hash = memhash(line + 1, len - 1);\n+\n+\tswitch (line[0]) {\n+\tcase ' ':\n+\t\thashmap_add(o->moved_del,\n+\t\t\tprepare_diff_line_moved_entry(line + 1, len - 1, 0));\n+\t\thashmap_add(o->moved_add,\n+\t\t\tprepare_diff_line_moved_entry(line + 1, len - 1, 0));\n+\t\to->hash_prev_added = hash;\n+\t\to->hash_prev_removed = hash;\n+\t\tbreak;\n+\tcase '+':\n+\t\tif (o->hash_prev_added) {\n+\t\t\td = prepare_diff_line_moved_entry(line + 1, len - 1,\n+\t\t\t\t\t\t\t  o->hash_prev_added);\n+\t\t\thashmap_add(o->moved_add, d);\n+\t\t}\n+\t\to->hash_prev_added = hash;\n+\t\to->hash_prev_removed = 0;\n+\t\tbreak;\n+\tcase '-':\n+\t\tif (o->hash_prev_removed) {\n+\t\t\td = prepare_diff_line_moved_entry(line + 1, len - 1,\n+\t\t\t\t\t\t\t  o->hash_prev_removed);\n+\t\t\thashmap_add(o->moved_del, d);\n+\t\t}\n+\t\to->hash_prev_added = 0;\n+\t\to->hash_prev_removed = hash;\n+\t\tbreak;\n+\tdefault:\n+\t\to->hash_prev_added = 0;\n+\t\to->hash_prev_removed = 0;\n+\t\tbreak;\n+\t}\n+}\n+\n static char *pprint_rename(const char *a, const char *b)\n {\n \tconst char *old = a;\n@@ -2279,6 +2383,57 @@ struct userdiff_driver *get_textconv(struct diff_filespec *one)\n \treturn userdiff_get_textconv(one->driver);\n }\n \n+static void prepare_moved_lines(struct diff_filepair *p, struct diff_options *o)\n+{\n+\tmmfile_t mf1, mf2;\n+\txpparam_t xpp;\n+\txdemitconf_t xecfg;\n+\tstruct emit_callback ecbdata;\n+\tstruct diff_filespec *one = p->one;\n+\tstruct diff_filespec *two = p->two;\n+\tstruct userdiff_driver *textconv_one = NULL;\n+\tstruct userdiff_driver *textconv_two = NULL;\n+\n+\tif (S_ISGITLINK(one->mode) ||\n+\t    S_ISGITLINK(two->mode))\n+\t\treturn;\n+\n+\tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n+\t\ttextconv_one = get_textconv(one);\n+\t\ttextconv_two = get_textconv(two);\n+\t}\n+\n+\tif (!DIFF_OPT_TST(o, TEXT) &&\n+\t    ( (!textconv_one && diff_filespec_is_binary(one)) ||\n+\t      (!textconv_two && diff_filespec_is_binary(two)) ))\n+\t      return;\n+\n+\tmf1.size = fill_textconv(textconv_one, one, &mf1.ptr);\n+\tmf2.size = fill_textconv(textconv_two, two, &mf2.ptr);\n+\n+\tmemset(&xpp, 0, sizeof(xpp));\n+\tmemset(&xecfg, 0, sizeof(xecfg));\n+\tmemset(&ecbdata, 0, sizeof(ecbdata));\n+\tecbdata.opt = o;\n+\n+\txpp.flags = o->xdl_opts;\n+\txecfg.ctxlen = 1;\n+\n+\tif (o->word_diff)\n+\t\tinit_diff_words_data(&ecbdata, o, one, two);\n+\tif (xdi_diff_outf(&mf1, &mf2, fn_prepare_consume, &ecbdata,\n+\t\t\t  &xpp, &xecfg))\n+\t\tdie(\"unable to generate generate moved color output for %s\",\n+\t\t    one->path);\n+\tif (o->word_diff)\n+\t\tfree_diff_words_data(&ecbdata);\n+\tif (textconv_one)\n+\t\tfree(mf1.ptr);\n+\tif (textconv_two)\n+\t\tfree(mf2.ptr);\n+\txdiff_clear_find_func(&xecfg);\n+}\n+\n static void builtin_diff(const char *name_a,\n \t\t\t const char *name_b,\n \t\t\t struct diff_filespec *one,\n@@ -3291,6 +3446,7 @@ void diff_setup(struct diff_options *options)\n \toptions->change = diff_change;\n \toptions->add_remove = diff_addremove;\n \toptions->use_color = diff_use_color_default;\n+\toptions->color_moved = diff_color_moved_default;\n \toptions->detect_rename = diff_detect_rename_default;\n \toptions->xdl_opts |= diff_algorithm;\n \tif (diff_compaction_heuristic)\n@@ -3413,6 +3569,9 @@ void diff_setup_done(struct diff_options *options)\n \n \tif (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1)\n \t\tdie(_(\"--follow requires exactly one pathspec\"));\n+\n+\tif (!options->use_color || external_diff())\n+\t\toptions->color_moved = 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n@@ -3863,6 +4022,10 @@ int diff_opt_parse(struct diff_options *options,\n \t}\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\toptions->use_color = 0;\n+\telse if (!strcmp(arg, \"--color-moved\"))\n+\t\toptions->color_moved = 1;\n+\telse if (!strcmp(arg, \"--no-color-moved\"))\n+\t\toptions->color_moved = 0;\n \telse if (!strcmp(arg, \"--color-words\")) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n@@ -4622,6 +4785,43 @@ void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)\n \t\twarning(rename_limit_advice, varname, needed);\n }\n \n+static void diff_flush_format_patch(struct diff_options *o)\n+{\n+\tint i;\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tif (o->color_moved) {\n+\t\to->moved_add = xmalloc(sizeof(*o->moved_add));\n+\t\to->moved_del = xmalloc(sizeof(*o->moved_del));\n+\t\thashmap_init(o->moved_add, (hashmap_cmp_fn)diff_line_moved_entry_cmp, 0);\n+\t\thashmap_init(o->moved_del, (hashmap_cmp_fn)diff_line_moved_entry_cmp, 0);\n+\n+\t\tfor (i = 0; i < q->nr; i++) {\n+\t\t\tstruct diff_filepair *p = q->queue[i];\n+\t\t\tif (check_pair_status(p) && !diff_unmodified_pair(p))\n+\t\t\t\tprepare_moved_lines(p, o);\n+\t\t}\n+\t}\n+\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tif (check_pair_status(p))\n+\t\t\tdiff_flush_patch(p, o);\n+\t}\n+\n+\tif (o->color_moved) {\n+\t\tstruct hashmap_iter iter;\n+\t\tstruct diff_line_moved_entry *e;\n+\t\thashmap_iter_init(o->moved_add, &iter);\n+\t\twhile ((e = hashmap_iter_next(&iter)))\n+\t\t\tfree(e->line);\n+\t\thashmap_iter_init(o->moved_del, &iter);\n+\t\twhile ((e = hashmap_iter_next(&iter)))\n+\t\t\tfree(e->line);\n+\t\thashmap_free(o->moved_add, 1);\n+\t\thashmap_free(o->moved_del, 1);\n+\t}\n+}\n+\n void diff_flush(struct diff_options *options)\n {\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n@@ -4696,6 +4896,7 @@ void diff_flush(struct diff_options *options)\n \t\tif (!options->file)\n \t\t\tdie_errno(\"Could not open /dev/null\");\n \t\toptions->close_file = 1;\n+\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n@@ -4716,11 +4917,7 @@ void diff_flush(struct diff_options *options)\n \t\t\t}\n \t\t}\n \n-\t\tfor (i = 0; i < q->nr; i++) {\n-\t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tif (check_pair_status(p))\n-\t\t\t\tdiff_flush_patch(p, options);\n-\t\t}\n+\t\tdiff_flush_format_patch(options);\n \t}\n \n \tif (output_format & DIFF_FORMAT_CALLBACK)\ndiff --git a/diff.h b/diff.h\nindex 7883729..236baa8 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -110,6 +110,12 @@ enum diff_words_type {\n \tDIFF_WORDS_COLOR\n };\n \n+struct diff_line_moved_entry {\n+\tstruct hashmap_entry ent;\n+\tchar *line;\n+\tint hash_prev_line;\n+};\n+\n struct diff_options {\n \tconst char *orderfile;\n \tconst char *pickaxe;\n@@ -178,6 +184,12 @@ struct diff_options {\n \tvoid *output_prefix_data;\n \n \tint diff_path_counter;\n+\n+\tint color_moved;\n+\tstruct hashmap *moved_add;\n+\tstruct hashmap *moved_del;\n+\tint hash_prev_added;\n+\tint hash_prev_removed;\n };\n \n enum color_diff {\n@@ -189,7 +201,9 @@ enum color_diff {\n \tDIFF_FILE_NEW = 5,\n \tDIFF_COMMIT = 6,\n \tDIFF_WHITESPACE = 7,\n-\tDIFF_FUNCINFO = 8\n+\tDIFF_FUNCINFO = 8,\n+\tDIFF_FILE_MOVED_TO = 9,\n+\tDIFF_FILE_MOVED_FROM = 10\n };\n const char *diff_get_color(int diff_use_color, enum color_diff ix);\n #define diff_get_color_opt(o, ix) \\\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 2434157..9fdc5bd 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -934,4 +934,48 @@ test_expect_success 'the same with --ws-error-highlight' '\n \ttest_cmp expected current\n '\n \n+test_expect_success 'detect moved code' '\n+\tgit reset --hard &&\n+\tcat >test.c <<-\\EOF &&\n+\n+\t#include<stdio.h>\n+\tmain()\n+\t{\n+\tprintf(\"Hello World\");\n+\t}\n+\tEOF\n+\tgit add test.c &&\n+\tgit commit -m \"add main function\" &&\n+\tgit mv test.c main.c &&\n+\tgit diff HEAD --no-renames | test_decode_color >actual &&\n+\tcat >expected <<-\\EOF &&\n+\t<BOLD>diff --git a/main.c b/main.c<RESET>\n+\t<BOLD>new file mode 100644<RESET>\n+\t<BOLD>index 0000000..f068530<RESET>\n+\t<BOLD>--- /dev/null<RESET>\n+\t<BOLD>+++ b/main.c<RESET>\n+\t<CYAN>@@ -0,0 +1,6 @@<RESET>\n+\t<GREEN>+<RESET>\n+\t<BLUE>+<RESET><BLUE>#include<stdio.h><RESET>\n+\t<BLUE>+<RESET><BLUE>main()<RESET>\n+\t<BLUE>+<RESET><BLUE>{<RESET>\n+\t<BLUE>+<RESET><BLUE>printf(\"Hello World\");<RESET>\n+\t<BLUE>+<RESET><BLUE>}<RESET>\n+\t<BOLD>diff --git a/test.c b/test.c<RESET>\n+\t<BOLD>deleted file mode 100644<RESET>\n+\t<BOLD>index f068530..0000000<RESET>\n+\t<BOLD>--- a/test.c<RESET>\n+\t<BOLD>+++ /dev/null<RESET>\n+\t<CYAN>@@ -1,6 +0,0 @@<RESET>\n+\t<RED>-<RESET>\n+\t<MAGENTA>-#include<stdio.h><RESET>\n+\t<MAGENTA>-main()<RESET>\n+\t<MAGENTA>-{<RESET>\n+\t<MAGENTA>-printf(\"Hello World\");<RESET>\n+\t<MAGENTA>-}<RESET>\n+\tEOF\n+\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n2.10.0.1.g233b7f3.dirty\n\n"},{"id":"301188","messageId":"87a1ddbf-2499-d5b4-55c4-aeed2b72acce@ramsayjones.plus.com","threadId":"44017","inReplyTo":"20160906070151.15163-1-stefanbeller@gmail.com","subject":"Re: [PATCHv4] diff.c: emit moved lines with a different color","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-09-06T13:37:41Z","receivedAt":"2016-09-06T13:37:55Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/09/16 08:01, Stefan Beller wrote:\n[snip]\n> This patch was motivated by e.g. reviewing 3b0c4200 (\"apply: move\n> libified code from builtin/apply.c to apply.{c,h}\", 2016-08-08)\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n>  * moved new data structures into struct diff_options\n>  * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature\n>  * color.diff.movedfrom and color.diff.movedto to control the colors\n>  * added a test\n>  \n>  Documentation/config.txt               |  12 +-\n>  Documentation/diff-options.txt         |   7 ++\n>  contrib/completion/git-completion.bash |   2 +\n>  diff.c                                 | 211 +++++++++++++++++++++++++++++++--\n>  diff.h                                 |  16 ++-\n>  t/t4015-diff-whitespace.sh             |  44 +++++++\n>  6 files changed, 282 insertions(+), 10 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0bcb679..5daf77a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the\n>  'git-diff-{asterisk}' plumbing commands.  Can be overridden on the\n>  command line with the `--color[=<when>]` option.\n>  \n> +color.moved::\n> +\tA boolean value, whether a diff should color moved lines\n> +\tdifferently. The moved lines are searched for in the diff only.\n> +\tDuplicated lines from somewhere in the project that are not\n> +\tpart of the diff are not colored as moved.\n> +\tDefaults to true.\n> +\n>  color.diff.<slot>::\n>  \tUse customized color for diff colorization.  `<slot>` specifies\n>  \twhich part of the patch to use the specified color, and is one\n>  \tof `context` (context text - `plain` is a historical synonym),\n>  \t`meta` (metainformation), `frag`\n>  \t(hunk header), 'func' (function in hunk header), `old` (removed lines),\n> -\t`new` (added lines), `commit` (commit headers), or `whitespace`\n> -\t(highlighting whitespace errors).\n> +\t`new` (added lines), `commit` (commit headers), `whitespace`\n> +\t(highlighting whitespace errors), `movedfrom` (removed lines that\n> +\treappear), `movedto` (added lines that were removed elsewhere).\n>  \n>  color.decorate.<slot>::\n>  \tUse customized color for 'git log --decorate' output.  `<slot>` is one\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 705a873..13b6a2a 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -234,6 +234,13 @@ ifdef::git-diff[]\n>  endif::git-diff[]\n>  \tIt is the same as `--color=never`.\n>  \n> +--[no-]color-moved::\n> +\tShow moved blocks in a different color.\n> +ifdef::git-diff[]\n> +\tIt can be changed by the `diff.ui` and `color.diff`\n> +\tconfiguration settings.\n> +endif::git-diff[]\n> +\n>  --word-diff[=<mode>]::\n>  \tShow a word diff, using the <mode> to delimit changed words.\n>  \tBy default, words are delimited by whitespace; see\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 9c8f738..9827c2e 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2115,6 +2115,8 @@ _git_config ()\n>  \t\tcolor.diff.old\n>  \t\tcolor.diff.plain\n>  \t\tcolor.diff.whitespace\n> +\t\tcolor.diff.movedfrom\n> +\t\tcolor.diff.movedto\n>  \t\tcolor.grep\n>  \t\tcolor.grep.context\n>  \t\tcolor.grep.filename\n> diff --git a/diff.c b/diff.c\n> index 534c12e..47685f3 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -18,6 +18,7 @@\n>  #include \"ll-merge.h\"\n>  #include \"string-list.h\"\n>  #include \"argv-array.h\"\n> +#include \"git-compat-util.h\"\n>  \n>  #ifdef NO_FAST_WORKING_DIRECTORY\n>  #define FAST_WORKING_DIRECTORY 0\n> @@ -30,6 +31,7 @@ static int diff_compaction_heuristic; /* experimental */\n>  static int diff_rename_limit_default = 400;\n>  static int diff_suppress_blank_empty;\n>  static int diff_use_color_default = -1;\n> +static int diff_color_moved_default = -1;\n>  static int diff_context_default = 3;\n>  static const char *diff_word_regex_cfg;\n>  static const char *external_diff_cmd_cfg;\n> @@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {\n>  \tGIT_COLOR_YELLOW,\t/* COMMIT */\n>  \tGIT_COLOR_BG_RED,\t/* WHITESPACE */\n>  \tGIT_COLOR_NORMAL,\t/* FUNCINFO */\n> +\tGIT_COLOR_BLUE,\t\t/* MOVED FROM */\n> +\tGIT_COLOR_MAGENTA,\t/* MOVED TO */\n>  };\n>  \n>  static int parse_diff_color_slot(const char *var)\n> @@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)\n>  \t\treturn DIFF_WHITESPACE;\n>  \tif (!strcasecmp(var, \"func\"))\n>  \t\treturn DIFF_FUNCINFO;\n> +\tif (!strcasecmp(var, \"movedfrom\"))\n> +\t\treturn DIFF_FILE_MOVED_FROM;\n> +\tif (!strcasecmp(var, \"movedto\"))\n> +\t\treturn DIFF_FILE_MOVED_TO;\n>  \treturn -1;\n>  }\n>  \n> @@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n>  \t\tdiff_use_color_default = git_config_colorbool(var, value);\n>  \t\treturn 0;\n>  \t}\n> +\tif (!strcmp(var, \"color.moved\")) {\n> +\t\tdiff_color_moved_default = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n>  \tif (!strcmp(var, \"diff.context\")) {\n>  \t\tdiff_context_default = git_config_int(var, value);\n>  \t\tif (diff_context_default < 0)\n> @@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n>  \treturn git_default_config(var, value, cb);\n>  }\n>  \n> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,\n> +\t\t\t\t     const struct diff_line_moved_entry *b,\n> +\t\t\t\t     const void *unused)\n> +{\n> +\treturn strcmp(a->line, b->line) &&\n> +\t       a->hash_prev_line == b->hash_prev_line;\n\nI doubt it would make much difference, but my knee-jerk reaction to\nthis was to suggest swapping the order of the expression, thus:\n\n\treturn a->hash_prev_line == b->hash_prev_line &&\n\t\tstrcmp(a->line, b->line);\n\n... but perhaps it doesn't read quite so well, and probably wouldn't affect\nperformance much (except in strange edge cases), so it may not be worth it.\n\nATB,\nRamsay Jones\n\n"},{"id":"301189","messageId":"15618224-a9f3-bbe7-3556-8fd8aab2a2a4@gmail.com","threadId":"44017","inReplyTo":"20160906070151.15163-1-stefanbeller@gmail.com","subject":"Re: [PATCHv4] diff.c: emit moved lines with a different color","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-06T14:05:39Z","receivedAt":"2016-09-06T14:06:01Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.09.2016 o 09:01, Stefan Beller pisze:\n\n> ---\n> \n>  * moved new data structures into struct diff_options\n>  * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature\n>  * color.diff.movedfrom and color.diff.movedto to control the colors\n>  * added a test\n[...]\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 0bcb679..5daf77a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the\n>  'git-diff-{asterisk}' plumbing commands.  Can be overridden on the\n>  command line with the `--color[=<when>]` option.\n>  \n> +color.moved::\n> +\tA boolean value, whether a diff should color moved lines\n> +\tdifferently. The moved lines are searched for in the diff only.\n> +\tDuplicated lines from somewhere in the project that are not\n> +\tpart of the diff are not colored as moved.\n> +\tDefaults to true.\n\n[...]\n> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n> index 705a873..13b6a2a 100644\n> --- a/Documentation/diff-options.txt\n> +++ b/Documentation/diff-options.txt\n> @@ -234,6 +234,13 @@ ifdef::git-diff[]\n>  endif::git-diff[]\n>  \tIt is the same as `--color=never`.\n>  \n> +--[no-]color-moved::\n> +\tShow moved blocks in a different color.\n> +ifdef::git-diff[]\n> +\tIt can be changed by the `diff.ui` and `color.diff`\n> +\tconfiguration settings.\n> +endif::git-diff[]\n\nIf not for `color.moved`, I would have thought that instead of adding\nnew command line option `--color-moved` (and the fact that it is on\nby default), we could simply reuse duplication of code movement\ndetection as a signal of stronger detection, namely \"-M -M\" (and also\n\"-C -C\" to handle copy detection) that git-blame uses...\n\n-- \nJakub Narębski\n\n"},{"id":"301211","messageId":"CAGZ79kZPzEYV=gdhzXQetXoe4+1zdh67eyL-gGh9EOCSbRwzWw@mail.gmail.com","threadId":"44017","inReplyTo":"15618224-a9f3-bbe7-3556-8fd8aab2a2a4@gmail.com","subject":"Re: [PATCHv4] diff.c: emit moved lines with a different color","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-06T17:03:13Z","receivedAt":"2016-09-06T17:25:50Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 06.09.2016 o 09:01, Stefan Beller pisze:\n>\n>> ---\n>>\n>>  * moved new data structures into struct diff_options\n>>  * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature\n>>  * color.diff.movedfrom and color.diff.movedto to control the colors\n>>  * added a test\n> [...]\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 0bcb679..5daf77a 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -974,14 +974,22 @@ This does not affect linkgit:git-format-patch[1] or the\n>>  'git-diff-{asterisk}' plumbing commands.  Can be overridden on the\n>>  command line with the `--color[=<when>]` option.\n>>\n>> +color.moved::\n>> +     A boolean value, whether a diff should color moved lines\n>> +     differently. The moved lines are searched for in the diff only.\n>> +     Duplicated lines from somewhere in the project that are not\n>> +     part of the diff are not colored as moved.\n>> +     Defaults to true.\n>\n> [...]\n>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\n>> index 705a873..13b6a2a 100644\n>> --- a/Documentation/diff-options.txt\n>> +++ b/Documentation/diff-options.txt\n>> @@ -234,6 +234,13 @@ ifdef::git-diff[]\n>>  endif::git-diff[]\n>>       It is the same as `--color=never`.\n>>\n>> +--[no-]color-moved::\n>> +     Show moved blocks in a different color.\n>> +ifdef::git-diff[]\n>> +     It can be changed by the `diff.ui` and `color.diff`\n>> +     configuration settings.\n>> +endif::git-diff[]\n>\n> If not for `color.moved`, I would have thought that instead of adding\n> new command line option `--color-moved` (and the fact that it is on\n> by default), we could simply reuse duplication of code movement\n> detection as a signal of stronger detection, namely \"-M -M\" (and also\n> \"-C -C\" to handle copy detection) that git-blame uses...\n\nCan you please elaborate on how you'd use that as a user?\n\nThe -M and -C options only operate on the file level, e.g.\nthese options are very good at things introduced via:\n\n    git mv A B\n    $EDIT B # only a little.\n\nSo these options make no sense when operating only on one\nfile or on many files that stay the same and only change very little.\n\nThe goal of my patch here is to improve cases like 11979b98\n(2005-11-18, http.c: reorder to avoid compilation failure.)\n\nIn that case we just move code around, not necessarily across file\nboundaries.\n\nSo that seems orthogonal to the -M/-C option as it operates on another\nlevel. (file vs line)\n\nIn another email you asked whether this new approach works in the\nword-by-word diff, which it unfortunately doesn't yet, but I would think\nthat it is the same problem (line vs word granularity)\n\nSo what I am asking here is, how would you imagine a better user interface\nfor what I am trying to do, or do you think I should adapt my goal?\n\nThanks,\nStefan\n\n>\n> --\n> Jakub Narębski\n>\n"},{"id":"301213","messageId":"82367750-adea-6dee-198a-e39ac7a84b31@gmail.com","threadId":"44017","inReplyTo":"CAGZ79kZPzEYV=gdhzXQetXoe4+1zdh67eyL-gGh9EOCSbRwzWw@mail.gmail.com","subject":"Re: [PATCHv4] diff.c: emit moved lines with a different color","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-06T17:51:26Z","receivedAt":"2016-09-06T17:51:44Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 06.09.2016 o 19:03, Stefan Beller pisze:\n> On Tue, Sep 6, 2016 at 7:05 AM, Jakub Narębski <jnareb@gmail.com> wrote:\n\n>> If not for `color.moved`, I would have thought that instead of adding\n>> new command line option `--color-moved` (and the fact that it is on\n>> by default), we could simply reuse duplication of code movement\n>> detection as a signal of stronger detection, namely \"-M -M\" (and also\n>> \"-C -C\" to handle copy detection) that git-blame uses...\n> \n> Can you please elaborate on how you'd use that as a user?\n> \n> The -M and -C options only operate on the file level, e.g.\n> these options are very good at things introduced via:\n> \n>     git mv A B\n>     $EDIT B # only a little.\n> \n> So these options make no sense when operating only on one\n> file or on many files that stay the same and only change very little.\n> \n> The goal of my patch here is to improve cases like 11979b98\n> (2005-11-18, http.c: reorder to avoid compilation failure.)\n>\n> In that case we just move code around, not necessarily across file\n> boundaries.\n>\n> So that seems orthogonal to the -M/-C option as it operates on another\n> level. (file vs line)\n\nThe idea for an alternative way of turning on color marking of moved\nlines was to follow an example of \"git blame\", where _doubling_\nof a command means more extensive move / copy detection (accompanied\nby new values for `diff.renames`).\n\nFrom git-blame(1) manpage:\n\n-C|<num>|\n     In addition to -M, detect lines moved or copied from other files\n     that were modified in the same commit. [...]. When this option\n     is given twice, the command additionally looks for copies from\n     other files in the commit that creates the file. When this option\n     is given three times, the command additionally looks for copies\n     from other files in any commit.\n\nColor marking of moved lines may be considered enhancing of exiting\nwhole-file movement and whole-file copy detection.\n\n\nBut it is not a good UI if the feature is to be turned on by default.\nYour proposal of adding `--color-moved` and `color.moved` is better.\n \n> In another email you asked whether this new approach works in the\n> word-by-word diff, which it unfortunately doesn't yet, but I would think\n> that it is the same problem (line vs word granularity).\n\nI don't know how it is done internally, but I think word diff is done\nby using words (as defined by `diff.<driver>.wordRegex`) in place\nof lines...\n\nBest,\n-- \nJakub Narębski\n\n"},{"id":"301314","messageId":"xmqqvay7lezg.fsf@gitster.mtv.corp.google.com","threadId":"44017","inReplyTo":"87a1ddbf-2499-d5b4-55c4-aeed2b72acce@ramsayjones.plus.com","subject":"Re: [PATCHv4] diff.c: emit moved lines with a different color","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-07T18:52:19Z","receivedAt":"2016-09-07T18:52:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n>> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,\n>> +\t\t\t\t     const struct diff_line_moved_entry *b,\n>> +\t\t\t\t     const void *unused)\n>> +{\n>> +\treturn strcmp(a->line, b->line) &&\n>> +\t       a->hash_prev_line == b->hash_prev_line;\n>\n> I doubt it would make much difference, but my knee-jerk reaction to\n> this was to suggest swapping the order of the expression, thus:\n>\n> \treturn a->hash_prev_line == b->hash_prev_line &&\n> \t\tstrcmp(a->line, b->line);\n>\n> ... but perhaps it doesn't read quite so well, and probably wouldn't affect\n> performance much (except in strange edge cases), so it may not be worth it.\n\nIt would make very much sense to do so, as the final version will be\na lot more involved than a mere strcmp() to make \"git diff -w\" to\nalso work as expected with this new feature.\n\n"}]}