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

[Stefan Beller]

On Fri, Aug 11, 2017 at 3:49 PM, Jonathan Tan  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 
> ---
>  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 = >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 &&
> +   diff --git a/bar b/bar
> +   --- a/bar
> +   +++ b/bar
> +   @@ -0,0 +1 @@
> +   +line1
> +   diff --git a/foo b/foo
> +   --- a/foo
> +   +++ b/foo
> +   @@ -1,2 +1 @@
> +irrelevant_line
> +   -line1
> +   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
>

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

[Jonathan Tan]

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.

Signed-off-by: Jonathan Tan 
---
 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 = >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 &&
+   diff --git a/bar b/bar
+   --- a/bar
+   +++ b/bar
+   @@ -0,0 +1 @@
+   +line1
+   diff --git a/foo b/foo
+   --- a/foo
+   +++ b/foo
+   @@ -1,2 +1 @@
+irrelevant_line
+   -line1
+   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