git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [RFC PATCH 2/3] diff: respect MIN_BLOCK_LENGTH for last block
Date: Mon, 14 Aug 2017 10:17:25 -0700
Message-ID: <CAGZ79kbDjV8j_Hhpr9iKau89TipE6+_VNEOwxxeicwuEqwQ8Aw@mail.gmail.com> (raw)
In-Reply-To: <ca94745f8d72a1d472462e9cb25ef3d2e1285f86.1502491372.git.jonathantanmy@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
>

  reply index

Thread overview: 30+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-08-11 22:49 [RFC PATCH 0/3] Fixes to "diff --color-moved" MIN_BLOCK_LENGTH handling Jonathan Tan
2017-08-11 22:49 ` [RFC PATCH 1/3] diff: avoid redundantly clearing a flag Jonathan Tan
2017-08-14 17:13   ` Stefan Beller
2017-08-11 22:49 ` [RFC PATCH 2/3] diff: respect MIN_BLOCK_LENGTH for last block Jonathan Tan
2017-08-14 17:17   ` Stefan Beller [this message]
2017-08-11 22:49 ` [RFC PATCH 3/3] diff: check MIN_BLOCK_LENGTH at start of new " Jonathan Tan
2017-08-14 17:22   ` Stefan Beller
2017-08-12  0:39 ` [RFC PATCH 0/3] Fixes to "diff --color-moved" MIN_BLOCK_LENGTH handling Junio C Hamano
2017-08-14 17:29   ` Stefan Beller
2017-08-14 19:37     ` Junio C Hamano
2017-08-14 19:51       ` Stefan Beller
2017-08-15 17:07         ` Junio C Hamano
2017-08-14 21:31 ` [PATCH v2 " Jonathan Tan
2017-08-14 21:31   ` [PATCH v2 1/3] diff: avoid redundantly clearing a flag Jonathan Tan
2017-08-14 21:31   ` [PATCH v2 2/3] diff: respect MIN_BLOCK_LENGTH for last block Jonathan Tan
2017-08-14 21:31   ` [PATCH v2 3/3] diff: check MIN_BLOCK_LENGTH at start of new " Jonathan Tan
2017-08-14 22:46     ` Stefan Beller
2017-08-14 23:57 ` [PATCH v3 0/3] "diff --color-moved" with different heuristic Jonathan Tan
2017-08-14 23:57   ` [PATCH v3 1/3] diff: avoid redundantly clearing a flag Jonathan Tan
2017-08-14 23:57   ` [PATCH v3 2/3] diff: respect MIN_BLOCK_LENGTH for last block Jonathan Tan
2017-08-14 23:57   ` [PATCH v3 3/3] diff: define block by number of non-space chars Jonathan Tan
2017-08-15  2:29     ` Stefan Beller
2017-08-15 19:54     ` Junio C Hamano
2017-08-15 20:06       ` Stefan Beller
2017-08-15 20:53         ` Junio C Hamano
2017-08-16  1:27 ` [PATCH v4 0/3] "diff --color-moved" with yet another heuristic Jonathan Tan
2017-08-16  1:27   ` [PATCH v4 1/3] diff: avoid redundantly clearing a flag Jonathan Tan
2017-08-16  1:27   ` [PATCH v4 2/3] diff: respect MIN_BLOCK_LENGTH for last block Jonathan Tan
2017-08-16  1:27   ` [PATCH v4 3/3] diff: define block by number of alphanumeric chars Jonathan Tan
2017-08-16  5:55   ` [PATCH v4 0/3] "diff --color-moved" with yet another heuristic Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kbDjV8j_Hhpr9iKau89TipE6+_VNEOwxxeicwuEqwQ8Aw@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox