On Tue, Apr 24, 2018 at 3:37 PM, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: > On Tue, 24 Apr 2018 14:03:23 -0700 > Stefan Beller <sbeller@xxxxxxxxxx> 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@xxxxxxxxxx/ > >> 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?
