From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.2 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD, STOX_REPLY_TYPE shortcircuit=no autolearn=ham autolearn_force=no version=3.4.0 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by dcvr.yhbt.net (Postfix) with ESMTP id 909D9209FD for ; Sat, 27 May 2017 07:05:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753173AbdE0HFy (ORCPT ); Sat, 27 May 2017 03:05:54 -0400 Received: from smtp-out-1.talktalk.net ([62.24.135.65]:2239 "EHLO smtp-out-1.talktalk.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753117AbdE0HFw (ORCPT ); Sat, 27 May 2017 03:05:52 -0400 Received: from PhilipOakley ([92.31.218.76]) by smtp.talktalk.net with SMTP id EVnJdJyIq9tMzEVnJdZek8; Sat, 27 May 2017 08:05:50 +0100 X-Originating-IP: [92.31.218.76] X-Spam: 0 X-OAuthority: v=2.2 cv=WOE9ZTkR c=1 sm=1 tr=0 a=e6L6E7eW+5Nb7SO+DvSdIg==:117 a=e6L6E7eW+5Nb7SO+DvSdIg==:17 a=8nJEP1OIZ-IA:10 a=1XWaLZrsAAAA:8 a=ybZZDoGAAAAA:8 a=gAEL2DdC16zHvGqDEJAA:9 a=f2jNNSDlbtQvzKUZ:21 a=Knjbdk-_RCDhHdmm:21 a=wPNLvfGTeEIA:10 a=0RhZnL1DYvcuLYC8JZ5M:22 Message-ID: <39D67E58AC02490FB6D6124BDF3EE2A9@PhilipOakley> Reply-To: "Philip Oakley" From: "Philip Oakley" To: "Stefan Beller" , Cc: , , , , , , "Stefan Beller" References: <20170527001820.25214-1-sbeller@google.com> <20170527001820.25214-2-sbeller@google.com> Subject: Re: [PATCH 1/1] diff.c: color moved lines differently Date: Sat, 27 May 2017 08:05:48 +0100 Organization: OPDS MIME-Version: 1.0 Content-Type: text/plain; format=flowed; charset="iso-8859-1"; reply-type=original Content-Transfer-Encoding: 7bit X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2900.5931 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2900.6157 X-CMAE-Envelope: MS4wfH2VmEctqhMgZ4EOelMnVL3id2HGEm9kcj6lItC+IcO74/uIaibXGCFUb1XyUz8p0hus1/A6fLdsTzp7nDqNW3vhOZb4+qrR1dZIlqiWPLzHuz/s64sc 8+1XLVujxsYoAOeDKiUuSfJbEUuYRaqYOhHzqQwZgFsYuHO1PzAHYs7S2yCxlU/HD6g9mMfTqG8jylQLxFW+q8fjoYB4ZMlYauFmbe7XGrxM37o9lc6cMUMQ vUlEZ2yrnwOwnhpuqyEd1aRVHJWeyoqkJtLxGip5QJM01DxngqdxzohXOQDFWJ0E0vSXf8i0p76rvLz52XK4p9QJc/y7KpyOXgpi60WL7TRTUGOBKDjqofQp 2x4BN7gGCE7DylyD3qAcDzsSJaceWq4dMxLCYT+jhNVhHOXuM1Y= Sender: git-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org a couple of mispellings in the doc parts: s/on location/one location/ [code not checked] ----- Original Message ----- From: "Stefan Beller" Subject: [PATCH 1/1] diff.c: color moved lines differently > When a patch consists mostly of moving blocks of code around, it can > be quite tedious to ensure that the blocks are moved verbatim, and not > undesirably modified in the move. To that end, color blocks that are > moved within the same patch differently. For example (OM, del, add, > and NM are different colors): > > [OM] -void sensitive_stuff(void) > [OM] -{ > [OM] - if (!is_authorized_user()) > [OM] - die("unauthorized"); > [OM] - sensitive_stuff(spanning, > [OM] - multiple, > [OM] - lines); > [OM] -} > > void another_function() > { > [del] - printf("foo"); > [add] + printf("bar"); > } > > [NM] +void sensitive_stuff(void) > [NM] +{ > [NM] + if (!is_authorized_user()) > [NM] + die("unauthorized"); > [NM] + sensitive_stuff(spanning, > [NM] + multiple, > [NM] + lines); > [NM] +} > > However adjacent blocks may be problematic. For example, in this > potentially malicious patch, the swapping of blocks can be spotted: > > [OM] -void sensitive_stuff(void) > [OM] -{ > [OMA] - if (!is_authorized_user()) > [OMA] - die("unauthorized"); > [OM] - sensitive_stuff(spanning, > [OM] - multiple, > [OM] - lines); > [OMA] -} > > void another_function() > { > [del] - printf("foo"); > [add] + printf("bar"); > } > > [NM] +void sensitive_stuff(void) > [NM] +{ > [NMA] + sensitive_stuff(spanning, > [NMA] + multiple, > [NMA] + lines); > [NM] + if (!is_authorized_user()) > [NM] + die("unauthorized"); > [NMA] +} > > If the moved code is larger, it is easier to hide some permutation in the > code, which is why some alternative coloring is needed. > > As the reviewers attention should be brought to the places, where the > difference is introduced to the moved code, we cannot just have one new > color for all of moved code. > > First I implemented an alternative design, which would try to fingerprint > a line by its neighbors to detect if we are in a block or at the boundary. > This idea iss error prone as it inspected each line and its neighboring > lines to determine if the line was (a) moved and (b) if was deep inside > a hunk by having matching neighboring lines. This is unreliable as the > we can construct hunks which have equal neighbors that just exceed the > number of lines inspected. (Think of 'AXYZBXYZCXYZD..' with each letter > as a line, that is permutated to AXYZCXYZBXYZD..'). > > Instead this provides a dynamic programming greedy algorithm that finds > the largest moved hunk and then has several modes on highlighting bounds. > > A note on the options '--submodule=diff' and '--color-words/--word-diff': > In the conversion to use emit_line in the prior patches both submodules > as well as word diff output carefully chose to call emit_line with sign=0. > All output with sign=0 is ignored for move detection purposes in this > patch, such that no weird looking output will be generated for these > cases. This leads to another thought: We could pass on '--color-moved' to > submodules such that they color up moved lines for themselves. If we'd do > so only line moves within a repository boundary are marked up. > > Helped-by: Jonathan Tan > Signed-off-by: Stefan Beller > Signed-off-by: Junio C Hamano > --- > Documentation/config.txt | 10 +- > Documentation/diff-options.txt | 32 ++++ > color.h | 2 + > diff.c | 342 +++++++++++++++++++++++++++++++++++-- > diff.h | 15 +- > t/t4015-diff-whitespace.sh | 373 > +++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 760 insertions(+), 14 deletions(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 475e874d51..73511a4603 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -1051,14 +1051,20 @@ This does not affect linkgit:git-format-patch[1] > or the > 'git-diff-{asterisk}' plumbing commands. Can be overridden on the > command line with the `--color[=]` option. > > +diff.colorMoved:: > + If set moved lines in a diff are colored differently, > + for details see '--color-moved' in linkgit:git-diff[1]. > + > color.diff.:: > Use customized color for diff colorization. `` specifies > which part of the patch to use the specified color, and is one > of `context` (context text - `plain` is a historical synonym), > `meta` (metainformation), `frag` > (hunk header), 'func' (function in hunk header), `old` (removed lines), > - `new` (added lines), `commit` (commit headers), or `whitespace` > - (highlighting whitespace errors). > + `new` (added lines), `commit` (commit headers), `whitespace` > + (highlighting whitespace errors), `oldMoved`, `newMoved`, > + `oldMovedAlternative` and `newMovedAlternative` (See the '' > + setting of '--color-moved' in linkgit:git-diff[1] for details). > > color.decorate.:: > Use customized color for 'git log --decorate' output. `` is one > diff --git a/Documentation/diff-options.txt > b/Documentation/diff-options.txt > index 89cc0f48de..25259dbbc3 100644 > --- a/Documentation/diff-options.txt > +++ b/Documentation/diff-options.txt > @@ -231,6 +231,38 @@ ifdef::git-diff[] > endif::git-diff[] > It is the same as `--color=never`. > > +--color-moved[=]:: > + Moved lines of code are colored differently. > +ifdef::git-diff[] > + It can be changed by the `diff.colorMoved` configuration setting. > +endif::git-diff[] > + The defaults to 'no' if the option is not given > + and to 'adjacentbounds' if the option with no mode is given. > + The mode must be one of: > ++ > +-- > +no:: > + Moved lines are not highlighted. > +nobounds:: > + Any line that is added in on location and was removed s/on location/one location/ > + in another location will be colored with 'color.diff.newmoved'. > + Any line that is removed in on location and was added s/on location/one location/ > + in another location will be colored with 'color.diff.oldmoved'. > +allbounds:: > + Based on 'nobounds'. Additionally blocks of moved code are > + detected and the first and last line of a block will be highlighted > + using 'color.diff.newMovedAlternate' or > + 'color.diff.oldMovedAlternate'. > +adjacentbounds:: > + The same as 'allbounds' except that highlighting is only performed > + at adjacent block boundaries of blocks that have the same sign. > +alternate:: > + Based on 'nobounds'. Additionally blocks of moved code are > + detected. If moved blocks are adjacent mark one of them with the > + alternative move color using 'color.diff.newMovedAlternate' or > + 'color.diff.oldMovedAlternate'. > +-- > + > --word-diff[=]:: > Show a word diff, using the to delimit changed words. > By default, words are delimited by whitespace; see -- Philip > diff --git a/color.h b/color.h > index 90627650fc..04b3b87929 100644 > --- a/color.h > +++ b/color.h > @@ -42,6 +42,8 @@ struct strbuf; > #define GIT_COLOR_BG_BLUE "\033[44m" > #define GIT_COLOR_BG_MAGENTA "\033[45m" > #define GIT_COLOR_BG_CYAN "\033[46m" > +#define GIT_COLOR_DI_IT_CYAN "\033[2;3;36m" > +#define GIT_COLOR_DI_IT_MAGENTA "\033[2;3;35m" > > /* A special value meaning "no color selected" */ > #define GIT_COLOR_NIL "NIL" > diff --git a/diff.c b/diff.c > index a3c16ef827..efd2530a89 100644 > --- a/diff.c > +++ b/diff.c > @@ -31,6 +31,7 @@ static int diff_indent_heuristic; /* experimental */ > static int diff_rename_limit_default = 400; > static int diff_suppress_blank_empty; > static int diff_use_color_default = -1; > +static int diff_color_moved_default; > static int diff_context_default = 3; > static int diff_interhunk_context_default; > static const char *diff_word_regex_cfg; > @@ -55,6 +56,10 @@ static char diff_colors[][COLOR_MAXLEN] = { > GIT_COLOR_YELLOW, /* COMMIT */ > GIT_COLOR_BG_RED, /* WHITESPACE */ > GIT_COLOR_NORMAL, /* FUNCINFO */ > + GIT_COLOR_DI_IT_MAGENTA,/* OLD_MOVED */ > + GIT_COLOR_BG_RED, /* OLD_MOVED ALTERNATIVE */ > + GIT_COLOR_DI_IT_CYAN, /* NEW_MOVED */ > + GIT_COLOR_BG_GREEN, /* NEW_MOVED ALTERNATIVE */ > }; > > static NORETURN void die_want_option(const char *option_name) > @@ -80,6 +85,14 @@ static int parse_diff_color_slot(const char *var) > return DIFF_WHITESPACE; > if (!strcasecmp(var, "func")) > return DIFF_FUNCINFO; > + if (!strcasecmp(var, "oldmoved")) > + return DIFF_FILE_OLD_MOVED; > + if (!strcasecmp(var, "oldmovedalternative")) > + return DIFF_FILE_OLD_MOVED_ALT; > + if (!strcasecmp(var, "newmoved")) > + return DIFF_FILE_NEW_MOVED; > + if (!strcasecmp(var, "newmovedalternative")) > + return DIFF_FILE_NEW_MOVED_ALT; > return -1; > } > > @@ -228,12 +241,35 @@ int git_diff_heuristic_config(const char *var, const > char *value, void *cb) > return 0; > } > > +static int parse_color_moved(const char *arg) > +{ > + if (!strcmp(arg, "no")) > + return MOVED_LINES_NO; > + else if (!strcmp(arg, "nobounds")) > + return MOVED_LINES_BOUNDARY_NO; > + else if (!strcmp(arg, "allbounds")) > + return MOVED_LINES_BOUNDARY_ALL; > + else if (!strcmp(arg, "adjacentbounds")) > + return MOVED_LINES_BOUNDARY_ADJACENT; > + else if (!strcmp(arg, "alternate")) > + return MOVED_LINES_ALTERNATE; > + else > + return -1; > +} > + > int git_diff_ui_config(const char *var, const char *value, void *cb) > { > if (!strcmp(var, "diff.color") || !strcmp(var, "color.diff")) { > diff_use_color_default = git_config_colorbool(var, value); > return 0; > } > + if (!strcmp(var, "diff.colormoved")) { > + int cm = parse_color_moved(value); > + if (cm < 0) > + return -1; > + diff_color_moved_default = cm; > + return 0; > + } > if (!strcmp(var, "diff.context")) { > diff_context_default = git_config_int(var, value); > if (diff_context_default < 0) > @@ -354,6 +390,88 @@ int git_diff_basic_config(const char *var, const char > *value, void *cb) > return git_default_config(var, value, cb); > } > > +struct moved_entry { > + struct hashmap_entry ent; > + const struct diff_line *line; > + struct moved_entry *next_line; > +}; > + > +static void get_ws_cleaned_string(const struct diff_line *l, > + struct strbuf *out) > +{ > + int i; > + for (i = 0; i < l->len; i++) { > + if (isspace(l->line[i])) > + continue; > + strbuf_addch(out, l->line[i]); > + } > +} > + > +static int diff_line_cmp_no_ws(const struct diff_line *a, > + const struct diff_line *b, > + const void *keydata) > +{ > + int ret; > + struct strbuf sba = STRBUF_INIT; > + struct strbuf sbb = STRBUF_INIT; > + > + get_ws_cleaned_string(a, &sba); > + get_ws_cleaned_string(b, &sbb); > + ret = sba.len != sbb.len || strncmp(sba.buf, sbb.buf, sba.len); > + > + strbuf_release(&sba); > + strbuf_release(&sbb); > + return ret; > +} > + > +static int diff_line_cmp(const struct diff_line *a, > + const struct diff_line *b, > + const void *keydata) > +{ > + return a->len != b->len || strncmp(a->line, b->line, a->len); > +} > + > +static int moved_entry_cmp(const struct moved_entry *a, > + const struct moved_entry *b, > + const void *keydata) > +{ > + return diff_line_cmp(a->line, b->line, keydata); > +} > + > +static int moved_entry_cmp_no_ws(const struct moved_entry *a, > + const struct moved_entry *b, > + const void *keydata) > +{ > + return diff_line_cmp_no_ws(a->line, b->line, keydata); > +} > + > +static unsigned get_line_hash(struct diff_line *line, unsigned ignore_ws) > +{ > + static struct strbuf sb = STRBUF_INIT; > + > + if (ignore_ws) { > + strbuf_reset(&sb); > + get_ws_cleaned_string(line, &sb); > + return memhash(sb.buf, sb.len); > + } else { > + return memhash(line->line, line->len); > + } > +} > + > +static struct moved_entry *prepare_entry(struct diff_options *o, > + int line_no) > +{ > + struct moved_entry *ret = xmalloc(sizeof(*ret)); > + unsigned ignore_ws = DIFF_XDL_TST(o, IGNORE_WHITESPACE); > + struct diff_line *l = &o->line_buffer[line_no]; > + > + ret->ent.hash = get_line_hash(l, ignore_ws); > + ret->line = l; > + ret->next_line = NULL; > + > + return ret; > +} > + > static char *quote_two(const char *one, const char *two) > { > int need_one = quote_c_style(one, NULL, NULL, 1); > @@ -516,6 +634,179 @@ static void check_blank_at_eof(mmfile_t *mf1, > mmfile_t *mf2, > ecbdata->blank_at_eof_in_postimage = (at - l2) + 1; > } > > +static void add_lines_to_move_detection(struct diff_options *o, > + struct hashmap *add_lines, > + struct hashmap *del_lines) > +{ > + struct moved_entry *prev_line = NULL; > + > + int n; > + for (n = 0; n < o->line_buffer_nr; n++) { > + int sign = 0; > + struct hashmap *hm; > + struct moved_entry *key; > + > + switch (o->line_buffer[n].sign) { > + case '+': > + sign = '+'; > + hm = add_lines; > + break; > + case '-': > + sign = '-'; > + hm = del_lines; > + break; > + case ' ': > + default: > + prev_line = NULL; > + continue; > + } > + > + key = prepare_entry(o, n); > + if (prev_line && > + prev_line->line->sign == sign) > + prev_line->next_line = key; > + > + hashmap_add(hm, key); > + prev_line = key; > + } > +} > + > +static void mark_color_as_moved_single_line(struct diff_options *o, > + struct diff_line *l, int alt_color) > +{ > + switch (l->sign) { > + case '+': > + l->set = diff_get_color_opt(o, > + DIFF_FILE_NEW_MOVED + alt_color); > + break; > + case '-': > + l->set = diff_get_color_opt(o, > + DIFF_FILE_OLD_MOVED + alt_color); > + break; > + default: > + die("BUG: we should have continued earlier?"); > + } > +} > + > +static void mark_color_as_moved(struct diff_options *o, > + struct hashmap *add_lines, > + struct hashmap *del_lines) > +{ > + struct moved_entry **pmb = NULL; /* potentially moved blocks */ > + struct diff_line *prev_line = NULL; > + int pmb_nr = 0, pmb_alloc = 0; > + int n, flipped_block = 0; > + > + for (n = 0; n < o->line_buffer_nr; n++) { > + struct hashmap *hm = NULL; > + struct moved_entry *key; > + struct moved_entry *match = NULL; > + struct diff_line *l = &o->line_buffer[n]; > + int i, lp, rp, adjacent_blocks = 0; > + > + /* Check for any match to color it as a move. */ > + switch (l->sign) { > + case '+': > + hm = del_lines; > + key = prepare_entry(o, n); > + match = hashmap_get(hm, key, o); > + free(key); > + break; > + case '-': > + hm = add_lines; > + key = prepare_entry(o, n); > + match = hashmap_get(hm, key, o); > + free(key); > + break; > + default: ; > + } > + > + if (!match) { > + pmb_nr = 0; > + if (prev_line && > + o->color_moved == MOVED_LINES_BOUNDARY_ALL) > + mark_color_as_moved_single_line(o, prev_line, 1); > + prev_line = NULL; > + continue; > + } > + > + if (o->color_moved == MOVED_LINES_BOUNDARY_NO) { > + mark_color_as_moved_single_line(o, l, 0); > + continue; > + } > + > + /* Check any potential block runs, advance each or nullify */ > + for (i = 0; i < pmb_nr; i++) { > + struct moved_entry *p = pmb[i]; > + struct moved_entry *pnext = (p && p->next_line) ? > + p->next_line : NULL; > + if (pnext && > + !diff_line_cmp(pnext->line, l, o)) { > + pmb[i] = p->next_line; > + } else { > + pmb[i] = NULL; > + } > + } > + > + /* Shrink the set of potential block to the remaining running */ > + for (lp = 0, rp = pmb_nr - 1; lp <= rp;) { > + while (lp < pmb_nr && pmb[lp]) > + lp++; > + /* lp points at the first NULL now */ > + > + while (rp > -1 && !pmb[rp]) > + rp--; > + /* rp points at the last non-NULL */ > + > + if (lp < pmb_nr && rp > -1 && lp < rp) { > + pmb[lp] = pmb[rp]; > + pmb[rp] = NULL; > + rp--; > + lp++; > + } > + } > + > + /* Remember the number of running sets */ > + pmb_nr = rp + 1; > + > + if (pmb_nr == 0) { > + /* > + * This line is the start of a new block. > + * Setup the set of potential blocks. > + */ > + for (; match; match = hashmap_get_next(hm, match)) { > + ALLOC_GROW(pmb, pmb_nr + 1, pmb_alloc); > + pmb[pmb_nr++] = match; > + } > + > + if (o->color_moved == MOVED_LINES_BOUNDARY_ALL) { > + adjacent_blocks = 1; > + } else { > + /* Check if two blocks are adjacent */ > + adjacent_blocks = prev_line && > + prev_line->sign == l->sign; > + } > + } > + > + if (o->color_moved == MOVED_LINES_ALTERNATE) { > + if (adjacent_blocks) > + flipped_block = (flipped_block + 1) % 2; > + mark_color_as_moved_single_line(o, l, flipped_block); > + } else { > + /* MOVED_LINES_BOUNDARY_{ADJACENT, ALL} */ > + mark_color_as_moved_single_line(o, l, adjacent_blocks); > + if (adjacent_blocks && prev_line) > + prev_line->set = l->set; > + } > + > + prev_line = l; > + } > + if (prev_line && o->color_moved == MOVED_LINES_BOUNDARY_ALL) > + mark_color_as_moved_single_line(o, prev_line, 1); > + > + free(pmb); > +} > + > static void emit_diff_line(struct diff_options *o, > struct diff_line *e) > { > @@ -3518,6 +3809,8 @@ void diff_setup(struct diff_options *options) > options->line_buffer = NULL; > options->line_buffer_nr = 0; > options->line_buffer_alloc = 0; > + > + options->color_moved = diff_color_moved_default; > } > > void diff_setup_done(struct diff_options *options) > @@ -3627,6 +3920,9 @@ void diff_setup_done(struct diff_options *options) > > if (DIFF_OPT_TST(options, FOLLOW_RENAMES) && options->pathspec.nr != 1) > die(_("--follow requires exactly one pathspec")); > + > + if (!options->use_color || external_diff()) > + options->color_moved = 0; > } > > static int opt_arg(const char *arg, int arg_short, const char *arg_long, > int *val) > @@ -4051,7 +4347,19 @@ int diff_opt_parse(struct diff_options *options, > } > else if (!strcmp(arg, "--no-color")) > options->use_color = 0; > - else if (!strcmp(arg, "--color-words")) { > + else if (!strcmp(arg, "--color-moved")) > + if (diff_color_moved_default) > + options->color_moved = diff_color_moved_default; > + else > + options->color_moved = MOVED_LINES_BOUNDARY_ADJACENT; > + else if (!strcmp(arg, "--no-color-moved")) > + options->color_moved = MOVED_LINES_NO; > + else if (skip_prefix(arg, "--color-moved=", &arg)) { > + int cm = parse_color_moved(arg); > + if (cm < 0) > + die("bad --color-moved argument: %s", arg); > + options->color_moved = cm; > + } else if (!strcmp(arg, "--color-words")) { > options->use_color = 1; > options->word_diff = DIFF_WORDS_COLOR; > } > @@ -4856,16 +5164,9 @@ static void diff_flush_patch_all_file_pairs(struct > diff_options *o) > { > int i; > struct diff_queue_struct *q = &diff_queued_diff; > - /* > - * For testing purposes we want to make sure the diff machinery > - * works completely with the buffer. If there is anything emitted > - * outside the emit_diff_line, then the order is screwed > - * up and the tests will fail. > - * > - * TODO (later in this series): > - * We'll unset this flag in a later patch. > - */ > - o->use_buffer = 1; > + > + if (o->color_moved) > + o->use_buffer = 1; > > for (i = 0; i < q->nr; i++) { > struct diff_filepair *p = q->queue[i]; > @@ -4874,6 +5175,24 @@ static void diff_flush_patch_all_file_pairs(struct > diff_options *o) > } > > if (o->use_buffer) { > + if (o->color_moved) { > + struct hashmap add_lines, del_lines; > + unsigned ignore_ws = DIFF_XDL_TST(o, IGNORE_WHITESPACE); > + > + hashmap_init(&del_lines, ignore_ws ? > + (hashmap_cmp_fn)moved_entry_cmp_no_ws : > + (hashmap_cmp_fn)moved_entry_cmp, 0); > + hashmap_init(&add_lines, ignore_ws ? > + (hashmap_cmp_fn)moved_entry_cmp_no_ws : > + (hashmap_cmp_fn)moved_entry_cmp, 0); > + > + add_lines_to_move_detection(o, &add_lines, &del_lines); > + mark_color_as_moved(o, &add_lines, &del_lines); > + > + hashmap_free(&add_lines, 0); > + hashmap_free(&del_lines, 0); > + } > + > for (i = 0; i < o->line_buffer_nr; i++) > emit_diff_line(o, &o->line_buffer[i]); > > @@ -4962,6 +5281,7 @@ void diff_flush(struct diff_options *options) > if (!options->file) > die_errno("Could not open /dev/null"); > options->close_file = 1; > + options->color_moved = 0; > for (i = 0; i < q->nr; i++) { > struct diff_filepair *p = q->queue[i]; > if (check_pair_status(p)) > diff --git a/diff.h b/diff.h > index be51e8f867..d9fbafd383 100644 > --- a/diff.h > +++ b/diff.h > @@ -7,6 +7,7 @@ > #include "tree-walk.h" > #include "pathspec.h" > #include "object.h" > +#include "hashmap.h" > > struct rev_info; > struct diff_options; > @@ -228,6 +229,14 @@ struct diff_options { > > struct diff_line *line_buffer; > int line_buffer_nr, line_buffer_alloc; > + > + enum { > + MOVED_LINES_NO = 0, > + MOVED_LINES_BOUNDARY_NO = 1, > + MOVED_LINES_BOUNDARY_ALL = 2, > + MOVED_LINES_BOUNDARY_ADJACENT = 3, > + MOVED_LINES_ALTERNATE = 4, > + } color_moved; > }; > > /* Emit [line_prefix] [set] line [reset] */ > @@ -243,7 +252,11 @@ enum color_diff { > DIFF_FILE_NEW = 5, > DIFF_COMMIT = 6, > DIFF_WHITESPACE = 7, > - DIFF_FUNCINFO = 8 > + DIFF_FUNCINFO = 8, > + DIFF_FILE_OLD_MOVED = 9, > + DIFF_FILE_OLD_MOVED_ALT = 10, > + DIFF_FILE_NEW_MOVED = 11, > + DIFF_FILE_NEW_MOVED_ALT = 12 > }; > const char *diff_get_color(int diff_use_color, enum color_diff ix); > #define diff_get_color_opt(o, ix) \ > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > index 289806d0c7..d4bd082af7 100755 > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -972,4 +972,377 @@ test_expect_success 'option overrides > diff.wsErrorHighlight' ' > > ' > > +test_expect_success 'detect moved code, complete file' ' > + git reset --hard && > + cat <<-\EOF >test.c && > + #include > + main() > + { > + printf("Hello World"); > + } > + EOF > + git add test.c && > + git commit -m "add main function" && > + git mv test.c main.c && > + test_config color.diff.oldMoved "normal red" && > + test_config color.diff.newMoved "normal green" && > + git diff HEAD --color-moved --no-renames | test_decode_color >actual && > + cat >expected <<-\EOF && > + diff --git a/main.c b/main.c > + new file mode 100644 > + index 0000000..a986c57 > + --- /dev/null > + +++ b/main.c > + @@ -0,0 +1,5 @@ > + +#include > + +main() > + +{ > + +printf("Hello World"); > + +} > + diff --git a/test.c b/test.c > + deleted file mode 100644 > + index a986c57..0000000 > + --- a/test.c > + +++ /dev/null > + @@ -1,5 +0,0 @@ > + -#include > + -main() > + -{ > + -printf("Hello World"); > + -} > + EOF > + > + test_cmp expected actual > +' > + > +test_expect_success 'detect moved code, inside file' ' > + git reset --hard && > + cat <<-\EOF >main.c && > + #include > + int stuff() > + { > + printf("Hello "); > + printf("World\n"); > + } > + > + int secure_foo(struct user *u) > + { > + if (!u->is_allowed_foo) > + return; > + foo(u); > + } > + > + int main() > + { > + foo(); > + } > + EOF > + cat <<-\EOF >test.c && > + #include > + int bar() > + { > + printf("Hello World, but different\n"); > + } > + > + int another_function() > + { > + bar(); > + } > + EOF > + git add main.c test.c && > + git commit -m "add main and test file" && > + cat <<-\EOF >main.c && > + #include > + int stuff() > + { > + printf("Hello "); > + printf("World\n"); > + } > + > + int main() > + { > + foo(); > + } > + EOF > + cat <<-\EOF >test.c && > + #include > + int bar() > + { > + printf("Hello World, but different\n"); > + } > + > + int secure_foo(struct user *u) > + { > + if (!u->is_allowed_foo) > + return; > + foo(u); > + } > + > + int another_function() > + { > + bar(); > + } > + EOF > + test_config color.diff.oldMoved "normal red" && > + test_config color.diff.newMoved "normal green" && > + test_config color.diff.oldMovedAlternative "bold red" && > + test_config color.diff.newMovedAlternative "bold green" && > + git diff HEAD --no-renames --color-moved| test_decode_color >actual && > + cat <<-\EOF >expected && > + diff --git a/main.c b/main.c > + index 27a619c..7cf9336 100644 > + --- a/main.c > + +++ b/main.c > + @@ -5,13 +5,6 @@ printf("Hello "); > + printf("World\n"); > + } > + > + -int secure_foo(struct user *u) > + -{ > + -if (!u->is_allowed_foo) > + -return; > + -foo(u); > + -} > + - > + int main() > + { > + foo(); > + diff --git a/test.c b/test.c > + index 1dc1d85..e34eb69 100644 > + --- a/test.c > + +++ b/test.c > + @@ -4,6 +4,13 @@ int bar() > + printf("Hello World, but different\n"); > + } > + > + +int secure_foo(struct user *u) > + +{ > + +if (!u->is_allowed_foo) > + +return; > + +foo(u); > + +} > + + > + int another_function() > + { > + bar(); > + EOF > + > + test_cmp expected actual > +' > + > +test_expect_success 'detect permutations inside moved code' ' > + git reset --hard && > + cat <<-\EOF >lines.txt && > + line 1 > + line 2 > + line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + line 10 > + line 11 > + line 12 > + line 13 > + line 14 > + line 15 > + line 16 > + EOF > + git add lines.txt && > + git commit -m "add poetry" && > + cat <<-\EOF >lines.txt && > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + line 1 > + line 2 > + line 3 > + line 14 > + line 15 > + line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + EOF > + test_config color.diff.oldMoved "magenta" && > + test_config color.diff.newMoved "cyan" && > + test_config color.diff.oldMovedAlternative "blue" && > + test_config color.diff.newMovedAlternative "yellow" && > + > + > + git diff HEAD --no-renames --color-moved=nobounds| test_decode_color > >actual && > + cat <<-\EOF >expected && > + diff --git a/lines.txt b/lines.txt > + index 47ea9c3..ba96a38 100644 > + --- a/lines.txt > + +++ b/lines.txt > + @@ -1,16 +1,16 @@ > + -line 1 > + -line 2 > + -line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + +line 1 > + +line 2 > + +line 3 > + +line 14 > + +line 15 > + +line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + -line 14 > + -line 15 > + -line 16 > + EOF > + test_cmp expected actual && > + > + git diff HEAD --no-renames --color-moved=adjacentbounds| > test_decode_color >actual && > + cat <<-\EOF >expected && > + diff --git a/lines.txt b/lines.txt > + index 47ea9c3..ba96a38 100644 > + --- a/lines.txt > + +++ b/lines.txt > + @@ -1,16 +1,16 @@ > + -line 1 > + -line 2 > + -line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + +line 1 > + +line 2 > + +line 3 > + +line 14 > + +line 15 > + +line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + -line 14 > + -line 15 > + -line 16 > + EOF > + test_cmp expected actual && > + > + test_config diff.colorMoved alternate && > + git diff HEAD --no-renames --color-moved| test_decode_color >actual && > + cat <<-\EOF >expected && > + diff --git a/lines.txt b/lines.txt > + index 47ea9c3..ba96a38 100644 > + --- a/lines.txt > + +++ b/lines.txt > + @@ -1,16 +1,16 @@ > + -line 1 > + -line 2 > + -line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + +line 1 > + +line 2 > + +line 3 > + +line 14 > + +line 15 > + +line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + -line 14 > + -line 15 > + -line 16 > + EOF > + test_cmp expected actual && > + > + test_config diff.colorMoved allbounds && > + git diff HEAD --no-renames --color-moved| test_decode_color >actual && > + cat <<-\EOF >expected && > + diff --git a/lines.txt b/lines.txt > + index 47ea9c3..ba96a38 100644 > + --- a/lines.txt > + +++ b/lines.txt > + @@ -1,16 +1,16 @@ > + -line 1 > + -line 2 > + -line 3 > + line 4 > + line 5 > + line 6 > + line 7 > + line 8 > + line 9 > + +line 1 > + +line 2 > + +line 3 > + +line 14 > + +line 15 > + +line 16 > + line 10 > + line 11 > + line 12 > + line 13 > + -line 14 > + -line 15 > + -line 16 > + EOF > + test_cmp expected actual > +' > + > +test_expect_success 'move detection does not mess up colored words' ' > + cat <<-\EOF >text.txt && > + Lorem Ipsum is simply dummy text of the printing and typesetting > industry. > + EOF > + git add text.txt && > + git commit -a -m "clean state" && > + cat <<-\EOF >text.txt && > + simply Lorem Ipsum dummy is text of the typesetting and printing > industry. > + EOF > + git diff --color-moved --word-diff >actual && > + git diff --word-diff >expect && > + test_cmp expect actual > +' > + > +test_expect_success 'move detection with submodules' ' > + test_create_repo bananas && > + echo ripe >bananas/recipe && > + git -C bananas add recipe && > + test_commit fruit && > + test_commit -C bananas recipe && > + git submodule add ./bananas && > + git add bananas && > + git commit -a -m "bananas are like a heavy library?" && > + echo foul >bananas/recipe && > + echo ripe >fruit.t && > + > + git diff --submodule=diff --color-moved >actual && > + > + # no move detection as the moved line is across repository boundaries. > + test_decode_color decoded_actual && > + ! grep BGREEN decoded_actual && > + ! grep BRED decoded_actual && > + > + # nor did we mess with it another way > + git diff --submodule=diff | test_decode_color >expect && > + test_cmp expect decoded_actual > +' > + > test_done > -- > 2.13.0.17.gab62347cd9 >