fsmonitor: start using an opaque token for last update

When using watchman for the fsmonitor there are race conditions when using a timestamp for the last update as noted in the watchman documentation "Using a timestamp is prone to race conditions in understanding the complete state of the file tree." https://facebook.github.io/watchman/docs/clockspec.html

Watchman uses something referred to as a clock id to remove these race conditions. In order to use a clock id for the last update, an opaque value needs to be returned by the fsmonitor hook and saved so that it can be passed back to the fsmonitor hook for the next query. This requires a new version for the fsmonitor index extension and new versions for the fsmonitor watchman hooks. We also need to make sure version 1 of the hook continues to work because users may not update their hook when they update git and we want things to continue to work on repos still using a version 1 hook.

Kevin Willford (4):
fsmonitor: change last update timestamp on the index_state to opaque
token
fsmonitor: handle version 2 of the hooks that will use opaque token
fsmonitor: add fsmonitor hook scripts for version 2
fsmonitor: update documentation for hook version and watchman hooks

 Documentation/config/core.txt              |  11 ++
 Documentation/githooks.txt                 |  13 +-
 cache.h                                    |   2 +-
 fsmonitor.c                                | 120 ++++++++++----
 t/helper/test-dump-fsmonitor.c             |   2 +-
 t/t7519-status-fsmonitor.sh                |   7 +-
 t/t7519/fsmonitor-all                      |   1 -
 t/t7519/fsmonitor-all-v2                   |  21 +++
 t/t7519/fsmonitor-watchman                 |   3 +-
 t/t7519/fsmonitor-watchman-v2              | 173 +++++++++++++++++++++
 templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------
 11 files changed, 405 insertions(+), 86 deletions(-)
 create mode 100755 t/t7519/fsmonitor-all-v2
 create mode 100755 t/t7519/fsmonitor-watchman-v2

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-510%2Fkewillford%2Ffsmonitor_opaque_token-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-510/kewillford/fsmonitor_opaque_token-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/510

gitgitgadget

From: Kevin Willford <[email protected]>

Some file system monitors might not use or take a timestamp for processing and in the case of watchman could have race conditions with using a timestamp. Watchman uses something called a clockid that is used for race free queries to it. The clockid for watchman is simply a string.

Change the fsmonitor_last_update from being a uint64_t to a char pointer so that any arbitrary data can be stored in it and passed back to the fsmonitor.

---
 cache.h                        |  2 +-
 fsmonitor.c                    | 49 ++++++++++++++++++++++------------
 t/helper/test-dump-fsmonitor.c |  2 +-
 3 files changed, 34 insertions(+), 19 deletions(-)

index 1554488d66..170c125db3 100644
@@ -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;

index 2786f47088..975f0ac890 100644
@@ -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) ? "+" : "-");

gitgitgadget

From: Kevin Willford <[email protected]>

Some file monitors like watchman will use something other than a timestamp to keep better track of what changes happen in between calls to query the fsmonitor. The clockid in watchman is a string. Now that the index is storing an opaque token for the last update the code needs to be updated to pass that opaque token to a verion 2 of the fsmonitor hook.

Because there are repos that already have version 1 of the hook and we want them to continue to work when git is updated, we need to handle both version 1 and version 2 of the hook. In order to do that a config value is being added core.fsmonitorHookVersion to force what version of the hook should be used. When this is not set it will default to -1 and then the code will attempt to call version 2 of the hook first. If that fails it will fallback to trying version 1.

---
 fsmonitor.c                 | 75 +++++++++++++++++++++++++++++++------
 t/t7519-status-fsmonitor.sh |  7 +++-
 t/t7519/fsmonitor-all       |  1 -
 t/t7519/fsmonitor-watchman  |  3 +-
 4 files changed, 71 insertions(+), 15 deletions(-)

index 9860587225..932bd9012d 100644
@@ -8,7 +8,8 @@
 
 #define INDEX_EXTENSION_VERSION1	(1)
 #define INDEX_EXTENSION_VERSION2	(2)
-#define HOOK_INTERFACE_VERSION		(1)
+#define HOOK_INTERFACE_VERSION1		(1)
+#define HOOK_INTERFACE_VERSION2		(2)
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 

@@ -25,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)
 {

@@ -158,8 +175,8 @@ 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;

@@ -167,6 +184,9 @@ void refresh_fsmonitor(struct index_state *istate)
 
 	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");

@@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)
 	 * should be inclusive to ensure we don't miss potential changes.
 	 */
 	last_update = getnanotime();
