[PATCH v3 0/6] Fast git status via a file system watcher

[PATCH v3 0/6] Fast git status via a file system watcher

[Ben Peart]

Changes from V2 include:
 - switch to nanoseconds for last update time saved in index extension
   and passed to hook
 - pass the hook a version to enable simpler future updates
 - fixup compiler warnings found with different compilers
 - update test to run properly on Mac
 - fix documentation formatting and spelling errors
 - update code formatting based on feedback
 - rename fsmonitor_dirty_bitmap to fsmonitor_dirty

Ben Peart (6):
  bswap: add 64 bit endianness helper get_be64
  dir: make lookup_untracked() available outside of dir.c
  fsmonitor: teach git to optionally utilize a file system monitor to
    speed up detecting new or changed files.
  fsmonitor: add test cases for fsmonitor extension
  fsmonitor: add documentation for the fsmonitor extension.
  fsmonitor: add a sample query-fsmonitor hook script for Watchman

 Documentation/config.txt                 |   7 +
 Documentation/githooks.txt               |  23 +++
 Documentation/technical/index-format.txt |  19 +++
 Makefile                                 |   1 +
 builtin/update-index.c                   |   1 +
 cache.h                                  |   5 +
 compat/bswap.h                           |   4 +
 config.c                                 |   5 +
 dir.c                                    |  16 ++-
 dir.h                                    |   5 +
 entry.c                                  |   1 +
 environment.c                            |   1 +
 fsmonitor.c                              | 238 +++++++++++++++++++++++++++++++
 fsmonitor.h                              |   9 ++
 read-cache.c                             |  28 +++-
 t/t7519-status-fsmonitor.sh              | 158 ++++++++++++++++++++
 templates/hooks--query-fsmonitor.sample  |  37 +++++
 unpack-trees.c                           |   1 +
 18 files changed, 556 insertions(+), 3 deletions(-)
 create mode 100644 fsmonitor.c
 create mode 100644 fsmonitor.h
 create mode 100755 t/t7519-status-fsmonitor.sh
 create mode 100644 templates/hooks--query-fsmonitor.sample

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index f7b4b4a844..48127e8729 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -450,12 +450,12 @@ processed by rebase.
 
 [[query-fsmonitor]]
 query-fsmonitor
-~~~~~~~~~~~~
+~~~~~~~~~~~~~~~
 
 This hook is invoked when the configuration option core.fsmonitor is
 set and git needs to identify changed or untracked files.  It takes
-a single argument which is the time in elapsed seconds since midnight,
-January 1, 1970.
+two arguments, a version (currently 1) and the time in elapsed
+nanoseconds since midnight, January 1, 1970.
 
 The hook should output to stdout the list of all files in the working
 directory that may have changed since the requested time.  The logic
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index b002d23c05..7aeeea6f94 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -307,7 +307,8 @@ The remaining data of each directory block is grouped by type:
   - 32-bit version number: the current supported version is 1.
 
   - 64-bit time: the extension data reflects all changes through the given
-	time which is stored as the seconds elapsed since midnight, January 1, 1970.
+	time which is stored as the nanoseconds elapsed since midnight,
+	January 1, 1970.
 
   - 32-bit bitmap size: the size of the CE_FSMONITOR_DIRTY bitmap.
 
diff --git a/cache.h b/cache.h
index 36423c77cc..58e5abf69f 100644
--- a/cache.h
+++ b/cache.h
@@ -344,8 +344,8 @@ struct index_state {
 	struct hashmap dir_hash;
 	unsigned char sha1[20];
 	struct untracked_cache *untracked;
-	time_t fsmonitor_last_update;
-	struct ewah_bitmap *fsmonitor_dirty_bitmap;
+	uint64_t fsmonitor_last_update;
+	struct ewah_bitmap *fsmonitor_dirty;
 };
 
 extern struct index_state the_index;
diff --git a/fsmonitor.c b/fsmonitor.c
index 9f08e66db9..3ce262d47d 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -5,6 +5,9 @@
 #include "strbuf.h"
 #include "fsmonitor.h"
 
+#define INDEX_EXTENSION_VERSION	1
+#define HOOK_INTERFACE_VERSION		1
+
 static struct untracked_cache_dir *find_untracked_cache_dir(
 	struct untracked_cache *uc, struct untracked_cache_dir *ucd,
 	const char *name)
@@ -56,20 +59,20 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 
 	hdr_version = get_be32(index);
 	index += sizeof(uint32_t);
-	if (hdr_version != 1)
+	if (hdr_version != INDEX_EXTENSION_VERSION)
 		return error("bad fsmonitor version %d", hdr_version);
 
-	istate->fsmonitor_last_update = (time_t)get_be64(index);
+	istate->fsmonitor_last_update = get_be64(index);
 	index += sizeof(uint64_t);
 
 	ewah_size = get_be32(index);
 	index += sizeof(uint32_t);
 
-	istate->fsmonitor_dirty_bitmap = ewah_new();
-	ret = ewah_read_mmap(istate->fsmonitor_dirty_bitmap, index, ewah_size);
+	istate->fsmonitor_dirty = ewah_new();
+	ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
 	if (ret != ewah_size) {
-		ewah_free(istate->fsmonitor_dirty_bitmap);
-		istate->fsmonitor_dirty_bitmap = NULL;
+		ewah_free(istate->fsmonitor_dirty);
+		istate->fsmonitor_dirty = NULL;
 		return error("failed to parse ewah bitmap reading fsmonitor index extension");
 	}
 
@@ -86,7 +89,7 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	uint32_t ewah_size = 0;
 	int fixup = 0;
 
-	hdr_version = htonl(1);
+	hdr_version = htonl(INDEX_EXTENSION_VERSION);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
 
 	tm = htonll((uint64_t)istate->fsmonitor_last_update);
@@ -132,18 +135,21 @@ static void mark_file_dirty(struct index_state *istate, const char *name)
 /*
  * Call the query-fsmonitor hook passing the time of the last saved results.
  */
-static int query_fsmonitor(time_t last_update, struct strbuf *query_result)
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
+	char ver[64];
 	char date[64];
-	const char *argv[3];
+	const char *argv[4];
 
 	if (!(argv[0] = find_hook("query-fsmonitor")))
 		return -1;
 
+	snprintf(ver, sizeof(version), "%d", version);
 	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
-	argv[1] = date;
-	argv[2] = NULL;
+	argv[1] = ver;
+	argv[2] = date;
+	argv[3] = NULL;
 	cp.argv = argv;
 	cp.out = -1;
 
