Merge branch 'jt/diff-color-move-fix' into next
authorJunio C Hamano <gitster@pobox.com>
Sun, 20 Aug 2017 06:07:18 +0000 (23:07 -0700)
committerJunio C Hamano <gitster@pobox.com>
Sun, 20 Aug 2017 06:07:18 +0000 (23:07 -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
index b1ab96477b5c251f2de9ddf99749745d6a3e73a7,b8c881605c195687df5b51a442465f3de8b0d1d3..a88c76741e781f5888fd467e17d02c633c87f15d
@@@ -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
index f84346b479a6f576c0a185629d110c185709c5c0,c50fcc7ea5c8bc2e28a976332bfe1d3e48338ad5..30e430b38dbd44ccef2a071ff1ee5648d2ca2a6f
--- 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);
  }
  
@@@ -858,6 -861,38 +858,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);
  }
@@@ -2814,7 -2848,7 +2845,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;