{"thread":{"id":"52588","subject":"[PATCH 0/4] fsmonitor: start using an opaque token for last update","startedAt":"2020-01-07T19:04:35Z","lastAt":"2020-02-05T13:15:17Z","messageCount":13,"participants":["Kevin Willford via GitGitGadget","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"389409","messageId":"pull.510.git.1578423871.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":null,"subject":"[PATCH 0/4] fsmonitor: start using an opaque token for last update","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T19:04:27Z","receivedAt":"2020-01-07T19:04:35Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"When using watchman for the fsmonitor there are race conditions when using a\ntimestamp for the last update as noted in the watchman documentation \"Using\na timestamp is prone to race conditions in understanding the complete state\nof the file tree.\" https://facebook.github.io/watchman/docs/clockspec.html\n\nWatchman uses something referred to as a clock id to remove these race\nconditions. In order to use a clock id for the last update, an opaque value\nneeds to be returned by the fsmonitor hook and saved so that it can be\npassed back to the fsmonitor hook for the next query. This requires a new\nversion for the fsmonitor index extension and new versions for the fsmonitor\nwatchman hooks. We also need to make sure version 1 of the hook continues to\nwork because users may not update their hook when they update git and we\nwant things to continue to work on repos still using a version 1 hook.\n\nKevin Willford (4):\n fsmonitor: change last update timestamp on the index_state to opaque\n token\n fsmonitor: handle version 2 of the hooks that will use opaque token\n fsmonitor: add fsmonitor hook scripts for version 2\n fsmonitor: update documentation for hook version and watchman hooks\n\n Documentation/config/core.txt | 11 ++\n Documentation/githooks.txt | 13 +-\n cache.h | 2 +-\n fsmonitor.c | 120 ++++++++++----\n t/helper/test-dump-fsmonitor.c | 2 +-\n t/t7519-status-fsmonitor.sh | 7 +-\n t/t7519/fsmonitor-all | 1 -\n t/t7519/fsmonitor-all-v2 | 21 +++\n t/t7519/fsmonitor-watchman | 3 +-\n t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++\n templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------\n 11 files changed, 405 insertions(+), 86 deletions(-)\n create mode 100755 t/t7519/fsmonitor-all-v2\n create mode 100755 t/t7519/fsmonitor-watchman-v2\n\n\nbase-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-510%2Fkewillford%2Ffsmonitor_opaque_token-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-510/kewillford/fsmonitor_opaque_token-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/510\n-- \ngitgitgadget\n"},{"id":"389410","messageId":"679bf4e0dd16f7d6f4ba1f05af50cca24b5abee4.1578423871.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.git.1578423871.gitgitgadget@gmail.com","subject":"[PATCH 1/4] fsmonitor: change last update timestamp on the index_state to opaque token","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T19:04:28Z","receivedAt":"2020-01-07T19:04:37Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nSome file system monitors might not use or take a timestamp for processing\nand in the case of watchman could have race conditions with using a\ntimestamp. Watchman uses something called a clockid that is used for race\nfree queries to it. The clockid for watchman is simply a string.\n\nChange the fsmonitor_last_update from being a uint64_t to a char pointer\nso that any arbitrary data can be stored in it and passed back to the\nfsmonitor.\n\nSigned-off-by: Kevin Willford \n---\n cache.h | 2 +-\n fsmonitor.c | 49 ++++++++++++++++++++++------------\n t/helper/test-dump-fsmonitor.c | 2 +-\n 3 files changed, 34 insertions(+), 19 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 1554488d66..170c125db3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -324,7 +324,7 @@ struct index_state {\n \tstruct hashmap dir_hash;\n \tstruct object_id oid;\n \tstruct untracked_cache *untracked;\n-\tuint64_t fsmonitor_last_update;\n+\tchar *fsmonitor_last_update;\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 868cca01e2..9860587225 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -6,8 +6,9 @@\n #include \"run-command.h\"\n #include \"strbuf.h\"\n \n-#define INDEX_EXTENSION_VERSION\t(1)\n-#define HOOK_INTERFACE_VERSION\t(1)\n+#define INDEX_EXTENSION_VERSION1\t(1)\n+#define INDEX_EXTENSION_VERSION2\t(2)\n+#define HOOK_INTERFACE_VERSION\t\t(1)\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n@@ -32,17 +33,26 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \tuint32_t ewah_size;\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tint ret;\n+\tuint64_t timestamp;\n+\tstruct strbuf last_update = STRBUF_INIT;\n \n-\tif (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))\n+\tif (sz < sizeof(uint32_t) + 1 + sizeof(uint32_t))\n \t\treturn error(\"corrupt fsmonitor extension (too short)\");\n \n \thdr_version = get_be32(index);\n \tindex += sizeof(uint32_t);\n-\tif (hdr_version != INDEX_EXTENSION_VERSION)\n+\tif (hdr_version == INDEX_EXTENSION_VERSION1) {\n+\t\ttimestamp = get_be64(index);\n+\t\tstrbuf_addf(&last_update, \"%\"PRIu64\"\", timestamp);\n+\t\tindex += sizeof(uint64_t);\n+\t} else if (hdr_version == INDEX_EXTENSION_VERSION2) {\n+\t\tstrbuf_addstr(&last_update, index);\n+\t\tindex += last_update.len + 1;\n+\t} else {\n \t\treturn error(\"bad fsmonitor version %d\", hdr_version);\n+\t}\n \n-\tistate->fsmonitor_last_update = get_be64(index);\n-\tindex += sizeof(uint64_t);\n+\tistate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);\n \n \tewah_size = get_be32(index);\n \tindex += sizeof(uint32_t);\n@@ -79,7 +89,6 @@ void fill_fsmonitor_bitmap(struct index_state *istate)\n void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n {\n \tuint32_t hdr_version;\n-\tuint64_t tm;\n \tuint32_t ewah_start;\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n@@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n-\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n+\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n \n-\tput_be64(&tm, istate->fsmonitor_last_update);\n-\tstrbuf_add(sb, &tm, sizeof(uint64_t));\n+\tstrbuf_addstr(sb, istate->fsmonitor_last_update);\n+\tstrbuf_addch(sb, 0); /* Want to keep a NUL */\n+\n \tfixup = sb->len;\n \tstrbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */\n \n@@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n }\n \n /*\n- * Call the query-fsmonitor hook passing the time of the last saved results.\n+ * Call the query-fsmonitor hook passing the last update token of the saved results.\n */\n-static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)\n+static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n@@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n \n \targv_array_push(&cp.args, core_fsmonitor);\n \targv_array_pushf(&cp.args, \"%d\", version);\n-\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n+\targv_array_pushf(&cp.args, \"%s\", last_update);\n \tcp.use_shell = 1;\n \tcp.dir = get_git_work_tree();\n \n@@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \tint query_success = 0;\n \tsize_t bol; /* beginning of line */\n \tuint64_t last_update;\n+\tstruct strbuf last_update_token = STRBUF_INIT;\n \tchar *buf;\n \tunsigned int i;\n \n@@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \t * should be inclusive to ensure we don't miss potential changes.\n \t */\n \tlast_update = getnanotime();\n+\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n \n \t/*\n \t * If we have a last update time, call query_fsmonitor for the set of\n@@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)\n \t}\n \tstrbuf_release(&query_result);\n \n-\t/* Now that we've updated istate, save the last_update time */\n-\tistate->fsmonitor_last_update = last_update;\n+\t/* Now that we've updated istate, save the last_update_token */\n+\tFREE_AND_NULL(istate->fsmonitor_last_update);\n+\tistate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);\n }\n \n void add_fsmonitor(struct index_state *istate)\n {\n \tunsigned int i;\n+\tstruct strbuf last_update = STRBUF_INIT;\n \n \tif (!istate->fsmonitor_last_update) {\n \t\ttrace_printf_key(&trace_fsmonitor, \"add fsmonitor\");\n \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n-\t\tistate->fsmonitor_last_update = getnanotime();\n+\t\tstrbuf_addf(&last_update, \"%\"PRIu64\"\", getnanotime());\n+\t\tistate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);\n \n \t\t/* reset the fsmonitor state */\n \t\tfor (i = 0; i < istate->cache_nr; i++)\n@@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)\n \tif (istate->fsmonitor_last_update) {\n \t\ttrace_printf_key(&trace_fsmonitor, \"remove fsmonitor\");\n \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n-\t\tistate->fsmonitor_last_update = 0;\n+\t\tFREE_AND_NULL(istate->fsmonitor_last_update);\n \t}\n }\n \ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 2786f47088..975f0ac890 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)\n \t\tprintf(\"no fsmonitor\\n\");\n \t\treturn 0;\n \t}\n-\tprintf(\"fsmonitor last update %\"PRIuMAX\"\\n\", (uintmax_t)istate->fsmonitor_last_update);\n+\tprintf(\"fsmonitor last update %s\\n\", istate->fsmonitor_last_update);\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n-- \ngitgitgadget\n\n"},{"id":"389411","messageId":"f969c4bc17b79f7c857987793cb0c23ad3f4e899.1578423871.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.git.1578423871.gitgitgadget@gmail.com","subject":"[PATCH 2/4] fsmonitor: handle version 2 of the hooks that will use opaque token","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T19:04:29Z","receivedAt":"2020-01-07T19:04:40Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nSome file monitors like watchman will use something other than a timestamp\nto keep better track of what changes happen in between calls to query\nthe fsmonitor. The clockid in watchman is a string. Now that the index\nis storing an opaque token for the last update the code needs to be\nupdated to pass that opaque token to a verion 2 of the fsmonitor hook.\n\nBecause there are repos that already have version 1 of the hook and we\nwant them to continue to work when git is updated, we need to handle\nboth version 1 and version 2 of the hook. In order to do that a\nconfig value is being added core.fsmonitorHookVersion to force what\nversion of the hook should be used. When this is not set it will default\nto -1 and then the code will attempt to call version 2 of the hook first.\nIf that fails it will fallback to trying version 1.\n\nSigned-off-by: Kevin Willford \n---\n fsmonitor.c | 75 +++++++++++++++++++++++++++++++------\n t/t7519-status-fsmonitor.sh | 7 +++-\n t/t7519/fsmonitor-all | 1 -\n t/t7519/fsmonitor-watchman | 3 +-\n 4 files changed, 71 insertions(+), 15 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 9860587225..932bd9012d 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -8,7 +8,8 @@\n \n #define INDEX_EXTENSION_VERSION1\t(1)\n #define INDEX_EXTENSION_VERSION2\t(2)\n-#define HOOK_INTERFACE_VERSION\t\t(1)\n+#define HOOK_INTERFACE_VERSION1\t\t(1)\n+#define HOOK_INTERFACE_VERSION2\t\t(2)\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n@@ -25,6 +26,22 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n }\n \n+static int fsmonitor_hook_version(void)\n+{\n+\tint hook_version;\n+\n+\tif (git_config_get_int(\"core.fsmonitorhookversion\", &hook_version))\n+\t\treturn -1;\n+\n+\tif (hook_version == HOOK_INTERFACE_VERSION1 ||\n+\t hook_version == HOOK_INTERFACE_VERSION2)\n+\t\treturn hook_version;\n+\n+\twarning(\"Invalid hook version '%i' in core.fsmonitorhookversion. \"\n+\t\t\"Must be 1 or 2.\", hook_version);\n+\treturn -1;\n+}\n+\n int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \tunsigned long sz)\n {\n@@ -158,8 +175,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n\n void refresh_fsmonitor(struct index_state *istate)\n {\n \tstruct strbuf query_result = STRBUF_INIT;\n-\tint query_success = 0;\n-\tsize_t bol; /* beginning of line */\n+\tint query_success = 0, hook_version = -1;\n+\tsize_t bol = 0; /* beginning of line */\n \tuint64_t last_update;\n \tstruct strbuf last_update_token = STRBUF_INIT;\n \tchar *buf;\n@@ -167,6 +184,9 @@ void refresh_fsmonitor(struct index_state *istate)\n \n \tif (!core_fsmonitor || istate->fsmonitor_has_run_once)\n \t\treturn;\n+\n+\thook_version = fsmonitor_hook_version();\n+\n \tistate->fsmonitor_has_run_once = 1;\n \n \ttrace_printf_key(&trace_fsmonitor, \"refresh fsmonitor\");\n@@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)\n \t * should be inclusive to ensure we don't miss potential changes.\n \t */\n \tlast_update = getnanotime();\n-\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n+\tif (hook_version == HOOK_INTERFACE_VERSION1)\n+\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n \n \t/*\n-\t * If we have a last update time, call query_fsmonitor for the set of\n-\t * changes since that time, else assume everything is possibly dirty\n+\t * If we have a last update token, call query_fsmonitor for the set of\n+\t * changes since that token, else assume everything is possibly dirty\n \t * and check it all.\n \t */\n \tif (istate->fsmonitor_last_update) {\n-\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,\n-\t\t\tistate->fsmonitor_last_update, &query_result);\n+\t\tif (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {\n+\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,\n+\t\t\t\tistate->fsmonitor_last_update, &query_result);\n+\n+\t\t\tif (query_success) {\n+\t\t\t\tif (hook_version < 0)\n+\t\t\t\t\thook_version = HOOK_INTERFACE_VERSION2;\n+\n+\t\t\t\t/*\n+\t\t\t\t * First entry will be the last update token\n+\t\t\t\t * Need to use a char * variable because static\n+\t\t\t\t * analysis was suggesting to use strbuf_addbuf\n+\t\t\t\t * but we don't want to copy the entire strbuf\n+\t\t\t\t * only the the chars up to the first NUL\n+\t\t\t\t */\n+\t\t\t\tbuf = query_result.buf;\n+\t\t\t\tstrbuf_addstr(&last_update_token, buf);\n+\t\t\t\tif (!last_update_token.len) {\n+\t\t\t\t\twarning(\"Empty last update token.\");\n+\t\t\t\t\tquery_success = 0;\n+\t\t\t\t} else {\n+\t\t\t\t\tbol = last_update_token.len + 1;\n+\t\t\t\t}\n+\t\t\t} else if (hook_version < 0) {\n+\t\t\t\thook_version = HOOK_INTERFACE_VERSION1;\n+\t\t\t\tif (!last_update_token.len)\n+\t\t\t\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (hook_version == HOOK_INTERFACE_VERSION1) {\n+\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,\n+\t\t\t\tistate->fsmonitor_last_update, &query_result);\n+\t\t}\n+\n \t\ttrace_performance_since(last_update, \"fsmonitor process '%s'\", core_fsmonitor);\n \t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor process '%s' returned %s\",\n \t\t\tcore_fsmonitor, query_success ? \"success\" : \"failure\");\n \t}\n \n \t/* a fsmonitor process can return '/' to indicate all entries are invalid */\n-\tif (query_success && query_result.buf[0] != '/') {\n+\tif (query_success && query_result.buf[bol] != '/') {\n \t\t/* Mark all entries returned by the monitor as dirty */\n \t\tbuf = query_result.buf;\n-\t\tbol = 0;\n-\t\tfor (i = 0; i < query_result.len; i++) {\n+\t\tfor (i = bol; i < query_result.len; i++) {\n \t\t\tif (buf[i] != '\\0')\n \t\t\t\tcontinue;\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex cf0fda2d5a..fbfdcca000 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -32,11 +32,12 @@ write_integration_script () {\n \t\techo \"$0: exactly 2 arguments expected\"\n \t\texit 2\n \tfi\n-\tif test \"$1\" != 1\n+\tif test \"$1\" != 2\n \tthen\n \t\techo \"Unsupported core.fsmonitor hook version.\" >&2\n \t\texit 1\n \tfi\n+\tprintf \"last_update_token\\0\"\n \tprintf \"untracked\\0\"\n \tprintf \"dir1/untracked\\0\"\n \tprintf \"dir2/untracked\\0\"\n@@ -107,6 +108,7 @@ EOF\n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\tprintf \"last_update_token\\0\"\n \tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n@@ -167,6 +169,7 @@ EOF\n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\tprintf \"last_update_token\\0\"\n \tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n@@ -207,6 +210,7 @@ EOF\n # test that *only* files returned by the integration script get flagged as invalid\n test_expect_success '*only* files returned by the integration script get flagged as invalid' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tprintf \"last_update_token\\0\"\n \tprintf \"dir1/modified\\0\"\n \tEOF\n \tclean_repo &&\n@@ -276,6 +280,7 @@ do\n \t\t# (if enabled) files unless it is told about them.\n \t\ttest_expect_success \"status doesn't detect unreported modifications\" '\n \t\t\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\t\tprintf \"last_update_token\\0\"\n \t\t\t:>marker\n \t\t\tEOF\n \t\t\tclean_repo &&\ndiff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all\nindex 691bc94dc2..94ab66bd3d 100755\n--- a/t/t7519/fsmonitor-all\n+++ b/t/t7519/fsmonitor-all\n@@ -17,7 +17,6 @@ fi\n \n if test \"$1\" != 1\n then\n-\techo \"Unsupported core.fsmonitor hook version.\" >&2\n \texit 1\n fi\n \ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex d8e7a1e5ba..264b9daf83 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -26,8 +26,7 @@ if ($version == 1) {\n \t# subtract one second to make sure watchman will return all changes\n \t$time = int ($time / 1000000000) - 1;\n } else {\n-\tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n-\t \"Falling back to scanning...\\n\";\n+\texit 1;\n }\n \n my $git_work_tree;\n-- \ngitgitgadget\n\n"},{"id":"389412","messageId":"e1fd5924d0441d1f4d6cce9f504a6a91f234c486.1578423871.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.git.1578423871.gitgitgadget@gmail.com","subject":"[PATCH 3/4] fsmonitor: add fsmonitor hook scripts for version 2","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T19:04:30Z","receivedAt":"2020-01-07T19:04:40Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nVersion 2 of the fsmonitor hooks is passed the version and an update\ntoken and must pass back a last update token to use for subsequent calls\nto the hook.\n\nSigned-off-by: Kevin Willford \n---\n t/t7519/fsmonitor-all-v2 | 21 +++\n t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++\n templates/hooks--fsmonitor-watchman.sample | 138 ++++++++++------\n 3 files changed, 281 insertions(+), 51 deletions(-)\n create mode 100755 t/t7519/fsmonitor-all-v2\n create mode 100755 t/t7519/fsmonitor-watchman-v2\n\ndiff --git a/t/t7519/fsmonitor-all-v2 b/t/t7519/fsmonitor-all-v2\nnew file mode 100755\nindex 0000000000..061907e88b\n--- /dev/null\n+++ b/t/t7519/fsmonitor-all-v2\n@@ -0,0 +1,21 @@\n+#!/usr/bin/perl\n+\n+use strict;\n+use warnings;\n+#\n+# An test hook script to integrate with git to test fsmonitor.\n+#\n+# The hook is passed a version (currently 2) and since token\n+# formatted as a string and outputs to stdout all files that have been\n+# modified since the given time. Paths must be relative to the root of\n+# the working tree and separated by a single NUL.\n+#\n+#echo \"$0 $*\" >&2\n+my ($version, $last_update_token) = @ARGV;\n+\n+if ($version ne 2) {\n+\tprint \"Unsupported query-fsmonitor hook version '$version'.\\n\";\n+\texit 1;\n+}\n+\n+print \"last_update_token\\0/\\0\"\ndiff --git a/t/t7519/fsmonitor-watchman-v2 b/t/t7519/fsmonitor-watchman-v2\nnew file mode 100755\nindex 0000000000..14ed0aa42d\n--- /dev/null\n+++ b/t/t7519/fsmonitor-watchman-v2\n@@ -0,0 +1,173 @@\n+#!/usr/bin/perl\n+\n+use strict;\n+use warnings;\n+use IPC::Open2;\n+\n+# An example hook script to integrate Watchman\n+# (https://facebook.github.io/watchman/) with git to speed up detecting\n+# new and modified files.\n+#\n+# The hook is passed a version (currently 2) and last update token\n+# formatted as a string and outputs to stdout a new update token and\n+# all files that have been modified since the update token. Paths must\n+# be relative to the root of the working tree and separated by a single NUL.\n+#\n+# To enable this hook, rename this file to \"query-watchman\" and set\n+# 'git config core.fsmonitor .git/hooks/query-watchman'\n+#\n+my ($version, $last_update_token) = @ARGV;\n+\n+# Uncomment for debugging\n+# print STDERR \"$0 $version $last_update_token\\n\";\n+\n+# Check the hook interface version\n+if ($version ne 2) {\n+\tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n+\t \"Falling back to scanning...\\n\";\n+}\n+\n+my $git_work_tree = get_working_dir();\n+\n+my $retry = 1;\n+\n+my $json_pkg;\n+eval {\n+\trequire JSON::XS;\n+\t$json_pkg = \"JSON::XS\";\n+\t1;\n+} or do {\n+\trequire JSON::PP;\n+\t$json_pkg = \"JSON::PP\";\n+};\n+\n+launch_watchman();\n+\n+sub launch_watchman {\n+\tmy $o = watchman_query();\n+\tif (is_work_tree_watched($o)) {\n+\t\toutput_result($o->{clock}, @{$o->{files}});\n+\t}\n+}\n+\n+sub output_result {\n+\tmy ($clockid, @files) = @_;\n+\n+\t# Uncomment for debugging watchman output\n+\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t# binmode $fh, \":utf8\";\n+\t# print $fh \"$clockid\\n@files\\n\";\n+\t# close $fh;\n+\n+\tbinmode STDOUT, \":utf8\";\n+\tprint $clockid;\n+\tprint \"\\0\";\n+\tlocal $, = \"\\0\";\n+\tprint @files;\n+}\n+\n+sub watchman_clock {\n+\tmy $response = qx/watchman clock \"$git_work_tree\"/;\n+\tdie \"Failed to get clock id on '$git_work_tree'.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $? != 0;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub watchman_query {\n+\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n+\tor die \"open2() failed: $!\\n\" .\n+\t\"Falling back to scanning...\\n\";\n+\n+\t# In the query expression below we're asking for names of files that\n+\t# changed since $last_update_token but not from the .git folder.\n+\t#\n+\t# To accomplish this, we're using the \"since\" generator to use the\n+\t# recency index to select candidate nodes and \"fields\" to limit the\n+\t# output to file names only. Then we're using the \"expression\" term to\n+\t# further constrain the results.\n+\tif (substr($last_update_token, 0, 1) eq \"c\") {\n+\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t}\n+\tmy $query = <<\"\tEND\";\n+\t\t[\"query\", \"$git_work_tree\", {\n+\t\t\t\"since\": $last_update_token,\n+\t\t\t\"fields\": [\"name\"],\n+\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n+\t\t}]\n+\tEND\n+\n+\t# Uncomment for debugging the watchman query\n+\t# open (my $fh, \">\", \".git/watchman-query.json\");\n+\t# print $fh $query;\n+\t# close $fh;\n+\n+\tprint CHLD_IN $query;\n+\tclose CHLD_IN;\n+\tmy $response = do {local $/; };\n+\n+\t# Uncomment for debugging the watch response\n+\t# open ($fh, \">\", \".git/watchman-response.json\");\n+\t# print $fh $response;\n+\t# close $fh;\n+\n+\tdie \"Watchman: command returned no output.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $response eq \"\";\n+\tdie \"Watchman: command returned invalid output: $response\\n\" .\n+\t\"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub is_work_tree_watched {\n+\tmy ($output) = @_;\n+\tmy $error = $output->{error};\n+\tif ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {\n+\t\t$retry--;\n+\t\tmy $response = qx/watchman watch \"$git_work_tree\"/;\n+\t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n+\t\t \"Falling back to scanning...\\n\" if $? != 0;\n+\t\t$output = $json_pkg->new->utf8->decode($response);\n+\t\t$error = $output->{error};\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\t# Uncomment for debugging watchman output\n+\t\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t\t# close $fh;\n+\n+\t\t# Watchman will always return all files on the first query so\n+\t\t# return the fast \"everything is dirty\" flag to git and do the\n+\t\t# Watchman query just to get it over with now so we won't pay\n+\t\t# the cost in git to look up each individual file.\n+\t\tmy $o = watchman_clock();\n+\t\t$error = $output->{error};\n+\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\toutput_result($o->{clock}, (\"/\"));\n+\t\t$last_update_token = $o->{clock};\n+\n+\t\teval { launch_watchman() };\n+\t\treturn 0;\n+\t}\n+\n+\tdie \"Watchman: $error.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $error;\n+\n+\treturn 1;\n+}\n+\n+sub get_working_dir {\n+\tmy $working_dir;\n+\tif ($^O =~ 'msys' || $^O =~ 'cygwin') {\n+\t\t$working_dir = Win32::GetCwd();\n+\t\t$working_dir =~ tr/\\\\/\\//;\n+\t} else {\n+\t\trequire Cwd;\n+\t\t$working_dir = Cwd::cwd();\n+\t}\n+\n+\treturn $working_dir;\n+}\ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex ef94fa2938..31d2070b2c 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -8,58 +8,83 @@ use IPC::Open2;\n # (https://facebook.github.io/watchman/) with git to speed up detecting\n # new and modified files.\n #\n-# The hook is passed a version (currently 1) and a time in nanoseconds\n-# formatted as a string and outputs to stdout all files that have been\n-# modified since the given time. Paths must be relative to the root of\n-# the working tree and separated by a single NUL.\n+# The hook is passed a version (currently 2) and last update token\n+# formatted as a string and outputs to stdout a new update token and\n+# all files that have been modified since the update token. Paths must\n+# be relative to the root of the working tree and separated by a single NUL.\n #\n # To enable this hook, rename this file to \"query-watchman\" and set\n # 'git config core.fsmonitor .git/hooks/query-watchman'\n #\n-my ($version, $time) = @ARGV;\n+my ($version, $last_update_token) = @ARGV;\n \n # Check the hook interface version\n-\n-if ($version == 1) {\n-\t# convert nanoseconds to seconds\n-\t# subtract one second to make sure watchman will return all changes\n-\t$time = int ($time / 1000000000) - 1;\n-} else {\n+if ($version ne 2) {\n \tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n \t \"Falling back to scanning...\\n\";\n }\n \n-my $git_work_tree;\n-if ($^O =~ 'msys' || $^O =~ 'cygwin') {\n-\t$git_work_tree = Win32::GetCwd();\n-\t$git_work_tree =~ tr/\\\\/\\//;\n-} else {\n-\trequire Cwd;\n-\t$git_work_tree = Cwd::cwd();\n-}\n+my $git_work_tree = get_working_dir();\n \n my $retry = 1;\n \n+my $json_pkg;\n+eval {\n+\trequire JSON::XS;\n+\t$json_pkg = \"JSON::XS\";\n+\t1;\n+} or do {\n+\trequire JSON::PP;\n+\t$json_pkg = \"JSON::PP\";\n+};\n+\n launch_watchman();\n \n sub launch_watchman {\n+\t$o = watchman_query();\n+\tif (is_work_tree_watched($o)) {\n+\t\toutput_result($o->{clock}, @{$o->{files}});\n+\t}\n+}\n+\n+sub output_result {\n+\tmy ($clockid, @files) = @_;\n+\n+\tbinmode STDOUT, \":utf8\";\n+\tprint $clockid;\n+\tprint \"\\0\";\n+\tlocal $, = \"\\0\";\n+\tprint @files;\n+}\n+\n+sub watchman_clock {\n+\tmy $response = qx/watchman clock \"$git_work_tree\"/;\n+\tdie \"Failed to get clock id on '$git_work_tree'.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $? != 0;\n \n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub watchman_query {\n \tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n-\t or die \"open2() failed: $!\\n\" .\n-\t \"Falling back to scanning...\\n\";\n+\tor die \"open2() failed: $!\\n\" .\n+\t\"Falling back to scanning...\\n\";\n \n \t# In the query expression below we're asking for names of files that\n-\t# changed since $time but were not transient (ie created after\n-\t# $time but no longer exist).\n+\t# changed since $last_update_token but not from the .git folder.\n \t#\n \t# To accomplish this, we're using the \"since\" generator to use the\n \t# recency index to select candidate nodes and \"fields\" to limit the\n-\t# output to file names only.\n-\n+\t# output to file names only. Then we're using the \"expression\" term to\n+\t# further constrain the results.\n+\tif (substr($last_update_token, 0, 1) eq \"c\") {\n+\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t}\n \tmy $query = <<\"\tEND\";\n \t\t[\"query\", \"$git_work_tree\", {\n-\t\t\t\"since\": $time,\n-\t\t\t\"fields\": [\"name\"]\n+\t\t\t\"since\": $last_update_token,\n+\t\t\t\"fields\": [\"name\"],\n+\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n \t\t}]\n \tEND\n \n@@ -68,24 +93,16 @@ sub launch_watchman {\n \tmy $response = do {local $/; };\n \n \tdie \"Watchman: command returned no output.\\n\" .\n-\t \"Falling back to scanning...\\n\" if $response eq \"\";\n+\t\"Falling back to scanning...\\n\" if $response eq \"\";\n \tdie \"Watchman: command returned invalid output: $response\\n\" .\n-\t \"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n-\n-\tmy $json_pkg;\n-\teval {\n-\t\trequire JSON::XS;\n-\t\t$json_pkg = \"JSON::XS\";\n-\t\t1;\n-\t} or do {\n-\t\trequire JSON::PP;\n-\t\t$json_pkg = \"JSON::PP\";\n-\t};\n-\n-\tmy $o = $json_pkg->new->utf8->decode($response);\n-\n-\tif ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n-\t\tprint STDERR \"Adding '$git_work_tree' to watchman's watch list.\\n\";\n+\t\"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub is_work_tree_watched {\n+\tmy ($output) = @_;\n+\tif ($retry > 0 and $output->{error} and $output->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n \t\t$retry--;\n \t\tqx/watchman watch \"$git_work_tree\"/;\n \t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n@@ -95,15 +112,34 @@ sub launch_watchman {\n \t\t# return the fast \"everything is dirty\" flag to git and do the\n \t\t# Watchman query just to get it over with now so we won't pay\n \t\t# the cost in git to look up each individual file.\n-\t\tprint \"/\\0\";\n+\t\tmy $o = watchman_clock();\n+\t\t$error = $output->{error};\n+\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\toutput_result($o->{clock}, (\"/\"));\n+\t\t$last_update_token = $o->{clock};\n+\n \t\teval { launch_watchman() };\n-\t\texit 0;\n+\t\treturn 0;\n \t}\n \n-\tdie \"Watchman: $o->{error}.\\n\" .\n-\t \"Falling back to scanning...\\n\" if $o->{error};\n+\tdie \"Watchman: $output->{error}.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $output->{error};\n \n-\tbinmode STDOUT, \":utf8\";\n-\tlocal $, = \"\\0\";\n-\tprint @{$o->{files}};\n+\treturn 1;\n+}\n+\n+sub get_working_dir {\n+\tmy $working_dir;\n+\tif ($^O =~ 'msys' || $^O =~ 'cygwin') {\n+\t\t$working_dir = Win32::GetCwd();\n+\t\t$working_dir =~ tr/\\\\/\\//;\n+\t} else {\n+\t\trequire Cwd;\n+\t\t$working_dir = Cwd::cwd();\n+\t}\n+\n+\treturn $working_dir;\n }\n-- \ngitgitgadget\n\n"},{"id":"389413","messageId":"8d381b7d44c431328a44f53ba1e59271e8720d94.1578423871.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.git.1578423871.gitgitgadget@gmail.com","subject":"[PATCH 4/4] fsmonitor: update documentation for hook version and watchman hooks","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-07T19:04:31Z","receivedAt":"2020-01-07T19:04:41Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nA new config value for core.fsmonitorHookVersion was added to be able\nto force the version of the fsmonitor hook. Possible values are 1 or 2.\nWhen this is not set the code will use a value of -1 and attempt to use\nversion 2 of the hook first and if that fails will attempt version 1.\n\nSigned-off-by: Kevin Willford \n---\n Documentation/config/core.txt | 11 +++++++++++\n Documentation/githooks.txt | 13 ++++++++++---\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 9e440b160d..74619a9c03 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -68,6 +68,17 @@ core.fsmonitor::\n \tavoiding unnecessary processing of files that have not changed.\n \tSee the \"fsmonitor-watchman\" section of linkgit:githooks[5].\n \n+core.fsmonitorHookVersion::\n+\tSets the version of hook that is to be used when calling fsmonitor.\n+\tThere are currently versions 1 and 2. When this is not set,\n+\tversion 2 will be tried first and if it fails then version 1\n+\twill be tried. Version 1 uses a timestamp as input to determine\n+\twhich files have changes since that time but some monitors\n+\tlike watchman have race conditions when used with a timestamp.\n+\tVersion 2 uses an opaque string so that the monitor can return\n+\tsomething that can be used to determine what files have changed\n+\twithout race conditions.\n+\n core.trustctime::\n \tIf false, the ctime differences between the index and the\n \tworking tree are ignored; useful when the inode change time\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 50365f2914..3dccab5375 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -490,9 +490,16 @@ fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \n This hook is invoked when the configuration option `core.fsmonitor` is\n-set to `.git/hooks/fsmonitor-watchman`. It takes two arguments, a version\n-(currently 1) and the time in elapsed nanoseconds since midnight,\n-January 1, 1970.\n+set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`\n+depending on the version of the hook to use.\n+\n+Version 1 takes two arguments, a version (1) and the time in elapsed\n+nanoseconds since midnight, January 1, 1970.\n+\n+Version 2 takes two arguments, a version (2) and a token that is used\n+for identifying changes since the token. For watchman this would be\n+a clock id. This version must output to stdout the new token followed\n+by a NUL before the list of files.\n \n The hook should output to stdout the list of all files in the working\n directory that may have changed since the requested time. The logic\n-- \ngitgitgadget\n"},{"id":"390273","messageId":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.git.1578423871.gitgitgadget@gmail.com","subject":"[PATCH v2 0/4] fsmonitor: start using an opaque token for last update","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T15:26:43Z","receivedAt":"2020-01-23T15:26:53Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"Only change for this version is to sync the watchman script that is in\ntemplates with the one in t/t7519 since it had some commented out debug code\nthat could be useful.\n\nAlso haven't received any feedback on this patch series which would be good\nto make sure I'm not making any obvious mistakes.\n\nThanks!\n\nKevin Willford (4):\n fsmonitor: change last update timestamp on the index_state to opaque\n token\n fsmonitor: handle version 2 of the hooks that will use opaque token\n fsmonitor: add fsmonitor hook scripts for version 2\n fsmonitor: update documentation for hook version and watchman hooks\n\n Documentation/config/core.txt | 11 ++\n Documentation/githooks.txt | 13 +-\n cache.h | 2 +-\n fsmonitor.c | 120 ++++++++++----\n t/helper/test-dump-fsmonitor.c | 2 +-\n t/t7519-status-fsmonitor.sh | 7 +-\n t/t7519/fsmonitor-all | 1 -\n t/t7519/fsmonitor-all-v2 | 21 +++\n t/t7519/fsmonitor-watchman | 3 +-\n t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++\n templates/hooks--fsmonitor-watchman.sample | 168 +++++++++++++-------\n 11 files changed, 434 insertions(+), 87 deletions(-)\n create mode 100755 t/t7519/fsmonitor-all-v2\n create mode 100755 t/t7519/fsmonitor-watchman-v2\n\n\nbase-commit: 0a76bd7381ec0dbb7c43776eb6d1ac906bca29e6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-510%2Fkewillford%2Ffsmonitor_opaque_token-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-510/kewillford/fsmonitor_opaque_token-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/510\n\nRange-diff vs v1:\n\n 1: 679bf4e0dd = 1: 679bf4e0dd fsmonitor: change last update timestamp on the index_state to opaque token\n 2: f969c4bc17 = 2: f969c4bc17 fsmonitor: handle version 2 of the hooks that will use opaque token\n 3: e1fd5924d0 ! 3: ecc288c848 fsmonitor: add fsmonitor hook scripts for version 2\n @@ -236,13 +236,16 @@ $^\n -my ($version, $time) = @ARGV;\n +my ($version, $last_update_token) = @ARGV;\n \n - # Check the hook interface version\n --\n +-# Check the hook interface version\n ++# Uncomment for debugging\n ++# print STDERR \"$0 $version $last_update_token\\n\";\n + \n -if ($version == 1) {\n -\t# convert nanoseconds to seconds\n -\t# subtract one second to make sure watchman will return all changes\n -\t$time = int ($time / 1000000000) - 1;\n -} else {\n ++# Check the hook interface version\n +if ($version ne 2) {\n \tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n \t \"Falling back to scanning...\\n\";\n @@ -273,7 +276,7 @@ $^\n launch_watchman();\n \n sub launch_watchman {\n -+\t$o = watchman_query();\n ++\tmy $o = watchman_query();\n +\tif (is_work_tree_watched($o)) {\n +\t\toutput_result($o->{clock}, @{$o->{files}});\n +\t}\n @@ -281,6 +284,12 @@ $^\n +\n +sub output_result {\n +\tmy ($clockid, @files) = @_;\n + \n ++\t# Uncomment for debugging watchman output\n ++\t# open (my $fh, \">\", \".git/watchman-output.out\");\n ++\t# binmode $fh, \":utf8\";\n ++\t# print $fh \"$clockid\\n@files\\n\";\n ++\t# close $fh;\n +\n +\tbinmode STDOUT, \":utf8\";\n +\tprint $clockid;\n @@ -293,7 +302,7 @@ $^\n +\tmy $response = qx/watchman clock \"$git_work_tree\"/;\n +\tdie \"Failed to get clock id on '$git_work_tree'.\\n\" .\n +\t\t\"Falling back to scanning...\\n\" if $? != 0;\n - \n ++\n +\treturn $json_pkg->new->utf8->decode($response);\n +}\n +\n @@ -328,9 +337,20 @@ $^\n \t\t}]\n \tEND\n \n -@@\n ++\t# Uncomment for debugging the watchman query\n ++\t# open (my $fh, \">\", \".git/watchman-query.json\");\n ++\t# print $fh $query;\n ++\t# close $fh;\n ++\n + \tprint CHLD_IN $query;\n + \tclose CHLD_IN;\n \tmy $response = do {local $/; };\n \n ++\t# Uncomment for debugging the watch response\n ++\t# open ($fh, \">\", \".git/watchman-response.json\");\n ++\t# print $fh $response;\n ++\t# close $fh;\n ++\n \tdie \"Watchman: command returned no output.\\n\" .\n -\t \"Falling back to scanning...\\n\" if $response eq \"\";\n +\t\"Falling back to scanning...\\n\" if $response eq \"\";\n @@ -358,11 +378,23 @@ $^\n +\n +sub is_work_tree_watched {\n +\tmy ($output) = @_;\n -+\tif ($retry > 0 and $output->{error} and $output->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n ++\tmy $error = $output->{error};\n ++\tif ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {\n \t\t$retry--;\n - \t\tqx/watchman watch \"$git_work_tree\"/;\n +-\t\tqx/watchman watch \"$git_work_tree\"/;\n ++\t\tmy $response = qx/watchman watch \"$git_work_tree\"/;\n \t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n -@@\n + \t\t \"Falling back to scanning...\\n\" if $? != 0;\n ++\t\t$output = $json_pkg->new->utf8->decode($response);\n ++\t\t$error = $output->{error};\n ++\t\tdie \"Watchman: $error.\\n\" .\n ++\t\t\"Falling back to scanning...\\n\" if $error;\n ++\n ++\t\t# Uncomment for debugging watchman output\n ++\t\t# open (my $fh, \">\", \".git/watchman-output.out\");\n ++\t\t# close $fh;\n + \n + \t\t# Watchman will always return all files on the first query so\n \t\t# return the fast \"everything is dirty\" flag to git and do the\n \t\t# Watchman query just to get it over with now so we won't pay\n \t\t# the cost in git to look up each individual file.\n @@ -383,8 +415,8 @@ $^\n \n -\tdie \"Watchman: $o->{error}.\\n\" .\n -\t \"Falling back to scanning...\\n\" if $o->{error};\n -+\tdie \"Watchman: $output->{error}.\\n\" .\n -+\t\"Falling back to scanning...\\n\" if $output->{error};\n ++\tdie \"Watchman: $error.\\n\" .\n ++\t\"Falling back to scanning...\\n\" if $error;\n \n -\tbinmode STDOUT, \":utf8\";\n -\tlocal $, = \"\\0\";\n 4: 8d381b7d44 = 4: 1db2a699ba fsmonitor: update documentation for hook version and watchman hooks\n\n-- \ngitgitgadget\n"},{"id":"390274","messageId":"f969c4bc17b79f7c857987793cb0c23ad3f4e899.1579793207.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","subject":"[PATCH v2 2/4] fsmonitor: handle version 2 of the hooks that will use opaque token","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T15:26:45Z","receivedAt":"2020-01-23T15:26:55Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nSome file monitors like watchman will use something other than a timestamp\nto keep better track of what changes happen in between calls to query\nthe fsmonitor. The clockid in watchman is a string. Now that the index\nis storing an opaque token for the last update the code needs to be\nupdated to pass that opaque token to a verion 2 of the fsmonitor hook.\n\nBecause there are repos that already have version 1 of the hook and we\nwant them to continue to work when git is updated, we need to handle\nboth version 1 and version 2 of the hook. In order to do that a\nconfig value is being added core.fsmonitorHookVersion to force what\nversion of the hook should be used. When this is not set it will default\nto -1 and then the code will attempt to call version 2 of the hook first.\nIf that fails it will fallback to trying version 1.\n\nSigned-off-by: Kevin Willford \n---\n fsmonitor.c | 75 +++++++++++++++++++++++++++++++------\n t/t7519-status-fsmonitor.sh | 7 +++-\n t/t7519/fsmonitor-all | 1 -\n t/t7519/fsmonitor-watchman | 3 +-\n 4 files changed, 71 insertions(+), 15 deletions(-)\n\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 9860587225..932bd9012d 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -8,7 +8,8 @@\n \n #define INDEX_EXTENSION_VERSION1\t(1)\n #define INDEX_EXTENSION_VERSION2\t(2)\n-#define HOOK_INTERFACE_VERSION\t\t(1)\n+#define HOOK_INTERFACE_VERSION1\t\t(1)\n+#define HOOK_INTERFACE_VERSION2\t\t(2)\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n@@ -25,6 +26,22 @@ static void fsmonitor_ewah_callback(size_t pos, void *is)\n \tce->ce_flags &= ~CE_FSMONITOR_VALID;\n }\n \n+static int fsmonitor_hook_version(void)\n+{\n+\tint hook_version;\n+\n+\tif (git_config_get_int(\"core.fsmonitorhookversion\", &hook_version))\n+\t\treturn -1;\n+\n+\tif (hook_version == HOOK_INTERFACE_VERSION1 ||\n+\t hook_version == HOOK_INTERFACE_VERSION2)\n+\t\treturn hook_version;\n+\n+\twarning(\"Invalid hook version '%i' in core.fsmonitorhookversion. \"\n+\t\t\"Must be 1 or 2.\", hook_version);\n+\treturn -1;\n+}\n+\n int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \tunsigned long sz)\n {\n@@ -158,8 +175,8 @@ static void fsmonitor_refresh_callback(struct index_state *istate, const char *n\n void refresh_fsmonitor(struct index_state *istate)\n {\n \tstruct strbuf query_result = STRBUF_INIT;\n-\tint query_success = 0;\n-\tsize_t bol; /* beginning of line */\n+\tint query_success = 0, hook_version = -1;\n+\tsize_t bol = 0; /* beginning of line */\n \tuint64_t last_update;\n \tstruct strbuf last_update_token = STRBUF_INIT;\n \tchar *buf;\n@@ -167,6 +184,9 @@ void refresh_fsmonitor(struct index_state *istate)\n \n \tif (!core_fsmonitor || istate->fsmonitor_has_run_once)\n \t\treturn;\n+\n+\thook_version = fsmonitor_hook_version();\n+\n \tistate->fsmonitor_has_run_once = 1;\n \n \ttrace_printf_key(&trace_fsmonitor, \"refresh fsmonitor\");\n@@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)\n \t * should be inclusive to ensure we don't miss potential changes.\n \t */\n \tlast_update = getnanotime();\n-\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n+\tif (hook_version == HOOK_INTERFACE_VERSION1)\n+\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n \n \t/*\n-\t * If we have a last update time, call query_fsmonitor for the set of\n-\t * changes since that time, else assume everything is possibly dirty\n+\t * If we have a last update token, call query_fsmonitor for the set of\n+\t * changes since that token, else assume everything is possibly dirty\n \t * and check it all.\n \t */\n \tif (istate->fsmonitor_last_update) {\n-\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,\n-\t\t\tistate->fsmonitor_last_update, &query_result);\n+\t\tif (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {\n+\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,\n+\t\t\t\tistate->fsmonitor_last_update, &query_result);\n+\n+\t\t\tif (query_success) {\n+\t\t\t\tif (hook_version < 0)\n+\t\t\t\t\thook_version = HOOK_INTERFACE_VERSION2;\n+\n+\t\t\t\t/*\n+\t\t\t\t * First entry will be the last update token\n+\t\t\t\t * Need to use a char * variable because static\n+\t\t\t\t * analysis was suggesting to use strbuf_addbuf\n+\t\t\t\t * but we don't want to copy the entire strbuf\n+\t\t\t\t * only the the chars up to the first NUL\n+\t\t\t\t */\n+\t\t\t\tbuf = query_result.buf;\n+\t\t\t\tstrbuf_addstr(&last_update_token, buf);\n+\t\t\t\tif (!last_update_token.len) {\n+\t\t\t\t\twarning(\"Empty last update token.\");\n+\t\t\t\t\tquery_success = 0;\n+\t\t\t\t} else {\n+\t\t\t\t\tbol = last_update_token.len + 1;\n+\t\t\t\t}\n+\t\t\t} else if (hook_version < 0) {\n+\t\t\t\thook_version = HOOK_INTERFACE_VERSION1;\n+\t\t\t\tif (!last_update_token.len)\n+\t\t\t\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (hook_version == HOOK_INTERFACE_VERSION1) {\n+\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,\n+\t\t\t\tistate->fsmonitor_last_update, &query_result);\n+\t\t}\n+\n \t\ttrace_performance_since(last_update, \"fsmonitor process '%s'\", core_fsmonitor);\n \t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor process '%s' returned %s\",\n \t\t\tcore_fsmonitor, query_success ? \"success\" : \"failure\");\n \t}\n \n \t/* a fsmonitor process can return '/' to indicate all entries are invalid */\n-\tif (query_success && query_result.buf[0] != '/') {\n+\tif (query_success && query_result.buf[bol] != '/') {\n \t\t/* Mark all entries returned by the monitor as dirty */\n \t\tbuf = query_result.buf;\n-\t\tbol = 0;\n-\t\tfor (i = 0; i < query_result.len; i++) {\n+\t\tfor (i = bol; i < query_result.len; i++) {\n \t\t\tif (buf[i] != '\\0')\n \t\t\t\tcontinue;\n \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex cf0fda2d5a..fbfdcca000 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -32,11 +32,12 @@ write_integration_script () {\n \t\techo \"$0: exactly 2 arguments expected\"\n \t\texit 2\n \tfi\n-\tif test \"$1\" != 1\n+\tif test \"$1\" != 2\n \tthen\n \t\techo \"Unsupported core.fsmonitor hook version.\" >&2\n \t\texit 1\n \tfi\n+\tprintf \"last_update_token\\0\"\n \tprintf \"untracked\\0\"\n \tprintf \"dir1/untracked\\0\"\n \tprintf \"dir2/untracked\\0\"\n@@ -107,6 +108,7 @@ EOF\n # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\tprintf \"last_update_token\\0\"\n \tEOF\n \tgit update-index --fsmonitor &&\n \tgit update-index --fsmonitor-valid dir1/modified &&\n@@ -167,6 +169,7 @@ EOF\n # test that newly added files are marked valid\n test_expect_success 'newly added files are marked valid' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\tprintf \"last_update_token\\0\"\n \tEOF\n \tgit add new &&\n \tgit add dir1/new &&\n@@ -207,6 +210,7 @@ EOF\n # test that *only* files returned by the integration script get flagged as invalid\n test_expect_success '*only* files returned by the integration script get flagged as invalid' '\n \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\tprintf \"last_update_token\\0\"\n \tprintf \"dir1/modified\\0\"\n \tEOF\n \tclean_repo &&\n@@ -276,6 +280,7 @@ do\n \t\t# (if enabled) files unless it is told about them.\n \t\ttest_expect_success \"status doesn't detect unreported modifications\" '\n \t\t\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n+\t\t\tprintf \"last_update_token\\0\"\n \t\t\t:>marker\n \t\t\tEOF\n \t\t\tclean_repo &&\ndiff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all\nindex 691bc94dc2..94ab66bd3d 100755\n--- a/t/t7519/fsmonitor-all\n+++ b/t/t7519/fsmonitor-all\n@@ -17,7 +17,6 @@ fi\n \n if test \"$1\" != 1\n then\n-\techo \"Unsupported core.fsmonitor hook version.\" >&2\n \texit 1\n fi\n \ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex d8e7a1e5ba..264b9daf83 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -26,8 +26,7 @@ if ($version == 1) {\n \t# subtract one second to make sure watchman will return all changes\n \t$time = int ($time / 1000000000) - 1;\n } else {\n-\tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n-\t \"Falling back to scanning...\\n\";\n+\texit 1;\n }\n \n my $git_work_tree;\n-- \ngitgitgadget\n\n"},{"id":"390275","messageId":"679bf4e0dd16f7d6f4ba1f05af50cca24b5abee4.1579793207.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","subject":"[PATCH v2 1/4] fsmonitor: change last update timestamp on the index_state to opaque token","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T15:26:44Z","receivedAt":"2020-01-23T15:26:55Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nSome file system monitors might not use or take a timestamp for processing\nand in the case of watchman could have race conditions with using a\ntimestamp. Watchman uses something called a clockid that is used for race\nfree queries to it. The clockid for watchman is simply a string.\n\nChange the fsmonitor_last_update from being a uint64_t to a char pointer\nso that any arbitrary data can be stored in it and passed back to the\nfsmonitor.\n\nSigned-off-by: Kevin Willford \n---\n cache.h | 2 +-\n fsmonitor.c | 49 ++++++++++++++++++++++------------\n t/helper/test-dump-fsmonitor.c | 2 +-\n 3 files changed, 34 insertions(+), 19 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 1554488d66..170c125db3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -324,7 +324,7 @@ struct index_state {\n \tstruct hashmap dir_hash;\n \tstruct object_id oid;\n \tstruct untracked_cache *untracked;\n-\tuint64_t fsmonitor_last_update;\n+\tchar *fsmonitor_last_update;\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tstruct mem_pool *ce_mem_pool;\n \tstruct progress *progress;\ndiff --git a/fsmonitor.c b/fsmonitor.c\nindex 868cca01e2..9860587225 100644\n--- a/fsmonitor.c\n+++ b/fsmonitor.c\n@@ -6,8 +6,9 @@\n #include \"run-command.h\"\n #include \"strbuf.h\"\n \n-#define INDEX_EXTENSION_VERSION\t(1)\n-#define HOOK_INTERFACE_VERSION\t(1)\n+#define INDEX_EXTENSION_VERSION1\t(1)\n+#define INDEX_EXTENSION_VERSION2\t(2)\n+#define HOOK_INTERFACE_VERSION\t\t(1)\n \n struct trace_key trace_fsmonitor = TRACE_KEY_INIT(FSMONITOR);\n \n@@ -32,17 +33,26 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,\n \tuint32_t ewah_size;\n \tstruct ewah_bitmap *fsmonitor_dirty;\n \tint ret;\n+\tuint64_t timestamp;\n+\tstruct strbuf last_update = STRBUF_INIT;\n \n-\tif (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))\n+\tif (sz < sizeof(uint32_t) + 1 + sizeof(uint32_t))\n \t\treturn error(\"corrupt fsmonitor extension (too short)\");\n \n \thdr_version = get_be32(index);\n \tindex += sizeof(uint32_t);\n-\tif (hdr_version != INDEX_EXTENSION_VERSION)\n+\tif (hdr_version == INDEX_EXTENSION_VERSION1) {\n+\t\ttimestamp = get_be64(index);\n+\t\tstrbuf_addf(&last_update, \"%\"PRIu64\"\", timestamp);\n+\t\tindex += sizeof(uint64_t);\n+\t} else if (hdr_version == INDEX_EXTENSION_VERSION2) {\n+\t\tstrbuf_addstr(&last_update, index);\n+\t\tindex += last_update.len + 1;\n+\t} else {\n \t\treturn error(\"bad fsmonitor version %d\", hdr_version);\n+\t}\n \n-\tistate->fsmonitor_last_update = get_be64(index);\n-\tindex += sizeof(uint64_t);\n+\tistate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);\n \n \tewah_size = get_be32(index);\n \tindex += sizeof(uint32_t);\n@@ -79,7 +89,6 @@ void fill_fsmonitor_bitmap(struct index_state *istate)\n void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n {\n \tuint32_t hdr_version;\n-\tuint64_t tm;\n \tuint32_t ewah_start;\n \tuint32_t ewah_size = 0;\n \tint fixup = 0;\n@@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n \n-\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n+\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n \n-\tput_be64(&tm, istate->fsmonitor_last_update);\n-\tstrbuf_add(sb, &tm, sizeof(uint64_t));\n+\tstrbuf_addstr(sb, istate->fsmonitor_last_update);\n+\tstrbuf_addch(sb, 0); /* Want to keep a NUL */\n+\n \tfixup = sb->len;\n \tstrbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */\n \n@@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n }\n \n /*\n- * Call the query-fsmonitor hook passing the time of the last saved results.\n+ * Call the query-fsmonitor hook passing the last update token of the saved results.\n */\n-static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)\n+static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n@@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n \n \targv_array_push(&cp.args, core_fsmonitor);\n \targv_array_pushf(&cp.args, \"%d\", version);\n-\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n+\targv_array_pushf(&cp.args, \"%s\", last_update);\n \tcp.use_shell = 1;\n \tcp.dir = get_git_work_tree();\n \n@@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \tint query_success = 0;\n \tsize_t bol; /* beginning of line */\n \tuint64_t last_update;\n+\tstruct strbuf last_update_token = STRBUF_INIT;\n \tchar *buf;\n \tunsigned int i;\n \n@@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)\n \t * should be inclusive to ensure we don't miss potential changes.\n \t */\n \tlast_update = getnanotime();\n+\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n \n \t/*\n \t * If we have a last update time, call query_fsmonitor for the set of\n@@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)\n \t}\n \tstrbuf_release(&query_result);\n \n-\t/* Now that we've updated istate, save the last_update time */\n-\tistate->fsmonitor_last_update = last_update;\n+\t/* Now that we've updated istate, save the last_update_token */\n+\tFREE_AND_NULL(istate->fsmonitor_last_update);\n+\tistate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);\n }\n \n void add_fsmonitor(struct index_state *istate)\n {\n \tunsigned int i;\n+\tstruct strbuf last_update = STRBUF_INIT;\n \n \tif (!istate->fsmonitor_last_update) {\n \t\ttrace_printf_key(&trace_fsmonitor, \"add fsmonitor\");\n \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n-\t\tistate->fsmonitor_last_update = getnanotime();\n+\t\tstrbuf_addf(&last_update, \"%\"PRIu64\"\", getnanotime());\n+\t\tistate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);\n \n \t\t/* reset the fsmonitor state */\n \t\tfor (i = 0; i < istate->cache_nr; i++)\n@@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)\n \tif (istate->fsmonitor_last_update) {\n \t\ttrace_printf_key(&trace_fsmonitor, \"remove fsmonitor\");\n \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n-\t\tistate->fsmonitor_last_update = 0;\n+\t\tFREE_AND_NULL(istate->fsmonitor_last_update);\n \t}\n }\n \ndiff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\nindex 2786f47088..975f0ac890 100644\n--- a/t/helper/test-dump-fsmonitor.c\n+++ b/t/helper/test-dump-fsmonitor.c\n@@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)\n \t\tprintf(\"no fsmonitor\\n\");\n \t\treturn 0;\n \t}\n-\tprintf(\"fsmonitor last update %\"PRIuMAX\"\\n\", (uintmax_t)istate->fsmonitor_last_update);\n+\tprintf(\"fsmonitor last update %s\\n\", istate->fsmonitor_last_update);\n \n \tfor (i = 0; i < istate->cache_nr; i++)\n \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n-- \ngitgitgadget\n\n"},{"id":"390276","messageId":"1db2a699ba3749bfb4ab1c048492f76f82830ee6.1579793208.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","subject":"[PATCH v2 4/4] fsmonitor: update documentation for hook version and watchman hooks","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T15:26:47Z","receivedAt":"2020-01-23T15:26:57Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nA new config value for core.fsmonitorHookVersion was added to be able\nto force the version of the fsmonitor hook. Possible values are 1 or 2.\nWhen this is not set the code will use a value of -1 and attempt to use\nversion 2 of the hook first and if that fails will attempt version 1.\n\nSigned-off-by: Kevin Willford \n---\n Documentation/config/core.txt | 11 +++++++++++\n Documentation/githooks.txt | 13 ++++++++++---\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config/core.txt b/Documentation/config/core.txt\nindex 9e440b160d..74619a9c03 100644\n--- a/Documentation/config/core.txt\n+++ b/Documentation/config/core.txt\n@@ -68,6 +68,17 @@ core.fsmonitor::\n \tavoiding unnecessary processing of files that have not changed.\n \tSee the \"fsmonitor-watchman\" section of linkgit:githooks[5].\n \n+core.fsmonitorHookVersion::\n+\tSets the version of hook that is to be used when calling fsmonitor.\n+\tThere are currently versions 1 and 2. When this is not set,\n+\tversion 2 will be tried first and if it fails then version 1\n+\twill be tried. Version 1 uses a timestamp as input to determine\n+\twhich files have changes since that time but some monitors\n+\tlike watchman have race conditions when used with a timestamp.\n+\tVersion 2 uses an opaque string so that the monitor can return\n+\tsomething that can be used to determine what files have changed\n+\twithout race conditions.\n+\n core.trustctime::\n \tIf false, the ctime differences between the index and the\n \tworking tree are ignored; useful when the inode change time\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 50365f2914..3dccab5375 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -490,9 +490,16 @@ fsmonitor-watchman\n ~~~~~~~~~~~~~~~~~~\n \n This hook is invoked when the configuration option `core.fsmonitor` is\n-set to `.git/hooks/fsmonitor-watchman`. It takes two arguments, a version\n-(currently 1) and the time in elapsed nanoseconds since midnight,\n-January 1, 1970.\n+set to `.git/hooks/fsmonitor-watchman` or `.git/hooks/fsmonitor-watchmanv2`\n+depending on the version of the hook to use.\n+\n+Version 1 takes two arguments, a version (1) and the time in elapsed\n+nanoseconds since midnight, January 1, 1970.\n+\n+Version 2 takes two arguments, a version (2) and a token that is used\n+for identifying changes since the token. For watchman this would be\n+a clock id. This version must output to stdout the new token followed\n+by a NUL before the list of files.\n \n The hook should output to stdout the list of all files in the working\n directory that may have changed since the requested time. The logic\n-- \ngitgitgadget\n"},{"id":"390277","messageId":"ecc288c84811efb537ef7d8b10068e3a633c9273.1579793207.git.gitgitgadget@gmail.com","threadId":"52588","inReplyTo":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","subject":"[PATCH v2 3/4] fsmonitor: add fsmonitor hook scripts for version 2","fromName":"Kevin Willford via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-01-23T15:26:46Z","receivedAt":"2020-01-23T15:26:58Z","isPatch":true,"sender":{"key":"kewillf@microsoft.com","avatar":null},"body":"From: Kevin Willford \n\nVersion 2 of the fsmonitor hooks is passed the version and an update\ntoken and must pass back a last update token to use for subsequent calls\nto the hook.\n\nSigned-off-by: Kevin Willford \n---\n t/t7519/fsmonitor-all-v2 | 21 +++\n t/t7519/fsmonitor-watchman-v2 | 173 +++++++++++++++++++++\n templates/hooks--fsmonitor-watchman.sample | 168 +++++++++++++-------\n 3 files changed, 310 insertions(+), 52 deletions(-)\n create mode 100755 t/t7519/fsmonitor-all-v2\n create mode 100755 t/t7519/fsmonitor-watchman-v2\n\ndiff --git a/t/t7519/fsmonitor-all-v2 b/t/t7519/fsmonitor-all-v2\nnew file mode 100755\nindex 0000000000..061907e88b\n--- /dev/null\n+++ b/t/t7519/fsmonitor-all-v2\n@@ -0,0 +1,21 @@\n+#!/usr/bin/perl\n+\n+use strict;\n+use warnings;\n+#\n+# An test hook script to integrate with git to test fsmonitor.\n+#\n+# The hook is passed a version (currently 2) and since token\n+# formatted as a string and outputs to stdout all files that have been\n+# modified since the given time. Paths must be relative to the root of\n+# the working tree and separated by a single NUL.\n+#\n+#echo \"$0 $*\" >&2\n+my ($version, $last_update_token) = @ARGV;\n+\n+if ($version ne 2) {\n+\tprint \"Unsupported query-fsmonitor hook version '$version'.\\n\";\n+\texit 1;\n+}\n+\n+print \"last_update_token\\0/\\0\"\ndiff --git a/t/t7519/fsmonitor-watchman-v2 b/t/t7519/fsmonitor-watchman-v2\nnew file mode 100755\nindex 0000000000..14ed0aa42d\n--- /dev/null\n+++ b/t/t7519/fsmonitor-watchman-v2\n@@ -0,0 +1,173 @@\n+#!/usr/bin/perl\n+\n+use strict;\n+use warnings;\n+use IPC::Open2;\n+\n+# An example hook script to integrate Watchman\n+# (https://facebook.github.io/watchman/) with git to speed up detecting\n+# new and modified files.\n+#\n+# The hook is passed a version (currently 2) and last update token\n+# formatted as a string and outputs to stdout a new update token and\n+# all files that have been modified since the update token. Paths must\n+# be relative to the root of the working tree and separated by a single NUL.\n+#\n+# To enable this hook, rename this file to \"query-watchman\" and set\n+# 'git config core.fsmonitor .git/hooks/query-watchman'\n+#\n+my ($version, $last_update_token) = @ARGV;\n+\n+# Uncomment for debugging\n+# print STDERR \"$0 $version $last_update_token\\n\";\n+\n+# Check the hook interface version\n+if ($version ne 2) {\n+\tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n+\t \"Falling back to scanning...\\n\";\n+}\n+\n+my $git_work_tree = get_working_dir();\n+\n+my $retry = 1;\n+\n+my $json_pkg;\n+eval {\n+\trequire JSON::XS;\n+\t$json_pkg = \"JSON::XS\";\n+\t1;\n+} or do {\n+\trequire JSON::PP;\n+\t$json_pkg = \"JSON::PP\";\n+};\n+\n+launch_watchman();\n+\n+sub launch_watchman {\n+\tmy $o = watchman_query();\n+\tif (is_work_tree_watched($o)) {\n+\t\toutput_result($o->{clock}, @{$o->{files}});\n+\t}\n+}\n+\n+sub output_result {\n+\tmy ($clockid, @files) = @_;\n+\n+\t# Uncomment for debugging watchman output\n+\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t# binmode $fh, \":utf8\";\n+\t# print $fh \"$clockid\\n@files\\n\";\n+\t# close $fh;\n+\n+\tbinmode STDOUT, \":utf8\";\n+\tprint $clockid;\n+\tprint \"\\0\";\n+\tlocal $, = \"\\0\";\n+\tprint @files;\n+}\n+\n+sub watchman_clock {\n+\tmy $response = qx/watchman clock \"$git_work_tree\"/;\n+\tdie \"Failed to get clock id on '$git_work_tree'.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $? != 0;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub watchman_query {\n+\tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n+\tor die \"open2() failed: $!\\n\" .\n+\t\"Falling back to scanning...\\n\";\n+\n+\t# In the query expression below we're asking for names of files that\n+\t# changed since $last_update_token but not from the .git folder.\n+\t#\n+\t# To accomplish this, we're using the \"since\" generator to use the\n+\t# recency index to select candidate nodes and \"fields\" to limit the\n+\t# output to file names only. Then we're using the \"expression\" term to\n+\t# further constrain the results.\n+\tif (substr($last_update_token, 0, 1) eq \"c\") {\n+\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t}\n+\tmy $query = <<\"\tEND\";\n+\t\t[\"query\", \"$git_work_tree\", {\n+\t\t\t\"since\": $last_update_token,\n+\t\t\t\"fields\": [\"name\"],\n+\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n+\t\t}]\n+\tEND\n+\n+\t# Uncomment for debugging the watchman query\n+\t# open (my $fh, \">\", \".git/watchman-query.json\");\n+\t# print $fh $query;\n+\t# close $fh;\n+\n+\tprint CHLD_IN $query;\n+\tclose CHLD_IN;\n+\tmy $response = do {local $/; };\n+\n+\t# Uncomment for debugging the watch response\n+\t# open ($fh, \">\", \".git/watchman-response.json\");\n+\t# print $fh $response;\n+\t# close $fh;\n+\n+\tdie \"Watchman: command returned no output.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $response eq \"\";\n+\tdie \"Watchman: command returned invalid output: $response\\n\" .\n+\t\"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub is_work_tree_watched {\n+\tmy ($output) = @_;\n+\tmy $error = $output->{error};\n+\tif ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {\n+\t\t$retry--;\n+\t\tmy $response = qx/watchman watch \"$git_work_tree\"/;\n+\t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n+\t\t \"Falling back to scanning...\\n\" if $? != 0;\n+\t\t$output = $json_pkg->new->utf8->decode($response);\n+\t\t$error = $output->{error};\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\t# Uncomment for debugging watchman output\n+\t\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t\t# close $fh;\n+\n+\t\t# Watchman will always return all files on the first query so\n+\t\t# return the fast \"everything is dirty\" flag to git and do the\n+\t\t# Watchman query just to get it over with now so we won't pay\n+\t\t# the cost in git to look up each individual file.\n+\t\tmy $o = watchman_clock();\n+\t\t$error = $output->{error};\n+\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\toutput_result($o->{clock}, (\"/\"));\n+\t\t$last_update_token = $o->{clock};\n+\n+\t\teval { launch_watchman() };\n+\t\treturn 0;\n+\t}\n+\n+\tdie \"Watchman: $error.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $error;\n+\n+\treturn 1;\n+}\n+\n+sub get_working_dir {\n+\tmy $working_dir;\n+\tif ($^O =~ 'msys' || $^O =~ 'cygwin') {\n+\t\t$working_dir = Win32::GetCwd();\n+\t\t$working_dir =~ tr/\\\\/\\//;\n+\t} else {\n+\t\trequire Cwd;\n+\t\t$working_dir = Cwd::cwd();\n+\t}\n+\n+\treturn $working_dir;\n+}\ndiff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample\nindex ef94fa2938..14ed0aa42d 100755\n--- a/templates/hooks--fsmonitor-watchman.sample\n+++ b/templates/hooks--fsmonitor-watchman.sample\n@@ -8,102 +8,166 @@ use IPC::Open2;\n # (https://facebook.github.io/watchman/) with git to speed up detecting\n # new and modified files.\n #\n-# The hook is passed a version (currently 1) and a time in nanoseconds\n-# formatted as a string and outputs to stdout all files that have been\n-# modified since the given time. Paths must be relative to the root of\n-# the working tree and separated by a single NUL.\n+# The hook is passed a version (currently 2) and last update token\n+# formatted as a string and outputs to stdout a new update token and\n+# all files that have been modified since the update token. Paths must\n+# be relative to the root of the working tree and separated by a single NUL.\n #\n # To enable this hook, rename this file to \"query-watchman\" and set\n # 'git config core.fsmonitor .git/hooks/query-watchman'\n #\n-my ($version, $time) = @ARGV;\n+my ($version, $last_update_token) = @ARGV;\n \n-# Check the hook interface version\n+# Uncomment for debugging\n+# print STDERR \"$0 $version $last_update_token\\n\";\n \n-if ($version == 1) {\n-\t# convert nanoseconds to seconds\n-\t# subtract one second to make sure watchman will return all changes\n-\t$time = int ($time / 1000000000) - 1;\n-} else {\n+# Check the hook interface version\n+if ($version ne 2) {\n \tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n \t \"Falling back to scanning...\\n\";\n }\n \n-my $git_work_tree;\n-if ($^O =~ 'msys' || $^O =~ 'cygwin') {\n-\t$git_work_tree = Win32::GetCwd();\n-\t$git_work_tree =~ tr/\\\\/\\//;\n-} else {\n-\trequire Cwd;\n-\t$git_work_tree = Cwd::cwd();\n-}\n+my $git_work_tree = get_working_dir();\n \n my $retry = 1;\n \n+my $json_pkg;\n+eval {\n+\trequire JSON::XS;\n+\t$json_pkg = \"JSON::XS\";\n+\t1;\n+} or do {\n+\trequire JSON::PP;\n+\t$json_pkg = \"JSON::PP\";\n+};\n+\n launch_watchman();\n \n sub launch_watchman {\n+\tmy $o = watchman_query();\n+\tif (is_work_tree_watched($o)) {\n+\t\toutput_result($o->{clock}, @{$o->{files}});\n+\t}\n+}\n+\n+sub output_result {\n+\tmy ($clockid, @files) = @_;\n \n+\t# Uncomment for debugging watchman output\n+\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t# binmode $fh, \":utf8\";\n+\t# print $fh \"$clockid\\n@files\\n\";\n+\t# close $fh;\n+\n+\tbinmode STDOUT, \":utf8\";\n+\tprint $clockid;\n+\tprint \"\\0\";\n+\tlocal $, = \"\\0\";\n+\tprint @files;\n+}\n+\n+sub watchman_clock {\n+\tmy $response = qx/watchman clock \"$git_work_tree\"/;\n+\tdie \"Failed to get clock id on '$git_work_tree'.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $? != 0;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub watchman_query {\n \tmy $pid = open2(\\*CHLD_OUT, \\*CHLD_IN, 'watchman -j --no-pretty')\n-\t or die \"open2() failed: $!\\n\" .\n-\t \"Falling back to scanning...\\n\";\n+\tor die \"open2() failed: $!\\n\" .\n+\t\"Falling back to scanning...\\n\";\n \n \t# In the query expression below we're asking for names of files that\n-\t# changed since $time but were not transient (ie created after\n-\t# $time but no longer exist).\n+\t# changed since $last_update_token but not from the .git folder.\n \t#\n \t# To accomplish this, we're using the \"since\" generator to use the\n \t# recency index to select candidate nodes and \"fields\" to limit the\n-\t# output to file names only.\n-\n+\t# output to file names only. Then we're using the \"expression\" term to\n+\t# further constrain the results.\n+\tif (substr($last_update_token, 0, 1) eq \"c\") {\n+\t\t$last_update_token = \"\\\"$last_update_token\\\"\";\n+\t}\n \tmy $query = <<\"\tEND\";\n \t\t[\"query\", \"$git_work_tree\", {\n-\t\t\t\"since\": $time,\n-\t\t\t\"fields\": [\"name\"]\n+\t\t\t\"since\": $last_update_token,\n+\t\t\t\"fields\": [\"name\"],\n+\t\t\t\"expression\": [\"not\", [\"dirname\", \".git\"]]\n \t\t}]\n \tEND\n \n+\t# Uncomment for debugging the watchman query\n+\t# open (my $fh, \">\", \".git/watchman-query.json\");\n+\t# print $fh $query;\n+\t# close $fh;\n+\n \tprint CHLD_IN $query;\n \tclose CHLD_IN;\n \tmy $response = do {local $/; };\n \n+\t# Uncomment for debugging the watch response\n+\t# open ($fh, \">\", \".git/watchman-response.json\");\n+\t# print $fh $response;\n+\t# close $fh;\n+\n \tdie \"Watchman: command returned no output.\\n\" .\n-\t \"Falling back to scanning...\\n\" if $response eq \"\";\n+\t\"Falling back to scanning...\\n\" if $response eq \"\";\n \tdie \"Watchman: command returned invalid output: $response\\n\" .\n-\t \"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n-\n-\tmy $json_pkg;\n-\teval {\n-\t\trequire JSON::XS;\n-\t\t$json_pkg = \"JSON::XS\";\n-\t\t1;\n-\t} or do {\n-\t\trequire JSON::PP;\n-\t\t$json_pkg = \"JSON::PP\";\n-\t};\n-\n-\tmy $o = $json_pkg->new->utf8->decode($response);\n-\n-\tif ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n-\t\tprint STDERR \"Adding '$git_work_tree' to watchman's watch list.\\n\";\n+\t\"Falling back to scanning...\\n\" unless $response =~ /^\\{/;\n+\n+\treturn $json_pkg->new->utf8->decode($response);\n+}\n+\n+sub is_work_tree_watched {\n+\tmy ($output) = @_;\n+\tmy $error = $output->{error};\n+\tif ($retry > 0 and $error and $error =~ m/unable to resolve root .* directory (.*) is not watched/) {\n \t\t$retry--;\n-\t\tqx/watchman watch \"$git_work_tree\"/;\n+\t\tmy $response = qx/watchman watch \"$git_work_tree\"/;\n \t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n \t\t \"Falling back to scanning...\\n\" if $? != 0;\n+\t\t$output = $json_pkg->new->utf8->decode($response);\n+\t\t$error = $output->{error};\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\t# Uncomment for debugging watchman output\n+\t\t# open (my $fh, \">\", \".git/watchman-output.out\");\n+\t\t# close $fh;\n \n \t\t# Watchman will always return all files on the first query so\n \t\t# return the fast \"everything is dirty\" flag to git and do the\n \t\t# Watchman query just to get it over with now so we won't pay\n \t\t# the cost in git to look up each individual file.\n-\t\tprint \"/\\0\";\n+\t\tmy $o = watchman_clock();\n+\t\t$error = $output->{error};\n+\n+\t\tdie \"Watchman: $error.\\n\" .\n+\t\t\"Falling back to scanning...\\n\" if $error;\n+\n+\t\toutput_result($o->{clock}, (\"/\"));\n+\t\t$last_update_token = $o->{clock};\n+\n \t\teval { launch_watchman() };\n-\t\texit 0;\n+\t\treturn 0;\n \t}\n \n-\tdie \"Watchman: $o->{error}.\\n\" .\n-\t \"Falling back to scanning...\\n\" if $o->{error};\n+\tdie \"Watchman: $error.\\n\" .\n+\t\"Falling back to scanning...\\n\" if $error;\n \n-\tbinmode STDOUT, \":utf8\";\n-\tlocal $, = \"\\0\";\n-\tprint @{$o->{files}};\n+\treturn 1;\n+}\n+\n+sub get_working_dir {\n+\tmy $working_dir;\n+\tif ($^O =~ 'msys' || $^O =~ 'cygwin') {\n+\t\t$working_dir = Win32::GetCwd();\n+\t\t$working_dir =~ tr/\\\\/\\//;\n+\t} else {\n+\t\trequire Cwd;\n+\t\t$working_dir = Cwd::cwd();\n+\t}\n+\n+\treturn $working_dir;\n }\n-- \ngitgitgadget\n\n"},{"id":"391163","messageId":"nycvar.QRO.7.76.6.2002051344020.3718@tvgsbejvaqbjf.bet","threadId":"52588","inReplyTo":"679bf4e0dd16f7d6f4ba1f05af50cca24b5abee4.1579793207.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/4] fsmonitor: change last update timestamp on the index_state to opaque token","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-05T12:49:17Z","receivedAt":"2020-02-05T12:49:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kevin,\n\nOn Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:\n\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 868cca01e2..9860587225 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -89,11 +98,12 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> \t\tBUG(\"fsmonitor_dirty has more entries than the index (%\"PRIuMAX\" > %u)\",\n> \t\t (uintmax_t)istate->fsmonitor_dirty->bit_size, istate->cache_nr);\n>\n> -\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION);\n> +\tput_be32(&hdr_version, INDEX_EXTENSION_VERSION2);\n> \tstrbuf_add(sb, &hdr_version, sizeof(uint32_t));\n>\n> -\tput_be64(&tm, istate->fsmonitor_last_update);\n> -\tstrbuf_add(sb, &tm, sizeof(uint64_t));\n> +\tstrbuf_addstr(sb, istate->fsmonitor_last_update);\n> +\tstrbuf_addch(sb, 0); /* Want to keep a NUL */\n\nI have a slight preference to use '\\0' here, which my brain somehow reads\nas `NUL`.\n\n> +\n> \tfixup = sb->len;\n> \tstrbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */\n>\n> @@ -110,9 +120,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)\n> }\n>\n> /*\n> - * Call the query-fsmonitor hook passing the time of the last saved results.\n> + * Call the query-fsmonitor hook passing the last update token of the saved results.\n> */\n> -static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)\n> +static int query_fsmonitor(int version, const char *last_update, struct strbuf *query_result)\n> {\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n>\n> @@ -121,7 +131,7 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que\n>\n> \targv_array_push(&cp.args, core_fsmonitor);\n> \targv_array_pushf(&cp.args, \"%d\", version);\n> -\targv_array_pushf(&cp.args, \"%\" PRIuMAX, (uintmax_t)last_update);\n> +\targv_array_pushf(&cp.args, \"%s\", last_update);\n\nMaybe `argv_array_push(&cp.args, last_update)`?\n\n> \tcp.use_shell = 1;\n> \tcp.dir = get_git_work_tree();\n>\n> @@ -151,6 +161,7 @@ void refresh_fsmonitor(struct index_state *istate)\n> \tint query_success = 0;\n> \tsize_t bol; /* beginning of line */\n> \tuint64_t last_update;\n> +\tstruct strbuf last_update_token = STRBUF_INIT;\n> \tchar *buf;\n> \tunsigned int i;\n>\n> @@ -164,6 +175,7 @@ void refresh_fsmonitor(struct index_state *istate)\n> \t * should be inclusive to ensure we don't miss potential changes.\n> \t */\n> \tlast_update = getnanotime();\n> +\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n>\n> \t/*\n> \t * If we have a last update time, call query_fsmonitor for the set of\n> @@ -217,18 +229,21 @@ void refresh_fsmonitor(struct index_state *istate)\n> \t}\n> \tstrbuf_release(&query_result);\n>\n> -\t/* Now that we've updated istate, save the last_update time */\n> -\tistate->fsmonitor_last_update = last_update;\n> +\t/* Now that we've updated istate, save the last_update_token */\n> +\tFREE_AND_NULL(istate->fsmonitor_last_update);\n> +\tistate->fsmonitor_last_update = strbuf_detach(&last_update_token, NULL);\n\nI see quite a few `strbuf_detach()` calls in this patch, and could imagine\nthat this is a good indicator that the `fsmonitor_last_update` attribute\nof `struct index_state` could be a `struct strbuf` instead, that is\n`strbuf_reset()`ed and `strbuf_addf()`ed to, rather than having the\nstrbufs as local variables.\n\nOther than that, this patch looks very good to me.\n\nThanks!\nDscho\n\n> }\n>\n> void add_fsmonitor(struct index_state *istate)\n> {\n> \tunsigned int i;\n> +\tstruct strbuf last_update = STRBUF_INIT;\n>\n> \tif (!istate->fsmonitor_last_update) {\n> \t\ttrace_printf_key(&trace_fsmonitor, \"add fsmonitor\");\n> \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n> -\t\tistate->fsmonitor_last_update = getnanotime();\n> +\t\tstrbuf_addf(&last_update, \"%\"PRIu64\"\", getnanotime());\n> +\t\tistate->fsmonitor_last_update = strbuf_detach(&last_update, NULL);\n>\n> \t\t/* reset the fsmonitor state */\n> \t\tfor (i = 0; i < istate->cache_nr; i++)\n> @@ -250,7 +265,7 @@ void remove_fsmonitor(struct index_state *istate)\n> \tif (istate->fsmonitor_last_update) {\n> \t\ttrace_printf_key(&trace_fsmonitor, \"remove fsmonitor\");\n> \t\tistate->cache_changed |= FSMONITOR_CHANGED;\n> -\t\tistate->fsmonitor_last_update = 0;\n> +\t\tFREE_AND_NULL(istate->fsmonitor_last_update);\n> \t}\n> }\n>\n> diff --git a/t/helper/test-dump-fsmonitor.c b/t/helper/test-dump-fsmonitor.c\n> index 2786f47088..975f0ac890 100644\n> --- a/t/helper/test-dump-fsmonitor.c\n> +++ b/t/helper/test-dump-fsmonitor.c\n> @@ -13,7 +13,7 @@ int cmd__dump_fsmonitor(int ac, const char **av)\n> \t\tprintf(\"no fsmonitor\\n\");\n> \t\treturn 0;\n> \t}\n> -\tprintf(\"fsmonitor last update %\"PRIuMAX\"\\n\", (uintmax_t)istate->fsmonitor_last_update);\n> +\tprintf(\"fsmonitor last update %s\\n\", istate->fsmonitor_last_update);\n>\n> \tfor (i = 0; i < istate->cache_nr; i++)\n> \t\tprintf((istate->cache[i]->ce_flags & CE_FSMONITOR_VALID) ? \"+\" : \"-\");\n> --\n> gitgitgadget\n>\n>\n"},{"id":"391165","messageId":"nycvar.QRO.7.76.6.2002051353170.3718@tvgsbejvaqbjf.bet","threadId":"52588","inReplyTo":"f969c4bc17b79f7c857987793cb0c23ad3f4e899.1579793207.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/4] fsmonitor: handle version 2 of the hooks that will use opaque token","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-05T13:11:36Z","receivedAt":"2020-02-05T13:11:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kevin,\n\nOn Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:\n\n> diff --git a/fsmonitor.c b/fsmonitor.c\n> index 9860587225..932bd9012d 100644\n> --- a/fsmonitor.c\n> +++ b/fsmonitor.c\n> @@ -175,27 +195,60 @@ void refresh_fsmonitor(struct index_state *istate)\n> \t * should be inclusive to ensure we don't miss potential changes.\n> \t */\n> \tlast_update = getnanotime();\n> -\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n> +\tif (hook_version == HOOK_INTERFACE_VERSION1)\n> +\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n>\n> \t/*\n> -\t * If we have a last update time, call query_fsmonitor for the set of\n> -\t * changes since that time, else assume everything is possibly dirty\n> +\t * If we have a last update token, call query_fsmonitor for the set of\n> +\t * changes since that token, else assume everything is possibly dirty\n> \t * and check it all.\n> \t */\n> \tif (istate->fsmonitor_last_update) {\n> -\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,\n> -\t\t\tistate->fsmonitor_last_update, &query_result);\n> +\t\tif (hook_version == -1 || hook_version == HOOK_INTERFACE_VERSION2) {\n> +\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION2,\n> +\t\t\t\tistate->fsmonitor_last_update, &query_result);\n> +\n> +\t\t\tif (query_success) {\n> +\t\t\t\tif (hook_version < 0)\n> +\t\t\t\t\thook_version = HOOK_INTERFACE_VERSION2;\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t * First entry will be the last update token\n> +\t\t\t\t * Need to use a char * variable because static\n> +\t\t\t\t * analysis was suggesting to use strbuf_addbuf\n> +\t\t\t\t * but we don't want to copy the entire strbuf\n> +\t\t\t\t * only the the chars up to the first NUL\n> +\t\t\t\t */\n> +\t\t\t\tbuf = query_result.buf;\n> +\t\t\t\tstrbuf_addstr(&last_update_token, buf);\n> +\t\t\t\tif (!last_update_token.len) {\n> +\t\t\t\t\twarning(\"Empty last update token.\");\n> +\t\t\t\t\tquery_success = 0;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tbol = last_update_token.len + 1;\n> +\t\t\t\t}\n> +\t\t\t} else if (hook_version < 0) {\n> +\t\t\t\thook_version = HOOK_INTERFACE_VERSION1;\n> +\t\t\t\tif (!last_update_token.len)\n> +\t\t\t\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (hook_version == HOOK_INTERFACE_VERSION1) {\n> +\t\t\tquery_success = !query_fsmonitor(HOOK_INTERFACE_VERSION1,\n> +\t\t\t\tistate->fsmonitor_last_update, &query_result);\n\nI suspect that `istate->fsmonitor_last_update` might be incorrect here and\nthat you need `last_update_token.buf` instead. Besides...\n\n> +\t\t}\n\nI could imagine that this would be easier to read if you initialized\n\n\tint interface_version = HOOK_INTERFACE_VERSION2;\n\tconst char *token = istate->fsmonitor_last_update;\n\nand then, at the beginning of this hunk, where you already added an `if`,\nextend it to\n\n\tif (hook_version == HOOK_INTERFACE_VERSION1) {\n\t\tinterface_version = HOOK_INTERFACE_VERSION1;\n\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n\t\ttoken = last_update_token.buf;\n\t}\n\nNow, you can call\n\n\t\tquery_success = !query_fsmonitor(interface_version,\n\t\t\ttoken, &query_result);\n\t\tif (!query_success && hook_version < 0) {\n\t\t\thook_version = HOOK_INTERFACE_VERSION1;\n\t\t\tstrbuf_reset(&last_update_token);\n\t\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n\t\t\ttoken = last_update_token.buf;\n\t\t\tquery_success = !query_fsmonitor(hook_version,\n\t\t\t\ttoken, &query_result);\n\t\t}\n\n\t\tif (query_success && interface_version == HOOK_INTERFACE_VERSION2)\n\t\t\tbol = last_update_token.len + 1;\n\n\nTechnically, you could force `hook_version` to non-negative, via:\n\n\t\tif (hook_version < 0) {\n\t\t\tif (query_success)\n\t\t\t\thook_version = HOOK_INTERFACE_VERSION2;\n\t\t\telse {\n\t\t\t\thook_version = HOOK_INTERFACE_VERSION1;\n\t\t\t\tstrbuf_reset(&last_update_token);\n\t\t\t\tstrbuf_addf(&last_update_token, \"%\"PRIu64\"\", last_update);\n\t\t\t\ttoken = last_update_token.buf;\n\t\t\t\tquery_success = !query_fsmonitor(hook_version,\n\t\t\t\t\ttoken, &query_result);\n\t\t\t}\n\t\t}\n\nbut that would not make anything quicker, and would make the code more\nconvoluted.\n\nIt is good that you keep the `fsmonitor-all` and `fsmonitor-watchman`\nversions at 1, to verify that this fall-back mechanism works. I wonder\nwhether we should add one explicit test for that, so that those two hooks\ncan be upgraded to the interface v2.\n\nThanks,\nDscho\n\n> +\n> \t\ttrace_performance_since(last_update, \"fsmonitor process '%s'\", core_fsmonitor);\n> \t\ttrace_printf_key(&trace_fsmonitor, \"fsmonitor process '%s' returned %s\",\n> \t\t\tcore_fsmonitor, query_success ? \"success\" : \"failure\");\n> \t}\n>\n> \t/* a fsmonitor process can return '/' to indicate all entries are invalid */\n> -\tif (query_success && query_result.buf[0] != '/') {\n> +\tif (query_success && query_result.buf[bol] != '/') {\n> \t\t/* Mark all entries returned by the monitor as dirty */\n> \t\tbuf = query_result.buf;\n> -\t\tbol = 0;\n> -\t\tfor (i = 0; i < query_result.len; i++) {\n> +\t\tfor (i = bol; i < query_result.len; i++) {\n> \t\t\tif (buf[i] != '\\0')\n> \t\t\t\tcontinue;\n> \t\t\tfsmonitor_refresh_callback(istate, buf + bol);\n> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\n> index cf0fda2d5a..fbfdcca000 100755\n> --- a/t/t7519-status-fsmonitor.sh\n> +++ b/t/t7519-status-fsmonitor.sh\n> @@ -32,11 +32,12 @@ write_integration_script () {\n> \t\techo \"$0: exactly 2 arguments expected\"\n> \t\texit 2\n> \tfi\n> -\tif test \"$1\" != 1\n> +\tif test \"$1\" != 2\n> \tthen\n> \t\techo \"Unsupported core.fsmonitor hook version.\" >&2\n> \t\texit 1\n> \tfi\n> +\tprintf \"last_update_token\\0\"\n> \tprintf \"untracked\\0\"\n> \tprintf \"dir1/untracked\\0\"\n> \tprintf \"dir2/untracked\\0\"\n> @@ -107,6 +108,7 @@ EOF\n> # test that \"update-index --fsmonitor-valid\" sets the fsmonitor valid bit\n> test_expect_success 'update-index --fsmonitor-valid\" sets the fsmonitor valid bit' '\n> \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\t\tprintf \"last_update_token\\0\"\n> \tEOF\n> \tgit update-index --fsmonitor &&\n> \tgit update-index --fsmonitor-valid dir1/modified &&\n> @@ -167,6 +169,7 @@ EOF\n> # test that newly added files are marked valid\n> test_expect_success 'newly added files are marked valid' '\n> \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\t\tprintf \"last_update_token\\0\"\n> \tEOF\n> \tgit add new &&\n> \tgit add dir1/new &&\n> @@ -207,6 +210,7 @@ EOF\n> # test that *only* files returned by the integration script get flagged as invalid\n> test_expect_success '*only* files returned by the integration script get flagged as invalid' '\n> \twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\tprintf \"last_update_token\\0\"\n> \tprintf \"dir1/modified\\0\"\n> \tEOF\n> \tclean_repo &&\n> @@ -276,6 +280,7 @@ do\n> \t\t# (if enabled) files unless it is told about them.\n> \t\ttest_expect_success \"status doesn't detect unreported modifications\" '\n> \t\t\twrite_script .git/hooks/fsmonitor-test<<-\\EOF &&\n> +\t\t\tprintf \"last_update_token\\0\"\n> \t\t\t:>marker\n> \t\t\tEOF\n> \t\t\tclean_repo &&\n> diff --git a/t/t7519/fsmonitor-all b/t/t7519/fsmonitor-all\n> index 691bc94dc2..94ab66bd3d 100755\n> --- a/t/t7519/fsmonitor-all\n> +++ b/t/t7519/fsmonitor-all\n> @@ -17,7 +17,6 @@ fi\n>\n> if test \"$1\" != 1\n> then\n> -\techo \"Unsupported core.fsmonitor hook version.\" >&2\n> \texit 1\n> fi\n>\n> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\n> index d8e7a1e5ba..264b9daf83 100755\n> --- a/t/t7519/fsmonitor-watchman\n> +++ b/t/t7519/fsmonitor-watchman\n> @@ -26,8 +26,7 @@ if ($version == 1) {\n> \t# subtract one second to make sure watchman will return all changes\n> \t$time = int ($time / 1000000000) - 1;\n> } else {\n> -\tdie \"Unsupported query-fsmonitor hook version '$version'.\\n\" .\n> -\t \"Falling back to scanning...\\n\";\n> +\texit 1;\n> }\n>\n> my $git_work_tree;\n> --\n> gitgitgadget\n>\n>\n"},{"id":"391166","messageId":"nycvar.QRO.7.76.6.2002051413460.3718@tvgsbejvaqbjf.bet","threadId":"52588","inReplyTo":"pull.510.v2.git.1579793207.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/4] fsmonitor: start using an opaque token for last update","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-02-05T13:15:10Z","receivedAt":"2020-02-05T13:15:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kevin,\n\nOn Thu, 23 Jan 2020, Kevin Willford via GitGitGadget wrote:\n\n> Only change for this version is to sync the watchman script that is in\n> templates with the one in t/t7519 since it had some commented out debug code\n> that could be useful.\n>\n> Also haven't received any feedback on this patch series which would be good\n> to make sure I'm not making any obvious mistakes.\n\nI just read through the series (focusing on the C part) and offered a\ncouple of suggestions, nothing major.\n\nWhile I would love to see the fallback-mechanism simplified a little I\nthink this patch series is already in a pretty good shape.\n\nThanks,\nDscho\n"}]}