Re: [RFC PATCH 2/3] diff: respect MIN_BLOCK_LENGTH for last block

[Stefan Beller <sbeller@google.com>]

On Fri, Aug 11, 2017 at 3:49 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> Currently, MIN_BLOCK_LENGTH is only checked when diff encounters a line
> that does not belong to the current block. In particular, this means
> that MIN_BLOCK_LENGTH is not checked after all lines are encountered.
>
> Perform that check.

Thanks for spotting! This fix looks straightforward correct.
(Also thanks for factoring out the adjustment, I am tempted to
start a bike shedding discussion about its name, though. :P)

> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  diff.c                     | 29 ++++++++++++++++++++++-------
>  t/t4015-diff-whitespace.sh | 35 +++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 7 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 4965ffbc4..95620b130 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -858,6 +858,26 @@ static int shrink_potential_moved_blocks(struct moved_entry **pmb,
>         return rp + 1;
>  }
>
> +/*
> + * If o->color_moved is COLOR_MOVED_PLAIN, this function does nothing.
> + *
> + * Otherwise, if the last block has fewer lines than
> + * COLOR_MOVED_MIN_BLOCK_LENGTH, 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.
> + */
> +static void adjust_last_block(struct diff_options *o, int n, int block_length)
> +{
> +       int i;
> +       if (block_length >= COLOR_MOVED_MIN_BLOCK_LENGTH ||
> +           o->color_moved == COLOR_MOVED_PLAIN)
> +               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,
> @@ -893,13 +913,7 @@ static void mark_color_as_moved(struct diff_options *o,
>                 }
>
>                 if (!match) {
> -                       if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH &&
> -                           o->color_moved != COLOR_MOVED_PLAIN) {
> -                               for (i = 1; 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;
> @@ -941,6 +955,7 @@ static void mark_color_as_moved(struct diff_options *o,
>                 if (flipped_block)
>                         l->flags |= DIFF_SYMBOL_MOVED_LINE_ALT;
>         }
> +       adjust_last_block(o, n, block_length);
>
>         free(pmb);
>  }
> diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
> index c3b697411..6f7758e5c 100755
> --- a/t/t4015-diff-whitespace.sh
> +++ b/t/t4015-diff-whitespace.sh
> @@ -1382,6 +1382,41 @@ EOF
>         test_cmp expected actual
>  '
>
> +test_expect_success '--color-moved block at end of diff output respects MIN_BLOCK_LENGTH' '
> +       git reset --hard &&
> +       >bar &&
> +       cat <<-\EOF >foo &&
> +       irrelevant_line
> +       line1
> +       EOF
> +       git add foo bar &&
> +       git commit -m x &&
> +
> +       cat <<-\EOF >bar &&
> +       line1
> +       EOF
> +       cat <<-\EOF >foo &&
> +       irrelevant_line
> +       EOF
> +
> +       git diff HEAD --color-moved=zebra --no-renames | grep -v "index" | test_decode_color >actual &&
> +       cat >expected <<-\EOF &&
> +       <BOLD>diff --git a/bar b/bar<RESET>
> +       <BOLD>--- a/bar<RESET>
> +       <BOLD>+++ b/bar<RESET>
> +       <CYAN>@@ -0,0 +1 @@<RESET>
> +       <GREEN>+<RESET><GREEN>line1<RESET>
> +       <BOLD>diff --git a/foo b/foo<RESET>
> +       <BOLD>--- a/foo<RESET>
> +       <BOLD>+++ b/foo<RESET>
> +       <CYAN>@@ -1,2 +1 @@<RESET>
> +        irrelevant_line<RESET>
> +       <RED>-line1<RESET>
> +       EOF
> +
> +       test_cmp expected actual
> +'
> +
>  test_expect_success 'move detection with submodules' '
>         test_create_repo bananas &&
>         echo ripe >bananas/recipe &&
> --
> 2.14.0.434.g98096fd7a8-goog
>