-	strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
+	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);

index cf0fda2d5a..fbfdcca000 100755
@@ -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 &&

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

index d8e7a1e5ba..264b9daf83 100755
@@ -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;

gitgitgadget

From: Kevin Willford <[email protected]>

Version 2 of the fsmonitor hooks is passed the version and an update token and must pass back a last update token to use for subsequent calls to the hook.

---
 t/t7519/fsmonitor-all-v2                   |  21 +++
 t/t7519/fsmonitor-watchman-v2              | 173 +++++++++++++++++++++
 templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------
 3 files changed, 281 insertions(+), 51 deletions(-)
 create mode 100755 t/t7519/fsmonitor-all-v2
 create mode 100755 t/t7519/fsmonitor-watchman-v2

new file mode 100755
index 0000000000..061907e88b
@@ -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"

gitgitgadget

From: Kevin Willford <[email protected]>

A new config value for core.fsmonitorHookVersion was added to be able to force the version of the fsmonitor hook. Possible values are 1 or 2. When this is not set the code will use a value of -1 and attempt to use version 2 of the hook first and if that fails will attempt version 1.

---
 Documentation/config/core.txt | 11 +++++++++++
 Documentation/githooks.txt    | 13 ++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

index 9e440b160d..74619a9c03 100644
@@ -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

index 50365f2914..3dccab5375 100644
@@ -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

gitgitgadget

Only change for this version is to sync the watchman script that is in templates with the one in t/t7519 since it had some commented out debug code that could be useful.

Also haven't received any feedback on this patch series which would be good to make sure I'm not making any obvious mistakes.

Thanks!

Kevin Willford (4):
fsmonitor: change last update timestamp on the index_state to opaque
token
fsmonitor: handle version 2 of the hooks that will use opaque token
fsmonitor: add fsmonitor hook scripts for version 2
fsmonitor: update documentation for hook version and watchman hooks

 Documentation/config/core.txt              |  11 ++
 Documentation/githooks.txt                 |  13 +-
 cache.h                                    |   2 +-
 fsmonitor.c                                | 120 ++++++++++----
 t/helper/test-dump-fsmonitor.c             |   2 +-
 t/t7519-status-fsmonitor.sh                |   7 +-
 t/t7519/fsmonitor-all                      |   1 -
 t/t7519/fsmonitor-all-v2                   |  21 +++
 t/t7519/fsmonitor-watchman                 |   3 +-
 t/t7519/fsmonitor-watchman-v2              | 173 +++++++++++++++++++++
 templates/hooks--fsmonitor-watchman.sample | 168 +++++++++++++-------
 11 files changed, 434 insertions(+), 87 deletions(-)
 create mode 100755 t/t7519/fsmonitor-all-v2
 create mode 100755 t/t7519/fsmonitor-watchman-v2

base-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-510%2Fkewillford%2Ffsmonitor_opaque_token-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-510/kewillford/fsmonitor_opaque_token-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/510

