Re: [PATCH 5/6] diff.c: omit uninteresting moved lines
From: Junio C Hamano <hidden>
Date: 2017-06-28 03:31:39
Stefan Beller [off-list ref] writes:
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':
Hypotheses is the plural form of that word, I think.
(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.It was puzzling what the above two paragraphs were. I took (a) and (b) were the hypotheses, and the two above, and also the next paragraphs, were the design that fell out of them. But that is not what is happening. You changed your mind and settled on the design in the next paragraph. Perhaps we can do without all of the "I thought about this but it didn't make sense" that is longer than the solution in the patch?
quoted hunk
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 <redacted> --- 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);