Re: [PATCH 5/6] diff.c: omit uninteresting moved lines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 28, 2017, 03:31 UTC
- Message-ID
- <xmqqwp7wyei3.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170628005651.8110-6-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 10 quoted lines
> 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.
Show 14 quoted lines
> (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?
Show 61 quoted lines
> 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);