From: Stefan Beller <sbeller@google.com>
Date: Tue, 06 Sep 2016 02:07:32 GMT
Subject: Re: [WIP PATCH v2] diff.c: emit moved lines with a different color
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:
>> +     `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.

>
>> 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.