Merge branch 'jt/diff-color-move-fix'
authorJunio C Hamano <gitster@pobox.com>
Sun, 27 Aug 2017 05:55:04 +0000 (22:55 -0700)
committerJunio C Hamano <gitster@pobox.com>
Sun, 27 Aug 2017 05:55:04 +0000 (22:55 -0700)
A handful of bugfixes and an improvement to "diff --color-moved".

* jt/diff-color-move-fix:
  diff: define block by number of alphanumeric chars
  diff: respect MIN_BLOCK_LENGTH for last block
  diff: avoid redundantly clearing a flag

1  2 
Documentation/diff-options.txt
diff.c

@@@ -254,13 -254,11 +254,11 @@@ plain:
        moved line, but it is not very useful in a review to determine
        if a block of code was moved without permutation.
  zebra::
-       Blocks of moved code are detected greedily. The detected blocks are
+       Blocks of moved text of at least 20 alphanumeric characters
+       are detected greedily. The detected blocks are
        painted using either the 'color.diff.{old,new}Moved' color or
        'color.diff.{old,new}MovedAlternative'. The change between
-       the two colors indicates that a new block was detected. If there
-       are fewer than 3 adjacent moved lines, they are not marked up
-       as moved, but the regular colors 'color.diff.{old,new}' will be
-       used.
+       the two colors indicates that a new block was detected.
  dimmed_zebra::
        Similar to 'zebra', but additional dimming of uninteresting parts
        of moved code is performed. The bordering lines of two adjacent
@@@ -336,14 -334,15 +334,14 @@@ ifndef::git-format-patch[
        with --exit-code.
  
  --ws-error-highlight=<kind>::
 -      Highlight whitespace errors on lines specified by <kind>
 -      in the color specified by `color.diff.whitespace`.  <kind>
 -      is a comma separated list of `old`, `new`, `context`.  When
 -      this option is not given, only whitespace errors in `new`
 -      lines are highlighted.  E.g. `--ws-error-highlight=new,old`
 -      highlights whitespace errors on both deleted and added lines.
 -      `all` can be used as a short-hand for `old,new,context`.
 -      The `diff.wsErrorHighlight` configuration variable can be
 -      used to specify the default behaviour.
 +      Highlight whitespace errors in the `context`, `old` or `new`
 +      lines of the diff.  Multiple values are separated by comma,
 +      `none` resets previous values, `default` reset the list to
 +      `new` and `all` is a shorthand for `old,new,context`.  When
 +      this option is not given, and the configuration variable
 +      `diff.wsErrorHighlight` is not set, only whitespace errors in
 +      `new` lines are highlighted. The whitespace errors are colored
 +      whith `color.diff.whitespace`.
  
  endif::git-format-patch[]
  
@@@ -427,7 -426,7 +425,7 @@@ endif::git-log[
        the diff between the preimage and `/dev/null`. The resulting patch
        is not meant to be applied with `patch` or `git apply`; this is
        solely for people who want to just concentrate on reviewing the
 -      text after the change. In addition, the output obviously lack
 +      text after the change. In addition, the output obviously lacks
        enough information to apply such a patch in reverse, even manually,
        hence the name of the option.
  +
diff --combined diff.c
--- 1/diff.c
--- 2/diff.c
+++ b/diff.c
@@@ -357,6 -357,9 +357,6 @@@ int git_diff_ui_config(const char *var
                return 0;
        }
  
 -      if (git_color_config(var, value, cb) < 0)
 -              return -1;
 -
        return git_diff_basic_config(var, value, cb);
  }
  
@@@ -464,6 -467,8 +464,6 @@@ static struct diff_tempfile 
        struct tempfile tempfile;
  } diff_temp[2];
  
 -typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
 -
  struct emit_callback {
        int color_diff;
        unsigned ws_rule;
        int blank_at_eof_in_postimage;
        int lno_in_preimage;
        int lno_in_postimage;
 -      sane_truncate_fn truncate;
        const char **label_path;
        struct diff_words_data *diff_words;
        struct diff_options *opt;
@@@ -855,6 -861,38 +855,38 @@@ static int shrink_potential_moved_block
        return rp + 1;
  }
  
+ /*
+  * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
+  *
+  * Otherwise, if the last block has fewer alphanumeric characters than
+  * COLOR_MOVED_MIN_ALNUM_COUNT, unset DIFF_SYMBOL_MOVED_LINE on all lines in
+  * that block.
+  *
+  * The last block consists of the (n - block_length)'th line up to but not
+  * including the nth line.
+  *
+  * NEEDSWORK: This uses the same heuristic as blame_entry_score() in blame.c.
+  * Think of a way to unify them.
+  */
+ static void adjust_last_block(struct diff_options *o, int n, int block_length)
+ {
+       int i, alnum_count = 0;
+       if (o->color_moved == COLOR_MOVED_PLAIN)
+               return;
+       for (i = 1; i < block_length + 1; i++) {
+               const char *c = o->emitted_symbols->buf[n - i].line;
+               for (; *c; c++) {
+                       if (!isalnum(*c))
+                               continue;
+                       alnum_count++;
+                       if (alnum_count >= COLOR_MOVED_MIN_ALNUM_COUNT)
+                               return;
+               }
+       }
+       for (i = 1; i < block_length + 1; i++)
+               o->emitted_symbols->buf[n - i].flags &= ~DIFF_SYMBOL_MOVED_LINE;
+ }
  /* Find blocks of moved code, delegate actual coloring decision to helper */
  static void mark_color_as_moved(struct diff_options *o,
                                struct hashmap *add_lines,
                }
  
                if (!match) {
-                       if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH &&
-                           o->color_moved != COLOR_MOVED_PLAIN) {
-                               for (i = 0; i < block_length + 1; i++) {
-                                       l = &o->emitted_symbols->buf[n - i];
-                                       l->flags &= ~DIFF_SYMBOL_MOVED_LINE;
-                               }
-                       }
+                       adjust_last_block(o, n, block_length);
                        pmb_nr = 0;
                        block_length = 0;
                        continue;
                }
  
                l->flags |= DIFF_SYMBOL_MOVED_LINE;
-               block_length++;
  
                if (o->color_moved == COLOR_MOVED_PLAIN)
                        continue;
                        }
  
                        flipped_block = (flipped_block + 1) % 2;
+                       adjust_last_block(o, n, block_length);
+                       block_length = 0;
                }
  
+               block_length++;
                if (flipped_block)
                        l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
        }
+       adjust_last_block(o, n, block_length);
  
        free(pmb);
  }
@@@ -1965,6 -2002,8 +1996,6 @@@ static unsigned long sane_truncate_line
        unsigned long allot;
        size_t l = len;
  
 -      if (ecb->truncate)
 -              return ecb->truncate(line, len);
        cp = line;
        allot = l;
        while (0 < l) {
@@@ -2809,7 -2848,7 +2840,7 @@@ static void show_dirstat_by_line(struc
                         * bytes per "line".
                         * This is stupid and ugly, but very cheap...
                         */
 -                      damage = (damage + 63) / 64;
 +                      damage = DIV_ROUND_UP(damage, 64);
                ALLOC_GROW(dir.files, dir.nr + 1, dir.alloc);
                dir.files[dir.nr].name = file->name;
                dir.files[dir.nr].changed = damage;