Merge branch 'kw/fsmonitor-watchman-racefix'

A new version of fsmonitor-watchman hook has been introduced, to
avoid races.

* kw/fsmonitor-watchman-racefix:
  fsmonitor: update documentation for hook version and watchman hooks
  fsmonitor: add fsmonitor hook scripts for version 2
  fsmonitor: handle version 2 of the hooks that will use opaque token
  fsmonitor: change last update timestamp on the index_state to opaque token

Author: gitster

Documentation/config/core.txt

@@ -68,6 +68,17 @@ core.fsmonitor::
 	avoiding unnecessary processing of files that have not changed.
 	See the "fsmonitor-watchman" section of linkgit:githooks[5].
 
+core.fsmonitorHookVersion::
+	Sets the version of hook that is to be used when calling fsmonitor.
+	There are currently versions 1 and 2. When this is not set,
+	version 2 will be tried first and if it fails then version 1
+	will be tried. Version 1 uses a timestamp as input to determine
+	which files have changes since that time but some monitors
+	like watchman have race conditions when used with a timestamp.
+	Version 2 uses an opaque string so that the monitor can return
+	something that can be used to determine what files have changed
+	without race conditions.
+
 core.trustctime::
 	If false, the ctime differences between the index and the
 	working tree are ignored; useful when the inode change time

Documentation/githooks.txt

@@ -490,9 +490,16 @@ fsmonitor-watchman
 ~~~~~~~~~~~~~~~~~~
 
 This hook is invoked when the configuration option `core.fsmonitor` is
