Re: [PATCH 2/5] Add watchman support to reduce index refresh cost

[David Turner]

On Sun, 2015-11-01 at 14:55 +0100, Nguyễn Thái Ngọc Duy wrote:
> The previous patch has the logic to clear bits in 'WAMA' bitmap. This
> patch has logic to set bits as told by watchman. The missing bit,
> _using_ these bits, are not here yet.
> 
> A lot of this code is written by David Turner originally, mostly from
> [1]. I'm just copying and polishing it a bit.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/248006

Our code has evolved somewhat from there[1].  It looks like you've
incorporated some of these updates.  But I wanted to call out one thing
in particular that it doesn't look like this patch handles: there's some
real ugliness on at least OSX around case-changing renames. 

You probably won't be able to use our code directly as-is, but we'll
want to do something about this situation.  This is thicket of TOCTOU
issues.  For instance, watchman learns that a file called FOO has
changed, but by the time it goes to the filesystem to look up the
canonical case, FOO has been renamed to foo already (or renamed and then
deleted).  

I won't say for sure that your code is insufficient as I haven't yet
tried it out, but we should ensure this case is handled before this is
merged.

[1] https://github.com/dturner-tw/git/tree/dturner/watchman

> +
> + pos = index_name_pos(istate, wm->name, strlen(wm->name));

This is the bit where case matters.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] Add watchman support to reduce index refresh cost

[Nguyễn Thái Ngọc Duy]

The previous patch has the logic to clear bits in 'WAMA' bitmap. This
patch has logic to set bits as told by watchman. The missing bit,
_using_ these bits, are not here yet.

A lot of this code is written by David Turner originally, mostly from
[1]. I'm just copying and polishing it a bit.

[1] http://article.gmane.org/gmane.comp.version-control.git/248006

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Makefile |   7 +++
 cache.h  |   1 +
 config.c |   5 +++
 configure.ac |   8 
 environment.c|   3 ++
 watchman-support.c (new) | 108 +++
 watchman-support.h (new) |   8 
 7 files changed, 140 insertions(+)
 create mode 100644 watchman-support.c
 create mode 100644 watchman-support.h

diff --git a/Makefile b/Makefile
index c01cd2e..761acb6 100644
--- a/Makefile
+++ b/Makefile
@@ -1389,6 +1389,12 @@ else
LIB_OBJS += thread-utils.o
 endif
 
+ifdef USE_WATCHMAN
+   LIB_H += watchman-support.h
+   LIB_OBJS += watchman-support.o
+   BASIC_CFLAGS += -DUSE_WATCHMAN
+endif
+
 ifdef HAVE_PATHS_H
BASIC_CFLAGS += -DHAVE_PATHS_H
 endif
@@ -2135,6 +2141,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@+
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@+
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
+   @echo USE_WATCHMAN=\''$(subst ','\'',$(subst 
','\'',$(USE_WATCHMAN)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/cache.h b/cache.h
index a05fd31..572299c 100644
--- a/cache.h
+++ b/cache.h
@@ -648,6 +648,7 @@ extern char *git_replace_ref_base;
 
 extern int fsync_object_files;
 extern int core_preload_index;
+extern int core_watchman_sync_timeout;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
 extern int protect_hfs;
diff --git a/config.c b/config.c
index 248a21a..6b63f66 100644
--- a/config.c
+++ b/config.c
@@ -881,6 +881,11 @@ static int git_default_core_config(const char *var, const 
char *value)
return 0;
}
 
+   if (!strcmp(var, "core.watchmansynctimeout")) {
+   core_watchman_sync_timeout = git_config_int(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.createobject")) {
if (!strcmp(value, "rename"))
object_creation_mode = OBJECT_CREATION_USES_RENAMES;
diff --git a/configure.ac b/configure.ac
index 76170ad..9772f79 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1092,6 +1092,14 @@ AC_COMPILE_IFELSE([BSD_SYSCTL_SRC],
HAVE_BSD_SYSCTL=])
 GIT_CONF_SUBST([HAVE_BSD_SYSCTL])
 
+#
+# Check for watchman client library
+
+AC_CHECK_LIB([watchman], [watchman_connect],
+   [USE_WATCHMAN=YesPlease],
+   [USE_WATCHMAN=])
+GIT_CONF_SUBST([USE_WATCHMAN])
+
 ## Other checks.
 # Define USE_PIC if you need the main git objects to be built with -fPIC
 # in order to build and link perl/Git.so.  x86-64 seems to need this.
diff --git a/environment.c b/environment.c
index 2da7fe2..84df431 100644
--- a/environment.c
+++ b/environment.c
@@ -87,6 +87,9 @@ int auto_comment_line_char;
 /* Parallel index stat data preload? */
 int core_preload_index = 1;
 
+int core_watchman_sync_timeout = 300;
+
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
diff --git a/watchman-support.c b/watchman-support.c
new file mode 100644
index 000..7f6c0a9
--- /dev/null
+++ b/watchman-support.c
@@ -0,0 +1,108 @@
+#include "cache.h"
+#include "watchman-support.h"
+#include "strbuf.h"
+#include 
+
+static struct watchman_query *make_query(const char *last_update)
+{
+   struct watchman_query *query = watchman_query();
+   watchman_query_set_fields(query, WATCHMAN_FIELD_NAME |
+WATCHMAN_FIELD_EXISTS |
+WATCHMAN_FIELD_NEWER);
+   watchman_query_set_empty_on_fresh(query, 1);
+   query->sync_timeout = core_watchman_sync_timeout;
+   if (*last_update)
+   watchman_query_set_since_oclock(query, last_update);
+   return query;
+}
+
+static struct watchman_query_result* query_watchman(
+   struct index_state *istate, struct watchman_connection *connection,
+   const char *fs_path, const char *last_update)
+{
+   struct watchman_error wm_error;
+   struct watchman_query *query;
+   struct watchman_expression *expr;
+   struct watchman_query_result *result;
+
+   query = make_query(last_update);
+   expr = watchman_true_expression();
+   result = watchman_do_query(connection, fs_path, query, expr, _error);
+   watchman_free_query(query);
+