@@ -152,38 +158,41 @@ static int query_fsmonitor(time_t last_update, struct strbuf *query_result)
 
 void process_fsmonitor_extension(struct index_state *istate)
 {
-	if (!istate->fsmonitor_dirty_bitmap)
+	if (!istate->fsmonitor_dirty)
 		return;
 
-	ewah_each_bit(istate->fsmonitor_dirty_bitmap, mark_no_fsmonitor, istate);
-	ewah_free(istate->fsmonitor_dirty_bitmap);
-	istate->fsmonitor_dirty_bitmap = NULL;
+	ewah_each_bit(istate->fsmonitor_dirty, mark_no_fsmonitor, istate);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 }
 
 void refresh_by_fsmonitor(struct index_state *istate)
 {
-	static int has_run_once = FALSE;
+	static int has_run_once = 0;
 	struct strbuf query_result = STRBUF_INIT;
 	int query_success = 0;
 	size_t bol = 0; /* beginning of line */
-	time_t last_update;
+	uint64_t last_update;
 	char *buf, *entry;
 	int i;
 
 	if (!core_fsmonitor || has_run_once)
 		return;
-	has_run_once = TRUE;
+	has_run_once = 1;
 
 	/*
 	 * This could be racy so save the date/time now and the hook
 	 * should be inclusive to ensure we don't miss potential changes.
 	 */
-	last_update = time(NULL);
+	last_update = getnanotime();
 
-	/* If we have a last update time, call query-monitor for the set of changes since that time */
-	if (istate->fsmonitor_last_update) {
-		query_success = !query_fsmonitor(istate->fsmonitor_last_update, &query_result);
-	}
+	/*
+	 * If we have a last update time, call query-monitor for the set of
+	 * changes since that time.
+	 */
+	if (istate->fsmonitor_last_update)
+		query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+							istate->fsmonitor_last_update, &query_result);
 
 	if (query_success) {
 		/* Mark all entries returned by the monitor as dirty */
@@ -210,11 +219,9 @@ void refresh_by_fsmonitor(struct index_state *istate)
 		 * untracked-cache itself, we can mark the untracked cache for
 		 * fsmonitor usage.
 		 */
-		if (istate->untracked) {
+		if (istate->untracked)
 			istate->untracked->use_fsmonitor = 1;
-		}
-	}
-	else {
+	} else {
 		/* if we can't update the cache, fall back to checking them all */
 		for (i = 0; i < istate->cache_nr; i++)
 			istate->cache[i]->ce_flags |= CE_FSMONITOR_DIRTY;
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index d3cffc758f..395db46d55 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -7,7 +7,7 @@ test_description='git status with file system watcher'
 clean_repo () {
 	git reset --hard HEAD
 	git clean -fd
-	rm marker -f
+	rm -f marker
 }
 
 dirty_repo () {
@@ -31,6 +31,11 @@ dirty_repo () {
 test_expect_success 'setup' '
 	mkdir -p .git/hooks &&
 	write_script .git/hooks/query-fsmonitor<<-\EOF &&
+	if [ $1 -ne 1 ]
+	then
+		echo -e "Unsupported query-fsmonitor hook version.\n" >&2
+		exit 1;
+	fi
 	: >marker
 	printf "untracked\0"
 	printf "dir1/untracked\0"
diff --git a/templates/hooks--query-fsmonitor.sample b/templates/hooks--query-fsmonitor.sample
index 4bd22f21d8..615f3332fa 100644
--- a/templates/hooks--query-fsmonitor.sample
+++ b/templates/hooks--query-fsmonitor.sample
@@ -4,13 +4,23 @@
 # (https://facebook.github.io/watchman/) with git to provide fast
 # git status.
 #
-# The hook is passed a time_t formatted as a string and outputs to
-# stdout all files that have been modified since the given time.
-# Paths must be relative to the root of the working tree and
+# The hook is passed a time in nanoseconds formatted as a string and
+# outputs to stdout all files that have been modified since the given
+# time. Paths must be relative to the root of the working tree and
 # separated by a single NUL.
 #
 # To enable this hook, rename this file to "query-fsmonitor"
 
+# check the hook interface version
+if [ $1 -eq 1 ]
+then
+	# convert nanoseconds to seconds
+	time_t=$(($2/1000000000))
+else
+	echo -e "Unsupported query-fsmonitor hook version.\nFalling back to scanning...\n" >&2
+	exit 1;
+fi
+
 # Convert unix style paths to escaped Windows style paths
 case "$(uname -s)" in
 MINGW*|MSYS_NT*)
@@ -22,6 +32,6 @@ MINGW*|MSYS_NT*)
 esac
 
 # Query Watchman for all the changes since the requested time
-echo "[\"query\", \"$GIT_WORK_TREE\", {\"since\": $1, \"fields\":[\"name\"]}]" | \
+echo "[\"query\", \"$GIT_WORK_TREE\", {\"since\": $time_t, \"fields\":[\"name\"]}]" | \
 watchman -j | \
 perl -e 'use JSON::PP; my $o = JSON::PP->new->utf8->decode(join("", <>)); die "Watchman: $o->{'error'}.\nFalling back to scanning...\n" if defined($o->{"error"}); print(join("\0", @{$o->{"files"}}));'


-- 
2.13.0.windows.1.9.gc201c67b71

[PATCH v3 1/6] bswap: add 64 bit endianness helper get_be64

[Ben Peart]

Add a new get_be64 macro to enable 64 bit endian conversions on memory
that may or may not be aligned.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 compat/bswap.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/compat/bswap.h b/compat/bswap.h
index d47c003544..f89fe7f4b5 100644
--- a/compat/bswap.h
+++ b/compat/bswap.h
@@ -158,6 +158,7 @@ static inline uint64_t git_bswap64(uint64_t x)
 
 #define get_be16(p)	ntohs(*(unsigned short *)(p))
 #define get_be32(p)	ntohl(*(unsigned int *)(p))
+#define get_be64(p)	ntohll(*(uint64_t *)(p))
 #define put_be32(p, v)	do { *(unsigned int *)(p) = htonl(v); } while (0)
 
 #else
@@ -170,6 +171,9 @@ static inline uint64_t git_bswap64(uint64_t x)
 	(*((unsigned char *)(p) + 1) << 16) | \
 	(*((unsigned char *)(p) + 2) <<  8) | \
 	(*((unsigned char *)(p) + 3) <<  0) )
+#define get_be64(p)	( \
+	((uint64_t)get_be32((unsigned char *)(p) + 0) << 32) | \
+	((uint64_t)get_be32((unsigned char *)(p) + 4) <<  0)
 #define put_be32(p, v)	do { \
 	unsigned int __v = (v); \
 	*((unsigned char *)(p) + 0) = __v >> 24; \
-- 
2.13.0.windows.1.9.gc201c67b71

[PATCH v3 2/6] dir: make lookup_untracked() available outside of dir.c

[Ben Peart]

Remove the static qualifier from lookup_untracked() and make it
available to other modules by exporting it from dir.h.  This will be
used later when we need to find entries to mark 'fsmonitor dirty.'

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 dir.c | 2 +-
 dir.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48c..1b5558fdf9 100644
--- a/dir.c
+++ b/dir.c
@@ -660,7 +660,7 @@ static void trim_trailing_spaces(char *buf)
  *
  * If "name" has the trailing slash, it'll be excluded in the search.
  */
