{"thread":{"id":"43993","subject":"[RFC/PATCH 0/2] Color moved code differently","startedAt":"2016-09-03T03:31:40Z","lastAt":"2016-09-04T22:20:09Z","messageCount":13,"participants":["Stefan Beller","Junio C Hamano","Jakub Narębski","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"300939","messageId":"20160903033120.20511-1-sbeller@google.com","threadId":"43993","inReplyTo":null,"subject":"[RFC/PATCH 0/2] Color moved code differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-03T03:31:18Z","receivedAt":"2016-09-03T03:31:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When moving code (e.g. a function is moved to another part of the file or\nto a different file), the review process is different than reviewing new\ncode. When reviewing moved code we are only interested in the diff as\nwhere there are differences in the moved code, e.g. namespace changes.\n\nHowever the inner part of these moved texts should not change.\nTo aid a developer reviewing such code, we'll color pure moved stuff\ndifferently.\n\nA line is colored differently if that line and the surroundign 2 lines\nappear as-is in the opposite part of the diff.\n\nExample:\nhttp://i.imgur.com/ay84q0q.png\n\nOr apply these patches and \n    git show e28eae3184b26d3cf3293e69403babb5c575342c\n    git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5\n    git show 8465541e8ce8eaf16e66ab847086779768c18f2d\n\nThe code quality sucks though, hence RFC.\n\nThanks,\nStefan\n\nStefan Beller (2):\n  diff.c: emit duplicate lines with a different color\n  WIP xdiff: markup duplicates differently\n\n diff.c        |  26 ++++++++++++\n diff.h        |   4 +-\n xdiff/xdiff.h |   1 +\n xdiff/xemit.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 4 files changed, 156 insertions(+), 3 deletions(-)\n\n-- \n2.10.0.rc2.23.gf336a1a.dirty\n\n"},{"id":"300940","messageId":"20160903033120.20511-2-sbeller@google.com","threadId":"43993","inReplyTo":"20160903033120.20511-1-sbeller@google.com","subject":"[PATCH 1/2] diff.c: emit duplicate lines with a different color","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-03T03:31:19Z","receivedAt":"2016-09-03T03:31:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Color is WIP, I just make space for a different case.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff.c | 26 ++++++++++++++++++++++++++\n diff.h |  4 +++-\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 534c12e..57d435c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -52,6 +52,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/* NEW DUPLICATE */\n+\tGIT_COLOR_CYAN,\t\t/* OLD DUPLICATE */\n };\n \n static int parse_diff_color_slot(const char *var)\n@@ -541,6 +543,14 @@ static void emit_add_line(const char *reset,\n \t\t\t  DIFF_FILE_NEW, WSEH_NEW, '+');\n }\n \n+static void emit_add_line_dup(const char *reset,\n+\t\t\t  struct emit_callback *ecbdata,\n+\t\t\t  const char *line, int len)\n+{\n+\temit_line_checked(reset, ecbdata, line, len,\n+\t\t\t  DIFF_FILE_DUPLICATE_NEW, WSEH_NEW, '+');\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@@ -549,6 +559,14 @@ static void emit_del_line(const char *reset,\n \t\t\t  DIFF_FILE_OLD, WSEH_OLD, '-');\n }\n \n+static void emit_del_line_dup(const char *reset,\n+\t\t\t  struct emit_callback *ecbdata,\n+\t\t\t  const char *line, int len)\n+{\n+\temit_line_checked(reset, ecbdata, line, len,\n+\t\t\t  DIFF_FILE_DUPLICATE_OLD, WSEH_OLD, '-');\n+}\n+\n static void emit_context_line(const char *reset,\n \t\t\t      struct emit_callback *ecbdata,\n \t\t\t      const char *line, int len)\n@@ -1300,6 +1318,10 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t}\n \n \tswitch (line[0]) {\n+\tcase '*':\n+\t\tecbdata->lno_in_postimage++;\n+\t\temit_add_line_dup(reset, ecbdata, line + 1, len - 1);\n+\t\tbreak;\n \tcase '+':\n \t\tecbdata->lno_in_postimage++;\n \t\temit_add_line(reset, ecbdata, line + 1, len - 1);\n@@ -1308,6 +1330,10 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)\n \t\tecbdata->lno_in_preimage++;\n \t\temit_del_line(reset, ecbdata, line + 1, len - 1);\n \t\tbreak;\n+\tcase '~':\n+\t\tecbdata->lno_in_preimage++;\n+\t\temit_del_line_dup(reset, ecbdata, line + 1, len - 1);\n+\t\tbreak;\n \tcase ' ':\n \t\tecbdata->lno_in_postimage++;\n \t\tecbdata->lno_in_preimage++;\ndiff --git a/diff.h b/diff.h\nindex 7883729..d500f0e 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -189,7 +189,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_DUPLICATE_NEW = 9,\n+\tDIFF_FILE_DUPLICATE_OLD = 10\n };\n const char *diff_get_color(int diff_use_color, enum color_diff ix);\n #define diff_get_color_opt(o, ix) \\\n-- \n2.10.0.rc2.23.gf336a1a.dirty\n\n"},{"id":"300941","messageId":"20160903033120.20511-3-sbeller@google.com","threadId":"43993","inReplyTo":"20160903033120.20511-1-sbeller@google.com","subject":"[RFC/PATCH 2/2] WIP xdiff: markup duplicates differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-03T03:31:20Z","receivedAt":"2016-09-03T03:31:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When moving code (e.g. a function is moved to another part of the file or\nto a different file), the review process is different than reviewing new\ncode. When reviewing moved code we are only interested in the diff as\nwhere there are differences in the moved code, e.g. namespace changes.\n\nHowever the inner part of these moved texts should not change.\nTo aid a developer reviewing such code, emit it with a different prefix\nthan the usual +,- to indicate it is overlapping code.\n\nExamples from recent history:\n    git show e28eae3184b26d3cf3293e69403babb5c575342c\n    git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5\n    git show 8465541e8ce8eaf16e66ab847086779768c18f2d\n\nThis doesn't work yet, but we should make this patch series work\nto ignore white space changes:\n9d1ca1dac0ebfd6e17d73e33b2d173926c139c2d\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n xdiff/xdiff.h |   1 +\n xdiff/xemit.c | 128 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 127 insertions(+), 2 deletions(-)\n\ndiff --git a/xdiff/xdiff.h b/xdiff/xdiff.h\nindex 7423f77..0744e01 100644\n--- a/xdiff/xdiff.h\n+++ b/xdiff/xdiff.h\n@@ -45,6 +45,7 @@ extern \"C\" {\n \n #define XDL_EMIT_FUNCNAMES (1 << 0)\n #define XDL_EMIT_FUNCCONTEXT (1 << 2)\n+#define XDL_EMIT_DUPLICATE (1 << 3)\n \n #define XDL_MMB_READONLY (1 << 0)\n \ndiff --git a/xdiff/xemit.c b/xdiff/xemit.c\nindex b52b4b9..4abafae 100644\n--- a/xdiff/xemit.c\n+++ b/xdiff/xemit.c\n@@ -22,6 +22,9 @@\n \n #include \"xinclude.h\"\n \n+#include \"git-compat-util.h\"\n+#include \"hashmap.h\"\n+\n static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec) {\n \n \t*rec = xdf->recs[ri]->ptr;\n@@ -158,12 +161,133 @@ static int is_empty_rec(xdfile_t *xdf, long ri)\n \treturn !len;\n }\n \n+struct hashmap *duplicates_added;\n+struct hashmap *duplicates_removed;\n+\n+struct dup_entry {\n+\tstruct hashmap_entry ent;\n+\txdfile_t *xdf;\n+\tlong index;\n+};\n+\n+static int dup_entry_cmp(const struct dup_entry *a,\n+\t\t\t   const struct dup_entry *b,\n+\t\t\t   const void *unused)\n+{\n+\tint d = XDL_MIN(strcspn(a->xdf->recs[a->index]->ptr, \"\\n\"),\n+\t\t\t\tstrcspn(b->xdf->recs[b->index]->ptr, \"\\n\"));\n+\n+\tif (!strncmp(a->xdf->recs[a->index]->ptr,\n+\t\t\tb->xdf->recs[b->index]->ptr,\n+\t\t\td))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n+struct dup_entry *prepare_entry(xdfile_t *xdf, long ri)\n+{\n+\tlong range_start = XDL_MAX(0, ri - 2);\n+\tlong range_end = XDL_MIN(xdf->nrec, ri + 2);\n+\tlong hash = 0;\n+\tint i;\n+\tstruct dup_entry *ret = xmalloc(sizeof(*ret));\n+\n+\tfor (i = range_start; i < range_end; i++)\n+\t\thash ^= memhash(xdf->recs[i]->ptr, xdf->recs[i]->size);\n+\n+\tret->ent.hash = hash;\n+\tret->xdf = xdf;\n+\tret->index = ri;\n+\treturn ret;\n+}\n+\n+int add_removal(xdfile_t *xdf, long ri)\n+{\n+\thashmap_add(duplicates_removed, prepare_entry(xdf, ri));\n+\treturn 0;\n+}\n+\n+int add_addition(xdfile_t *xdf, long ri)\n+{\n+\thashmap_add(duplicates_added, prepare_entry(xdf, ri));\n+\treturn 0;\n+}\n+\n+int xdl_markup_duplicates(xdfenv_t *xe, xdchange_t *xscr,\n+\t\t\t  xdemitconf_t const *xecfg)\n+{\n+\tlong s1, s2;\n+\txdchange_t *xch, *xche;\n+\n+\tduplicates_added = xmalloc(sizeof(*duplicates_added));\n+\tduplicates_removed = xmalloc(sizeof(*duplicates_removed));\n+\thashmap_init(duplicates_added, (hashmap_cmp_fn)dup_entry_cmp, 0);\n+\thashmap_init(duplicates_removed, (hashmap_cmp_fn)dup_entry_cmp, 0);\n+\n+\tfor (xch = xscr; xch; xch = xche->next) {\n+\t\txche = xdl_get_hunk(&xch, xecfg);\n+\t\tif (!xch)\n+\t\t\tbreak;\n+\n+\t\tfor (s1 = xch->i1, s2 = xch->i2;; xch = xch->next) {\n+\n+\t\t\t/*\n+\t\t\t * Removes lines from the first file.\n+\t\t\t */\n+\t\t\tfor (s1 = xch->i1; s1 < xch->i1 + xch->chg1; s1++)\n+\t\t\t\tif (add_removal(&xe->xdf1, s1) < 0)\n+\t\t\t\t\treturn -1;\n+\n+\t\t\t/*\n+\t\t\t * Adds lines from the second file.\n+\t\t\t */\n+\t\t\tfor (s2 = xch->i2; s2 < xch->i2 + xch->chg2; s2++)\n+\t\t\t\tif (add_addition(&xe->xdf2, s2) < 0)\n+\t\t\t\t\treturn -1;\n+\n+\t\t\tif (xch == xche)\n+\t\t\t\tbreak;\n+\t\t\ts1 = xch->i1 + xch->chg1;\n+\t\t\ts2 = xch->i2 + xch->chg2;\n+\t\t}\n+\t}\n+\treturn 0;\n+}\n+\n+static int xdl_check_and_emit_record(xdfile_t *xdf, long ri,\n+\t\t\t\t     char *pre, xdemitcb_t *ecb,\n+\t\t\t\t     int duplicate_handling)\n+{\n+\tconst char *hacked_pre = pre;\n+\n+\tif (duplicate_handling) {\n+\t\tstruct dup_entry *keydata = prepare_entry(xdf, ri);\n+\n+\t\tif (*pre == '+' &&\n+\t\t    hashmap_get(duplicates_removed, keydata, keydata))\n+\t\t\t\thacked_pre = \"*\";\n+\n+\t\tif (*pre == '-' &&\n+\t\t    hashmap_get(duplicates_added, keydata, keydata))\n+\t\t\t\thacked_pre = \"~\";\n+\t\tfree(keydata);\n+\t}\n+\n+\treturn xdl_emit_record(xdf, ri, hacked_pre, ecb);\n+}\n+\n int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t  xdemitconf_t const *xecfg) {\n \tlong s1, s2, e1, e2, lctx;\n \txdchange_t *xch, *xche;\n \tlong funclineprev = -1;\n \tstruct func_line func_line = { 0 };\n+\tint duplicate_handling = 0;\n+\n+\t/* for testing I added a `|| 1` */\n+\tduplicate_handling = xecfg->flags & XDL_EMIT_DUPLICATE || 1;\n+\tif (duplicate_handling)\n+\t\txdl_markup_duplicates(xe, xscr, xecfg);\n \n \tfor (xch = xscr; xch; xch = xche->next) {\n \t\txche = xdl_get_hunk(&xch, xecfg);\n@@ -279,14 +403,14 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,\n \t\t\t * Removes lines from the first file.\n \t\t\t */\n \t\t\tfor (s1 = xch->i1; s1 < xch->i1 + xch->chg1; s1++)\n-\t\t\t\tif (xdl_emit_record(&xe->xdf1, s1, \"-\", ecb) < 0)\n+\t\t\t\tif (xdl_check_and_emit_record(&xe->xdf1, s1, \"-\", ecb, duplicate_handling) < 0)\n \t\t\t\t\treturn -1;\n \n \t\t\t/*\n \t\t\t * Adds lines from the second file.\n \t\t\t */\n \t\t\tfor (s2 = xch->i2; s2 < xch->i2 + xch->chg2; s2++)\n-\t\t\t\tif (xdl_emit_record(&xe->xdf2, s2, \"+\", ecb) < 0)\n+\t\t\t\tif (xdl_check_and_emit_record(&xe->xdf2, s2, \"+\", ecb, duplicate_handling) < 0)\n \t\t\t\t\treturn -1;\n \n \t\t\tif (xch == xche)\n-- \n2.10.0.rc2.23.gf336a1a.dirty\n\n"},{"id":"300943","messageId":"xmqqtwdxqxh6.fsf@gitster.mtv.corp.google.com","threadId":"43993","inReplyTo":"20160903033120.20511-1-sbeller@google.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-03T07:00:21Z","receivedAt":"2016-09-03T07:00:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> A line is colored differently if that line and the surroundign 2 lines\n> appear as-is in the opposite part of the diff.\n>\n> Example:\n> http://i.imgur.com/ay84q0q.png\n>\n> Or apply these patches and \n>     git show e28eae3184b26d3cf3293e69403babb5c575342c\n>     git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5\n>     git show 8465541e8ce8eaf16e66ab847086779768c18f2d\n\nI like this as a concept.  Two quick comments are\n\n * On 1/2, you would also need to teach diff-color-slot the\n   correspondence between the name used by configuration and the\n   enum used as the index into the diff_colors[] array.  I think\n   these are not \"DUPLICATE\", but \"MOVE\", so I'd suggest renaming\n   dup-new and dup-old to some words or phrases that have \"MOVED\"\n   and \"TO\" or \"FROM\" in them (e.g. \"DIFF_MOVED_FROM\",\n   \"DIFF_MOVED_TO\").\n\n * On 2/2, doing it at xdiff.c level may be limiting this good idea\n   to flourish to its full potential, as the interface is fed only\n   one diff_filepair at a time.  All the examples you pointed at\n   above have line movement within a single path because of this\n   design limitation.  I do not think 2/2 would serve as a small but\n   good first step to build on top of to enhance the feature to\n   notice line movements across files and the design (not the\n   implementation) needs rethinking.\n\nThe idea has a potential to help reviewing inter-file movement of\nlines in 3b0c4200 (\"apply: move libified code from builtin/apply.c\nto apply.{c,h}\", 2016-08-08).  You can see what was _changed_ in the\npart that has been moved across files with \"show -B -M\", and\nsometimes that is useful, but at the same time, you cannot see what\nwas moved without changing, which often is necessary to understand\nthe changes and notice things like \"you moved this across files\nwithout changing, but this and that you did not change need to be\nadjusted\".\n\nThe coloring of \"these are moved verbatim\" in the style this series\ngives would be very helpful for reviewing such a change.\n\n"},{"id":"300946","messageId":"eb88af2c-d7b1-295e-5f23-a85045bde753@gmail.com","threadId":"43993","inReplyTo":"20160903033120.20511-3-sbeller@google.com","subject":"Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-03T12:25:08Z","receivedAt":"2016-09-03T12:25:29Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 03.09.2016 o 05:31, Stefan Beller pisze:\n\n> When moving code (e.g. a function is moved to another part of the file or\n> to a different file), the review process is different than reviewing new\n> code. When reviewing moved code we are only interested in the diff as\n> where there are differences in the moved code, e.g. namespace changes.\n> \n> However the inner part of these moved texts should not change.\n> To aid a developer reviewing such code, emit it with a different prefix\n> than the usual +,- to indicate it is overlapping code.\n\nWhat would be this different prefix?\n\n\nSide note: I wonder if the cousin of unified diff, namely context diff[1],\nis something that we can and should support.\n\n[1]: https://www.gnu.org/software/diffutils/manual/html_node/Context-Format.html\n     https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Context.html\n\n*** lao\t2002-02-21 23:30:39.942229878 -0800\n--- tzu\t2002-02-21 23:30:50.442260588 -0800\n***************\n*** 1,7 ****\n- The Way that can be told of is not the eternal Way;\n- The name that can be named is not the eternal name.\n  The Nameless is the origin of Heaven and Earth;\n! The Named is the mother of all things.\n  Therefore let there always be non-being,\n    so we may see their subtlety,\n  And let there always be being,\n--- 1,6 ----\n  The Nameless is the origin of Heaven and Earth;\n! The named is the mother of all things.\n! \n  Therefore let there always be non-being,\n    so we may see their subtlety,\n  And let there always be being,\n***************\n*** 9,11 ****\n--- 8,13 ----\n  The two are the same,\n  But after they are produced,\n    they have different names.\n+ They both may be called deep and profound.\n+ Deeper and more profound,\n+ The door of all subtleties!\n"},{"id":"300963","messageId":"CAGZ79kYGnkcOxviukj9a8gyaERip5aunXcvsdH-UpBCb=vrVeQ@mail.gmail.com","threadId":"43993","inReplyTo":"xmqqtwdxqxh6.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-04T05:23:35Z","receivedAt":"2016-09-04T05:24:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Sep 3, 2016 at 12:00 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> A line is colored differently if that line and the surroundign 2 lines\n>> appear as-is in the opposite part of the diff.\n>>\n>> Example:\n>> http://i.imgur.com/ay84q0q.png\n>>\n>> Or apply these patches and\n>>     git show e28eae3184b26d3cf3293e69403babb5c575342c\n>>     git show bc9204d4ef6e0672389fdfb0d398fa9a39dba3d5\n>>     git show 8465541e8ce8eaf16e66ab847086779768c18f2d\n>\n> I like this as a concept.  Two quick comments are\n\nGreat!\n\n>\n>  * On 1/2, you would also need to teach diff-color-slot the\n>    correspondence between the name used by configuration and the\n>    enum used as the index into the diff_colors[] array.\n\nSo I would need to add code to diff_parse_color_slot just below the\ndefinition of the struct.\n\n>    I think\n>    these are not \"DUPLICATE\", but \"MOVE\", so I'd suggest renaming\n>    dup-new and dup-old to some words or phrases that have \"MOVED\"\n>    and \"TO\" or \"FROM\" in them (e.g. \"DIFF_MOVED_FROM\",\n>    \"DIFF_MOVED_TO\").\n\nOk, sounds sensible.\n\n>\n>  * On 2/2, doing it at xdiff.c level may be limiting this good idea\n>    to flourish to its full potential, as the interface is fed only\n>    one diff_filepair at a time.\n\nI realized that after I implemented it. I agree we would want to have\nit function cross file.\n\nSo from my current understanding of the code,\n* diffcore_std would call a new function diffcore_detect_moved(void)\n   just before diffcore_apply_filter is called.\n* The new function diffcore_detect_moved would then check if the\n   diff is a valid textual diff (i.e. real files, not submodules, but\n   deletion/creation of one file is allowed)\n   If so we generate the diff internally and as in 2/2 would\n   hash all added/removed lines with context and store it.\n* Instead checking for a different symbol in fn_out_consume, we consult\n   the hashmap whether we want to color the line as a \"moved\" line.\n\n>     All the examples you pointed at\n>    above have line movement within a single path because of this\n>    design limitation.  I do not think 2/2 would serve as a small but\n>    good first step to build on top of to enhance the feature to\n>    notice line movements across files and the design (not the\n>    implementation) needs rethinking.\n\nAfter reading the code more closely I agree. I initially put it there\nto see if it is feasible or just messing up the diff. And as I have\na bit of knowledge of the xdl internals due to the first version of\nthe diff slider heuristic, I went with that.\n\nFrom a cursory look at the history, you seemed to be very involved in\nthe implementation of diff.c, so I'd appreciate strong guidance on\nthe design level.\n\n>\n> The idea has a potential to help reviewing inter-file movement of\n> lines in 3b0c4200 (\"apply: move libified code from builtin/apply.c\n> to apply.{c,h}\", 2016-08-08).  You can see what was _changed_ in the\n> part that has been moved across files with \"show -B -M\", and\n> sometimes that is useful, but at the same time, you cannot see what\n> was moved without changing, which often is necessary to understand\n> the changes and notice things like \"you moved this across files\n> without changing, but this and that you did not change need to be\n> adjusted\".\n>\n> The coloring of \"these are moved verbatim\" in the style this series\n> gives would be very helpful for reviewing such a change.\n>\n\nRight, that is what triggered me to implement it.\nAnother such case that I reviewed was\nhttps://git.eclipse.org/r/#/c/78645/\nand I felt very insecure about reviewing thousands lines of code\nthat was \"just moved\".\n\nThanks,\nStefan\n"},{"id":"300964","messageId":"CAGZ79kYEieYGFAgORc8yaF3=8-L1E7K4afNGxDH5AgM5nHFgFw@mail.gmail.com","threadId":"43993","inReplyTo":"eb88af2c-d7b1-295e-5f23-a85045bde753@gmail.com","subject":"Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-04T05:31:33Z","receivedAt":"2016-09-04T05:31:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Sep 3, 2016 at 5:25 AM, Jakub Narębski <jnareb@gmail.com> wrote:\n> W dniu 03.09.2016 o 05:31, Stefan Beller pisze:\n>\n>> When moving code (e.g. a function is moved to another part of the file or\n>> to a different file), the review process is different than reviewing new\n>> code. When reviewing moved code we are only interested in the diff as\n>> where there are differences in the moved code, e.g. namespace changes.\n>>\n>> However the inner part of these moved texts should not change.\n>> To aid a developer reviewing such code, emit it with a different prefix\n>> than the usual +,- to indicate it is overlapping code.\n>\n> What would be this different prefix?\n\nI will discard the part of the different prefix as the design of 2/2\nwill change.\n\n\n\n>\n>\n> Side note: I wonder if the cousin of unified diff, namely context diff[1],\n> is something that we can and should support.\n\n\n\n>\n> [1]: https://www.gnu.org/software/diffutils/manual/html_node/Context-Format.html\n>      https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Context.html\n>\n> *** lao 2002-02-21 23:30:39.942229878 -0800\n> --- tzu 2002-02-21 23:30:50.442260588 -0800\n> ***************\n> *** 1,7 ****\n> - The Way that can be told of is not the eternal Way;\n> - The name that can be named is not the eternal name.\n>   The Nameless is the origin of Heaven and Earth;\n> ! The Named is the mother of all things.\n>   Therefore let there always be non-being,\n>     so we may see their subtlety,\n>   And let there always be being,\n> --- 1,6 ----\n>   The Nameless is the origin of Heaven and Earth;\n> ! The named is the mother of all things.\n> !\n\nSo the line moved here?\nIs it intentional that the line differs though?\n(capitalisation of 'named\")\nNot sure I can read this diff correctly.\n\nI think for this small side project I'd rather want\nto 'just' support colors of moved code;)\n\n>   Therefore let there always be non-being,\n>     so we may see their subtlety,\n>   And let there always be being,\n> ***************\n> *** 9,11 ****\n> --- 8,13 ----\n>   The two are the same,\n>   But after they are produced,\n>     they have different names.\n> + They both may be called deep and profound.\n> + Deeper and more profound,\n> + The door of all subtleties!\n"},{"id":"300965","messageId":"xmqqpookqi8k.fsf@gitster.mtv.corp.google.com","threadId":"43993","inReplyTo":"CAGZ79kYGnkcOxviukj9a8gyaERip5aunXcvsdH-UpBCb=vrVeQ@mail.gmail.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-04T06:41:47Z","receivedAt":"2016-09-04T06:43:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n>>  * On 2/2, doing it at xdiff.c level may be limiting this good idea\n>>    to flourish to its full potential, as the interface is fed only\n>>    one diff_filepair at a time.\n>\n> I realized that after I implemented it. I agree we would want to have\n> it function cross file.\n>\n> So from my current understanding of the code,\n> * diffcore_std would call a new function diffcore_detect_moved(void)\n>    just before diffcore_apply_filter is called.\n> * The new function diffcore_detect_moved would then check if the\n>    diff is a valid textual diff (i.e. real files, not submodules, but\n>    deletion/creation of one file is allowed)\n>    If so we generate the diff internally and as in 2/2 would\n>    hash all added/removed lines with context and store it.\n\nI do not think you should step outside diff_flush().  Only when\nproducing textual diff, you would have to run the textual diff\ntwice by going over the q twice:\n\n * The first pass would run diff_flush_patch(), which would call\n   into xdiff the usual way, but the callback from xdiff would\n   capture the removed lines and the added lines without making any\n   output.\n\n * The second pass would run diff_flush_patch(), but the callback\n   from xdiff would be called with additional information, namely,\n   the removed and the added lines captured in the first pass.\n\n * I suspect that the fn_out_consume() function that is used for a\n   normal case (i.e. when we are not doing this more expensive\n   \"moved to/moved from\" coloring) can be used for the second pass\n   above (i.e. the \"priv\" aka \"ecbdata\" may need to be extended so\n   that it can tell which mode of operation it is asked to perform),\n   but if there is not enough similarity between the second pass of\n   this \"moved from/moved to\" mode and the normal mode of output, it\n   is also OK to have two different callback functions, i.e. the\n   original one to be used in the normal mode, the second one that\n   knows the \"these are moved without modification\" coloring.  The\n   callback for the first pass is sufficiently different and I think\n   it is better to invent a new callback function to be used in the\n   first pass, instead of reusing fn_out_consume().\n\n   The fn_out_consume() function working in the \"second pass of\n   moved from/moved to mode\" would inspect line[] and see if it is\n   an added or a removed line, and then:\n\n   - if it is an added line, and it appears as a removed line\n     elsewhere in the patchset (you obtained the information in the\n     first pass), you show it as \"this was moved from elsewhere\".\n\n   - if it is a removed line, and it appears as an added line\n     elsewhere in the patchset (you obtained the information in the\n     first pass), you show it as \"this was moved to elsewhere\".\n\nOr something like that.\n"},{"id":"300967","messageId":"xmqqlgz8qhxa.fsf@gitster.mtv.corp.google.com","threadId":"43993","inReplyTo":"eb88af2c-d7b1-295e-5f23-a85045bde753@gmail.com","subject":"Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-04T06:48:33Z","receivedAt":"2016-09-04T06:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narębski <jnareb@gmail.com> writes:\n\n> Side note: I wonder if the cousin of unified diff, namely context diff[1],\n> is something that we can and should support.\n\nYes, the lack of support for the copied context (instead of the\nunified context) diff format has bugged me over the years.  Reading\ncopied context diff format however is rapidly becoming a lost art,\nunfortunately, partly due to our popularity, and the need for the\nformat has become a lessor issue.\n\nI wonder if a pair of a pre-processor (on the input side, before\nrunning 'git apply') and a post-processor (on the output side,\nmunging the output from 'git show/diff') would be sufficient.\n\nIn any case, I agree with you that it has nothing to do with what\nStefan is doing here.\n"},{"id":"300977","messageId":"CAGZ79kZp1EeDYDNiFGVLYbth8gRAe8=WLP=i1YeUwM-1TX4B9g@mail.gmail.com","threadId":"43993","inReplyTo":"xmqqpookqi8k.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-09-04T08:28:58Z","receivedAt":"2016-09-04T08:29:43Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Sep 3, 2016 at 11:41 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>>>  * On 2/2, doing it at xdiff.c level may be limiting this good idea\n>>>    to flourish to its full potential, as the interface is fed only\n>>>    one diff_filepair at a time.\n>>\n>> I realized that after I implemented it. I agree we would want to have\n>> it function cross file.\n>>\n>> So from my current understanding of the code,\n>> * diffcore_std would call a new function diffcore_detect_moved(void)\n>>    just before diffcore_apply_filter is called.\n>> * The new function diffcore_detect_moved would then check if the\n>>    diff is a valid textual diff (i.e. real files, not submodules, but\n>>    deletion/creation of one file is allowed)\n>>    If so we generate the diff internally and as in 2/2 would\n>>    hash all added/removed lines with context and store it.\n>\n> I do not think you should step outside diff_flush().\n\nAfter looking further into it, I think we need to step outside\nor at the very least deep down in the xdl cellar from there,\nbecause we need the context around the line both in the\npre and after image.\n\n\n>  Only when\n> producing textual diff, you would have to run the textual diff\n> twice by going over the q twice:\n>\n>  * The first pass would run diff_flush_patch(), which would call\n>    into xdiff the usual way, but the callback from xdiff would\n>    capture the removed lines and the added lines without making any\n>    output.\n\nThe callback doesn't have the context around one line as easily\naccessible as the xdl emit function, so rather we'd pass in a flag,\ni.e. the hashmap where to store the added/removed lines or NULL.\n\nBoth passes (the first to find and the second enhanced pass that\noutputs the lines) need to have access to the context of the respective\nfile.\n\n>\n>  * The second pass would run diff_flush_patch(), but the callback\n>    from xdiff would be called with additional information, namely,\n>    the removed and the added lines captured in the first pass.\n>\n>  * I suspect that the fn_out_consume() function that is used for a\n>    normal case (i.e. when we are not doing this more expensive\n>    \"moved to/moved from\" coloring) can be used for the second pass\n>    above (i.e. the \"priv\" aka \"ecbdata\" may need to be extended so\n>    that it can tell which mode of operation it is asked to perform),\n>    but if there is not enough similarity between the second pass of\n>    this \"moved from/moved to\" mode and the normal mode of output, it\n>    is also OK to have two different callback functions, i.e. the\n>    original one to be used in the normal mode, the second one that\n>    knows the \"these are moved without modification\" coloring.  The\n>    callback for the first pass is sufficiently different and I think\n>    it is better to invent a new callback function to be used in the\n>    first pass, instead of reusing fn_out_consume().\n\nThe callback doesn't have easy accessible data IMHO,\ne.g. we may get these lines in the callback:\n\n(function context not required:)\n---8<---\n {\n        int i;\n        for (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n-               unsigned int val;\n-               /*\n-                * hex[1]=='\\0' is caught when val is checked below,\n-                * but if hex[0] is NUL we have to avoid reading\n-                * past the end of the string:\n-                */\n-               if (!hex[0])\n-                       return -1;\n-               val = (hexval(hex[0]) << 4) | hexval(hex[1]);\n-               if (val & ~0xff)\n+               int val = hex2chr(hex);\n+               if (val < 0)\n                        return -1;\n                *sha1++ = val;\n                hex += 2;\n---8<---\n\nAnd for the first + line (int val = hex2chr(hex);) we need\nto hash that line and 2 (made up threshold) before and after:\n\n---8<---\n        int i;\n        for (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n               int val = hex2chr(hex);\n               if (val < 0)\n                        return -1;\n---8<---\n\nand this is hard to reconstruct from the above diff fed into the\ncallback. So we have to do it in the xdl emit phase.\n\nSo I think the wrong part of the initial patch 2/2 is the too\nlate recording of the moved lines.\nSo rather whenever we call diff_flush_patch(), we have to first\ncall diff_prepare_moved_line_detection() first for all file pairs.\n\n>\n>    The fn_out_consume() function working in the \"second pass of\n>    moved from/moved to mode\" would inspect line[] and see if it is\n>    an added or a removed line, and then:\n>\n>    - if it is an added line, and it appears as a removed line\n>      elsewhere in the patchset (you obtained the information in the\n>      first pass), you show it as \"this was moved from elsewhere\".\n>\n>    - if it is a removed line, and it appears as an added line\n>      elsewhere in the patchset (you obtained the information in the\n>      first pass), you show it as \"this was moved to elsewhere\".\n>\n> Or something like that.\n"},{"id":"300979","messageId":"CA+P7+xp=s4uOvqTS0wuduE0WmxOsWo_EYT5zW0U_2gfcHT7mGg@mail.gmail.com","threadId":"43993","inReplyTo":"20160903033120.20511-1-sbeller@google.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2016-09-04T09:57:59Z","receivedAt":"2016-09-04T09:58:37Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Fri, Sep 2, 2016 at 8:31 PM, Stefan Beller <sbeller@google.com> wrote:\n> When moving code (e.g. a function is moved to another part of the file or\n> to a different file), the review process is different than reviewing new\n> code. When reviewing moved code we are only interested in the diff as\n> where there are differences in the moved code, e.g. namespace changes.\n>\n> However the inner part of these moved texts should not change.\n> To aid a developer reviewing such code, we'll color pure moved stuff\n> differently.\n>\n> A line is colored differently if that line and the surroundign 2 lines\n> appear as-is in the opposite part of the diff.\n>\n> Example:\n> http://i.imgur.com/ay84q0q.png\n>\n\nIn the example, the first and last lines of duplicate copies don't get\ncolored differently, and that threw  me off. I feel like that was\nmaybe not intentional? If it was, can you explain why?\n\nThanks,\nJake\n"},{"id":"300980","messageId":"2001e1f2-7ba5-6084-292b-aa97f9b99c90@gmail.com","threadId":"43993","inReplyTo":"CAGZ79kYEieYGFAgORc8yaF3=8-L1E7K4afNGxDH5AgM5nHFgFw@mail.gmail.com","subject":"Re: [RFC/PATCH 2/2] WIP xdiff: markup duplicates differently","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-09-04T10:35:14Z","receivedAt":"2016-09-04T10:51:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 04.09.2016 o 07:31, Stefan Beller pisze:\n> On Sat, Sep 3, 2016 at 5:25 AM, Jakub Narębski <jnareb@gmail.com> wrote:\n>> W dniu 03.09.2016 o 05:31, Stefan Beller pisze:\n>>\n>>> When moving code (e.g. a function is moved to another part of the file or\n>>> to a different file), the review process is different than reviewing new\n>>> code. When reviewing moved code we are only interested in the diff as\n>>> where there are differences in the moved code, e.g. namespace changes.\n>>>\n>>> However the inner part of these moved texts should not change.\n>>> To aid a developer reviewing such code, emit it with a different prefix\n>>> than the usual +,- to indicate it is overlapping code.\n>>\n>> What would be this different prefix?\n> \n> I will discard the part of the different prefix as the design of 2/2\n> will change.\n\nIt would be nice to have at least an option of using different prefix\n(or pair of prefixes), as not always it is possible to use color to\nmarkup duplicates.\n\nP.S. BTW. does this work with word-diff?\n\nBest regards,\n-- \nJakub Narębski\n\n"},{"id":"301076","messageId":"xmqq60qbqpdd.fsf@gitster.mtv.corp.google.com","threadId":"43993","inReplyTo":"xmqqpookqi8k.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC/PATCH 0/2] Color moved code differently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-04T22:19:58Z","receivedAt":"2016-09-04T22:20:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I do not think you should step outside diff_flush().  Only when\n> producing textual diff, you would have to run the textual diff\n> twice by going over the q twice:\n>\n>  * The first pass would run diff_flush_patch(), which would call\n>    into xdiff the usual way, but the callback from xdiff would\n>    capture the removed lines and the added lines without making any\n>    output.\n>\n>  * The second pass would run diff_flush_patch(), but the callback\n>    from xdiff would be called with additional information, namely,\n>    the removed and the added lines captured in the first pass.\n> ...\n>    The fn_out_consume() function working in the \"second pass of\n>    moved from/moved to mode\" would inspect line[] and see if it is\n>    an added or a removed line, and then:\n>\n>    - if it is an added line, and it appears as a removed line\n>      elsewhere in the patchset (you obtained the information in the\n>      first pass), you show it as \"this was moved from elsewhere\".\n>\n>    - if it is a removed line, and it appears as an added line\n>      elsewhere in the patchset (you obtained the information in the\n>      first pass), you show it as \"this was moved to elsewhere\".\n>\n> Or something like that.\n\nActually, the first pass above ends up queuing majority of the diff\noutput in-core _anyway_, so it would make much more sense not to\nrely on the reproducibility of xdiff machinery by not calling it\ntwice.  Instead, I would imagine that the first pass can queue all\nwhat it receives in the callback, then with the whole-patch picture\nat hand, you can call updated fn_out_consume() yourself (instead of\nletting xdiff machinery to call you), and that would become the\n\"second pass\".\n"}]}