Re: [PATCH 1/2] xdiff-interface: export comparing and hashing strings

[Stefan Beller]

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

[Junio C Hamano]

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

[René Scharfe]

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

[Stefan Beller]

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 + 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

[Eric Sunshine]

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

[Stefan Beller]

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 + 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