-static struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
 						    struct untracked_cache_dir *dir,
 						    const char *name, int len)
 {
diff --git a/dir.h b/dir.h
index bf23a470af..9e387551bd 100644
--- a/dir.h
+++ b/dir.h
@@ -339,4 +339,7 @@ extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git
 extern void relocate_gitdir(const char *path,
 			    const char *old_git_dir,
 			    const char *new_git_dir);
+struct untracked_cache_dir *lookup_untracked(struct untracked_cache *uc,
+					     struct untracked_cache_dir *dir,
+					     const char *name, int len);
 #endif
-- 
2.13.0.windows.1.9.gc201c67b71

[PATCH v3 3/6] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

[Ben Peart]

When the index is read from disk, the query-fsmonitor index extension is
used to flag the last known potentially dirty index and untracked cache
entries.

If git finds out some entries are 'fsmonitor-dirty', but are really
unchanged (e.g. the file was changed, then reverted back), then Git will
clear the marking in the extension. If git adds or updates an index
entry, it is marked 'fsmonitor-dirty' to ensure it is checked for
changes in the working directory.

Before the 'fsmonitor-dirty' flags are used to limit the scope of the
files to be checked, the query-fsmonitor hook proc is called with the
time the index was last updated.  The hook proc returns the list of
files changed since that last updated time and the list of
potentially dirty entries is updated to reflect the current state.

refresh_index() and valid_cached_dir() are updated so that any entry not
flagged as potentially dirty is not checked as it cannot have any
changes.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Makefile               |   1 +
 builtin/update-index.c |   1 +
 cache.h                |   5 ++
 config.c               |   5 ++
 dir.c                  |  14 +++
 dir.h                  |   2 +
 entry.c                |   1 +
 environment.c          |   1 +
 fsmonitor.c            | 238 +++++++++++++++++++++++++++++++++++++++++++++++++
 fsmonitor.h            |   9 ++
 read-cache.c           |  28 +++++-
 unpack-trees.c         |   1 +
 12 files changed, 304 insertions(+), 2 deletions(-)
 create mode 100644 fsmonitor.c
 create mode 100644 fsmonitor.h

diff --git a/Makefile b/Makefile
index e35542e631..488a4466cc 100644
--- a/Makefile
+++ b/Makefile
@@ -760,6 +760,7 @@ LIB_OBJS += ewah/ewah_rlw.o
 LIB_OBJS += exec_cmd.o
 LIB_OBJS += fetch-pack.o
 LIB_OBJS += fsck.o
+LIB_OBJS += fsmonitor.o
 LIB_OBJS += gettext.o
 LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ebfc09faa0..32fd977b43 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -232,6 +232,7 @@ static int mark_ce_flags(const char *path, int flag, int mark)
 		else
 			active_cache[pos]->ce_flags &= ~flag;
 		active_cache[pos]->ce_flags |= CE_UPDATE_IN_BASE;
+		active_cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
 		cache_tree_invalidate_path(&the_index, path);
 		active_cache_changed |= CE_ENTRY_CHANGED;
 		return 0;
diff --git a/cache.h b/cache.h
index 188811920c..58e5abf69f 100644
--- a/cache.h
+++ b/cache.h
@@ -201,6 +201,7 @@ struct cache_entry {
 #define CE_ADDED             (1 << 19)
 
 #define CE_HASHED            (1 << 20)
+#define CE_FSMONITOR_DIRTY   (1 << 21)
 #define CE_WT_REMOVE         (1 << 22) /* remove in work directory */
 #define CE_CONFLICTED        (1 << 23)
 
@@ -324,6 +325,7 @@ static inline unsigned int canon_mode(unsigned int mode)
 #define CACHE_TREE_CHANGED	(1 << 5)
 #define SPLIT_INDEX_ORDERED	(1 << 6)
 #define UNTRACKED_CHANGED	(1 << 7)
+#define FSMONITOR_CHANGED	(1 << 8)
 
 struct split_index;
 struct untracked_cache;
@@ -342,6 +344,8 @@ struct index_state {
 	struct hashmap dir_hash;
 	unsigned char sha1[20];
 	struct untracked_cache *untracked;
+	uint64_t fsmonitor_last_update;
+	struct ewah_bitmap *fsmonitor_dirty;
 };
 
 extern struct index_state the_index;
@@ -766,6 +770,7 @@ extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
 extern int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
+extern int core_fsmonitor;
 
 /*
  * Include broken refs in all ref iterations, which will
diff --git a/config.c b/config.c
index bb4d735701..1a8108636d 100644
--- a/config.c
+++ b/config.c
@@ -1232,6 +1232,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.fsmonitor")) {
+		core_fsmonitor = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/dir.c b/dir.c
index 1b5558fdf9..37f1c580a5 100644
--- a/dir.c
+++ b/dir.c
@@ -16,6 +16,7 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
+#include "fsmonitor.h"
 
 /*
  * Tells read_directory_recursive how a file or directory should be treated.
@@ -1652,6 +1653,18 @@ static int valid_cached_dir(struct dir_struct *dir,
 	if (!untracked)
 		return 0;
 
+	refresh_by_fsmonitor(&the_index);
+	if (dir->untracked->use_fsmonitor) {
+		/*
+		 * With fsmonitor, we can trust the untracked cache's
+		 * valid field.
+		 */
+		if (untracked->valid)
+			goto skip_stat;
+		else
+			invalidate_directory(dir->untracked, untracked);
+	}
+
 	if (stat(path->len ? path->buf : ".", &st)) {
 		invalidate_directory(dir->untracked, untracked);
 		memset(&untracked->stat_data, 0, sizeof(untracked->stat_data));
@@ -1665,6 +1678,7 @@ static int valid_cached_dir(struct dir_struct *dir,
 		return 0;
 	}
 
+skip_stat:
 	if (untracked->check_only != !!check_only) {
 		invalidate_directory(dir->untracked, untracked);
 		return 0;
diff --git a/dir.h b/dir.h
index 9e387551bd..ff6a00abcc 100644
--- a/dir.h
+++ b/dir.h
@@ -139,6 +139,8 @@ struct untracked_cache {
 	int gitignore_invalidated;
 	int dir_invalidated;
 	int dir_opened;
+	/* fsmonitor invalidation data */
+	unsigned int use_fsmonitor : 1;
 };
 
 struct dir_struct {
diff --git a/entry.c b/entry.c
index d2b512da90..c2d3c1079c 100644
--- a/entry.c
+++ b/entry.c
@@ -221,6 +221,7 @@ static int write_entry(struct cache_entry *ce,
 			lstat(ce->name, &st);
 		fill_stat_cache_info(ce, &st);
 		ce->ce_flags |= CE_UPDATE_IN_BASE;
+		ce->ce_flags |= CE_FSMONITOR_DIRTY;
 		state->istate->cache_changed |= CE_ENTRY_CHANGED;
 	}
 	return 0;
diff --git a/environment.c b/environment.c
index 560408953c..1afabbae8c 100644
--- a/environment.c
+++ b/environment.c
@@ -64,6 +64,7 @@ int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 unsigned long pack_size_limit_cfg;
 enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
 enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
+int core_fsmonitor;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
diff --git a/fsmonitor.c b/fsmonitor.c
new file mode 100644
index 0000000000..3ce262d47d
--- /dev/null
+++ b/fsmonitor.c
@@ -0,0 +1,238 @@
+#include "cache.h"
+#include "dir.h"
+#include "ewah/ewok.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "fsmonitor.h"
+
+#define INDEX_EXTENSION_VERSION	1
+#define HOOK_INTERFACE_VERSION		1
+
+static struct untracked_cache_dir *find_untracked_cache_dir(
+	struct untracked_cache *uc, struct untracked_cache_dir *ucd,
+	const char *name)
+{
+	const char *end;
+	struct untracked_cache_dir *dir = ucd;
+
+	if (!*name)
+		return dir;
+
+	end = strchr(name, '/');
+	if (end) {
+		dir = lookup_untracked(uc, ucd, name, end - name);
+		if (dir)
+			return find_untracked_cache_dir(uc, dir, end + 1);
+	}
+
+	return dir;
+}
+
+/* This function will be passed to ewah_each_bit() */
+static void mark_no_fsmonitor(size_t pos, void *is)
+{
+	struct index_state *istate = is;
+	struct untracked_cache_dir *dir;
+	struct cache_entry *ce = istate->cache[pos];
+
+	assert(pos < istate->cache_nr);
+	ce->ce_flags |= CE_FSMONITOR_DIRTY;
+
+	if (!istate->untracked || !istate->untracked->root)
+		return;
+
+	dir = find_untracked_cache_dir(istate->untracked, istate->untracked->root, ce->name);
+	if (dir)
+		dir->valid = 0;
+}
+
+int read_fsmonitor_extension(struct index_state *istate, const void *data,
+	unsigned long sz)
+{
+	const char *index = data;
+	uint32_t hdr_version;
+	uint32_t ewah_size;
+	int ret;
+
+	if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
+		return error("corrupt fsmonitor extension (too short)");
+
+	hdr_version = get_be32(index);
+	index += sizeof(uint32_t);
+	if (hdr_version != INDEX_EXTENSION_VERSION)
+		return error("bad fsmonitor version %d", hdr_version);
+
+	istate->fsmonitor_last_update = get_be64(index);
+	index += sizeof(uint64_t);
+
+	ewah_size = get_be32(index);
+	index += sizeof(uint32_t);
+
+	istate->fsmonitor_dirty = ewah_new();
+	ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
+	if (ret != ewah_size) {
+		ewah_free(istate->fsmonitor_dirty);
+		istate->fsmonitor_dirty = NULL;
+		return error("failed to parse ewah bitmap reading fsmonitor index extension");
+	}
+
+	return 0;
+}
+
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
+{
+	uint32_t hdr_version;
+	uint64_t tm;
+	struct ewah_bitmap *bitmap;
+	int i;
+	uint32_t ewah_start;
+	uint32_t ewah_size = 0;
+	int fixup = 0;
+
+	hdr_version = htonl(INDEX_EXTENSION_VERSION);
+	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
+
+	tm = htonll((uint64_t)istate->fsmonitor_last_update);
+	strbuf_add(sb, &tm, sizeof(uint64_t));
+	fixup = sb->len;
+	strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
+
+	ewah_start = sb->len;
+	bitmap = ewah_new();
+	for (i = 0; i < istate->cache_nr; i++)
+		if (istate->cache[i]->ce_flags & CE_FSMONITOR_DIRTY)
+			ewah_set(bitmap, i);
+	ewah_serialize_strbuf(bitmap, sb);
+	ewah_free(bitmap);
+
+	/* fix up size field */
+	ewah_size = htonl(sb->len - ewah_start);
+	memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
+}
+
+static void mark_file_dirty(struct index_state *istate, const char *name)
+{
+	struct untracked_cache_dir *dir;
+	int pos;
+
+	/* find it in the index and mark that entry as dirty */
+	pos = index_name_pos(istate, name, strlen(name));
+	if (pos >= 0)
+		istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
+
+	/*
+	 * Find the corresponding directory in the untracked cache
+	 * and mark it as invalid
+	 */
+	if (!istate->untracked || !istate->untracked->root)
+		return;
+
+	dir = find_untracked_cache_dir(istate->untracked, istate->untracked->root, name);
+	if (dir)
+		dir->valid = 0;
+}
+
+/*
+ * Call the query-fsmonitor hook passing the time of the last saved results.
+ */
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char ver[64];
+	char date[64];
+	const char *argv[4];
+
+	if (!(argv[0] = find_hook("query-fsmonitor")))
+		return -1;
+
+	snprintf(ver, sizeof(version), "%d", version);
+	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
+	argv[1] = ver;
+	argv[2] = date;
+	argv[3] = NULL;
+	cp.argv = argv;
+	cp.out = -1;
+
+	return capture_command(&cp, query_result, 1024);
+}
+
+void process_fsmonitor_extension(struct index_state *istate)
+{
+	if (!istate->fsmonitor_dirty)
+		return;
+
+	ewah_each_bit(istate->fsmonitor_dirty, mark_no_fsmonitor, istate);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
+}
+
+void refresh_by_fsmonitor(struct index_state *istate)
+{
+	static int has_run_once = 0;
+	struct strbuf query_result = STRBUF_INIT;
+	int query_success = 0;
+	size_t bol = 0; /* beginning of line */
+	uint64_t last_update;
+	char *buf, *entry;
+	int i;
+
+	if (!core_fsmonitor || has_run_once)
+		return;
+	has_run_once = 1;
+
+	/*
+	 * This could be racy so save the date/time now and the hook
+	 * should be inclusive to ensure we don't miss potential changes.
+	 */
+	last_update = getnanotime();
+
+	/*
+	 * If we have a last update time, call query-monitor for the set of
+	 * changes since that time.
+	 */
+	if (istate->fsmonitor_last_update)
+		query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+							istate->fsmonitor_last_update, &query_result);
+
+	if (query_success) {
+		/* Mark all entries returned by the monitor as dirty */
+		buf = entry = query_result.buf;
+		for (i = 0; i < query_result.len; i++) {
+			if (buf[i] != '\0')
+				continue;
+			mark_file_dirty(istate, buf + bol);
+			bol = i + 1;
+		}
+		if (bol < query_result.len)
+			mark_file_dirty(istate, buf + bol);
+
+		/* Mark all clean entries up-to-date */
+		for (i = 0; i < istate->cache_nr; i++) {
+			struct cache_entry *ce = istate->cache[i];
+			if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
+				continue;
+			ce_mark_uptodate(ce);
+		}
+
+		/*
+		 * Now that we've marked the invalid entries in the
+		 * untracked-cache itself, we can mark the untracked cache for
+		 * fsmonitor usage.
+		 */
+		if (istate->untracked)
+			istate->untracked->use_fsmonitor = 1;
+	} else {
+		/* if we can't update the cache, fall back to checking them all */
+		for (i = 0; i < istate->cache_nr; i++)
+			istate->cache[i]->ce_flags |= CE_FSMONITOR_DIRTY;
+
+		/* mark the untracked cache as unusable for fsmonitor */
+		if (istate->untracked)
+			istate->untracked->use_fsmonitor = 0;
+	}
+	strbuf_release(&query_result);
+
+	/* Now that we've updated istate, save the last_update time */
+	istate->fsmonitor_last_update = last_update;
+	istate->cache_changed |= FSMONITOR_CHANGED;
+}
diff --git a/fsmonitor.h b/fsmonitor.h
new file mode 100644
index 0000000000..57b061688f
--- /dev/null
+++ b/fsmonitor.h
@@ -0,0 +1,9 @@
+#ifndef FSMONITOR_H
+#define FSMONITOR_H
+
+int read_fsmonitor_extension(struct index_state *istate, const void *data, unsigned long sz);
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate);
+void process_fsmonitor_extension(struct index_state *istate);
+void refresh_by_fsmonitor(struct index_state *istate);
+
+#endif
diff --git a/read-cache.c b/read-cache.c
index 3339de8124..5b52f08db2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -18,6 +18,7 @@
 #include "varint.h"
 #include "split-index.h"
 #include "utf8.h"
+#include "fsmonitor.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -37,11 +38,12 @@
 #define CACHE_EXT_RESOLVE_UNDO 0x52455543 /* "REUC" */
 #define CACHE_EXT_LINK 0x6c696e6b	  /* "link" */
 #define CACHE_EXT_UNTRACKED 0x554E5452	  /* "UNTR" */
+#define CACHE_EXT_FSMONITOR 0x46534D4E	  /* "FSMN" */
 
 /* changes that can be kept in $GIT_DIR/index (basically all extensions) */
 #define EXTMASK (RESOLVE_UNDO_CHANGED | CACHE_TREE_CHANGED | \
 		 CE_ENTRY_ADDED | CE_ENTRY_REMOVED | CE_ENTRY_CHANGED | \
-		 SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED)
+		 SPLIT_INDEX_ORDERED | UNTRACKED_CHANGED | FSMONITOR_CHANGED)
 
 struct index_state the_index;
 static const char *alternate_index_output;
