Re: [PATCHv4] diff.c: emit moved lines with a different color

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 06/09/16 08:01, Stefan Beller wrote:
[snip]
> This patch was motivated by e.g. reviewing 3b0c4200 ("apply: move
> libified code from builtin/apply.c to apply.{c,h}", 2016-08-08)
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>  * moved new data structures into struct diff_options
>  * color.moved=bool as well as --[no-]color-moved to {dis,en}able the new feature
>  * color.diff.movedfrom and color.diff.movedto to control the colors
>  * added a test
>  
>  Documentation/config.txt               |  12 +-
>  Documentation/diff-options.txt         |   7 ++
>  contrib/completion/git-completion.bash |   2 +
>  diff.c                                 | 211 +++++++++++++++++++++++++++++++--
>  diff.h                                 |  16 ++-
>  t/t4015-diff-whitespace.sh             |  44 +++++++
>  6 files changed, 282 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 0bcb679..5daf77a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -974,14 +974,22 @@ 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[=<when>]` option.
>  
> +color.moved::
> +	A boolean value, whether a diff should color moved lines
> +	differently. The moved lines are searched for in the diff only.
> +	Duplicated lines from somewhere in the project that are not
> +	part of the diff are not colored as moved.
> +	Defaults to true.
> +
>  color.diff.<slot>::
>  	Use customized color for diff colorization.  `<slot>` 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), `movedfrom` (removed lines that
> +	reappear), `movedto` (added lines that were removed elsewhere).
>  
>  color.decorate.<slot>::
>  	Use customized color for 'git log --decorate' output.  `<slot>` is one
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 705a873..13b6a2a 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -234,6 +234,13 @@ ifdef::git-diff[]
>  endif::git-diff[]
>  	It is the same as `--color=never`.
>  
> +--[no-]color-moved::
> +	Show moved blocks in a different color.
> +ifdef::git-diff[]
> +	It can be changed by the `diff.ui` and `color.diff`
> +	configuration settings.
> +endif::git-diff[]
> +
>  --word-diff[=<mode>]::
>  	Show a word diff, using the <mode> to delimit changed words.
>  	By default, words are delimited by whitespace; see
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 9c8f738..9827c2e 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2115,6 +2115,8 @@ _git_config ()
>  		color.diff.old
>  		color.diff.plain
>  		color.diff.whitespace
> +		color.diff.movedfrom
> +		color.diff.movedto
>  		color.grep
>  		color.grep.context
>  		color.grep.filename
> diff --git a/diff.c b/diff.c
> index 534c12e..47685f3 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -18,6 +18,7 @@
>  #include "ll-merge.h"
>  #include "string-list.h"
>  #include "argv-array.h"
> +#include "git-compat-util.h"
>  
>  #ifdef NO_FAST_WORKING_DIRECTORY
>  #define FAST_WORKING_DIRECTORY 0
> @@ -30,6 +31,7 @@ static int diff_compaction_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 = -1;
>  static int diff_context_default = 3;
>  static const char *diff_word_regex_cfg;
>  static const char *external_diff_cmd_cfg;
> @@ -52,6 +54,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_YELLOW,	/* COMMIT */
>  	GIT_COLOR_BG_RED,	/* WHITESPACE */
>  	GIT_COLOR_NORMAL,	/* FUNCINFO */
> +	GIT_COLOR_BLUE,		/* MOVED FROM */
> +	GIT_COLOR_MAGENTA,	/* MOVED TO */
>  };
>  
>  static int parse_diff_color_slot(const char *var)
> @@ -72,6 +76,10 @@ static int parse_diff_color_slot(const char *var)
>  		return DIFF_WHITESPACE;
>  	if (!strcasecmp(var, "func"))
>  		return DIFF_FUNCINFO;
> +	if (!strcasecmp(var, "movedfrom"))
> +		return DIFF_FILE_MOVED_FROM;
> +	if (!strcasecmp(var, "movedto"))
> +		return DIFF_FILE_MOVED_TO;
>  	return -1;
>  }
>  
> @@ -180,6 +188,10 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
>  		diff_use_color_default = git_config_colorbool(var, value);
>  		return 0;
>  	}
> +	if (!strcmp(var, "color.moved")) {
> +		diff_color_moved_default = git_config_bool(var, value);
> +		return 0;
> +	}
>  	if (!strcmp(var, "diff.context")) {
>  		diff_context_default = git_config_int(var, value);
>  		if (diff_context_default < 0)
> @@ -287,6 +299,26 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
>  	return git_default_config(var, value, cb);
>  }
>  
> +static int diff_line_moved_entry_cmp(const struct diff_line_moved_entry *a,
> +				     const struct diff_line_moved_entry *b,
> +				     const void *unused)
> +{
> +	return strcmp(a->line, b->line) &&
> +	       a->hash_prev_line == b->hash_prev_line;

I doubt it would make much difference, but my knee-jerk reaction to
this was to suggest swapping the order of the expression, thus:

	return a->hash_prev_line == b->hash_prev_line &&
		strcmp(a->line, b->line);

... but perhaps it doesn't read quite so well, and probably wouldn't affect
performance much (except in strange edge cases), so it may not be worth it.

ATB,
Ramsay Jones