Re: [PATCHv2 0/7] Moved code detection: ignore space on uniform indentation

From: Stefan Beller <sbeller@google.com>
Date: Apr 24, 2018, 22:58 UTC
Message-ID: <CAGZ79kZSMURd_s6yxUCJoDjQzu-NjnFzp+740GFnYZurnw_2yw@mail.gmail.com>
In-Reply-To: <20180424153736.480b609d0af412ec46311eb6@google.com>

On Tue, Apr 24, 2018 at 3:37 PM, Jonathan Tan <jonathantanmy@google.com> wrote:

Show 25 quoted lines

> On Tue, 24 Apr 2018 14:03:23 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> v2:
>> I think I have addressed Jonathans feedback
>> * by using a string instead of counting the first character only.
>> * refined tests slightly (easier to read)
>> * moved white space handling for moved blocks into its own flag field,
>>   keeping the enum for the actual mode of move detection.
>
> For reference, v1 is here:
> https://public-inbox.org/git/20180402224854.86922-1-sbeller@google.com/
>
>> Stefan Beller (7):
>>   xdiff/xdiff.h: remove unused flags
>>   xdiff/xdiffi.c: remove unneeded function declarations
>>   diff.c: do not pass diff options as keydata to hashmap
>>   diff.c: adjust hash function signature to match hashmap expectation
>>   diff.c: add a blocks mode for moved code detection
>>   diff.c: decouple white space treatment from move detection algorithm
>>   diff.c: add --color-moved-ignore-space-delta option
>
> I'm not sure if we should add a new "blocks" mode, or if we should
> modify the existing plain mode to have the minimum block length instead.
> I reviewed the code as if we want the new "blocks" mode.

Thanks for the review!

I think keeping plain is useful, see 176841f0c9 (diff.c: color moved lines differently, plain mode, 2017-06-30)

    diff.c: color moved lines differently, plain mode

    Add the 'plain' mode for move detection of code. This omits the checking
    for adjacent blocks, so it is not as useful. If you have a lot of the
    same blocks moved in the same patch, the 'Zebra' would end up slow as it
    is O(n^2) (n is number of same blocks). So this may be useful there and
    is generally easy to add. Instead be very literal at the move detection,
    do not skip over short blocks here.

Although if we do not care about that use case we can just add heuristics to plain.

As eluded to in Ævars email, we might want to break it up into multiple options as well?