@@ -61,6 +63,7 @@ static void replace_index_entry(struct index_state *istate, int nr, struct cache
 	free(old);
 	set_index_entry(istate, nr, ce);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	ce->ce_flags |= CE_FSMONITOR_DIRTY;
 	istate->cache_changed |= CE_ENTRY_CHANGED;
 }
 
@@ -777,6 +780,7 @@ int chmod_index_entry(struct index_state *istate, struct cache_entry *ce,
 	}
 	cache_tree_invalidate_path(istate, ce->name);
 	ce->ce_flags |= CE_UPDATE_IN_BASE;
+	ce->ce_flags |= CE_FSMONITOR_DIRTY;
 	istate->cache_changed |= CE_ENTRY_CHANGED;
 
 	return 0;
@@ -1344,6 +1348,8 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 	const char *added_fmt;
 	const char *unmerged_fmt;
 
+	refresh_by_fsmonitor(istate);
+
 	modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
 	deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
 	typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
@@ -1380,8 +1386,11 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 			continue;
 
 		new = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);
-		if (new == ce)
+		if (new == ce) {
+			ce->ce_flags &= ~CE_FSMONITOR_DIRTY;
 			continue;
+		}
+
 		if (!new) {
 			const char *fmt;
 
@@ -1391,6 +1400,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
 				 */
 				ce->ce_flags &= ~CE_VALID;
 				ce->ce_flags |= CE_UPDATE_IN_BASE;
+				ce->ce_flags |= CE_FSMONITOR_DIRTY;
 				istate->cache_changed |= CE_ENTRY_CHANGED;
 			}
 			if (quiet)
