Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings
On Fri, Oct 27, 2017 at 12:12 AM, Junio C Hamano wrote: > René Scharfe writes: > >> Am 25.10.2017 um 20:49 schrieb Stefan Beller: >>> +/* >>> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. >>> + * Returns 1 if the strings are deemed equal, 0 otherwise. >>> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces >>> + * are treated for the comparision. >>> + */ >>> +extern int xdiff_compare_lines(const char *l1, long s1, >>> + const char *l2, long s2, long flags); >> >> With the added comment it's OK here. >> >> Still, I find the tendency in libxdiff to use the shortest possible >> variable names to be hard on the eyes. That math-like notation may have >> its place in that external library, but I think we should be careful >> lest it spreads. > > Yeah, I tend to agree. The xdiff-interface is at the (surprise!) > interface layer, so we could make it follow the style of either one, > but we seem to have picked up the convention of the lower layer, > so... > > By the way, I was looking at the code around this area while > reviewing the cr-at-eol thing, and noticed a couple of things: > > > * combine-diff.c special cases only IGNORE_WHITESPACE and >IGNORE_WHITESPACE_CHANGE but no other IGNORE_WHITESPACE things; I >have a suspicion that this is not because IGNORE_WHITESPACE_AT_EOL >does not have to special cased by the codepath, but only because >the combine-diff.c refactoring predates the introduction of >ws-at-eol thing? I wonder how much overlap between the IGNORE_WHITESPACE_AT_EOL and CRLF-AT-EOL is (maybe these can be combined, as per the root of this discussion) >The machinery uses its own match_string_spaces() helper; it may >make sense to use the same xdiff_compare_lines() in its callers >and get rid of this helper function. Speaking of xdiff_compare_lines, it serves the special purpose of the move detection as well as its internal use cases, but we may need to change its interface/implementation in the future, to align it with strcmp, currently the compare function only returns equality, not an order. > * diff_setup_done() sets DIFF_FROM_CONTENTS when any of the >IGNORE_WHITESPACE bits is true, to allow "git diff -q >--ignore-something" would do the right thing. We do not however >give a similar special case to XDF_IGNORE_BLANK_LINES. > >$ echo >>Makefile && git diff $option --ignore-blank-lines Makefile > >exits with 1 when option=--exit-code and it exits with 0 when >option=-q > > > For now I'll leve these as #leftoverbits, but I may come back to the > latter soonish. I won't come back to the former until Stefan's > refactor hits 'master', though. which is presumably after the 2.15 release. To tack on the #leftoverbits: The --color-moved detection doesn't pay attention to XDF_IGNORE_BLANK_LINES (which is tricky as it is on the per-line layer. If we want to ignore spurious blank lines, we'd have to check for this flag in diff.c in mark_color_as_moved(..) in the block /* Check any potential block runs, advance each or nullify */ Thanks, Stefan
Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings
René Scharfe writes: > Am 25.10.2017 um 20:49 schrieb Stefan Beller: >> +/* >> + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. >> + * Returns 1 if the strings are deemed equal, 0 otherwise. >> + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces >> + * are treated for the comparision. >> + */ >> +extern int xdiff_compare_lines(const char *l1, long s1, >> + const char *l2, long s2, long flags); > > With the added comment it's OK here. > > Still, I find the tendency in libxdiff to use the shortest possible > variable names to be hard on the eyes. That math-like notation may have > its place in that external library, but I think we should be careful > lest it spreads. Yeah, I tend to agree. The xdiff-interface is at the (surprise!) interface layer, so we could make it follow the style of either one, but we seem to have picked up the convention of the lower layer, so... By the way, I was looking at the code around this area while reviewing the cr-at-eol thing, and noticed a couple of things: * combine-diff.c special cases only IGNORE_WHITESPACE and IGNORE_WHITESPACE_CHANGE but no other IGNORE_WHITESPACE things; I have a suspicion that this is not because IGNORE_WHITESPACE_AT_EOL does not have to special cased by the codepath, but only because the combine-diff.c refactoring predates the introduction of ws-at-eol thing? The machinery uses its own match_string_spaces() helper; it may make sense to use the same xdiff_compare_lines() in its callers and get rid of this helper function. * diff_setup_done() sets DIFF_FROM_CONTENTS when any of the IGNORE_WHITESPACE bits is true, to allow "git diff -q --ignore-something" would do the right thing. We do not however give a similar special case to XDF_IGNORE_BLANK_LINES. $ echo >>Makefile && git diff $option --ignore-blank-lines Makefile exits with 1 when option=--exit-code and it exits with 0 when option=-q For now I'll leve these as #leftoverbits, but I may come back to the latter soonish. I won't come back to the former until Stefan's refactor hits 'master', though.
Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings
Am 25.10.2017 um 20:49 schrieb Stefan Beller: > +/* > + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. > + * Returns 1 if the strings are deemed equal, 0 otherwise. > + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces > + * are treated for the comparision. > + */ > +extern int xdiff_compare_lines(const char *l1, long s1, > +const char *l2, long s2, long flags); With the added comment it's OK here. Still, I find the tendency in libxdiff to use the shortest possible variable names to be hard on the eyes. That math-like notation may have its place in that external library, but I think we should be careful lest it spreads. René
[PATCH 1/2] xdiff-interface: export comparing and hashing strings
This will turn out to be useful in a later patch. xdl_recmatch is exported in xdiff/xutils.h, to be used by various xdiff/*.c files, but not outside of xdiff/. This one makes it available to the outside, too. While at it, add documentation. Signed-off-by: Stefan Beller --- xdiff-interface.c | 12 xdiff-interface.h | 16 2 files changed, 28 insertions(+) diff --git a/xdiff-interface.c b/xdiff-interface.c index 018e033089..770e1f7f81 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -5,6 +5,7 @@ #include "xdiff/xdiffi.h" #include "xdiff/xemit.h" #include "xdiff/xmacros.h" +#include "xdiff/xutils.h" struct xdiff_emit_state { xdiff_emit_consume_fn consume; @@ -296,6 +297,17 @@ void xdiff_clear_find_func(xdemitconf_t *xecfg) } } +unsigned long xdiff_hash_string(const char *s, size_t len, long flags) +{ + return xdl_hash_record(&s, s + len, flags); +} + +int xdiff_compare_lines(const char *l1, long s1, + const char *l2, long s2, long flags) +{ + return xdl_recmatch(l1, s1, l2, s2, flags); +} + int git_xmerge_style = -1; int git_xmerge_config(const char *var, const char *value, void *cb) diff --git a/xdiff-interface.h b/xdiff-interface.h index 6f6ba9095d..135fc05d72 100644 --- a/xdiff-interface.h +++ b/xdiff-interface.h @@ -29,4 +29,20 @@ extern void xdiff_clear_find_func(xdemitconf_t *xecfg); extern int git_xmerge_config(const char *var, const char *value, void *cb); extern int git_xmerge_style; +/* + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. + * Returns 1 if the strings are deemed equal, 0 otherwise. + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces + * are treated for the comparision. + */ +extern int xdiff_compare_lines(const char *l1, long s1, + const char *l2, long s2, long flags); + +/* + * Returns a hash of the string s of length len. + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces + * are treated for the hash. + */ +extern unsigned long xdiff_hash_string(const char *s, size_t len, long flags); + #endif -- 2.15.0.rc2.6.g953226eb5f
Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings
On Tue, Oct 24, 2017 at 7:42 PM, Stefan Beller wrote: > This will turn out to be useful in a later patch. > > xdl_recmatch is exported in xdiff/xutils.h, to be used by various > xdiff/*.c files, but not outside of xdiff/. This one makes it available > to the outside, too. > > While at it, add documentation. > > Signed-off-by: Stefan Beller > --- > diff --git a/xdiff-interface.h b/xdiff-interface.h > @@ -29,4 +29,20 @@ extern void xdiff_clear_find_func(xdemitconf_t *xecfg); > +/* > + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. > + * Returns 1 if the strings are deemed equal, 0 otherwise. > + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces > + * are treated for the comparision. > + */ > +extern int xdiff_compare_lines(const char *a, long len_a, > + const char *b, long len_b, long flags); The comment block talks about 'l1', 'l2', 's1', and 's2', but the declaration uses 'a', 'b, 'len_a', 'len_b'. Confusing.
[PATCH 1/2] xdiff-interface: export comparing and hashing strings
This will turn out to be useful in a later patch. xdl_recmatch is exported in xdiff/xutils.h, to be used by various xdiff/*.c files, but not outside of xdiff/. This one makes it available to the outside, too. While at it, add documentation. Signed-off-by: Stefan Beller --- xdiff-interface.c | 11 +++ xdiff-interface.h | 16 2 files changed, 27 insertions(+) diff --git a/xdiff-interface.c b/xdiff-interface.c index 018e033089..9b35af2455 100644 --- a/xdiff-interface.c +++ b/xdiff-interface.c @@ -5,6 +5,7 @@ #include "xdiff/xdiffi.h" #include "xdiff/xemit.h" #include "xdiff/xmacros.h" +#include "xdiff/xutils.h" struct xdiff_emit_state { xdiff_emit_consume_fn consume; @@ -296,6 +297,16 @@ void xdiff_clear_find_func(xdemitconf_t *xecfg) } } +unsigned long xdiff_hash_string(const char *s, size_t len, long flags) +{ + return xdl_hash_record(&s, s + len, flags); +} + +int xdiff_compare_lines(const char *a, long len_a, const char *b, long len_b, long flags) +{ + return xdl_recmatch(a, len_a, b, len_b, flags); +} + int git_xmerge_style = -1; int git_xmerge_config(const char *var, const char *value, void *cb) diff --git a/xdiff-interface.h b/xdiff-interface.h index 6f6ba9095d..6f5abaf8d3 100644 --- a/xdiff-interface.h +++ b/xdiff-interface.h @@ -29,4 +29,20 @@ extern void xdiff_clear_find_func(xdemitconf_t *xecfg); extern int git_xmerge_config(const char *var, const char *value, void *cb); extern int git_xmerge_style; +/* + * Compare the strings l1 with l2 which are of size s1 and s2 respectively. + * Returns 1 if the strings are deemed equal, 0 otherwise. + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces + * are treated for the comparision. + */ +extern int xdiff_compare_lines(const char *a, long len_a, + const char *b, long len_b, long flags); + +/* + * Returns a hash of the string s of length len. + * The `flags` given as XDF_WHITESPACE_FLAGS determine how white spaces + * are treated for the hash. + */ +extern unsigned long xdiff_hash_string(const char *s, size_t len, long flags); + #endif -- 2.15.0.rc2.6.g953226eb5f