Re: [WIP PATCH v2] diff.c: emit moved lines with a different color

From: Stefan Beller <sbeller@google.com>
Date: Sep 6, 2016, 02:07 UTC
Message-ID: <CAGZ79kaCtYYSv-Wnz6-Xz6Mt1TwstO-GMBYWyhbO7tM9g5gumA@mail.gmail.com>
In-Reply-To: <xmqq1t0yqinr.fsf@gitster.mtv.corp.google.com>

On Mon, Sep 5, 2016 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>> +     `new` (added lines), `commit` (commit headers), `whitespace`
>> +     (highlighting whitespace errors), `moved-old` (removed lines that
>> +     reappear), `moved-new` (added lines that were removed elsewhere).
>
> Could we have a config to disable this rather costly new feature,
> too?

And by config option you mean both a command line parameter `--color=yes-but-no-move-detection` as well as a diff.color config option?

As it is currently `--color=<when>` with when={always, never,auto}, I don't think we want to add it as another parameter there, so maybe --color-type=<style> with style={minimal, full} whereas the minimal/full describes the amount of work needed. Though I think that is bad as there might be other orthogonal features to this.

So maybe just a `--[no-]color-moved` ?

As a config option we'd go with color.moved=<bool> for now?

I imagine we may want to refine the moved detection algorithm in the future, e.g. moved just in the patch, or moved from elsewhere in the repo or whether the moved detection takes permutations into account etc, so actually we'd want to have color.moved={none, this-patch} for now. The command line parameter --color-moved=<style> would be the same.

>
> Also the first and the third level configuration names (the <slot>
> is at the third level) used by the core-git do not use dashed-words
> format.  Please adhere to the current convention.

will do.

Show 24 quoted lines

>
>> diff --git a/diff.c b/diff.c
>> index 534c12e..d37cb4f 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -18,6 +18,7 @@
>>  #include "ll-merge.h"
>>  #include "string-list.h"
>>  #include "argv-array.h"
>> +#include "git-compat-util.h"
>>
>>  #ifdef NO_FAST_WORKING_DIRECTORY
>>  #define FAST_WORKING_DIRECTORY 0
>> @@ -42,6 +43,11 @@ static int diff_dirstat_permille_default = 30;
>>  static struct diff_options default_diff_options;
>>  static long diff_algorithm;
>>
>> +static struct hashmap *duplicates_added;
>> +static struct hashmap *duplicates_removed;
>> +static int hash_previous_line_added;
>> +static int hash_previous_line_removed;
>
> I think these should be added as new fields to diff_options
> structure.

Makes sense.