@@ -1549,6 +1559,9 @@ static int read_index_extension(struct index_state *istate,
 	case CACHE_EXT_UNTRACKED:
 		istate->untracked = read_untracked_extension(data, sz);
 		break;
+	case CACHE_EXT_FSMONITOR:
+		read_fsmonitor_extension(istate, data, sz);
+		break;
 	default:
 		if (*ext < 'A' || 'Z' < *ext)
 			return error("index uses %.4s extension, which we do not understand",
@@ -1721,6 +1734,7 @@ static void post_read_index_from(struct index_state *istate)
 	check_ce_order(istate);
 	tweak_untracked_cache(istate);
 	tweak_split_index(istate);
+	process_fsmonitor_extension(istate);
 }
 
 /* remember to discard_cache() before reading a different cache! */
@@ -2300,6 +2314,16 @@ static int do_write_index(struct index_state *istate, int newfd,
 		if (err)
 			return -1;
 	}
+	if (!strip_extensions && istate->fsmonitor_last_update) {
+		struct strbuf sb = STRBUF_INIT;
+
+		write_fsmonitor_extension(&sb, istate);
+		err = write_index_ext_header(&c, newfd, CACHE_EXT_FSMONITOR, sb.len) < 0
+			|| ce_write(&c, newfd, sb.buf, sb.len) < 0;
+		strbuf_release(&sb);
+		if (err)
+			return -1;
+	}
 
 	if (ce_flush(&c, newfd, istate->sha1) || fstat(newfd, &st))
 		return -1;
diff --git a/unpack-trees.c b/unpack-trees.c
index aa15111fef..259e6960b9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -412,6 +412,7 @@ static int apply_sparse_checkout(struct index_state *istate,
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
 	if (was_skip_worktree != ce_skip_worktree(ce)) {
 		ce->ce_flags |= CE_UPDATE_IN_BASE;
+		ce->ce_flags |= CE_FSMONITOR_DIRTY;
 		istate->cache_changed |= CE_ENTRY_CHANGED;
 	}
 
-- 
2.13.0.windows.1.9.gc201c67b71

[PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

Add test cases that ensure status results are correct when using the new
fsmonitor extension.  Test untracked, modified, and new files by
ensuring the results are identical to when not using the extension.

Add a test to ensure updates to the index properly mark corresponding
entries in the index extension as dirty so that the status is correct
after commands that modify the index but don't trigger changes in the
working directory.

Add a test that verifies that if the fsmonitor extension doesn't tell
git about a change, it doesn't discover it on its own.  This ensures
git is honoring the extension and that we get the performance benefits
desired.

All test hooks output a marker file that is used to ensure the hook
was actually used to generate the test results.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 t/t7519-status-fsmonitor.sh | 158 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100755 t/t7519-status-fsmonitor.sh

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
new file mode 100755
index 0000000000..395db46d55
--- /dev/null
+++ b/t/t7519-status-fsmonitor.sh
@@ -0,0 +1,158 @@
+#!/bin/sh
+
+test_description='git status with file system watcher'
+
+. ./test-lib.sh
+
+clean_repo () {
+	git reset --hard HEAD
+	git clean -fd
+	rm -f marker
+}
+
+dirty_repo () {
+	: >untracked
+	: >dir1/untracked
+	: >dir2/untracked
+	echo 1 >modified
+	echo 2 >dir1/modified
+	echo 3 >dir2/modified
+	echo 4 >new
+	echo 5 >dir1/new
+	echo 6 >dir2/new
+	git add new
+	git add dir1/new
+	git add dir2/new
+}
+
+# The test query-fsmonitor hook proc will output a marker file we can use to
+# ensure the hook was actually used to generate the correct results.
+
+test_expect_success 'setup' '
+	mkdir -p .git/hooks &&
+	write_script .git/hooks/query-fsmonitor<<-\EOF &&
+	if [ $1 -ne 1 ]
+	then
+		echo -e "Unsupported query-fsmonitor hook version.\n" >&2
+		exit 1;
+	fi
+	: >marker
+	printf "untracked\0"
+	printf "dir1/untracked\0"
+	printf "dir2/untracked\0"
+	printf "modified\0"
+	printf "dir1/modified\0"
+	printf "dir2/modified\0"
+	printf "new\0""
+	printf "dir1/new\0"
+	printf "dir2/new\0"
+	EOF
+	: >tracked &&
+	: >modified &&
+	mkdir dir1 &&
+	: >dir1/tracked &&
+	: >dir1/modified &&
+	mkdir dir2 &&
+	: >dir2/tracked &&
+	: >dir2/modified &&
+	git add . &&
+	test_tick &&
+	git commit -m initial &&
+	dirty_repo
+'
+
+cat >.gitignore <<\EOF
+.gitignore
+expect*
+output*
+marker*
+EOF
+
+# Status is well tested elsewhere so we'll just ensure that the results are
+# the same when using core.fsmonitor. First call after turning on the option
+# does a complete scan so need to do two calls to ensure we test the new
+# codepath.
+
+test_expect_success 'status with core.untrackedcache true' '
+	git config core.fsmonitor true  &&
+	git config core.untrackedcache true &&
+	git -c core.fsmonitor=false -c core.untrackedcache=true status >expect &&
+	clean_repo &&
+	git status &&
+	test_path_is_missing marker &&
+	dirty_repo &&
+	git status >output &&
+	test_path_is_file marker &&
+	test_i18ncmp expect output
+'
+
+
+test_expect_success 'status with core.untrackedcache false' '
+	git config core.fsmonitor true &&
+	git config core.untrackedcache false &&
+	git -c core.fsmonitor=false -c core.untrackedcache=false status >expect &&
+	clean_repo &&
+	git status &&
+	test_path_is_missing marker &&
+	dirty_repo &&
+	git status >output &&
+	test_path_is_file marker &&
+	test_i18ncmp expect output
+'
+
+# Ensure commands that call refresh_index() to move the index back in time
+# properly invalidate the fsmonitor cache
+
+test_expect_success 'refresh_index() invalidates fsmonitor cache' '
+	git config core.fsmonitor true &&
+	git config core.untrackedcache true &&
+	clean_repo &&
+	git status &&
+	test_path_is_missing marker &&
+	dirty_repo &&
+	write_script .git/hooks/query-fsmonitor<<-\EOF &&
+	:>marker
+	EOF
+	git add . &&
+	git commit -m "to reset" &&
+	git status &&
+	test_path_is_file marker &&
+	git reset HEAD~1 &&
+	git status >output &&
+	test_path_is_file marker &&
+	git -c core.fsmonitor=false status >expect &&
+	test_i18ncmp expect output
+'
+
+# Now make sure it's actually skipping the check for modified and untracked
+# files unless it is told about them.  Note, after "git reset --hard HEAD" no
+# extensions exist other than 'TREE' so do a "git status" to get the extension
+# written before testing the results.
+
+test_expect_success 'status doesnt detect unreported modifications' '
+	git config core.fsmonitor true &&
+	git config core.untrackedcache true &&
+	write_script .git/hooks/query-fsmonitor<<-\EOF &&
+	:>marker
+	EOF
+	clean_repo &&
+	git status &&
+	test_path_is_missing marker &&
+	: >untracked &&
+	echo 2 >dir1/modified &&
+	git status >output &&
+	test_path_is_file marker &&
+	test_i18ngrep ! "Changes not staged for commit:" output &&
+	test_i18ngrep ! "Untracked files:" output &&
+	write_script .git/hooks/query-fsmonitor<<-\EOF &&
+	:>marker
+	printf "untracked%s\0"
+	printf "dir1/modified\0"
+	EOF
+	git status >output &&
+	test_path_is_file marker &&
+	test_i18ngrep "Changes not staged for commit:" output &&
+	test_i18ngrep "Untracked files:" output
+'
+
+test_done
-- 
2.13.0.windows.1.9.gc201c67b71

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> wrote:

[...]

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
> new file mode 100755
> index 0000000000..395db46d55
> --- /dev/null
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -0,0 +1,158 @@
> +#!/bin/sh
> +
> +test_description='git status with file system watcher'
> +
> +. ./test-lib.sh
> +
> +clean_repo () {
> +       git reset --hard HEAD
> +       git clean -fd
> +       rm -f marker
> +}

Maybe link all the commands in the function with "&&".

> +dirty_repo () {
> +       : >untracked
> +       : >dir1/untracked
> +       : >dir2/untracked
> +       echo 1 >modified
> +       echo 2 >dir1/modified
> +       echo 3 >dir2/modified
> +       echo 4 >new
> +       echo 5 >dir1/new
> +       echo 6 >dir2/new
> +       git add new
> +       git add dir1/new
> +       git add dir2/new
> +}

Idem.

> +# The test query-fsmonitor hook proc will output a marker file we can use to
> +# ensure the hook was actually used to generate the correct results.
> +
> +test_expect_success 'setup' '
> +       mkdir -p .git/hooks &&
> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +       if [ $1 -ne 1 ]
> +       then
> +               echo -e "Unsupported query-fsmonitor hook version.\n" >&2
> +               exit 1;
> +       fi
> +       : >marker
> +       printf "untracked\0"
> +       printf "dir1/untracked\0"
> +       printf "dir2/untracked\0"
> +       printf "modified\0"
> +       printf "dir1/modified\0"
> +       printf "dir2/modified\0"
> +       printf "new\0""
> +       printf "dir1/new\0"
> +       printf "dir2/new\0"

Maybe something like the following to save a few lines and remove some
redundancies:

       printf "%s\0" untracked dir1/untracked dir2/untracked \
                             modified dir1/modified dir2/modified \
                             new dir1/new dir2/new

or perhaps:

       for f in untracked modified new
       do
              printf "%s\0" "$f" "dir1/$f" "dir2/$f"
       done

> +       EOF
> +       : >tracked &&
> +       : >modified &&
> +       mkdir dir1 &&
> +       : >dir1/tracked &&
> +       : >dir1/modified &&
> +       mkdir dir2 &&
> +       : >dir2/tracked &&
> +       : >dir2/modified &&
> +       git add . &&
> +       test_tick &&
> +       git commit -m initial &&
> +       dirty_repo
> +'
> +
> +cat >.gitignore <<\EOF
> +.gitignore
> +expect*
> +output*
> +marker*
> +EOF

This could be part of the previous setup test.

> +# Status is well tested elsewhere so we'll just ensure that the results are
> +# the same when using core.fsmonitor. First call after turning on the option
> +# does a complete scan so need to do two calls to ensure we test the new
> +# codepath.
> +
> +test_expect_success 'status with core.untrackedcache true' '

If this test is using untracked cache, it should perhaps first check
that untracked cache can be used on the current file system.
t7063-status-untracked-cache.sh does that with the following:

test_lazy_prereq UNTRACKED_CACHE '
    { git update-index --test-untracked-cache; ret=$?; } &&
    test $ret -ne 1
'

if ! test_have_prereq UNTRACKED_CACHE; then
    skip_all='This system does not support untracked cache'
    test_done
fi

> +       git config core.fsmonitor true  &&
> +       git config core.untrackedcache true &&
> +       git -c core.fsmonitor=false -c core.untrackedcache=true status >expect &&

I don't understand why there is " -c core.untrackedcache=true" in the
above command as you already set core.untrackedcache to true on the
previous line.

> +       clean_repo &&
> +       git status &&
> +       test_path_is_missing marker &&
> +       dirty_repo &&
> +       git status >output &&
> +       test_path_is_file marker &&
> +       test_i18ncmp expect output
> +'
> +
> +

Spurious new line.

> +test_expect_success 'status with core.untrackedcache false' '
> +       git config core.fsmonitor true &&
> +       git config core.untrackedcache false &&
> +       git -c core.fsmonitor=false -c core.untrackedcache=false status >expect &&

Again core.untrackedcache is already set to false on the previous line.

> +       clean_repo &&
> +       git status &&
> +       test_path_is_missing marker &&
> +       dirty_repo &&
> +       git status >output &&
> +       test_path_is_file marker &&
> +       test_i18ncmp expect output
> +'
> +
> +# Ensure commands that call refresh_index() to move the index back in time
> +# properly invalidate the fsmonitor cache
> +
> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> +       git config core.fsmonitor true &&
> +       git config core.untrackedcache true &&
> +       clean_repo &&
> +       git status &&
> +       test_path_is_missing marker &&
> +       dirty_repo &&
> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +       :>marker
> +       EOF
> +       git add . &&
> +       git commit -m "to reset" &&
> +       git status &&
> +       test_path_is_file marker &&
> +       git reset HEAD~1 &&
> +       git status >output &&
> +       test_path_is_file marker &&

You already checked that "marker" exists 3 lines above, and as far as
I can see nothing could remove this file since the previous test, as
the hook can only create it.
So I wonder if something is missing or if this test is redundant.

> +       git -c core.fsmonitor=false status >expect &&
> +       test_i18ncmp expect output
> +'
> +
> +# Now make sure it's actually skipping the check for modified and untracked
> +# files unless it is told about them.  Note, after "git reset --hard HEAD" no
> +# extensions exist other than 'TREE' so do a "git status" to get the extension
> +# written before testing the results.
> +
> +test_expect_success 'status doesnt detect unreported modifications' '

Maybe:

test_expect_success "status doesn't detect unreported modifications" '

> +       git config core.fsmonitor true &&
> +       git config core.untrackedcache true &&
> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +       :>marker
> +       EOF
> +       clean_repo &&
> +       git status &&
> +       test_path_is_missing marker &&
> +       : >untracked &&
> +       echo 2 >dir1/modified &&
> +       git status >output &&
> +       test_path_is_file marker &&
> +       test_i18ngrep ! "Changes not staged for commit:" output &&
> +       test_i18ngrep ! "Untracked files:" output &&
> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
> +       :>marker
> +       printf "untracked%s\0"

Why is there a %s in the above?

> +       printf "dir1/modified\0"
> +       EOF
> +       git status >output &&
> +       test_path_is_file marker &&
> +       test_i18ngrep "Changes not staged for commit:" output &&
> +       test_i18ngrep "Untracked files:" output
> +'
> +
> +test_done
> --
> 2.13.0.windows.1.9.gc201c67b71
>

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 9:18 AM, Christian Couder wrote:
> On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> wrote:
> 
> [...]
> 
>> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
>> new file mode 100755
>> index 0000000000..395db46d55
>> --- /dev/null
>> +++ b/t/t7519-status-fsmonitor.sh
>> @@ -0,0 +1,158 @@
>> +#!/bin/sh
>> +
>> +test_description='git status with file system watcher'
>> +
>> +. ./test-lib.sh
>> +
>> +clean_repo () {
>> +       git reset --hard HEAD
>> +       git clean -fd
>> +       rm -f marker
>> +}
> 
> Maybe link all the commands in the function with "&&".
> 
>> +dirty_repo () {
>> +       : >untracked
>> +       : >dir1/untracked
>> +       : >dir2/untracked
>> +       echo 1 >modified
>> +       echo 2 >dir1/modified
>> +       echo 3 >dir2/modified
>> +       echo 4 >new
>> +       echo 5 >dir1/new
>> +       echo 6 >dir2/new
>> +       git add new
>> +       git add dir1/new
>> +       git add dir2/new
>> +}
> 
> Idem.
> 

I did a quick search through the existing test scripts and the majority 
do not link commands together with && when they are in a sub function 
like this.  I find not having them linked together is easier to write, 
maintain and is more readable.

>> +# The test query-fsmonitor hook proc will output a marker file we can use to
>> +# ensure the hook was actually used to generate the correct results.
>> +
>> +test_expect_success 'setup' '
>> +       mkdir -p .git/hooks &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       if [ $1 -ne 1 ]
>> +       then
>> +               echo -e "Unsupported query-fsmonitor hook version.\n" >&2
>> +               exit 1;
>> +       fi
>> +       : >marker
>> +       printf "untracked\0"
>> +       printf "dir1/untracked\0"
>> +       printf "dir2/untracked\0"
>> +       printf "modified\0"
>> +       printf "dir1/modified\0"
>> +       printf "dir2/modified\0"
>> +       printf "new\0""
>> +       printf "dir1/new\0"
>> +       printf "dir2/new\0"
> 
> Maybe something like the following to save a few lines and remove some
> redundancies:
> 
>         printf "%s\0" untracked dir1/untracked dir2/untracked \
>                               modified dir1/modified dir2/modified \
>                               new dir1/new dir2/new
> 
> or perhaps:
> 
>         for f in untracked modified new
>         do
>                printf "%s\0" "$f" "dir1/$f" "dir2/$f"
>         done
> 

That is a clever solution that certainly is fewer lines of code. 
However, I have to read the loop and think through the logic to figure 
out what it is doing vs the existing implementation where what it is 
doing is apparent from just glancing at the code.  I was also trying to 
maintain consistency with the other status test code in t7508-status.sh

>> +       EOF
>> +       : >tracked &&
>> +       : >modified &&
>> +       mkdir dir1 &&
>> +       : >dir1/tracked &&
>> +       : >dir1/modified &&
>> +       mkdir dir2 &&
>> +       : >dir2/tracked &&
>> +       : >dir2/modified &&
>> +       git add . &&
>> +       test_tick &&
>> +       git commit -m initial &&
>> +       dirty_repo
>> +'
>> +
>> +cat >.gitignore <<\EOF
>> +.gitignore
>> +expect*
>> +output*
>> +marker*
>> +EOF
> 
> This could be part of the previous setup test.
> 

I had followed the pattern in t7508-status.sh with this but I can move 
it in if that is the preferred model.

>> +# Status is well tested elsewhere so we'll just ensure that the results are
>> +# the same when using core.fsmonitor. First call after turning on the option
>> +# does a complete scan so need to do two calls to ensure we test the new
>> +# codepath.
>> +
>> +test_expect_success 'status with core.untrackedcache true' '
> 
> If this test is using untracked cache, it should perhaps first check
> that untracked cache can be used on the current file system.
> t7063-status-untracked-cache.sh does that with the following:
> 
> test_lazy_prereq UNTRACKED_CACHE '
>      { git update-index --test-untracked-cache; ret=$?; } &&
>      test $ret -ne 1
> '
> 
> if ! test_have_prereq UNTRACKED_CACHE; then
>      skip_all='This system does not support untracked cache'
>      test_done
> fi
> 

Good point. I'll change it so that untracked cache is only turned on if 
it is available and that the one test that requires it is skipped if it 
isn't available.

>> +       git config core.fsmonitor true  &&
>> +       git config core.untrackedcache true &&
>> +       git -c core.fsmonitor=false -c core.untrackedcache=true status >expect &&
> 
> I don't understand why there is " -c core.untrackedcache=true" in the
> above command as you already set core.untrackedcache to true on the
> previous line.
> 

Defensive programming. :) The global setting was to ensure it was set 
when the test sub-functions clean and dirty were called and the command 
line settings were used to make it explicit what was being tested.  I 
can remove them if it is causing confusion.

>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       dirty_repo &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +
> 
> Spurious new line.
> 

Fixed

>> +test_expect_success 'status with core.untrackedcache false' '
>> +       git config core.fsmonitor true &&
>> +       git config core.untrackedcache false &&
>> +       git -c core.fsmonitor=false -c core.untrackedcache=false status >expect &&
> 
> Again core.untrackedcache is already set to false on the previous line.
> 
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       dirty_repo &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +# Ensure commands that call refresh_index() to move the index back in time
>> +# properly invalidate the fsmonitor cache
>> +
>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
>> +       git config core.fsmonitor true &&
>> +       git config core.untrackedcache true &&
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       dirty_repo &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       :>marker
>> +       EOF
>> +       git add . &&
>> +       git commit -m "to reset" &&
>> +       git status &&
>> +       test_path_is_file marker &&
>> +       git reset HEAD~1 &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
> 
> You already checked that "marker" exists 3 lines above, and as far as
> I can see nothing could remove this file since the previous test, as
> the hook can only create it.
> So I wonder if something is missing or if this test is redundant.
> 

Testing it each time ensures it is being created when it is supposed to 
be (ie when the test believes it is using the query-fsmonitor hook) and 
that it isn't when it isn't supposed to be (ie when the hook should not 
be called).

>> +       git -c core.fsmonitor=false status >expect &&
>> +       test_i18ncmp expect output
>> +'
>> +
>> +# Now make sure it's actually skipping the check for modified and untracked
>> +# files unless it is told about them.  Note, after "git reset --hard HEAD" no
>> +# extensions exist other than 'TREE' so do a "git status" to get the extension
>> +# written before testing the results.
>> +
>> +test_expect_success 'status doesnt detect unreported modifications' '
> 
> Maybe:
> 
> test_expect_success "status doesn't detect unreported modifications" '

Fixed

> 
>> +       git config core.fsmonitor true &&
>> +       git config core.untrackedcache true &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       :>marker
>> +       EOF
>> +       clean_repo &&
>> +       git status &&
>> +       test_path_is_missing marker &&
>> +       : >untracked &&
>> +       echo 2 >dir1/modified &&
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ngrep ! "Changes not staged for commit:" output &&
>> +       test_i18ngrep ! "Untracked files:" output &&
>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>> +       :>marker
>> +       printf "untracked%s\0"
> 
> Why is there a %s in the above?

Fixed

> 
>> +       printf "dir1/modified\0"
>> +       EOF
>> +       git status >output &&
>> +       test_path_is_file marker &&
>> +       test_i18ngrep "Changes not staged for commit:" output &&
>> +       test_i18ngrep "Untracked files:" output
>> +'
>> +
>> +test_done
>> --
>> 2.13.0.windows.1.9.gc201c67b71
>>

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Junio C Hamano]

Ben Peart <peartben@gmail.com> writes:

> I did a quick search through the existing test scripts and the
> majority do not link commands together with && when they are in a sub
> function like this.  I find not having them linked together is easier
> to write, maintain and is more readable.

I had an impression that it is true only when the series of commands
are not about Git.  When testing git commands, we should expect any
of them to be broken (by somebody else ;-) and prepare to notice it.

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/30/2017 6:37 PM, Junio C Hamano wrote:
> Ben Peart <peartben@gmail.com> writes:
> 
>> I did a quick search through the existing test scripts and the
>> majority do not link commands together with && when they are in a sub
>> function like this.  I find not having them linked together is easier
>> to write, maintain and is more readable.
> 
> I had an impression that it is true only when the series of commands
> are not about Git.  When testing git commands, we should expect any
> of them to be broken (by somebody else ;-) and prepare to notice it.
> 

Fixed

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Christian Couder]

