[PATCH 5/6] diff.c: omit uninteresting moved lines

From: Stefan Beller <sbeller@google.com>
Date: Jun 28, 2017, 00:56 UTC
Message-ID: <20170628005651.8110-6-sbeller@google.com>
In-Reply-To: <20170628005651.8110-1-sbeller@google.com>

It is useful to have moved lines colored, but there are annoying corner cases, such as a single line moved, that is very common. For example in a typical patch of C code, we have closing braces that end statement blocks or functions.

While it is technically true that these lines are moved as they show up elsewhere, it is harmful for the review as the reviewers attention is drawn to such a minor side annoyance.

One of the first solutions considered, started off by these hypothesis':
  (a) The more blocks of the same code we have, the less interesting it is.
  (b) The shorter a block of moved code is the less need of markup there
      is for review.

      Introduce a heuristic which drops any potential moved blocks if their
      length is shorter than the number of potential moved blocks.

      This heuristic was chosen as it is agnostic of the content (in other
      languages or contents to manage, we may have longer lines, e.g. in
      shell the closing of a condition is already 2 characters. Thinking
      about Latex documents tracked in Git, there can also be some
      boilerplate code with lots of characters) while taking both
      hypothesis' into account. An alternative considered was the number
      of non-whitespace characters in a line for example.

Thinking further about this, a linear relation between number of moved blocks and number of lines of code seems like a bad idea to start with. So let's start with a simpler solution of hardcoding the number of lines to 3.

Note, that the length is applied across all blocks to find the 'lonely' blocks that pollute new code, but do not interfere with a permutated block where each permutation has less lines than 3.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 diff.c | 11 ++++++++++-
 diff.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 015c854530..1d93e98e3a 100644
--- a/diff.c
+++ b/diff.c
@@ -853,7 +853,8 @@ static void mark_color_as_moved(struct diff_options *o,
 {
 	struct moved_entry **pmb = NULL; /* potentially moved blocks */
 	int pmb_nr = 0, pmb_alloc = 0;
-	int n, flipped_block = 1;
+	int n, flipped_block = 1, block_length = 0;
+
 
 	for (n = 0; n < o->emitted_symbols->nr; n++) {
 		struct hashmap *hm = NULL;
@@ -880,11 +881,19 @@ static void mark_color_as_moved(struct diff_options *o,
 		}
 
 		if (!match) {
+			if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH) {
+				for (i = 0; i < block_length + 1; i++) {
+					l = &o->emitted_symbols->buf[n - i];
+					l->flags &= ~DIFF_SYMBOL_MOVED_LINE;
+				}
+			}
 			pmb_nr = 0;
+			block_length = 0;
 			continue;
 		}
 
 		l->flags |= DIFF_SYMBOL_MOVED_LINE;
+		block_length++;
 
 		if (o->color_moved == COLOR_MOVED_PLAIN)
 			continue;
diff --git a/diff.h b/diff.h
index 9298d211d7..cc1224a93b 100644
--- a/diff.h
+++ b/diff.h
@@ -195,6 +195,7 @@ struct diff_options {
 		COLOR_MOVED_DEFAULT = 2,
 		COLOR_MOVED_ZEBRA_DIM = 3,
 	} color_moved;
+	#define COLOR_MOVED_MIN_BLOCK_LENGTH 3
 };
 
 void diff_emit_submodule_del(struct diff_options *o, const char *line);

-- 
2.13.0.31.g9b732c453e