fsmonitor: handle version 2 of the hooks that will use opaque token

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.

Signed-off-by: Kevin Willford <Kevin.Willford@microsoft.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/fsmonitor.c b/fsmonitor.c
index 98605872259d0dfb2b2f9a67be6abe2f3b17cc26..932bd9012daf9eb91dd445374f52a78ceade918b 100644 (file)
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -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);

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index cf0fda2d5aa534d3ed4f64be48a52201535d854f..fbfdcca000777461aed96c19eb7c2cc4b298c86c 100755 (executable)
--- a/t/t7519-status-fsmonitor.sh
+++ b/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 &&

diff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all
index 691bc94dc2c8e8e362599d43de3b39a17b959f17..94ab66bd3d8686f92d66547b79bb42adbe5e3f4a 100755 (executable)
--- a/t/t7519/fsmonitor-all
+++ b/t/t7519/fsmonitor-all
@@ -17,7 +17,6 @@ fi
 
 if test "$1" != 1
 then
-       echo "Unsupported core.fsmonitor hook version." >&2
        exit 1
 fi
 

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index d8e7a1e5ba85c0a9e7736435c923adb18bb4549c..264b9daf834ec83685c9388f03b78288d605e548 100755 (executable)
--- a/t/t7519/fsmonitor-watchman
+++ b/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;