On Tue, May 30, 2017 at 11:21 PM, Ben Peart <peartben@gmail.com> wrote:
>
> On 5/30/2017 9:18 AM, Christian Couder wrote:
>>
>> On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> wrote:
>>
>>> +       printf "untracked\0"
>>> +       printf "dir1/untracked\0"
>>> +       printf "dir2/untracked\0"
>>> +       printf "modified\0"
>>> +       printf "dir1/modified\0"
>>> +       printf "dir2/modified\0"
>>> +       printf "new\0""
>>> +       printf "dir1/new\0"
>>> +       printf "dir2/new\0"
>>
>> Maybe something like the following to save a few lines and remove some
>> redundancies:
>>
>>         printf "%s\0" untracked dir1/untracked dir2/untracked \
>>                               modified dir1/modified dir2/modified \
>>                               new dir1/new dir2/new
>>
>> or perhaps:
>>
>>         for f in untracked modified new
>>         do
>>                printf "%s\0" "$f" "dir1/$f" "dir2/$f"
>>         done
>
> That is a clever solution that certainly is fewer lines of code. However, I
> have to read the loop and think through the logic to figure out what it is
> doing vs the existing implementation where what it is doing is apparent from
> just glancing at the code.  I was also trying to maintain consistency with
> the other status test code in t7508-status.sh

