libart: better defined behavior when a full prefix key is encountered

Summary: a facet of the libart implementation that is undocumented
except for a series of issues (linked below) is this: an inserted key
must not be a full prefix of another key that is already stored in the
tree.  The recommendation is to store keys that have a terminator
character.

armon/libart#4
armon/libart#12
armon/libart#14
armon/libart#17

Our usage in watchman can't guarantee to provide a NUL-terminated input,
so this diff adjusts the key comparison routines to generate an implicit
or synthetic NUL terminator character when comparing one character
beyond the end of a key, and by asserting (or allowing ASAN to complain)
if we try to look more than 1 character beyond the end.

I also tidied up the function signatures of a couple of matching
functions so that the semantics are clearer (return boolean true for
success rather than integer 0) and removed some redundant casts when
invoking callbacks.

Test Plan: added an explicit test for the full-prefix issue.

Built with -fsanitize=address and ran the integration tests as well as
the sparse/unsparse checks I've been using for the pending list changes.

Makefile.am

@@ -201,7 +201,9 @@ integration: all py-integration
 tests_art_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
 tests_art_t_LDADD = $(ART_LIB) $(TAP_LIB)
 tests_art_t_SOURCES = \
-	tests/art_test.c
+	tests/art_test.c \
+	tests/log_stub.c \
+	log.c
 
 tests_argv_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
 tests_argv_t_LDADD = $(JSON_LIB) $(TAP_LIB)
@@ -213,6 +215,7 @@ tests_ignore_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
 tests_ignore_t_LDADD = $(ART_LIB) $(TAP_LIB) $(JSON_LIB)
 tests_ignore_t_SOURCES = \
 	tests/ignore_test.c \
+	tests/log_stub.c \
 	watcher/helper.c \
 	hash.c \
 	ht.c \
@@ -224,6 +227,7 @@ tests_pending_t_CPPFLAGS = $(THIRDPARTY_CPPFLAGS)
 tests_pending_t_LDADD = $(ART_LIB) $(TAP_LIB) $(JSON_LIB)
 tests_pending_t_SOURCES = \
 	tests/pending_test.c \
+	tests/log_stub.c \
 	watcher/helper.c \
 	hash.c \
 	ht.c \

tests/art_test.c

@@ -13,6 +13,7 @@
 
 #include "thirdparty/tap.h"
 #include "thirdparty/libart/src/art.h"
+#include "watchman.h"
 
 #define stringy2(line)  #line
 #define stringy(line)   stringy2(line)
@@ -430,6 +431,34 @@ void test_art_long_prefix(void) {
   fail_unless(res == 0);
 }
 
