Re: [PATCH v4 6/9] diff.c: add a blocks mode for moved code detection

[Brandon Williams <bmwill@google.com>]

On 06/28, Stefan Beller wrote:
> The new "blocks" mode provides a middle ground between plain and zebra.
> It is as intuitive (few colors) as plain, but still has the requirement
> for a minimum of lines/characters to count a block as moved.
> 
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>  (https://public-inbox.org/git/87o9j0uljo.fsf@evledraar.gmail.com/)
> Signed-off-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/diff-options.txt |  8 ++++--
>  diff.c                         |  6 +++--
>  diff.h                         |  5 ++--
>  t/t4015-diff-whitespace.sh     | 49 ++++++++++++++++++++++++++++++++--
>  4 files changed, 60 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e3a44f03cdc..ba56169de31 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -276,10 +276,14 @@ plain::
>  	that are added somewhere else in the diff. This mode picks up any
>  	moved line, but it is not very useful in a review to determine
>  	if a block of code was moved without permutation.
> -zebra::
> +blocks::
>  	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
> +	painted using either the 'color.diff.{old,new}Moved' color.
> +	Adjacent blocks cannot be told apart.
> +zebra::
> +	Blocks of moved text are detected as in 'blocks' mode. The 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.
>  dimmed_zebra::
> diff --git a/diff.c b/diff.c
> index d1bae900cdc..95c51c0b7df 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -271,6 +271,8 @@ static int parse_color_moved(const char *arg)
>  		return COLOR_MOVED_NO;
>  	else if (!strcmp(arg, "plain"))
>  		return COLOR_MOVED_PLAIN;
> +	else if (!strcmp(arg, "blocks"))
> +		return COLOR_MOVED_BLOCKS;
>  	else if (!strcmp(arg, "zebra"))
>  		return COLOR_MOVED_ZEBRA;
>  	else if (!strcmp(arg, "default"))
> @@ -278,7 +280,7 @@ static int parse_color_moved(const char *arg)
>  	else if (!strcmp(arg, "dimmed_zebra"))
>  		return COLOR_MOVED_ZEBRA_DIM;
>  	else
> -		return error(_("color moved setting must be one of 'no', 'default', 'zebra', 'dimmed_zebra', 'plain'"));
> +		return error(_("color moved setting must be one of 'no', 'default', 'blocks', 'zebra', 'dimmed_zebra', 'plain'"));
>  }
>  
>  int git_diff_ui_config(const char *var, const char *value, void *cb)
> @@ -903,7 +905,7 @@ static void mark_color_as_moved(struct diff_options *o,
>  
>  		block_length++;
>  
> -		if (flipped_block)
> +		if (flipped_block && o->color_moved != COLOR_MOVED_BLOCKS)
>  			l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
>  	}
>  	adjust_last_block(o, n, block_length);
> diff --git a/diff.h b/diff.h
> index d29560f822c..7bd4f182c33 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -208,8 +208,9 @@ struct diff_options {
>  	enum {
>  		COLOR_MOVED_NO = 0,
>  		COLOR_MOVED_PLAIN = 1,
> -		COLOR_MOVED_ZEBRA = 2,
> -		COLOR_MOVED_ZEBRA_DIM = 3,
> +		COLOR_MOVED_BLOCKS = 2,
> +		COLOR_MOVED_ZEBRA = 3,
> +		COLOR_MOVED_ZEBRA_DIM = 4,
>  	} color_moved;
>  	#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
>  	#define COLOR_MOVED_MIN_ALNUM_COUNT 20
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index ddbc3901385..e54529f026d 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -1223,7 +1223,7 @@ test_expect_success 'plain moved code, inside file' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
> +test_expect_success 'detect blocks of moved code' '
>  	git reset --hard &&
>  	cat <<-\EOF >lines.txt &&
>  		long line 1
> @@ -1271,6 +1271,50 @@ test_expect_success 'detect permutations inside moved code -- dimmed_zebra' '
>  	test_config color.diff.newMovedDimmed "normal cyan" &&
>  	test_config color.diff.oldMovedAlternativeDimmed "normal blue" &&
>  	test_config color.diff.newMovedAlternativeDimmed "normal yellow" &&
> +	git diff HEAD --no-renames --color-moved=blocks --color >actual.raw &&
> +	grep -v "index" actual.raw | test_decode_color >actual &&
> +	cat <<-\EOF >expected &&
> +	<BOLD>diff --git a/lines.txt b/lines.txt<RESET>
> +	<BOLD>--- a/lines.txt<RESET>
> +	<BOLD>+++ b/lines.txt<RESET>
> +	<CYAN>@@ -1,16 +1,16 @@<RESET>
> +	<MAGENTA>-long line 1<RESET>
> +	<MAGENTA>-long line 2<RESET>
> +	<MAGENTA>-long line 3<RESET>
> +	 line 4<RESET>
> +	 line 5<RESET>
> +	 line 6<RESET>
> +	 line 7<RESET>
> +	 line 8<RESET>
> +	 line 9<RESET>
> +	<CYAN>+<RESET><CYAN>long line 1<RESET>
> +	<CYAN>+<RESET><CYAN>long line 2<RESET>
> +	<CYAN>+<RESET><CYAN>long line 3<RESET>
> +	<CYAN>+<RESET><CYAN>long line 14<RESET>
> +	<CYAN>+<RESET><CYAN>long line 15<RESET>
> +	<CYAN>+<RESET><CYAN>long line 16<RESET>
> +	 line 10<RESET>
> +	 line 11<RESET>
> +	 line 12<RESET>
> +	 line 13<RESET>
> +	<MAGENTA>-long line 14<RESET>
> +	<MAGENTA>-long line 15<RESET>
> +	<MAGENTA>-long line 16<RESET>
> +	EOF
> +	test_cmp expected actual
> +

Theres an empty line here.  Not worth fixing if its the only issue
though.

-- 
Brandon Williams