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

From: Stefan Beller <sbeller@google.com>
Date: Sep 6, 2016, 02:09 UTC
Message-ID: <CAGZ79kYPtvJ7iF8G5ihUCa5NwvDFa_Rzh+N7ar9QA2x74p2Z9g@mail.gmail.com>
In-Reply-To: <CA+P7+xqx4PHHoKT_PK5s5eOtZMG9QJTSHED_=+BHZ=U330AQtw@mail.gmail.com>

On Mon, Sep 5, 2016 at 6:09 PM, Jacob Keller <jacob.keller@gmail.com> wrote:

Show 23 quoted lines

> On Mon, Sep 5, 2016 at 11:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <stefanbeller@gmail.com> writes:
>>
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index 0bcb679..f4f51c2 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -980,8 +980,9 @@ color.diff.<slot>::
>>>       of `context` (context text - `plain` is a historical synonym),
>>>       `meta` (metainformation), `frag`
>>>       (hunk header), 'func' (function in hunk header), `old` (removed lines),
>>> -     `new` (added lines), `commit` (commit headers), or `whitespace`
>>> -     (highlighting whitespace errors).
>>> +     `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?
>
> That seems entirely reasonable, though we *do* have a configuration
> for disabling color altogether.. is there any numbers on how much more
> this costs to compute?

This new coloring is linear to the size of the patch, i.e. O(number of added/removed lines) in memory and for computational efforts I'd think it is O(n log n) as inserting into the hashmap is an amortized log n.

Show 10 quoted lines

>
>>> +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.
>
> Agreed, those seem like good choices for diff_options.

yup.