Ok fair enough.

>>> +       EOF
>>> +       : >tracked &&
>>> +       : >modified &&
>>> +       mkdir dir1 &&
>>> +       : >dir1/tracked &&
>>> +       : >dir1/modified &&
>>> +       mkdir dir2 &&
>>> +       : >dir2/tracked &&
>>> +       : >dir2/modified &&
>>> +       git add . &&
>>> +       test_tick &&
>>> +       git commit -m initial &&
>>> +       dirty_repo
>>> +'
>>> +
>>> +cat >.gitignore <<\EOF
>>> +.gitignore
>>> +expect*
>>> +output*
>>> +marker*
>>> +EOF
>>
>> This could be part of the previous setup test.
>
> I had followed the pattern in t7508-status.sh with this but I can move it in
> if that is the preferred model.

Yeah, I think it is preferred these days to have all the setup code
inside tests.

>>> +       git config core.fsmonitor true  &&
>>> +       git config core.untrackedcache true &&
>>> +       git -c core.fsmonitor=false -c core.untrackedcache=true status
>>> >expect &&
>>
>> I don't understand why there is " -c core.untrackedcache=true" in the
>> above command as you already set core.untrackedcache to true on the
>> previous line.
>
> Defensive programming. :) The global setting was to ensure it was set when
> the test sub-functions clean and dirty were called and the command line
> settings were used to make it explicit what was being tested.  I can remove
> them if it is causing confusion.

I think it is a bit confusing indeed.

>>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
>>> +       git config core.fsmonitor true &&
>>> +       git config core.untrackedcache true &&
>>> +       clean_repo &&
>>> +       git status &&
>>> +       test_path_is_missing marker &&
>>> +       dirty_repo &&
>>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>>> +       :>marker
>>> +       EOF
>>> +       git add . &&
>>> +       git commit -m "to reset" &&
>>> +       git status &&
>>> +       test_path_is_file marker &&

Ok so "marker" is there now.

