{"thread":{"id":"49030","subject":"[PATCH 0/7] improve range-diffs coloring and [RFC] move detection","startedAt":"2018-08-04T01:53:24Z","lastAt":"2018-08-06T20:19:02Z","messageCount":13,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"354464","messageId":"20180804015317.182683-1-sbeller@google.com","threadId":"49030","inReplyTo":null,"subject":"[PATCH 0/7] improve range-diffs coloring and [RFC] move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:10Z","receivedAt":"2018-08-04T01:53:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This builds on top of sb/range-diff-colors, which builds on js/range-diff.\nAs Thomas seemed happy with the range-diff and said\n\"and fwiw I didn't think there's anything major that needs to be addressed.\"\nI found it reasonable to build more on top of those series.\n[1] https://public-inbox.org/git/20180729215053.GE9955@hank.intra.tgummerer.com/\nIt can also be found via\n  git fetch https://github.com/stefanbeller/git moved-in-patches\n\nThis series provides for 2 goals, which share a common base refactoring:\n\nThe refactoring part is to allow the diff machinery to prefix\nold/new/context lines with prefixes as chosen by the user.\n\nThe first feature is to slightly adapt coloring of range-diff to have the\nfile markers\n\n  --- a/diff.c\n  +++ b/diff.c\n  \nbe shown as context color. This is done by using custom prefixes for\nold/new/context lines in the inner diffs and indenting all other lines\ninstead by a white space indicating it to be context.\n\nThe second feature is more RFC-ish in nature and just exposes the\nmechanism to the user in a meaning-ful way.\nAll of the diff family (including format-patch) as well as apply/am\ncan use */~ instead of +/- for moved lines of code.\n\nThanks,\nStefan\n\nStefan Beller (7):\n  diff.c: emit_line_0 to take string instead of first sign\n  diff.c: add --output-indicator-{new, old, context}\n  range-diff: make use of different output indicators\n  range-diff: indent special lines as context\n  diff.c: rename color_moved to markup_moved\n  diff.c: factor determine_line_color out of\n    emit_diff_symbol_from_struct\n  diff/am: enhance diff format to use */~ for moved lines\n\n apply.c               |  12 +++\n diff.c                | 180 ++++++++++++++++++++++++++----------------\n diff.h                |  10 ++-\n range-diff.c          |  17 +++-\n t/t3206-range-diff.sh |  12 +--\n 5 files changed, 153 insertions(+), 78 deletions(-)\n\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354465","messageId":"20180804015317.182683-2-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 1/7] diff.c: emit_line_0 to take string instead of first sign","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:11Z","receivedAt":"2018-08-04T01:53:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"By providing a string as the first part of the emission we can extend\nit later more easily.\n\nWhile at it, document emit_line_0.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff.c | 28 +++++++++++++++++-----------\n 1 file changed, 17 insertions(+), 11 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2bd4d3d6839..57a8a38755e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -575,9 +575,15 @@ static void check_blank_at_eof(mmfile_t *mf1, mmfile_t *mf2,\n \tecbdata->blank_at_eof_in_postimage = (at - l2) + 1;\n }\n \n+/*\n+ * Emits\n+ * <set_sign> <first> <reset> <set> <second> <reset> LF\n+ * if they are present. 'first' is a NULL terminated string,\n+ * 'second' is a buffer of length 'len'.\n+ */\n static void emit_line_0(struct diff_options *o,\n \t\t\tconst char *set_sign, const char *set, const char *reset,\n-\t\t\tint first, const char *line, int len)\n+\t\t\tconst char *first, const char *second, int len)\n {\n \tint has_trailing_newline, has_trailing_carriage_return;\n \tint reverse = !!set && !!set_sign;\n@@ -587,11 +593,11 @@ static void emit_line_0(struct diff_options *o,\n \n \tfputs(diff_line_prefix(o), file);\n \n-\thas_trailing_newline = (len > 0 && line[len-1] == '\\n');\n+\thas_trailing_newline = (len > 0 && second[len-1] == '\\n');\n \tif (has_trailing_newline)\n \t\tlen--;\n \n-\thas_trailing_carriage_return = (len > 0 && line[len-1] == '\\r');\n+\thas_trailing_carriage_return = (len > 0 && second[len-1] == '\\r');\n \tif (has_trailing_carriage_return)\n \t\tlen--;\n \n@@ -609,7 +615,7 @@ static void emit_line_0(struct diff_options *o,\n \t}\n \n \tif (first)\n-\t\tfputc(first, file);\n+\t\tfputs(first, file);\n \n \tif (!len)\n \t\tgoto end_of_line;\n@@ -620,7 +626,7 @@ static void emit_line_0(struct diff_options *o,\n \t\tfputs(set, file);\n \t\tneeds_reset = 1;\n \t}\n-\tfwrite(line, len, 1, file);\n+\tfwrite(second, len, 1, file);\n \tneeds_reset |= len > 0;\n \n end_of_line:\n@@ -635,7 +641,7 @@ static void emit_line_0(struct diff_options *o,\n static void emit_line(struct diff_options *o, const char *set, const char *reset,\n \t\t      const char *line, int len)\n {\n-\temit_line_0(o, set, NULL, reset, 0, line, len);\n+\temit_line_0(o, set, NULL, reset, NULL, line, len);\n }\n \n enum diff_symbol {\n@@ -996,7 +1002,7 @@ static void dim_moved_lines(struct diff_options *o)\n static void emit_line_ws_markup(struct diff_options *o,\n \t\t\t\tconst char *set_sign, const char *set,\n \t\t\t\tconst char *reset,\n-\t\t\t\tchar sign, const char *line, int len,\n+\t\t\t\tconst char *sign, const char *line, int len,\n \t\t\t\tunsigned ws_rule, int blank_at_eof)\n {\n \tconst char *ws = NULL;\n@@ -1039,7 +1045,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\tcontext = diff_get_color_opt(o, DIFF_CONTEXT);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tputc('\\n', o->file);\n-\t\temit_line_0(o, context, NULL, reset, '\\\\',\n+\t\temit_line_0(o, context, NULL, reset, \"\\\\\",\n \t\t\t    nneof, strlen(nneof));\n \t\tbreak;\n \tcase DIFF_SYMBOL_SUBMODULE_HEADER:\n@@ -1077,7 +1083,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse if (c == '-')\n \t\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, ' ', line, len,\n+\t\temit_line_ws_markup(o, set_sign, set, reset, \" \", line, len,\n \t\t\t\t    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_PLUS:\n@@ -1120,7 +1126,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n \t\t\tflags |= WS_IGNORE_FIRST_SPACE;\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, '+', line, len,\n+\t\temit_line_ws_markup(o, set_sign, set, reset, \"+\", line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n \t\tbreak;\n@@ -1163,7 +1169,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_DIM);\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, '-', line, len,\n+\t\temit_line_ws_markup(o, set_sign, set, reset, \"-\", line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_WORDS_PORCELAIN:\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354466","messageId":"20180804015317.182683-3-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 2/7] diff.c: add --output-indicator-{new, old, context}","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:12Z","receivedAt":"2018-08-04T01:53:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This will prove useful in range-diff in a later patch as we will be able to\ndifferentiate between adding a new file (that line is starting with +++\nand then the file name) and regular new lines.\n\nIt could also be useful for experimentation in new patch formats, i.e.\nwe could teach git to emit moved lines with lines other than +/-.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff.c | 21 +++++++++++++++++----\n diff.h |  5 +++++\n 2 files changed, 22 insertions(+), 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 57a8a38755e..2e711809700 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1032,7 +1032,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\t\t struct emitted_diff_symbol *eds)\n {\n \tstatic const char *nneof = \" No newline at end of file\\n\";\n-\tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo;\n+\tconst char *context, *reset, *set, *set_sign, *meta, *fraginfo, *first;\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tenum diff_symbol s = eds->s;\n@@ -1083,7 +1083,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse if (c == '-')\n \t\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, \" \", line, len,\n+\t\tfirst = o->output_indicators[OI_CONTEXT] ?\n+\t\t\to->output_indicators[OI_CONTEXT] : \" \";\n+\t\temit_line_ws_markup(o, set_sign, set, reset, first, line, len,\n \t\t\t\t    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_PLUS:\n@@ -1126,7 +1128,10 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_BOLD);\n \t\t\tflags |= WS_IGNORE_FIRST_SPACE;\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, \"+\", line, len,\n+\n+\t\tfirst = o->output_indicators[OI_NEW] ?\n+\t\t\to->output_indicators[OI_NEW] : \"+\";\n+\t\temit_line_ws_markup(o, set_sign, set, reset, first, line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n \t\tbreak;\n@@ -1169,7 +1174,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\telse\n \t\t\t\tset = diff_get_color_opt(o, DIFF_CONTEXT_DIM);\n \t\t}\n-\t\temit_line_ws_markup(o, set_sign, set, reset, \"-\", line, len,\n+\t\tfirst = o->output_indicators[OI_OLD] ?\n+\t\t\to->output_indicators[OI_OLD] : \"-\";\n+\t\temit_line_ws_markup(o, set_sign, set, reset, first, line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_WORDS_PORCELAIN:\n@@ -4670,6 +4677,12 @@ int diff_opt_parse(struct diff_options *options,\n \t\t options->output_format |= DIFF_FORMAT_DIFFSTAT;\n \t} else if (!strcmp(arg, \"--no-compact-summary\"))\n \t\t options->flags.stat_with_summary = 0;\n+\telse if (skip_prefix(arg, \"--output-indicator-new=\", &arg))\n+\t\toptions->output_indicators[OI_NEW] = arg;\n+\telse if (skip_prefix(arg, \"--output-indicator-old=\", &arg))\n+\t\toptions->output_indicators[OI_OLD] = arg;\n+\telse if (skip_prefix(arg, \"--output-indicator-context=\", &arg))\n+\t\toptions->output_indicators[OI_CONTEXT] = arg;\n \n \t/* renames options */\n \telse if (starts_with(arg, \"-B\") ||\ndiff --git a/diff.h b/diff.h\nindex a08a3b2a293..b8bbe7baeb8 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -194,6 +194,11 @@ struct diff_options {\n \tFILE *file;\n \tint close_file;\n \n+#define OI_NEW 0\n+#define OI_OLD 1\n+#define OI_CONTEXT 2\n+\tconst char *output_indicators[3];\n+\n \tstruct pathspec pathspec;\n \tpathchange_fn_t pathchange;\n \tchange_fn_t change;\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354467","messageId":"20180804015317.182683-4-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 3/7] range-diff: make use of different output indicators","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:13Z","receivedAt":"2018-08-04T01:53:31Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This change itself only changes the internal communication and should\nhave no visible effect to the user. We instruct the diff code that produces\nthe inner diffs to use X, Y, Z instead of the usual markers for new, old\nand context lines\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n range-diff.c | 15 ++++++++++++++-\n 1 file changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex 347b4a79f25..a4ff945427e 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -38,6 +38,9 @@ static int read_patches(const char *range, struct string_list *list)\n \n \targv_array_pushl(&cp.args, \"log\", \"--no-color\", \"-p\", \"--no-merges\",\n \t\t\t\"--reverse\", \"--date-order\", \"--decorate=no\",\n+\t\t\t\"--output-indicator-new=X\",\n+\t\t\t\"--output-indicator-old=Y\",\n+\t\t\t\"--output-indicator-context=Z\",\n \t\t\t\"--no-abbrev-commit\", range,\n \t\t\tNULL);\n \tcp.out = -1;\n@@ -108,8 +111,18 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\t * we are not interested.\n \t\t\t */\n \t\t\tcontinue;\n-\t\telse\n+\t\telse if (line.buf[0] == 'X') {\n+\t\t\tstrbuf_addch(&buf, '+');\n+\t\t\tstrbuf_add(&buf, line.buf + 1, line.len - 1);\n+\t\t} else if (line.buf[0] == 'Y') {\n+\t\t\tstrbuf_addch(&buf, '-');\n+\t\t\tstrbuf_add(&buf, line.buf + 1, line.len - 1);\n+\t\t} else if (line.buf[0] == 'Z') {\n+\t\t\tstrbuf_addch(&buf, ' ');\n+\t\t\tstrbuf_add(&buf, line.buf + 1, line.len - 1);\n+\t\t} else {\n \t\t\tstrbuf_addbuf(&buf, &line);\n+\t\t}\n \n \t\tstrbuf_addch(&buf, '\\n');\n \t\tutil->diffsize++;\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354468","messageId":"20180804015317.182683-5-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 4/7] range-diff: indent special lines as context","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:14Z","receivedAt":"2018-08-04T01:53:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The range-diff coloring is a bit fuzzy when it comes to special lines of\na diff, such as indicating new and old files with +++ and ---, as it\nwould pickup the first character and interpret it for its coloring, which\nseems annoying as in regular diffs, these lines are colored bold via\nDIFF_METAINFO.\n\nBy indenting these lines by a white space, they will be treated as context\nwhich is much more useful, an example [1] on the range diff series itself:\n\n[...]\n    + diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt\n    + new file mode 100644\n    + --- /dev/null\n    + +++ b/Documentation/git-range-diff.txt\n    +@@\n    ++git-range-diff(1)\n[...]\n    +\n      diff --git a/Makefile b/Makefile\n      --- a/Makefile\n      +++ b/Makefile\n[...]\n\nThe first lines that introduce the new file for the man page will have the\n'+' sign colored and the rest of the line will be bold.\n\nThe later lines that indicate a change to the Makefile will be treated as\ncontext both in the outer and inner diff, such that those lines stay\nregular color.\n\n[1] ./git-range-diff pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4\n    These tags are found at https://github.com/gitgitgadget/git\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n range-diff.c          |  2 ++\n t/t3206-range-diff.sh | 12 ++++++------\n 2 files changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/range-diff.c b/range-diff.c\nindex a4ff945427e..91d5f12180d 100644\n--- a/range-diff.c\n+++ b/range-diff.c\n@@ -85,6 +85,7 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (!util->diff_offset)\n \t\t\t\tutil->diff_offset = buf.len;\n+\t\t\tstrbuf_addch(&buf, ' ');\n \t\t\tstrbuf_addbuf(&buf, &line);\n \t\t} else if (in_header) {\n \t\t\tif (starts_with(line.buf, \"Author: \")) {\n@@ -121,6 +122,7 @@ static int read_patches(const char *range, struct string_list *list)\n \t\t\tstrbuf_addch(&buf, ' ');\n \t\t\tstrbuf_add(&buf, line.buf + 1, line.len - 1);\n \t\t} else {\n+\t\t\tstrbuf_addch(&buf, ' ');\n \t\t\tstrbuf_addbuf(&buf, &line);\n \t\t}\n \ndiff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh\nindex e3b0764b433..0cd23cbff41 100755\n--- a/t/t3206-range-diff.sh\n+++ b/t/t3206-range-diff.sh\n@@ -133,9 +133,9 @@ test_expect_success 'changed message' '\n \t    Z\n \t    +    Also a silly comment here!\n \t    +\n-\t    Zdiff --git a/file b/file\n-\t    Z--- a/file\n-\t    Z+++ b/file\n+\t    Z diff --git a/file b/file\n+\t    Z --- a/file\n+\t    Z +++ b/file\n \t3:  147e64e = 3:  b9cb956 s/11/B/\n \t4:  a63e992 = 4:  8add5f1 s/12/B/\n \tEOF\n@@ -152,9 +152,9 @@ test_expect_success 'dual-coloring' '\n \t:     <RESET>\n \t:    <REVERSE><GREEN>+<RESET> <BOLD>   Also a silly comment here!<RESET>\n \t:    <REVERSE><GREEN>+<RESET>\n-\t:     diff --git a/file b/file<RESET>\n-\t:    <RED> --- a/file<RESET>\n-\t:    <GREEN> +++ b/file<RESET>\n+\t:      diff --git a/file b/file<RESET>\n+\t:      --- a/file<RESET>\n+\t:      +++ b/file<RESET>\n \t:<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>\n \t:    <REVERSE><CYAN>@@ -10,7 +10,7 @@<RESET>\n \t:      9<RESET>\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354469","messageId":"20180804015317.182683-6-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 5/7] diff.c: rename color_moved to markup_moved","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:15Z","receivedAt":"2018-08-04T01:53:35Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This just renames a variable to make the next patch easier to review.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff.c | 28 ++++++++++++++--------------\n diff.h |  2 +-\n 2 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 2e711809700..d3829c7d086 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -821,7 +821,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n }\n \n /*\n- * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.\n+ * If o->markup_moved is COLOR_MOVED_PLAIN, this function does nothing.\n  *\n  * Otherwise, if the last block has fewer alphanumeric characters than\n  * COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in\n@@ -836,7 +836,7 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,\n static void adjust_last_block(struct diff_options *o, int n, int block_length)\n {\n \tint i, alnum_count = 0;\n-\tif (o->color_moved == COLOR_MOVED_PLAIN)\n+\tif (o->markup_moved == COLOR_MOVED_PLAIN)\n \t\treturn;\n \tfor (i = 1; i < block_length + 1; i++) {\n \t\tconst char *c = o->emitted_symbols->buf[n - i].line;\n@@ -895,7 +895,7 @@ static void mark_color_as_moved(struct diff_options *o,\n \n \t\tl->flags |= DIFF_SYMBOL_MOVED_LINE;\n \n-\t\tif (o->color_moved == COLOR_MOVED_PLAIN)\n+\t\tif (o->markup_moved == COLOR_MOVED_PLAIN)\n \t\t\tcontinue;\n \n \t\t/* Check any potential block runs, advance each or nullify */\n@@ -4220,7 +4220,7 @@ void diff_setup(struct diff_options *options)\n \t\toptions->b_prefix = \"b/\";\n \t}\n \n-\toptions->color_moved = diff_color_moved_default;\n+\toptions->markup_moved = diff_color_moved_default;\n }\n \n void diff_setup_done(struct diff_options *options)\n@@ -4333,7 +4333,7 @@ void diff_setup_done(struct diff_options *options)\n \t\tdie(_(\"--follow requires exactly one pathspec\"));\n \n \tif (!options->use_color || external_diff())\n-\t\toptions->color_moved = 0;\n+\t\toptions->markup_moved = 0;\n }\n \n static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *val)\n@@ -4796,16 +4796,16 @@ int diff_opt_parse(struct diff_options *options,\n \t\toptions->use_color = 0;\n \telse if (!strcmp(arg, \"--color-moved\")) {\n \t\tif (diff_color_moved_default)\n-\t\t\toptions->color_moved = diff_color_moved_default;\n-\t\tif (options->color_moved == COLOR_MOVED_NO)\n-\t\t\toptions->color_moved = COLOR_MOVED_DEFAULT;\n+\t\t\toptions->markup_moved = diff_color_moved_default;\n+\t\tif (options->markup_moved == COLOR_MOVED_NO)\n+\t\t\toptions->markup_moved = COLOR_MOVED_DEFAULT;\n \t} else if (!strcmp(arg, \"--no-color-moved\"))\n-\t\toptions->color_moved = COLOR_MOVED_NO;\n+\t\toptions->markup_moved = COLOR_MOVED_NO;\n \telse if (skip_prefix(arg, \"--color-moved=\", &arg)) {\n \t\tint cm = parse_color_moved(arg);\n \t\tif (cm < 0)\n \t\t\tdie(\"bad --color-moved argument: %s\", arg);\n-\t\toptions->color_moved = cm;\n+\t\toptions->markup_moved = cm;\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\n@@ -5623,7 +5623,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \tif (WSEH_NEW & WS_RULE_MASK)\n \t\tBUG(\"WS rules bit mask overlaps with diff symbol flags\");\n \n-\tif (o->color_moved)\n+\tif (o->markup_moved)\n \t\to->emitted_symbols = &esm;\n \n \tfor (i = 0; i < q->nr; i++) {\n@@ -5633,7 +5633,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \t}\n \n \tif (o->emitted_symbols) {\n-\t\tif (o->color_moved) {\n+\t\tif (o->markup_moved) {\n \t\t\tstruct hashmap add_lines, del_lines;\n \n \t\t\thashmap_init(&del_lines,\n@@ -5643,7 +5643,7 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \n \t\t\tadd_lines_to_move_detection(o, &add_lines, &del_lines);\n \t\t\tmark_color_as_moved(o, &add_lines, &del_lines);\n-\t\t\tif (o->color_moved == COLOR_MOVED_ZEBRA_DIM)\n+\t\t\tif (o->markup_moved == COLOR_MOVED_ZEBRA_DIM)\n \t\t\t\tdim_moved_lines(o);\n \n \t\t\thashmap_free(&add_lines, 0);\n@@ -5731,7 +5731,7 @@ void diff_flush(struct diff_options *options)\n \t\t\tfclose(options->file);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n \t\toptions->close_file = 1;\n-\t\toptions->color_moved = 0;\n+\t\toptions->markup_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))\ndiff --git a/diff.h b/diff.h\nindex b8bbe7baeb8..0dd1651dda4 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -217,7 +217,7 @@ struct diff_options {\n \t\tCOLOR_MOVED_PLAIN = 1,\n \t\tCOLOR_MOVED_ZEBRA = 2,\n \t\tCOLOR_MOVED_ZEBRA_DIM = 3,\n-\t} color_moved;\n+\t} markup_moved;\n \t#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA\n \t#define COLOR_MOVED_MIN_ALNUM_COUNT 20\n };\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354470","messageId":"20180804015317.182683-7-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[PATCH 6/7] diff.c: factor determine_line_color out of emit_diff_symbol_from_struct","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:16Z","receivedAt":"2018-08-04T01:53:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Factoring out code that determines color for both the new and old code\neven saves some lines, though minuscule. However we introduce an offset\ncomputation in the color array.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n diff.c | 88 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 44 insertions(+), 44 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d3829c7d086..56bab011df7 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1028,6 +1028,48 @@ static void emit_line_ws_markup(struct diff_options *o,\n \t}\n }\n \n+static const char *determine_line_color(struct diff_options *o,\n+\t\t\t\t\tstruct emitted_diff_symbol *eds)\n+{\n+\tconst char *set;\n+\tunsigned flags = eds->flags;\n+\n+\t/*\n+\t * To have these offsets work, we need to keep\n+\t * DIFF_FILE_OLD_MOVED{_ALT, _ALT_DIM, DIM, _}\n+\t * in the same order as their _NEW_ equivalents; we do not need\n+\t * to care about DIFF_FILE_{NEW, OLD} and their relations to others.\n+\t */\n+\tconst int off = (eds->s == DIFF_SYMBOL_PLUS) ?\n+\t\tDIFF_FILE_NEW_MOVED - DIFF_FILE_OLD_MOVED : 0;\n+\n+\tswitch (flags & (DIFF_SYMBOL_MOVED_LINE |\n+\t\t\t DIFF_SYMBOL_MOVED_LINE_ALT |\n+\t\t\t DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {\n+\tcase DIFF_SYMBOL_MOVED_LINE |\n+\t     DIFF_SYMBOL_MOVED_LINE_ALT |\n+\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n+\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT_DIM + off);\n+\t\tbreak;\n+\tcase DIFF_SYMBOL_MOVED_LINE |\n+\t     DIFF_SYMBOL_MOVED_LINE_ALT:\n+\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT + off);\n+\t\tbreak;\n+\tcase DIFF_SYMBOL_MOVED_LINE |\n+\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n+\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_DIM + off);\n+\t\tbreak;\n+\tcase DIFF_SYMBOL_MOVED_LINE:\n+\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED + off);\n+\t\tbreak;\n+\tdefault:\n+\t\tset = (eds->s == DIFF_SYMBOL_PLUS) ?\n+\t\t\tdiff_get_color_opt(o, DIFF_FILE_NEW):\n+\t\t\tdiff_get_color_opt(o, DIFF_FILE_OLD);\n+\t}\n+\treturn set;\n+}\n+\n static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\t\t struct emitted_diff_symbol *eds)\n {\n@@ -1089,28 +1131,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\t    flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);\n \t\tbreak;\n \tcase DIFF_SYMBOL_PLUS:\n-\t\tswitch (flags & (DIFF_SYMBOL_MOVED_LINE |\n-\t\t\t\t DIFF_SYMBOL_MOVED_LINE_ALT |\n-\t\t\t\t DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_ALT |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_ALT_DIM);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_ALT:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_ALT);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED_DIM);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_NEW_MOVED);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_NEW);\n-\t\t}\n+\t\tset = determine_line_color(o, eds);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tif (!o->flags.dual_color_diffed_diffs)\n \t\t\tset_sign = NULL;\n@@ -1136,28 +1157,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n \t\tbreak;\n \tcase DIFF_SYMBOL_MINUS:\n-\t\tswitch (flags & (DIFF_SYMBOL_MOVED_LINE |\n-\t\t\t\t DIFF_SYMBOL_MOVED_LINE_ALT |\n-\t\t\t\t DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_ALT |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT_DIM);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_ALT:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_ALT);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE |\n-\t\t     DIFF_SYMBOL_MOVED_LINE_UNINTERESTING:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED_DIM);\n-\t\t\tbreak;\n-\t\tcase DIFF_SYMBOL_MOVED_LINE:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tset = diff_get_color_opt(o, DIFF_FILE_OLD);\n-\t\t}\n+\t\tset = determine_line_color(o, eds);\n \t\treset = diff_get_color_opt(o, DIFF_RESET);\n \t\tif (!o->flags.dual_color_diffed_diffs)\n \t\t\tset_sign = NULL;\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354471","messageId":"20180804015317.182683-8-sbeller@google.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"[RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-04T01:53:17Z","receivedAt":"2018-08-04T01:53:40Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Try it out via\n    ./git-format-patch --mark-moved 15ef69314d^..15ef69314d\nto see if you like it.\n\nThis separates the coloring decision from the detection of moved lines.\nWhen giving --mark-moved, move detection is still performed and the output\nmarkers are adjusted to */~ for new and old code.\n\ngit-apply and git-am will also accept these patches by rewriting those\nsigns back to +/-.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n apply.c | 12 ++++++++++++\n diff.c  | 21 +++++++++++++++++++++\n diff.h  |  5 ++++-\n 3 files changed, 37 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 23a0f25ded8..cc42a4fa02a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2900,6 +2900,12 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\t    ws_blank_line(patch + 1, plen, ws_rule))\n \t\t\t\tis_blank_context = 1;\n \t\t\t/* fallthrough */\n+\t\tcase '~':\n+\t\t\t/*\n+\t\t\t * For now ignore moved line indicators and apply\n+\t\t\t * as a regular old line\n+\t\t\t */\n+\t\t\t/* fallthrough */\n \t\tcase '-':\n \t\t\tmemcpy(old, patch + 1, plen);\n \t\t\tadd_line_info(&preimage, old, plen,\n@@ -2908,6 +2914,12 @@ static int apply_one_fragment(struct apply_state *state,\n \t\t\tif (first == '-')\n \t\t\t\tbreak;\n \t\t\t/* fallthrough */\n+\t\tcase '*':\n+\t\t\t/*\n+\t\t\t * For now ignore moved line indicators and apply\n+\t\t\t * as a regular new line\n+\t\t\t */\n+\t\t\t/* fallthrough */\n \t\tcase '+':\n \t\t\t/* --no-add does not add new lines */\n \t\t\tif (first == '+' && state->no_add)\ndiff --git a/diff.c b/diff.c\nindex 56bab011df7..8e39e77229f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1043,6 +1043,9 @@ static const char *determine_line_color(struct diff_options *o,\n \tconst int off = (eds->s == DIFF_SYMBOL_PLUS) ?\n \t\tDIFF_FILE_NEW_MOVED - DIFF_FILE_OLD_MOVED : 0;\n \n+\tif (!o->color_moved)\n+\t\tgoto default_color;\n+\n \tswitch (flags & (DIFF_SYMBOL_MOVED_LINE |\n \t\t\t DIFF_SYMBOL_MOVED_LINE_ALT |\n \t\t\t DIFF_SYMBOL_MOVED_LINE_UNINTERESTING)) {\n@@ -1063,6 +1066,7 @@ static const char *determine_line_color(struct diff_options *o,\n \t\tset = diff_get_color_opt(o, DIFF_FILE_OLD_MOVED + off);\n \t\tbreak;\n \tdefault:\n+default_color:\n \t\tset = (eds->s == DIFF_SYMBOL_PLUS) ?\n \t\t\tdiff_get_color_opt(o, DIFF_FILE_NEW):\n \t\t\tdiff_get_color_opt(o, DIFF_FILE_OLD);\n@@ -1152,6 +1156,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \n \t\tfirst = o->output_indicators[OI_NEW] ?\n \t\t\to->output_indicators[OI_NEW] : \"+\";\n+\t\tif (o->output_indicators[OI_MOVED_NEW] &&\n+\t\t   (flags & DIFF_SYMBOL_MOVED_LINE))\n+\t\t\tfirst = \"*\";\n \t\temit_line_ws_markup(o, set_sign, set, reset, first, line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);\n@@ -1176,6 +1183,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \t\t}\n \t\tfirst = o->output_indicators[OI_OLD] ?\n \t\t\to->output_indicators[OI_OLD] : \"-\";\n+\t\tif (o->output_indicators[OI_MOVED_NEW] &&\n+\t\t   (flags & DIFF_SYMBOL_MOVED_LINE))\n+\t\t\tfirst = \"~\";\n \t\temit_line_ws_markup(o, set_sign, set, reset, first, line, len,\n \t\t\t\t    flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);\n \t\tbreak;\n@@ -4795,6 +4805,7 @@ int diff_opt_parse(struct diff_options *options,\n \telse if (!strcmp(arg, \"--no-color\"))\n \t\toptions->use_color = 0;\n \telse if (!strcmp(arg, \"--color-moved\")) {\n+\t\toptions->color_moved = 1;\n \t\tif (diff_color_moved_default)\n \t\t\toptions->markup_moved = diff_color_moved_default;\n \t\tif (options->markup_moved == COLOR_MOVED_NO)\n@@ -4806,6 +4817,16 @@ 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->markup_moved = cm;\n+\t\toptions->color_moved = 1;\n+\t} else if (skip_prefix(arg, \"--mark-moved\", &arg)) {\n+\t\t/*\n+\t\t * NEEDSWORK:\n+\t\t * Once merged with 51da15eb230 (diff.c: add a blocks mode for\n+\t\t * moved code detection, 2018-07-16), make it COLOR_MOVED_BLOCKS\n+\t\t */\n+\t\toptions->markup_moved = COLOR_MOVED_PLAIN;\n+\t\toptions->output_indicators[OI_MOVED_NEW] = \"*\";\n+\t\toptions->output_indicators[OI_MOVED_OLD] = \"~\";\n \t} else if (skip_to_optional_arg_default(arg, \"--color-words\", &options->word_regex, NULL)) {\n \t\toptions->use_color = 1;\n \t\toptions->word_diff = DIFF_WORDS_COLOR;\ndiff --git a/diff.h b/diff.h\nindex 0dd1651dda4..0058602c849 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -197,7 +197,9 @@ struct diff_options {\n #define OI_NEW 0\n #define OI_OLD 1\n #define OI_CONTEXT 2\n-\tconst char *output_indicators[3];\n+#define OI_MOVED_NEW 3\n+#define OI_MOVED_OLD 4\n+\tconst char *output_indicators[5];\n \n \tstruct pathspec pathspec;\n \tpathchange_fn_t pathchange;\n@@ -211,6 +213,7 @@ struct diff_options {\n \n \tint diff_path_counter;\n \n+\tunsigned color_moved : 1;\n \tstruct emitted_diff_symbols *emitted_symbols;\n \tenum {\n \t\tCOLOR_MOVED_NO = 0,\n-- \n2.18.0.597.ga71716f1ad-goog\n\n"},{"id":"354495","messageId":"xmqqd0uyt6hj.fsf@gitster-ct.c.googlers.com","threadId":"49030","inReplyTo":"20180804015317.182683-1-sbeller@google.com","subject":"Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-04T16:57:12Z","receivedAt":"2018-08-04T16:57:17Z","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> This builds on top of sb/range-diff-colors, which builds on js/range-diff.\n\nAs another round of js/range-diff is expected, according to\n\n<nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>\n\nI will refrain from queuing this right now.  Possible conflict\nresolution that won't be reusable when the base one is rerolled and\nthis and another topic that depend on the current round of\njs/range-diff are rebased on top is not something I can spend my\ntime on this week.\n"},{"id":"354496","messageId":"xmqqtvoarr3d.fsf@gitster-ct.c.googlers.com","threadId":"49030","inReplyTo":"20180804015317.182683-8-sbeller@google.com","subject":"Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-04T17:15:02Z","receivedAt":"2018-08-04T17:15:34Z","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> Try it out via\n>     ./git-format-patch --mark-moved 15ef69314d^..15ef69314d\n> to see if you like it.\n>\n> This separates the coloring decision from the detection of moved lines.\n> When giving --mark-moved, move detection is still performed and the output\n> markers are adjusted to */~ for new and old code.\n>\n> git-apply and git-am will also accept these patches by rewriting those\n> signs back to +/-.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nThis does not have anything to do with the range-diff topic, but\nwould stand on its own merit.  \n\nI have a mixed feeling about this.\n\nIf you need to convince \"GNU patch\" maintainers to accept these two\nsigns, then probably it is not worth the battle of incompatiblity.\nIf it is truly a worthy innovation, they would follow suit, which is\nhow they learned to take our renaming diffs without us prodding\nthem.  I just do not get the gut feeling that it would happen for\nthis particular thing, and I am not convinced myself enough to sell\nthis to \"patch\" maintainers and expect to be taken seriously.\n\nWhen reviewing anything complex that would be helped by moved code\nhighlighting, I do not think a normal person would choose to review\nsuch a change only inside MUA.  I certainly won't.  I'd rather apply\nthe patch and view it within a larger context than the piece of\ne-mail that was originally sent offers, with better tools like -W\nand --color-moved applied locally.  So in that sense, I do not think\nI'd appreciate lines that begin with '~'/'*' as different kind of\n'-'/'+', as helpful hints; at least until my eyes get used to them,\nthey would only appear as distraction.\n\nIn other words, I have this nagging suspicion that people who\nsuggested to you that this would help in e-mail workflow are\nmisguided and they do not understand e-mail workflow in the first\nplace, but perhaps it is just me.\n\nThanks.\n"},{"id":"354583","messageId":"CAGZ79kZnVEsvpicNu7LXkRcHuRqGvESfvG3DL5O_2kPVYrW-Gg@mail.gmail.com","threadId":"49030","inReplyTo":"xmqqd0uyt6hj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-06T06:01:31Z","receivedAt":"2018-08-06T06:01:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Aug 4, 2018 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > This builds on top of sb/range-diff-colors, which builds on js/range-diff.\n>\n> As another round of js/range-diff is expected, according to\n>\n> <nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>\n\nOh right. I forgot to mention that in this cover letter, but Johannes\nhas had work issues last week and people threw lots of stuff at him,\nso I concluded the resend might take a while. Hence I just put it out there\nin case we're happy with the status quo of that series.\n\n> I will refrain from queuing this right now.  Possible conflict\n> resolution that won't be reusable when the base one is rerolled and\n> this and another topic that depend on the current round of\n> js/range-diff are rebased on top is not something I can spend my\n> time on this week.\n\nThanks! I'll resend when appropriate.\nStefan\n"},{"id":"354584","messageId":"CAGZ79kYA4BNe1N+_S=vqwsn2wNH6qQz=kcgwc9Ufr8nfQT85rA@mail.gmail.com","threadId":"49030","inReplyTo":"xmqqtvoarr3d.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFC PATCH 7/7] diff/am: enhance diff format to use */~ for moved lines","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-08-06T06:07:25Z","receivedAt":"2018-08-06T06:07:39Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Aug 4, 2018 at 10:15 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > Try it out via\n> >     ./git-format-patch --mark-moved 15ef69314d^..15ef69314d\n> > to see if you like it.\n> >\n> > This separates the coloring decision from the detection of moved lines.\n> > When giving --mark-moved, move detection is still performed and the output\n> > markers are adjusted to */~ for new and old code.\n> >\n> > git-apply and git-am will also accept these patches by rewriting those\n> > signs back to +/-.\n> >\n> > Signed-off-by: Stefan Beller <sbeller@google.com>\n> > ---\n>\n> This does not have anything to do with the range-diff topic, but\n> would stand on its own merit.\n\nYes. I should have emphasized this more in the cover letter.\nThis is more a \"while at it\" thing, that is easy to do due to the\nrefactoring in previous patches.\n\n> I have a mixed feeling about this.\n\nMe, too.\n\n> If you need to convince \"GNU patch\" maintainers to accept these two\n> signs, then probably it is not worth the battle of incompatiblity.\n> If it is truly a worthy innovation, they would follow suit, which is\n> how they learned to take our renaming diffs without us prodding\n> them.  I just do not get the gut feeling that it would happen for\n> this particular thing, and I am not convinced myself enough to sell\n> this to \"patch\" maintainers and expect to be taken seriously.\n\nok.\n\n> When reviewing anything complex that would be helped by moved code\n> highlighting, I do not think a normal person would choose to review\n> such a change only inside MUA.  I certainly won't.  I'd rather apply\n> the patch and view it within a larger context than the piece of\n> e-mail that was originally sent offers, with better tools like -W\n> and --color-moved applied locally.  So in that sense, I do not think\n> I'd appreciate lines that begin with '~'/'*' as different kind of\n> '-'/'+', as helpful hints; at least until my eyes get used to them,\n> they would only appear as distraction.\n\nMy use case would be patches that are *not* complex, but still shuffling\nlots of code around, e.g. reordering functions/paragraphs in a file.\n\n> In other words, I have this nagging suspicion that people who\n> suggested to you that this would help in e-mail workflow are\n> misguided and they do not understand e-mail workflow in the first\n> place, but perhaps it is just me.\n\nThere are no other people that suggested this.\nIt was really just a quick shot \"while at it\" as we had the\nrefactoring in place that enables this, and I think for trivial\npatches (non-complex, but lots of changes) it *may* be beneficial.\nBut it is more for corner cases, I guess.\n\nStefan\n"},{"id":"354682","messageId":"xmqq36vrnt8t.fsf@gitster-ct.c.googlers.com","threadId":"49030","inReplyTo":"CAGZ79kZnVEsvpicNu7LXkRcHuRqGvESfvG3DL5O_2kPVYrW-Gg@mail.gmail.com","subject":"Re: [PATCH 0/7] improve range-diffs coloring and [RFC] move detection","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-06T20:18:58Z","receivedAt":"2018-08-06T20:19:02Z","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 Sat, Aug 4, 2018 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>> > This builds on top of sb/range-diff-colors, which builds on js/range-diff.\n>>\n>> As another round of js/range-diff is expected, according to\n>>\n>> <nycvar.QRO.7.76.6.1808011800570.71@tvgsbejvaqbjf.bet>\n>\n> Oh right. I forgot to mention that in this cover letter, but Johannes\n> has had work issues last week and people threw lots of stuff at him,\n> so I concluded the resend might take a while. Hence I just put it out there\n> in case we're happy with the status quo of that series.\n> ...\n>\n> Thanks! I'll resend when appropriate.\n\nThanks; that way I have one less thing I have to remember ;-)\n\n"}]}