Re: [PATCHv2 0/2] (x)diff cleanup: remove duplicate code
On Tue, Oct 24, 2017 at 10:11 PM, Junio C Hamano wrote: > Stefan Beller 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
Re: [PATCHv2 0/2] (x)diff cleanup: remove duplicate code
Stefan Beller 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 ;-).
[PATCHv2 0/2] (x)diff cleanup: remove duplicate code
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. Thanks, Stefan v1: Junio wrote: > * I was hoping that the next_byte() and string_hash() thing, once >they are cleaned up, will eventually be shared with the xdiff/ >code at the lower layer, which needs to do pretty much the same >in order to implement various whitespace ignoring options. I am >not sure how well the approach taken by the WIP patch meshes with >the needs of the lower layer. This series does exactly this; although I chose to reuse the code in xdiff/xutils.c instead of the new fancy next_byte/string_hash, as that code has seen more exercise already (hence I assume it has fewer bugs if any as well as its performance implications are well understood). However to do so, we need to pollute xdiff/xutils.c and include hashmap.h there (which also requires cache.h as that header has an inline function using BUG()), which I find a bit unfortunate but worth the trade off of using better tested code. Thanks, Stefan Stefan Beller (2): xdiff-interface: export comparing and hashing strings diff.c: get rid of duplicate implementation diff.c| 82 +++ xdiff-interface.c | 11 xdiff-interface.h | 16 +++ 3 files changed, 31 insertions(+), 78 deletions(-) -- 2.15.0.rc2.6.g953226eb5f