>>> +       git reset HEAD~1 &&
>>> +       git status >output &&
>>> +       test_path_is_file marker &&
>>
>> You already checked that "marker" exists 3 lines above, and as far as
>> I can see nothing could remove this file since the previous test, as
>> the hook can only create it.
>> So I wonder if something is missing or if this test is redundant.
>
> Testing it each time ensures it is being created when it is supposed to be
> (ie when the test believes it is using the query-fsmonitor hook) and that it
> isn't when it isn't supposed to be (ie when the hook should not be called).

I would agree with that if the "marker" file was removed after the
previous "test_path_is_file marker", but I don't see any "clean_repo"
or "rm marker" call that removes it.

Re: [PATCH v3 4/6] fsmonitor: add test cases for fsmonitor extension

[Ben Peart]

On 5/31/2017 12:33 AM, Christian Couder wrote:
>>>> +test_expect_success 'refresh_index() invalidates fsmonitor cache' '
>>>> +       git config core.fsmonitor true &&
>>>> +       git config core.untrackedcache true &&
>>>> +       clean_repo &&
>>>> +       git status &&
>>>> +       test_path_is_missing marker &&
>>>> +       dirty_repo &&
>>>> +       write_script .git/hooks/query-fsmonitor<<-\EOF &&
>>>> +       :>marker
>>>> +       EOF
>>>> +       git add . &&
>>>> +       git commit -m "to reset" &&
>>>> +       git status &&
>>>> +       test_path_is_file marker &&
> 
> Ok so "marker" is there now.
> 
>>>> +       git reset HEAD~1 &&
>>>> +       git status >output &&
>>>> +       test_path_is_file marker &&
>>>
>>> You already checked that "marker" exists 3 lines above, and as far as
>>> I can see nothing could remove this file since the previous test, as
>>> the hook can only create it.
>>> So I wonder if something is missing or if this test is redundant.
>>
>> Testing it each time ensures it is being created when it is supposed to be
>> (ie when the test believes it is using the query-fsmonitor hook) and that it
>> isn't when it isn't supposed to be (ie when the hook should not be called).
> 
> I would agree with that if the "marker" file was removed after the
> previous "test_path_is_file marker", but I don't see any "clean_repo"
> or "rm marker" call that removes it.
> 

Got it.  I've added a call to "rm -f marker" to ensure the marker isn't 
left over from the previous status command.  I found another instance of 
where this could happen in "status doesn't detect unreported 
modifications" and fixed it as well.  Thanks!

[PATCH v3 5/6] fsmonitor: add documentation for the fsmonitor extension.

[Ben Peart]

This includes the core.fsmonitor setting, the query-fsmonitor hook,
and the fsmonitor index extension.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
---
 Documentation/config.txt                 |  7 +++++++
 Documentation/githooks.txt               | 23 +++++++++++++++++++++++
 Documentation/technical/index-format.txt | 19 +++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e0b9fd0bc3..5211388167 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -389,6 +389,13 @@ core.protectNTFS::
 	8.3 "short" names.
 	Defaults to `true` on Windows, and `false` elsewhere.
 
+core.fsmonitor::
+	If set to true, call the query-fsmonitor hook proc which will
+	identify all files that may have had changes since the last
+	request. This information is used to speed up operations like
+	'git commit' and 'git status' by limiting what git must scan to
+	detect changes.
+
 core.trustctime::
 	If false, the ctime differences between the index and the
 	working tree are ignored; useful when the inode change time
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 706091a569..48127e8729 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -448,6 +448,29 @@ The commits are guaranteed to be listed in the order that they were
 processed by rebase.
 
 
+[[query-fsmonitor]]
+query-fsmonitor
+~~~~~~~~~~~~~~~
+
+This hook is invoked when the configuration option core.fsmonitor is
+set and git needs to identify changed or untracked files.  It takes
+two arguments, a version (currently 1) and the time in elapsed
+nanoseconds since midnight, January 1, 1970.
+
+The hook should output to stdout the list of all files in the working
+directory that may have changed since the requested time.  The logic
+should be inclusive so that it does not miss any potential changes.
+The paths should be relative to the root of the working directory
+and be separated by a single NUL.
+
+Git will limit what files it checks for changes as well as which
+directories are checked for untracked files based on the path names
+given.
+
+The exit status determines whether git will use the data from the
+hook to limit its search.  On error, it will fall back to verifying
+all files and folders.
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/Documentation/technical/index-format.txt b/Documentation/technical/index-format.txt
index ade0b0c445..7aeeea6f94 100644
--- a/Documentation/technical/index-format.txt
+++ b/Documentation/technical/index-format.txt
@@ -295,3 +295,22 @@ The remaining data of each directory block is grouped by type:
     in the previous ewah bitmap.
 
   - One NUL.
+
+== File System Monitor cache
+
+  The file system monitor cache tracks files for which the query-fsmonitor
+  hook has told us about changes.  The signature for this extension is
+  { 'F', 'S', 'M', 'N' }.
+
+  The extension starts with
+
+  - 32-bit version number: the current supported version is 1.
+
+  - 64-bit time: the extension data reflects all changes through the given
+	time which is stored as the nanoseconds elapsed since midnight,
+	January 1, 1970.
+
+  - 32-bit bitmap size: the size of the CE_FSMONITOR_DIRTY bitmap.
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+    is CE_FSMONITOR_DIRTY.
-- 
2.13.0.windows.1.9.gc201c67b71

[PATCH v3 6/6] fsmonitor: add a sample query-fsmonitor hook script for Watchman

[Ben Peart]

This hook script integrates the new fsmonitor capabilities of git with
the cross platform Watchman file watching service. To use the script:

Download and install Watchman from https://facebook.github.io/watchman/
and instruct Watchman to watch your working directory for changes
('watchman watch-project /usr/src/git').

Rename the sample integration hook from query-fsmonitor.sample to
query-fsmonitor.

Configure git to use the extension ('git config core.fsmonitor true')
and optionally turn on the untracked cache for optimal performance
('git config core.untrackedcache true').

Signed-off-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 templates/hooks--query-fsmonitor.sample | 37 +++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 templates/hooks--query-fsmonitor.sample

diff --git a/templates/hooks--query-fsmonitor.sample b/templates/hooks--query-fsmonitor.sample
new file mode 100644
index 0000000000..615f3332fa
--- /dev/null
+++ b/templates/hooks--query-fsmonitor.sample
@@ -0,0 +1,37 @@
+#!/bin/sh
+#
+# An example hook script to integrate Watchman
+# (https://facebook.github.io/watchman/) with git to provide fast
+# git status.
+#
+# The hook is passed a time in nanoseconds formatted as a string and
+# outputs to stdout all files that have been modified since the given
+# time. Paths must be relative to the root of the working tree and
+# separated by a single NUL.
+#
+# To enable this hook, rename this file to "query-fsmonitor"
+
+# check the hook interface version
+if [ $1 -eq 1 ]
+then
+	# convert nanoseconds to seconds
+	time_t=$(($2/1000000000))
+else
+	echo -e "Unsupported query-fsmonitor hook version.\nFalling back to scanning...\n" >&2
+	exit 1;
+fi
+
+# Convert unix style paths to escaped Windows style paths
+case "$(uname -s)" in
+MINGW*|MSYS_NT*)
+  GIT_WORK_TREE="$(cygpath -aw "$PWD" | sed 's,\\,\\\\,g')"
+  ;;
+*)
+  GIT_WORK_TREE="$PWD"
+  ;;
+esac
+
+# Query Watchman for all the changes since the requested time
+echo "[\"query\", \"$GIT_WORK_TREE\", {\"since\": $time_t, \"fields\":[\"name\"]}]" | \
+watchman -j | \
+perl -e 'use JSON::PP; my $o = JSON::PP->new->utf8->decode(join("", <>)); die "Watchman: $o->{'error'}.\nFalling back to scanning...\n" if defined($o->{"error"}); print(join("\0", @{$o->{"files"}}));'
-- 
2.13.0.windows.1.9.gc201c67b71

Re: [PATCH v3 6/6] fsmonitor: add a sample query-fsmonitor hook script for Watchman

[Christian Couder]

On Thu, May 25, 2017 at 8:36 PM, Ben Peart <peartben@gmail.com> wrote:

>  templates/hooks--query-fsmonitor.sample | 37 +++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 templates/hooks--query-fsmonitor.sample

Please make this file executable like the other sample hook scripts.