+static int dump_iter(void *data, const unsigned char *key, unsigned int key_len,
+                     void *value) {
+  diag("iter leaf: data=%p key_len=%d %.*s value=%p", data, (int)key_len,
+       (int)key_len, key, value);
+  return 0;
+}
+
+void test_art_prefix(void) {
+  art_tree t;
+  void *v;
+
+  art_tree_init(&t);
+
+  fail_unless(art_insert(&t, (const unsigned char*)"food", 4, "food") == NULL);
+  fail_unless(art_insert(&t, (const unsigned char*)"foo", 3, "foo") == NULL);
+  diag("size is now %d", art_size(&t));
+  fail_unless(art_size(&t) == 2);
+  fail_unless((v = art_search(&t, (const unsigned char*)"food", 4)) != NULL);
+  diag("food lookup yields %s", v);
+  fail_unless(v && strcmp((char*)v, "food") == 0);
+
+  art_iter(&t, dump_iter, NULL);
+
+  fail_unless((v = art_search(&t, (const unsigned char*)"foo", 3)) != NULL);
+  diag("foo lookup yields %s", v);
+  fail_unless(v && strcmp((char*)v, "foo") == 0);
+}
+
 void test_art_insert_search_uuid(void) {
   art_tree t;
   art_leaf *l;
@@ -486,7 +515,7 @@ int main(int argc, char **argv) {
   (void)argc;
   (void)argv;
 
-  plan_tests(109);
+  plan_tests(116);
   test_art_init_and_destroy();
   test_art_insert();
   test_art_insert_verylong();
@@ -496,6 +525,7 @@ int main(int argc, char **argv) {
   test_art_iter_prefix();
   test_art_long_prefix();
   test_art_insert_search_uuid();
+  test_art_prefix();
 
   return exit_status();
 }

tests/ignore_test.c

@@ -3,7 +3,6 @@
 
 #include "watchman.h"
 #include "thirdparty/tap.h"
-#include "thirdparty/libart/src/art.h"
 
 // A list that looks similar to one used in one of our repos
 const char *ignore_dirs[] = {".buckd",
@@ -36,20 +35,6 @@ const char *ignore_dirs[] = {".buckd",
 
 const char *ignore_vcs[] = {".hg", ".svn", ".git"};
 
-void w_request_shutdown(void) {}
-
-bool w_should_log_to_clients(int level)
-{
-  unused_parameter(level);
-  return false;
-}
-
-void w_log_to_clients(int level, const char *buf)
-{
-  unused_parameter(level);
-  unused_parameter(buf);
-}
-
 struct test_case {
   const char *path;
   bool ignored;

tests/log_stub.c

@@ -0,0 +1,21 @@
+/* Copyright 2016-present Facebook, Inc.
+ * Licensed under the Apache License, Version 2.0. */
+
+#include "watchman.h"
+
+// These are logging stubs to facilitate testing code that pulls in w_log
+// either directly or indirectly.
+
+void w_request_shutdown(void) {}
+
+bool w_should_log_to_clients(int level)
+{
+  unused_parameter(level);
+  return false;
+}
+
+void w_log_to_clients(int level, const char *buf)
+{
+  unused_parameter(level);
+  unused_parameter(buf);
+}

tests/pending_test.c

@@ -9,21 +9,6 @@ struct pending_list {
   struct watchman_pending_fs *pending, *avail, *end;
 };
 
-void w_request_shutdown(void) {}
-
-bool w_should_log_to_clients(int level)
-{
-  unused_parameter(level);
-  return false;
-}
-
-void w_log_to_clients(int level, const char *buf)
-{
-  unused_parameter(level);
-  unused_parameter(buf);
-}
-
-
 struct watchman_pending_fs *next_pending(struct pending_list *list) {
   if (list->avail == list->end) {
     fail("make list alloc size bigger (used %u entries)",
@@ -67,7 +52,6 @@ size_t process_items(struct watchman_pending_collection *coll) {
     // doesn't exist on the filesystem.  We're measuring hot cache
     // (best case) stat performance here.
     w_lstat(__FILE__, &st, true);
-    //diag("lstat(%.*s)", item->path->len, item->path->buf);
     w_pending_fs_free(item);
 
     drained++;
@@ -89,9 +73,11 @@ static void bench_pending(void) {
   list.avail = list.pending;
   list.end = list.pending + alloc_size;
 
+  // Build a list ordered from the root (top) down to the leaves.
   build_list(&list, root_name, tree_depth, num_files_per_dir, num_dirs_per_dir);
   diag("built list with %u items", list.avail - list.pending);
 
+  // Benchmark insertion in top-down order.
   struct timeval start, end;
   {
     struct watchman_pending_collection coll;
@@ -111,7 +97,9 @@ static void bench_pending(void) {
          w_timeval_diff(start, end), drained);
   }
 
-  // and now in reverse
+  // and now in reverse order; this is from the leaves of the filesystem
+  // tree up to the root, or bottom-up.  This simulates the workload of
+  // a recursive delete of a filesystem tree.
   {
     struct watchman_pending_collection coll;
     struct watchman_pending_fs *item;