Range-diff vs v1:

 1:  679bf4e0dd = 1:  679bf4e0dd fsmonitor: change last update timestamp on the index_state to opaque token
 2:  f969c4bc17 = 2:  f969c4bc17 fsmonitor: handle version 2 of the hooks that will use opaque token
 3:  e1fd5924d0 ! 3:  ecc288c848 fsmonitor: add fsmonitor hook scripts for version 2
     @@ -236,13 +236,16 @@ $^
      -my ($version, $time) = @ARGV;
      +my ($version, $last_update_token) = @ARGV;
       
     - # Check the hook interface version
     --
     +-# Check the hook interface version
     ++# Uncomment for debugging
     ++# print STDERR "$0 $version $last_update_token\n";
     + 
      -if ($version == 1) {
      -	# convert nanoseconds to seconds
      -	# subtract one second to make sure watchman will return all changes
      -	$time = int ($time / 1000000000) - 1;
      -} else {
     ++# Check the hook interface version
      +if ($version ne 2) {
       	die "Unsupported query-fsmonitor hook version '$version'.\n" .
       	    "Falling back to scanning...\n";
     @@ -273,7 +276,7 @@ $^
       launch_watchman();
       
       sub launch_watchman {
     -+	$o = watchman_query();
     ++	my $o = watchman_query();
      +	if (is_work_tree_watched($o)) {
      +		output_result($o->{clock}, @{$o->{files}});
      +	}
     @@ -281,6 +284,12 @@ $^
      +
      +sub output_result {
      +	my ($clockid, @files) = @_;
     + 
     ++	# Uncomment for debugging watchman output
     ++	# open (my $fh, ">", ".git/watchman-output.out");
     ++	# binmode $fh, ":utf8";
     ++	# print $fh "$clockid\n@files\n";
     ++	# close $fh;
      +
      +	binmode STDOUT, ":utf8";
      +	print $clockid;
     @@ -293,7 +302,7 @@ $^
      +	my $response = qx/watchman clock "$git_work_tree"/;
      +	die "Failed to get clock id on '$git_work_tree'.\n" .
      +		"Falling back to scanning...\n" if $? != 0;
     - 
     ++
      +	return $json_pkg->new->utf8->decode($response);
      +}
      +
     @@ -328,9 +337,20 @@ $^
       		}]
       	END
       
     -@@
     ++	# Uncomment for debugging the watchman query
     ++	# open (my $fh, ">", ".git/watchman-query.json");
     ++	# print $fh $query;
     ++	# close $fh;
     ++
     + 	print CHLD_IN $query;
     + 	close CHLD_IN;
       	my $response = do {local $/; <CHLD_OUT>};
       
     ++	# Uncomment for debugging the watch response
     ++	# open ($fh, ">", ".git/watchman-response.json");
     ++	# print $fh $response;
     ++	# close $fh;
     ++
       	die "Watchman: command returned no output.\n" .
      -	    "Falling back to scanning...\n" if $response eq "";
      +	"Falling back to scanning...\n" if $response eq "";
     @@ -358,11 +378,23 @@ $^
      +
      +sub is_work_tree_watched {
      +	my ($output) = @_;
     -+	if ($retry > 0 and $output->{error} and $output->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {
     ++	my $error = $output->{error};
     ++	if ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {
       		$retry--;
     - 		qx/watchman watch "$git_work_tree"/;
     +-		qx/watchman watch "$git_work_tree"/;
     ++		my $response = qx/watchman watch "$git_work_tree"/;
       		die "Failed to make watchman watch '$git_work_tree'.\n" .
     -@@
     + 		    "Falling back to scanning...\n" if $? != 0;
     ++		$output = $json_pkg->new->utf8->decode($response);
     ++		$error = $output->{error};
     ++		die "Watchman: $error.\n" .
     ++		"Falling back to scanning...\n" if $error;
     ++
     ++		# Uncomment for debugging watchman output
     ++		# open (my $fh, ">", ".git/watchman-output.out");
     ++		# close $fh;
     + 
     + 		# Watchman will always return all files on the first query so
       		# return the fast "everything is dirty" flag to git and do the
       		# Watchman query just to get it over with now so we won't pay
       		# the cost in git to look up each individual file.
     @@ -383,8 +415,8 @@ $^
       
      -	die "Watchman: $o->{error}.\n" .
      -	    "Falling back to scanning...\n" if $o->{error};
     -+	die "Watchman: $output->{error}.\n" .
     -+	"Falling back to scanning...\n" if $output->{error};
     ++	die "Watchman: $error.\n" .
     ++	"Falling back to scanning...\n" if $error;
       
      -	binmode STDOUT, ":utf8";
      -	local $, = "\0";

4: 8d381b7d44 = 4: 1db2a699ba fsmonitor: update documentation for hook version and watchman hooks

gitgitgadget

From: Kevin Willford <[email protected]>

Some file system monitors might not use or take a timestamp for processing and in the case of watchman could have race conditions with using a timestamp. Watchman uses something called a clockid that is used for race free queries to it. The clockid for watchman is simply a string.

Change the fsmonitor_last_update from being a uint64_t to a char pointer so that any arbitrary data can be stored in it and passed back to the fsmonitor.

---
 cache.h                        |  2 +-
 fsmonitor.c                    | 49 ++++++++++++++++++++++------------
 t/helper/test-dump-fsmonitor.c |  2 +-
 3 files changed, 34 insertions(+), 19 deletions(-)

index 1554488d66..170c125db3 100644
@@ -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;

index 2786f47088..975f0ac890 100644
@@ -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) ? "+" : "-");

gitgitgadget

From: Kevin Willford <[email protected]>

Some file monitors like watchman will use something other than a timestamp to keep better track of what changes happen in between calls to query the fsmonitor. The clockid in watchman is a string. Now that the index is storing an opaque token for the last update the code needs to be updated to pass that opaque token to a verion 2 of the fsmonitor hook.

Because there are repos that already have version 1 of the hook and we want them to continue to work when git is updated, we need to handle both version 1 and version 2 of the hook. In order to do that a config value is being added core.fsmonitorHookVersion to force what version of the hook should be used. When this is not set it will default to -1 and then the code will attempt to call version 2 of the hook first. If that fails it will fallback to trying version 1.

---
 fsmonitor.c                 | 75 +++++++++++++++++++++++++++++++------
 t/t7519-status-fsmonitor.sh |  7 +++-
 t/t7519/fsmonitor-all       |  1 -
 t/t7519/fsmonitor-watchman  |  3 +-
 4 files changed, 71 insertions(+), 15 deletions(-)

index 9860587225..932bd9012d 100644
@@ -8,7 +8,8 @@
 
 #define INDEX_EXTENSION_VERSION1	(1)
 #define INDEX_EXTENSION_VERSION2	(2)
-#define HOOK_INTERFACE_VERSION		(1)
+#define HOOK_INTERFACE_VERSION1		(1)
+#define HOOK_INTERFACE_VERSION2		(2)
 
 struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);
 

@@ -25,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)
 {

@@ -158,8 +175,8 @@ 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;

@@ -167,6 +184,9 @@ void refresh_fsmonitor(struct index_state *istate)
 
 	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");

@@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)
 	 * should be inclusive to ensure we don't miss potential changes.
 	 */
 	last_update = getnanotime();
-	strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
+	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);

