On Tue, Oct 24, 2017 at 10:11 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Stefan Beller <sbeller@xxxxxxxxxx> writes: > >> v2: >> * I realized that we don't have to change the hashing function of xdiff; >> we can rather change the hashing function for the move detection, >> which is less fundamental. >> (That way I can shrink the series down to 2 patches) >> * commented and renamed function parameters in the exposed xdiff functions. >> * applies on top of jk/diff-color-moved-fix. > > Yes, by reusing the line hashing and comparison from xdiff/ we can > ensure that we will use consistent comparison function, and the > thing we need to focus on will become how correctly the caller uses > the xdiff interface. This looks much better than the previous one. > > Eric's comment on the function parameters is right. We keep them in > sync with the naming convention of xdiff/ as long as they are still > part of xdiff layer, and the convention there is that the lines > being compared are l1[] and l2[] whose lengths are s1 and s2, if I > am not mistaken (well, I am not, as I just touched the function > there during my lunch break ;-). > Yes, I am torn on the naming as Rene commented on it and did not like the (l1, s1) and rather would see a (a, alen) / (b, blen) approach. And I thought that was a good idea initially as that patch is explicitly about exposing the internals of xdiff to the world. So when the world copes better with (a, alen), then the function added in xdiff-interface.c should take that parameter as to not confuse the outside world. And one could argue that xdiff-interface is not part of the xdiff layer any more as that is the glue-gap to the rest of Git. However I agree that we may want to keep l1[], s1 at that place, as the rest of the xdiff interface looks like it is kept in line with xdiff. Thanks, Stefan