-set to `.git/hooks/fsmonitor-watchman`.  It takes two arguments, a version
-(currently 1) and the time in elapsed nanoseconds since midnight,
-January 1, 1970.
+set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`
+depending on the version of the hook to use.
+
+Version 1 takes two arguments, a version (1) and the time in elapsed
+nanoseconds since midnight, January 1, 1970.
+
+Version 2 takes two arguments, a version (2) and a token that is used
+for identifying changes since the token. For watchman this would be
+a clock id. This version must output to stdout the new token followed
+by a NUL before the list of files.
 
 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

cache.h

@@ -324,7 +324,7 @@ struct index_state {
 	struct hashmap dir_hash;
 	struct object_id oid;
 	struct untracked_cache *untracked;
-	uint64_t fsmonitor_last_update;
+	char *fsmonitor_last_update;
 	struct ewah_bitmap *fsmonitor_dirty;
 	struct mem_pool *ce_mem_pool;
 	struct progress *progress;

fsmonitor.c

@@ -6,8 +6,10 @@
 #include "run-command.h"
 #include "strbuf.h"
 
-#define INDEX_EXTENSION_VERSION	(1)
-#define HOOK_INTERFACE_VERSION	(1)
+#define INDEX_EXTENSION_VERSION1	(1)
+#define INDEX_EXTENSION_VERSION2	(2)
+#define HOOK_INTERFACE_VERSION1		(1)
+#define HOOK_INTERFACE_VERSION2		(2)
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 
@@ -24,6 +26,22 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)
 	ce->ce_flags &= ~CE_FSMONITOR_VALID;
 }
 
+static int fsmonitor_hook_version(void)
+{
+	int hook_version;
+
+	if (git_config_get_int("core.fsmonitorhookversion", &hook_version))
+		return -1;
+
+	if (hook_version == HOOK_INTERFACE_VERSION1 ||
+	    hook_version == HOOK_INTERFACE_VERSION2)
+		return hook_version;
+
+	warning("Invalid hook version '%i' in core.fsmonitorhookversion. "
+		"Must be 1 or 2.", hook_version);
+	return -1;
+}
+
 int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	unsigned long sz)
 {
@@ -32,17 +50,26 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	uint32_t ewah_size;
 	struct ewah_bitmap *fsmonitor_dirty;
 	int ret;
+	uint64_t timestamp;
+	struct strbuf last_update = STRBUF_INIT;
 
-	if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
+	if (sz < sizeof(uint32_t) + 1 + 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)
+	if (hdr_version == INDEX_EXTENSION_VERSION1) {
+		timestamp = get_be64(index);
+		strbuf_addf(&last_update, "%"PRIu64"", timestamp);
+		index += sizeof(uint64_t);
+	} else if (hdr_version == INDEX_EXTENSION_VERSION2) {
+		strbuf_addstr(&last_update, index);
+		index += last_update.len + 1;
+	} else {
 		return error("bad fsmonitor version %d", hdr_version);
+	}
 
-	istate->fsmonitor_last_update = get_be64(index);
-	index += sizeof(uint64_t);
+	istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
 
 	ewah_size = get_be32(index);
 	index += sizeof(uint32_t);
@@ -79,7 +106,6 @@ void fill_fsmonitor_bitmap(struct index_state *istate)
 void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 {
 	uint32_t hdr_version;
-	uint64_t tm;
 	uint32_t ewah_start;
 	uint32_t ewah_size = 0;
 	int fixup = 0;
@@ -89,11 +115,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 		BUG("fsmonitor_dirty has more entries than the index (%"PRIuMAX" > %u)",
 		    (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);
 
-	put_be32(&hdr_version, INDEX_EXTENSION_VERSION);
+	put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);
 	strbuf_add(sb, &hdr_version, sizeof(uint32_t));
 
-	put_be64(&tm, istate->fsmonitor_last_update);
-	strbuf_add(sb, &tm, sizeof(uint64_t));
+	strbuf_addstr(sb, istate->fsmonitor_last_update);
+	strbuf_addch(sb, 0); /* Want to keep a NUL */
+
 	fixup = sb->len;
 	strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
 
@@ -110,9 +137,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 }
 
 /*
- * Call the query-fsmonitor hook passing the time of the last saved results.
+ * Call the query-fsmonitor hook passing the last update token of the saved results.
  */
-static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 
@@ -121,7 +148,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
 
 	argv_array_push(&cp.args, core_fsmonitor);
 	argv_array_pushf(&cp.args, "%d", version);
-	argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
+	argv_array_pushf(&cp.args, "%s", last_update);
 	cp.use_shell = 1;
 	cp.dir = get_git_work_tree();
 
@@ -148,14 +175,18 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n
 void refresh_fsmonitor(struct index_state *istate)
 {
 	struct strbuf query_result = STRBUF_INIT;
-	int query_success = 0;
-	size_t bol; /* beginning of line */
+	int query_success = 0, hook_version = -1;
+	size_t bol = 0; /* beginning of line */
 	uint64_t last_update;
+	struct strbuf last_update_token = STRBUF_INIT;
 	char *buf;
 	unsigned int i;
 
 	if (!core_fsmonitor || istate->fsmonitor_has_run_once)
 		return;
+
+	hook_version = fsmonitor_hook_version();
+
 	istate->fsmonitor_has_run_once = 1;
 
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");
@@ -164,26 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)
 	 * should be inclusive to ensure we don't miss potential changes.
 	 */
 	last_update = getnanotime();
+	if (hook_version == HOOK_INTERFACE_VERSION1)
+		strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
 
 	/*
-	 * If we have a last update time, call query_fsmonitor for the set of
-	 * changes since that time, else assume everything is possibly dirty
+	 * If we have a last update token, call query_fsmonitor for the set of
+	 * changes since that token, else assume everything is possibly dirty
 	 * and check it all.
 	 */
 	if (istate->fsmonitor_last_update) {
-		query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
-			istate->fsmonitor_last_update, &query_result);
+		if (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {
+			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,
+				istate->fsmonitor_last_update, &query_result);
+
+			if (query_success) {
+				if (hook_version < 0)
+					hook_version = HOOK_INTERFACE_VERSION2;
+
+				/*
+				 * First entry will be the last update token
+				 * Need to use a char * variable because static
+				 * analysis was suggesting to use strbuf_addbuf
+				 * but we don't want to copy the entire strbuf
+				 * only the the chars up to the first NUL
+				 */
+				buf = query_result.buf;
+				strbuf_addstr(&last_update_token, buf);
+				if (!last_update_token.len) {
+					warning("Empty last update token.");
+					query_success = 0;
+				} else {
+					bol = last_update_token.len + 1;
+				}
+			} else if (hook_version < 0) {
+				hook_version = HOOK_INTERFACE_VERSION1;
+				if (!last_update_token.len)
+					strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
+			}
+		}
+
+		if (hook_version == HOOK_INTERFACE_VERSION1) {
+			query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,
+				istate->fsmonitor_last_update, &query_result);
+		}
+
 		trace_performance_since(last_update, "fsmonitor process '%s'", core_fsmonitor);
 		trace_printf_key(&trace_fsmonitor, "fsmonitor process '%s' returned %s",
 			core_fsmonitor, query_success ? "success" : "failure");
 	}
 
 	/* a fsmonitor process can return '/' to indicate all entries are invalid */
-	if (query_success && query_result.buf[0] != '/') {
+	if (query_success && query_result.buf[bol] != '/') {
 		/* Mark all entries returned by the monitor as dirty */
 		buf = query_result.buf;
-		bol = 0;
-		for (i = 0; i < query_result.len; i++) {
+		for (i = bol; i < query_result.len; i++) {
 			if (buf[i] != '\0')
 				continue;
 			fsmonitor_refresh_callback(istate, buf + bol);
@@ -217,18 +282,21 @@ void refresh_fsmonitor(struct index_state *istate)
 	}
 	strbuf_release(&query_result);
 
-	/* Now that we've updated istate, save the last_update time */
-	istate->fsmonitor_last_update = last_update;
+	/* Now that we've updated istate, save the last_update_token */
+	FREE_AND_NULL(istate->fsmonitor_last_update);
+	istate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);
 }
 
 void add_fsmonitor(struct index_state *istate)
 {
 	unsigned int i;
+	struct strbuf last_update = STRBUF_INIT;
 
 	if (!istate->fsmonitor_last_update) {
 		trace_printf_key(&trace_fsmonitor, "add fsmonitor");
 		istate->cache_changed |= FSMONITOR_CHANGED;
-		istate->fsmonitor_last_update = getnanotime();
+		strbuf_addf(&last_update, "%"PRIu64"", getnanotime());
+		istate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);
 
 		/* reset the fsmonitor state */
 		for (i = 0; i < istate->cache_nr; i++)
@@ -250,7 +318,7 @@ void remove_fsmonitor(struct index_state *istate)
 	if (istate->fsmonitor_last_update) {
 		trace_printf_key(&trace_fsmonitor, "remove fsmonitor");
 		istate->cache_changed |= FSMONITOR_CHANGED;
-		istate->fsmonitor_last_update = 0;
+		FREE_AND_NULL(istate->fsmonitor_last_update);
 	}
 }
 

t/helper/test-dump-fsmonitor.c

@@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)
 		printf("no fsmonitor\n");
 		return 0;
 	}
-	printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate->fsmonitor_last_update);
+	printf("fsmonitor last update %s\n", istate->fsmonitor_last_update);
 
 	for (i = 0; i < istate->cache_nr; i++)
 		printf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? "+" : "-");

t/t7519-status-fsmonitor.sh

@@ -32,11 +32,12 @@ write_integration_script () {
 		echo "$0: exactly 2 arguments expected"
 		exit 2
 	fi
-	if test "$1" != 1
+	if test "$1" != 2
 	then
 		echo "Unsupported core.fsmonitor hook version." >&2
 		exit 1
 	fi
+	printf "last_update_token\0"
 	printf "untracked\0"
 	printf "dir1/untracked\0"
 	printf "dir2/untracked\0"
@@ -107,6 +108,7 @@ EOF
 # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
 test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor valid bit' '
 	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+		printf "last_update_token\0"
 	EOF
 	git update-index --fsmonitor &&
 	git update-index --fsmonitor-valid dir1/modified &&
@@ -167,6 +169,7 @@ EOF
 # test that newly added files are marked valid
 test_expect_success 'newly added files are marked valid' '
 	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+		printf "last_update_token\0"
 	EOF
 	git add new &&
 	git add dir1/new &&
@@ -207,6 +210,7 @@ EOF
 # test that *only* files returned by the integration script get flagged as invalid
 test_expect_success '*only* files returned by the integration script get flagged as invalid' '
 	write_script .git/hooks/fsmonitor-test<<-\EOF &&
+	printf "last_update_token\0"
 	printf "dir1/modified\0"
 	EOF
 	clean_repo &&
@@ -276,6 +280,7 @@ do
 		# (if enabled) files unless it is told about them.
 		test_expect_success "status doesn't detect unreported modifications" '
 			write_script .git/hooks/fsmonitor-test<<-\EOF &&
+			printf "last_update_token\0"
 			:>marker
 			EOF
 			clean_repo &&

t/t7519/fsmonitor-all

@@ -17,7 +17,6 @@ fi
 
 if test "$1" != 1
 then
-	echo "Unsupported core.fsmonitor hook version." >&2
 	exit 1
 fi
 

t/t7519/fsmonitor-all-v2

@@ -0,0 +1,21 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+#
+# An test hook script to integrate with git to test fsmonitor.
+#
+# The hook is passed a version (currently 2) and since token
+# 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.
+#
+#echo "$0 $*" >&2
+my ($version, $last_update_token) = @ARGV;
+
+if ($version ne 2) {
+	print "Unsupported query-fsmonitor hook version '$version'.\n";
+	exit 1;
+}
+
+print "last_update_token\0/\0"

t/t7519/fsmonitor-watchman

@@ -26,8 +26,7 @@ if ($version == 1) {
 	# subtract one second to make sure watchman will return all changes
 	$time = int ($time / 1000000000) - 1;
 } else {
-	die "Unsupported query-fsmonitor hook version '$version'.\n" .
-	    "Falling back to scanning...\n";
+	exit 1;
 }
 
 my $git_work_tree;