Re: [PATCH 0/2] color-moved: ignore all space changes by default

From: Stefan Beller <sbeller@google.com>
Date: Oct 26, 2017, 17:59 UTC
Message-ID: <CAGZ79kZ5o6SjSa2C4rSY3ExEstThXziK7zzF93Y7uT1udE6+Yg@mail.gmail.com>
In-Reply-To: <CA+P7+xp2S9U1y0ENj0Z=B6-DY3XWKCg16xCCzVjHDLwAcMbGZQ@mail.gmail.com>

On Thu, Oct 26, 2017 at 1:42 AM, Jacob Keller <jacob.keller@gmail.com> wrote:

Show 12 quoted lines

>>
>> Hello,
>>
>> I'm not sure if this is a good default. I think it's not obvious
>> that moved code gets treated differently than regular changes. I
>> wouldn't expect git diff to ignore whitespace changes (without me
>> telling it to) and so when I see moved code I expect they were
>> moved as is.
>>
>> And there are languages where indentation is relevant (e.g.
>> Python, YAML) and as color-moved is also treated as review tool
>> to detect unwanted changes this new default can be dangerous.

That makes sense.

Show 13 quoted lines

>>
>> The new options sound like a good addition but I don't think the
>> defaults should change. However unrelated to this decision,
>> please add config settings in addition to these new options so
>> users can globally configure the behavior they want.
>>
>> Regards
>> Simon
>> --
>
> Even languages which are indentation sensitive often move blocks of
> lines between indentation levels a lot. I personally think the default
> could change.

A safe default for such languages would be when the change in whitespace across lines is taking into account, i.e. when lots of lines are copied, but all of them miss two tab indentation, then it is probably fine to color it all the same. However if there would be a couple of lines in that moved block of code that have a different indentation than most other lines, this could indeed change the program flow in python.

So ideally we'd compare the whitespace delta across lines. Note sure if that is easy. Maybe instead of ignoring whitespaces completely we'd rather make up a "white space delta", e.g. by using word-diff between the old and new line, and then keeping a hash of that space delta for each line. The move detection would then also compare the hashes of adjacent moved lines.

> However, I would suspect the best path forward is leave the default
> "exact match" and allow users who care and know about the feature to
> change their config settings.

I think that is what I'll do for now as it seems simple enough.

Thanks, Stefan