index cf0fda2d5a..fbfdcca000 100755
@@ -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 &&

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

index d8e7a1e5ba..264b9daf83 100755
@@ -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;

gitgitgadget

From: Kevin Willford <[email protected]>

A new config value for core.fsmonitorHookVersion was added to be able to force the version of the fsmonitor hook. Possible values are 1 or 2. When this is not set the code will use a value of -1 and attempt to use version 2 of the hook first and if that fails will attempt version 1.

---
 Documentation/config/core.txt | 11 +++++++++++
 Documentation/githooks.txt    | 13 ++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

index 9e440b160d..74619a9c03 100644
@@ -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

index 50365f2914..3dccab5375 100644
@@ -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

gitgitgadget

From: Kevin Willford <[email protected]>

Version 2 of the fsmonitor hooks is passed the version and an update token and must pass back a last update token to use for subsequent calls to the hook.

---
 t/t7519/fsmonitor-all-v2                   |  21 +++
 t/t7519/fsmonitor-watchman-v2              | 173 +++++++++++++++++++++
 templates/hooks--fsmonitor-watchman.sample | 168 +++++++++++++-------
 3 files changed, 310 insertions(+), 52 deletions(-)
 create mode 100755 t/t7519/fsmonitor-all-v2
 create mode 100755 t/t7519/fsmonitor-watchman-v2

new file mode 100755
index 0000000000..061907e88b
@@ -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"

gitgitgadget

Hi Kevin,

Hi Kevin,

	int interface_version = HOOK_INTERFACE_VERSION2;
	const char *token = istate->fsmonitor_last_update;

	if (hook_version == HOOK_INTERFACE_VERSION1) {
		interface_version = HOOK_INTERFACE_VERSION1;
		strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
		token = last_update_token.buf;
	}

		query_success = !query_fsmonitor(interface_version,
			token, &query_result);
		if (!query_success && hook_version < 0) {
			hook_version = HOOK_INTERFACE_VERSION1;
			strbuf_reset(&last_update_token);
			strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
			token = last_update_token.buf;
			query_success = !query_fsmonitor(hook_version,
				token, &query_result);
		}

		if (query_success && interface_version == HOOK_INTERFACE_VERSION2)
			bol = last_update_token.len + 1;

		if (hook_version < 0) {
			if (query_success)
				hook_version = HOOK_INTERFACE_VERSION2;
			else {
				hook_version = HOOK_INTERFACE_VERSION1;
				strbuf_reset(&last_update_token);
				strbuf_addf(&last_update_token, "%"PRIu64"", last_update);
				token = last_update_token.buf;
				query_success = !query_fsmonitor(hook_version,
					token, &query_result);
			}
		}

Hi Kevin,

I just read through the series (focusing on the C part) and offered a couple of suggestions, nothing major.

While I would love to see the fallback-mechanism simplified a little I think this patch series is already in a pretty good shape.

Thanks,
Dscho