{"thread":{"id":"55316","subject":"[PATCH 0/3] teach git to respect fsmonitor in diff-index","startedAt":"2021-03-14T22:18:28Z","lastAt":"2021-03-18T22:58:03Z","messageCount":15,"participants":["Nipunn Koorapati via GitGitGadget","Eric Sunshine","Nipunn Koorapati","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"419106","messageId":"pull.903.git.1615760258.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":null,"subject":"[PATCH 0/3] teach git to respect fsmonitor in diff-index","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-14T22:17:34Z","receivedAt":"2021-03-14T22:18:28Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Skip lstat deletion check during git diff-index (similar to how it already\ndoes so in git diff-files). Add perf benchmark for this case. Add assert for\nguaranteeing that fsmonitor is refreshed in this case.\n\nNipunn Koorapati (3):\n  fsmonitor: skip lstat deletion check during git diff-index\n  fsmonitor: add assertion that fsmonitor is valid to check_removed\n  fsmonitor: add perf test for git diff HEAD\n\n diff-lib.c                | 23 +++++++++++++++--------\n fsmonitor.h               | 11 +++++++++++\n t/helper/test-chmtime.c   |  3 ++-\n t/perf/p7519-fsmonitor.sh |  4 ++++\n 4 files changed, 32 insertions(+), 9 deletions(-)\n\n\nbase-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-903%2Fnipunn1313%2Fnk%2Ffsmonitor-in-diff-index-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-903/nipunn1313/nk/fsmonitor-in-diff-index-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/903\n-- \ngitgitgadget\n"},{"id":"419107","messageId":"75a3c46c405549d1f5127097729c556a7e297587.1615760258.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.git.1615760258.gitgitgadget@gmail.com","subject":"[PATCH 1/3] fsmonitor: skip lstat deletion check during git diff-index","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-14T22:17:35Z","receivedAt":"2021-03-14T22:18:28Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nTeach git to honor fsmonitor rather than issuing an lstat\nwhen checking for dirty local deletes. Eliminates O(files)\nlstats during `git diff HEAD`\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n diff-lib.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b73cc1859a49..3fb538ad18e9 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -30,7 +30,7 @@\n  */\n static int check_removed(const struct cache_entry *ce, struct stat *st)\n {\n-\tif (lstat(ce->name, st) < 0) {\n+\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n \t\treturn 1;\n@@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n \tstruct object_id oid;\n \tconst char *name;\n \tchar merge_base_hex[GIT_MAX_HEXSZ + 1];\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n@@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (merge_base) {\n \t\tdiff_get_merge_base(revs, &oid);\n \t\tname = oid_to_hex_r(merge_base_hex, &oid);\n-- \ngitgitgadget\n\n"},{"id":"419108","messageId":"740302586dd8902c46567fc2de9b7296cce4eda2.1615760258.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.git.1615760258.gitgitgadget@gmail.com","subject":"[PATCH 3/3] fsmonitor: add perf test for git diff HEAD","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-14T22:17:37Z","receivedAt":"2021-03-14T22:18:28Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nUpdate the xargs call so that if your large repo contains\nsymlinks, test-tool chmtime failure does not end the script.\n\nOn Linux\nTest                                                          this tree           upstream/master\n---------------------------------------------------------------------------------------------------------\n7519.4: status (fsmonitor=fsmonitor-watchman)                 0.52(0.43+0.10)     0.53(0.49+0.05) +1.9%\n7519.5: status -uno (fsmonitor=fsmonitor-watchman)            0.21(0.15+0.07)     0.22(0.13+0.09) +4.8%\n7519.6: status -uall (fsmonitor=fsmonitor-watchman)           1.65(0.93+0.71)     1.69(1.03+0.65) +2.4%\n7519.7: status (dirty) (fsmonitor=fsmonitor-watchman)         11.99(11.34+1.58)   11.95(11.02+1.79) -0.3%\n7519.8: diff (fsmonitor=fsmonitor-watchman)                   0.25(0.17+0.26)     0.25(0.18+0.26) +0.0%\n7519.9: diff HEAD (fsmonitor=fsmonitor-watchman)              0.39(0.25+0.34)     0.89(0.35+0.74) +128.2%\n7519.10: diff -- 0_files (fsmonitor=fsmonitor-watchman)       0.16(0.13+0.04)     0.16(0.12+0.05) +0.0%\n7519.11: diff -- 10_files (fsmonitor=fsmonitor-watchman)      0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%\n7519.12: diff -- 100_files (fsmonitor=fsmonitor-watchman)     0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%\n7519.13: diff -- 1000_files (fsmonitor=fsmonitor-watchman)    0.16(0.11+0.06)     0.16(0.12+0.05) +0.0%\n7519.14: diff -- 10000_files (fsmonitor=fsmonitor-watchman)   0.18(0.13+0.06)     0.17(0.10+0.08) -5.6%\n7519.15: add (fsmonitor=fsmonitor-watchman)                   2.25(1.53+0.68)     2.25(1.47+0.74) +0.0%\n7519.18: status (fsmonitor=disabled)                          0.88(0.73+1.03)     0.89(0.67+1.08) +1.1%\n7519.19: status -uno (fsmonitor=disabled)                     0.45(0.43+0.89)     0.45(0.34+0.98) +0.0%\n7519.20: status -uall (fsmonitor=disabled)                    1.88(1.16+1.58)     1.88(1.22+1.51) +0.0%\n7519.21: status (dirty) (fsmonitor=disabled)                  7.53(7.05+2.11)     7.53(6.98+2.04) +0.0%\n7519.22: diff (fsmonitor=disabled)                            0.42(0.37+0.92)     0.42(0.38+0.91) +0.0%\n7519.23: diff HEAD (fsmonitor=disabled)                       0.44(0.41+0.90)     0.44(0.40+0.91) +0.0%\n7519.24: diff -- 0_files (fsmonitor=disabled)                 0.13(0.09+0.05)     0.13(0.09+0.05) +0.0%\n7519.25: diff -- 10_files (fsmonitor=disabled)                0.13(0.10+0.04)     0.13(0.10+0.04) +0.0%\n7519.26: diff -- 100_files (fsmonitor=disabled)               0.13(0.09+0.05)     0.13(0.10+0.04) +0.0%\n7519.27: diff -- 1000_files (fsmonitor=disabled)              0.13(0.09+0.06)     0.13(0.09+0.05) +0.0%\n7519.28: diff -- 10000_files (fsmonitor=disabled)             0.14(0.11+0.05)     0.14(0.10+0.05) +0.0%\n7519.29: add (fsmonitor=disabled)                             2.43(1.61+1.64)     2.43(1.69+1.57) +0.0%\n\nOn linux (2.29.2 vs w/ this patch):\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff 2>&1 | grep lstat\n  0.04    0.000063           3        20         6 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff HEAD 2>&1 | grep lstat\n 94.98    5.242262          10    523783        13 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff 2>&1 | grep lstat\n  0.38    0.000032           5         7         3 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep lstat\n 99.44    0.741892           9     81634        10 lstat\n\nOn mac (2.29.2 vs w/ this patch):\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff 2>&1 | grep \"^lstat64 \"\nlstat64                                         8\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff HEAD 2>&1 | grep \"^lstat64 \"\nlstat64                                    120242\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff 2>&1 | grep \"^lstat64 \"\nlstat64                                         4\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep \"^lstat64 \"\nlstat64                                      4497\n\nThere are still a bunch of lstats - on directories, but not every file. Progress!\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n t/helper/test-chmtime.c   | 3 ++-\n t/perf/p7519-fsmonitor.sh | 4 ++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex aa22af48c2a6..a8b143d11ab7 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -111,7 +111,8 @@ int cmd__chmtime(int argc, const char **argv)\n \t\tif (stat(argv[i], &sb) < 0) {\n \t\t\tfprintf(stderr, \"Failed to stat %s: %s\\n\",\n \t\t\t        argv[i], strerror(errno));\n-\t\t\treturn 1;\n+\t\t\t// Skip and move on - eg if it's a broken symlink\n+\t\t\tcontinue;\n \t\t}\n \n #ifdef GIT_WINDOWS_NATIVE\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex b657564aed60..5eb5044a103c 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -216,6 +216,10 @@ test_fsmonitor_suite() {\n \t\tgit diff\n \t'\n \n+\ttest_perf_w_drop_caches \"diff HEAD ($DESC)\" '\n+\t\tgit diff HEAD\n+\t'\n+\n \ttest_perf_w_drop_caches \"diff -- 0_files ($DESC)\" '\n \t\tgit diff -- 1_file\n \t'\n-- \ngitgitgadget\n"},{"id":"419109","messageId":"dda5b537a3f0706ebf933e2b2efd996267e9d9b1.1615760258.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.git.1615760258.gitgitgadget@gmail.com","subject":"[PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-14T22:17:36Z","receivedAt":"2021-03-14T22:18:28Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nValidate that fsmonitor is valid to futureproof against bugs where\ncheck_removed might be called from places that haven't refreshed.\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n diff-lib.c  | 18 +++++++++++-------\n fsmonitor.h | 11 +++++++++++\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 3fb538ad18e9..e5a58c9259cf 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -28,8 +28,9 @@\n  * exists for ce that is a submodule -- it is a submodule that is not\n  * checked out).  Return negative for an error.\n  */\n-static int check_removed(const struct cache_entry *ce, struct stat *st)\n+static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n {\n+\tassert(is_fsmonitor_refreshed(istate));\n \tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n@@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tmemset(&(dpath->parent[0]), 0,\n \t\t\t       sizeof(struct combine_diff_parent)*5);\n \n-\t\t\tchanged = check_removed(ce, &st);\n+\t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (!changed)\n \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\telse {\n@@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t} else {\n \t\t\tstruct stat st;\n \n-\t\t\tchanged = check_removed(ce, &st);\n+\t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (changed) {\n \t\t\t\tif (changed < 0) {\n \t\t\t\t\tperror(ce->name);\n@@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,\n \t\t       oid, oid_valid, ce->name, dirty_submodule);\n }\n \n-static int get_stat_data(const struct cache_entry *ce,\n+static int get_stat_data(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce,\n \t\t\t const struct object_id **oidp,\n \t\t\t unsigned int *modep,\n \t\t\t int cached, int match_missing,\n@@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,\n \tif (!cached && !ce_uptodate(ce)) {\n \t\tint changed;\n \t\tstruct stat st;\n-\t\tchanged = check_removed(ce, &st);\n+\t\tchanged = check_removed(istate, ce, &st);\n \t\tif (changed < 0)\n \t\t\treturn -1;\n \t\telse if (changed) {\n@@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,\n \tconst struct object_id *oid;\n \tunsigned int mode;\n \tunsigned dirty_submodule = 0;\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n \t/*\n \t * New file in the index: it might actually be different in\n \t * the working tree.\n \t */\n-\tif (get_stat_data(new_file, &oid, &mode, cached, match_missing,\n+\tif (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,\n \t    &dirty_submodule, &revs->diffopt) < 0)\n \t\treturn;\n \n@@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,\n \tunsigned int mode, oldmode;\n \tconst struct object_id *oid;\n \tunsigned dirty_submodule = 0;\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n-\tif (get_stat_data(new_entry, &oid, &mode, cached, match_missing,\n+\tif (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,\n \t\t\t  &dirty_submodule, &revs->diffopt) < 0) {\n \t\tif (report_missing)\n \t\t\tdiff_index_show_file(revs, \"-\", old_entry,\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 7f1794b90b00..c12f10117544 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);\n  */\n int fsmonitor_is_trivial_response(const struct strbuf *query_result);\n \n+/*\n+ * Check if refresh_fsmonitor has been called at least once.\n+ * refresh_fsmonitor is idempotent. Returns true if fsmonitor is\n+ * not enabled (since the state will be \"fresh\" w/ CE_FSMONITOR_VALID unset)\n+ * This version is useful for assertions\n+ */\n+static inline int is_fsmonitor_refreshed(const struct index_state *istate)\n+{\n+    return !core_fsmonitor || istate->fsmonitor_has_run_once;\n+}\n+\n /*\n  * Set the given cache entries CE_FSMONITOR_VALID bit. This should be\n  * called any time the cache entry has been updated to reflect the\n-- \ngitgitgadget\n\n"},{"id":"419110","messageId":"CAPig+cT9aAPGzysqAz2OBrZP-7Ci+h+W5wFgnRm8bsde9K6zdw@mail.gmail.com","threadId":"55316","inReplyTo":"dda5b537a3f0706ebf933e2b2efd996267e9d9b1.1615760258.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-14T22:35:24Z","receivedAt":"2021-03-14T22:36:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 14, 2021 at 6:19 PM Nipunn Koorapati via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> Validate that fsmonitor is valid to futureproof against bugs where\n> check_removed might be called from places that haven't refreshed.\n>\n> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n> ---\n> diff --git a/fsmonitor.h b/fsmonitor.h\n> @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);\n> +/*\n> + * Check if refresh_fsmonitor has been called at least once.\n> + * refresh_fsmonitor is idempotent. Returns true if fsmonitor is\n> + * not enabled (since the state will be \"fresh\" w/ CE_FSMONITOR_VALID unset)\n> + * This version is useful for assertions\n> + */\n> +static inline int is_fsmonitor_refreshed(const struct index_state *istate)\n> +{\n> +    return !core_fsmonitor || istate->fsmonitor_has_run_once;\n> +}\n\nUnusual 4-space indentation rather than typical 1-tab.\n"},{"id":"419197","messageId":"CAN8Z4-VFekryNYczJBkFQpkLJngcHJ5JBH0tb5ObNsrc241uSw@mail.gmail.com","threadId":"55316","inReplyTo":"CAPig+cT9aAPGzysqAz2OBrZP-7Ci+h+W5wFgnRm8bsde9K6zdw@mail.gmail.com","subject":"Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2021-03-15T22:01:37Z","receivedAt":"2021-03-15T22:02:41Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> Unusual 4-space indentation rather than typical 1-tab.\n\nThanks for identifying - will fix in the next patch series. Perhaps in\na separate patch we could add a unit test that validates the codebase\nfor such style?\n"},{"id":"419201","messageId":"CAPig+cRzaQuu+GTdTky1vGyO9yCDHoQx2u9XXWm5HCfXAXDXwg@mail.gmail.com","threadId":"55316","inReplyTo":"CAN8Z4-VFekryNYczJBkFQpkLJngcHJ5JBH0tb5ObNsrc241uSw@mail.gmail.com","subject":"Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-03-15T22:51:20Z","receivedAt":"2021-03-15T22:52:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 15, 2021 at 6:01 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:\n> > Unusual 4-space indentation rather than typical 1-tab.\n>\n> Thanks for identifying - will fix in the next patch series. Perhaps in\n> a separate patch we could add a unit test that validates the codebase\n> for such style?\n\nMy guess is that a test which complains about existing style\nviolations would not be particularly helpful since it's output likely\nwould be noisy due to existing style violations. (For the same reason,\nwe wouldn't want to complain about style violations in tests either\nsince existing test scripts are full of violations.) What is more\ninteresting than identifying existing violations is identifying\nviolations before they make it into the codebase. For instance, you\ncould run your patches through `checkpatch.pl`[1] before submitting\nthem.\n\n[1]: https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl\n"},{"id":"419579","messageId":"75a3c46c405549d1f5127097729c556a7e297587.1616016143.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.v2.git.1616016143.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T21:22:21Z","receivedAt":"2021-03-17T21:23:22Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nTeach git to honor fsmonitor rather than issuing an lstat\nwhen checking for dirty local deletes. Eliminates O(files)\nlstats during `git diff HEAD`\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n diff-lib.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b73cc1859a49..3fb538ad18e9 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -30,7 +30,7 @@\n  */\n static int check_removed(const struct cache_entry *ce, struct stat *st)\n {\n-\tif (lstat(ce->name, st) < 0) {\n+\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n \t\treturn 1;\n@@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n \tstruct object_id oid;\n \tconst char *name;\n \tchar merge_base_hex[GIT_MAX_HEXSZ + 1];\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n \tif (revs->pending.nr != 1)\n \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n@@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n \ttrace_performance_enter();\n \tent = revs->pending.objects;\n \n+\trefresh_fsmonitor(istate);\n+\n \tif (merge_base) {\n \t\tdiff_get_merge_base(revs, &oid);\n \t\tname = oid_to_hex_r(merge_base_hex, &oid);\n-- \ngitgitgadget\n\n"},{"id":"419580","messageId":"pull.903.v2.git.1616016143.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.git.1615760258.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] teach git to respect fsmonitor in diff-index","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T21:22:20Z","receivedAt":"2021-03-17T21:23:22Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Skip lstat deletion check during git diff-index (similar to how it already\ndoes so in git diff-files). Add perf benchmark for this case. Add assert for\nguaranteeing that fsmonitor is refreshed in this case.\n\nUpdate since Patch Series V1:\n\n * Fix spaces->tabs issue in fsmonitor.h\n * Remove comment in test-chmtime.c - to avoid it going stale\n\ncc: Eric Sunshine sunshine@sunshineco.com\n\nNipunn Koorapati (3):\n  fsmonitor: skip lstat deletion check during git diff-index\n  fsmonitor: add assertion that fsmonitor is valid to check_removed\n  fsmonitor: add perf test for git diff HEAD\n\n diff-lib.c                | 23 +++++++++++++++--------\n fsmonitor.h               | 11 +++++++++++\n t/helper/test-chmtime.c   |  4 ++--\n t/perf/p7519-fsmonitor.sh |  4 ++++\n 4 files changed, 32 insertions(+), 10 deletions(-)\n\n\nbase-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-903%2Fnipunn1313%2Fnk%2Ffsmonitor-in-diff-index-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-903/nipunn1313/nk/fsmonitor-in-diff-index-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/903\n\nRange-diff vs v1:\n\n 1:  75a3c46c4055 = 1:  75a3c46c4055 fsmonitor: skip lstat deletion check during git diff-index\n 2:  dda5b537a3f0 ! 2:  afd326c5011b fsmonitor: add assertion that fsmonitor is valid to check_removed\n     @@ fsmonitor.h: void refresh_fsmonitor(struct index_state *istate);\n      + */\n      +static inline int is_fsmonitor_refreshed(const struct index_state *istate)\n      +{\n     -+    return !core_fsmonitor || istate->fsmonitor_has_run_once;\n     ++\treturn !core_fsmonitor || istate->fsmonitor_has_run_once;\n      +}\n      +\n       /*\n 3:  740302586dd8 ! 3:  f9d0fd594fdb fsmonitor: add perf test for git diff HEAD\n     @@ Commit message\n      \n       ## t/helper/test-chmtime.c ##\n      @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)\n     + \t\tuintmax_t mtime;\n     + \n       \t\tif (stat(argv[i], &sb) < 0) {\n     - \t\t\tfprintf(stderr, \"Failed to stat %s: %s\\n\",\n     +-\t\t\tfprintf(stderr, \"Failed to stat %s: %s\\n\",\n     ++\t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n       \t\t\t        argv[i], strerror(errno));\n      -\t\t\treturn 1;\n     -+\t\t\t// Skip and move on - eg if it's a broken symlink\n      +\t\t\tcontinue;\n       \t\t}\n       \n\n-- \ngitgitgadget\n"},{"id":"419581","messageId":"f9d0fd594fdb515e3a3cd4bd14b8b09532ccc483.1616016143.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.v2.git.1616016143.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] fsmonitor: add perf test for git diff HEAD","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T21:22:23Z","receivedAt":"2021-03-17T21:23:22Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nUpdate the xargs call so that if your large repo contains\nsymlinks, test-tool chmtime failure does not end the script.\n\nOn Linux\nTest                                                          this tree           upstream/master\n---------------------------------------------------------------------------------------------------------\n7519.4: status (fsmonitor=fsmonitor-watchman)                 0.52(0.43+0.10)     0.53(0.49+0.05) +1.9%\n7519.5: status -uno (fsmonitor=fsmonitor-watchman)            0.21(0.15+0.07)     0.22(0.13+0.09) +4.8%\n7519.6: status -uall (fsmonitor=fsmonitor-watchman)           1.65(0.93+0.71)     1.69(1.03+0.65) +2.4%\n7519.7: status (dirty) (fsmonitor=fsmonitor-watchman)         11.99(11.34+1.58)   11.95(11.02+1.79) -0.3%\n7519.8: diff (fsmonitor=fsmonitor-watchman)                   0.25(0.17+0.26)     0.25(0.18+0.26) +0.0%\n7519.9: diff HEAD (fsmonitor=fsmonitor-watchman)              0.39(0.25+0.34)     0.89(0.35+0.74) +128.2%\n7519.10: diff -- 0_files (fsmonitor=fsmonitor-watchman)       0.16(0.13+0.04)     0.16(0.12+0.05) +0.0%\n7519.11: diff -- 10_files (fsmonitor=fsmonitor-watchman)      0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%\n7519.12: diff -- 100_files (fsmonitor=fsmonitor-watchman)     0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%\n7519.13: diff -- 1000_files (fsmonitor=fsmonitor-watchman)    0.16(0.11+0.06)     0.16(0.12+0.05) +0.0%\n7519.14: diff -- 10000_files (fsmonitor=fsmonitor-watchman)   0.18(0.13+0.06)     0.17(0.10+0.08) -5.6%\n7519.15: add (fsmonitor=fsmonitor-watchman)                   2.25(1.53+0.68)     2.25(1.47+0.74) +0.0%\n7519.18: status (fsmonitor=disabled)                          0.88(0.73+1.03)     0.89(0.67+1.08) +1.1%\n7519.19: status -uno (fsmonitor=disabled)                     0.45(0.43+0.89)     0.45(0.34+0.98) +0.0%\n7519.20: status -uall (fsmonitor=disabled)                    1.88(1.16+1.58)     1.88(1.22+1.51) +0.0%\n7519.21: status (dirty) (fsmonitor=disabled)                  7.53(7.05+2.11)     7.53(6.98+2.04) +0.0%\n7519.22: diff (fsmonitor=disabled)                            0.42(0.37+0.92)     0.42(0.38+0.91) +0.0%\n7519.23: diff HEAD (fsmonitor=disabled)                       0.44(0.41+0.90)     0.44(0.40+0.91) +0.0%\n7519.24: diff -- 0_files (fsmonitor=disabled)                 0.13(0.09+0.05)     0.13(0.09+0.05) +0.0%\n7519.25: diff -- 10_files (fsmonitor=disabled)                0.13(0.10+0.04)     0.13(0.10+0.04) +0.0%\n7519.26: diff -- 100_files (fsmonitor=disabled)               0.13(0.09+0.05)     0.13(0.10+0.04) +0.0%\n7519.27: diff -- 1000_files (fsmonitor=disabled)              0.13(0.09+0.06)     0.13(0.09+0.05) +0.0%\n7519.28: diff -- 10000_files (fsmonitor=disabled)             0.14(0.11+0.05)     0.14(0.10+0.05) +0.0%\n7519.29: add (fsmonitor=disabled)                             2.43(1.61+1.64)     2.43(1.69+1.57) +0.0%\n\nOn linux (2.29.2 vs w/ this patch):\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff 2>&1 | grep lstat\n  0.04    0.000063           3        20         6 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff HEAD 2>&1 | grep lstat\n 94.98    5.242262          10    523783        13 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff 2>&1 | grep lstat\n  0.38    0.000032           5         7         3 lstat\nnipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep lstat\n 99.44    0.741892           9     81634        10 lstat\n\nOn mac (2.29.2 vs w/ this patch):\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff 2>&1 | grep \"^lstat64 \"\nlstat64                                         8\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff HEAD 2>&1 | grep \"^lstat64 \"\nlstat64                                    120242\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff 2>&1 | grep \"^lstat64 \"\nlstat64                                         4\nnipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep \"^lstat64 \"\nlstat64                                      4497\n\nThere are still a bunch of lstats - on directories, but not every file. Progress!\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n t/helper/test-chmtime.c   | 4 ++--\n t/perf/p7519-fsmonitor.sh | 4 ++++\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c\nindex aa22af48c2a6..524b55ca496c 100644\n--- a/t/helper/test-chmtime.c\n+++ b/t/helper/test-chmtime.c\n@@ -109,9 +109,9 @@ int cmd__chmtime(int argc, const char **argv)\n \t\tuintmax_t mtime;\n \n \t\tif (stat(argv[i], &sb) < 0) {\n-\t\t\tfprintf(stderr, \"Failed to stat %s: %s\\n\",\n+\t\t\tfprintf(stderr, \"Failed to stat %s: %s. Skipping\\n\",\n \t\t\t        argv[i], strerror(errno));\n-\t\t\treturn 1;\n+\t\t\tcontinue;\n \t\t}\n \n #ifdef GIT_WINDOWS_NATIVE\ndiff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh\nindex b657564aed60..5eb5044a103c 100755\n--- a/t/perf/p7519-fsmonitor.sh\n+++ b/t/perf/p7519-fsmonitor.sh\n@@ -216,6 +216,10 @@ test_fsmonitor_suite() {\n \t\tgit diff\n \t'\n \n+\ttest_perf_w_drop_caches \"diff HEAD ($DESC)\" '\n+\t\tgit diff HEAD\n+\t'\n+\n \ttest_perf_w_drop_caches \"diff -- 0_files ($DESC)\" '\n \t\tgit diff -- 1_file\n \t'\n-- \ngitgitgadget\n"},{"id":"419582","messageId":"afd326c5011b09d89b6354817c1913d85142c335.1616016143.git.gitgitgadget@gmail.com","threadId":"55316","inReplyTo":"pull.903.v2.git.1616016143.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-17T21:22:22Z","receivedAt":"2021-03-17T21:23:22Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati <nipunn@dropbox.com>\n\nValidate that fsmonitor is valid to futureproof against bugs where\ncheck_removed might be called from places that haven't refreshed.\n\nSigned-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n---\n diff-lib.c  | 18 +++++++++++-------\n fsmonitor.h | 11 +++++++++++\n 2 files changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 3fb538ad18e9..e5a58c9259cf 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -28,8 +28,9 @@\n  * exists for ce that is a submodule -- it is a submodule that is not\n  * checked out).  Return negative for an error.\n  */\n-static int check_removed(const struct cache_entry *ce, struct stat *st)\n+static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n {\n+\tassert(is_fsmonitor_refreshed(istate));\n \tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n \t\tif (!is_missing_file_error(errno))\n \t\t\treturn -1;\n@@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\tmemset(&(dpath->parent[0]), 0,\n \t\t\t       sizeof(struct combine_diff_parent)*5);\n \n-\t\t\tchanged = check_removed(ce, &st);\n+\t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (!changed)\n \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n \t\t\telse {\n@@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t} else {\n \t\t\tstruct stat st;\n \n-\t\t\tchanged = check_removed(ce, &st);\n+\t\t\tchanged = check_removed(istate, ce, &st);\n \t\t\tif (changed) {\n \t\t\t\tif (changed < 0) {\n \t\t\t\t\tperror(ce->name);\n@@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,\n \t\t       oid, oid_valid, ce->name, dirty_submodule);\n }\n \n-static int get_stat_data(const struct cache_entry *ce,\n+static int get_stat_data(const struct index_state *istate,\n+\t\t\t const struct cache_entry *ce,\n \t\t\t const struct object_id **oidp,\n \t\t\t unsigned int *modep,\n \t\t\t int cached, int match_missing,\n@@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,\n \tif (!cached && !ce_uptodate(ce)) {\n \t\tint changed;\n \t\tstruct stat st;\n-\t\tchanged = check_removed(ce, &st);\n+\t\tchanged = check_removed(istate, ce, &st);\n \t\tif (changed < 0)\n \t\t\treturn -1;\n \t\telse if (changed) {\n@@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,\n \tconst struct object_id *oid;\n \tunsigned int mode;\n \tunsigned dirty_submodule = 0;\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n \t/*\n \t * New file in the index: it might actually be different in\n \t * the working tree.\n \t */\n-\tif (get_stat_data(new_file, &oid, &mode, cached, match_missing,\n+\tif (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,\n \t    &dirty_submodule, &revs->diffopt) < 0)\n \t\treturn;\n \n@@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,\n \tunsigned int mode, oldmode;\n \tconst struct object_id *oid;\n \tunsigned dirty_submodule = 0;\n+\tstruct index_state *istate = revs->diffopt.repo->index;\n \n-\tif (get_stat_data(new_entry, &oid, &mode, cached, match_missing,\n+\tif (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,\n \t\t\t  &dirty_submodule, &revs->diffopt) < 0) {\n \t\tif (report_missing)\n \t\t\tdiff_index_show_file(revs, \"-\", old_entry,\ndiff --git a/fsmonitor.h b/fsmonitor.h\nindex 7f1794b90b00..f20d72631d76 100644\n--- a/fsmonitor.h\n+++ b/fsmonitor.h\n@@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);\n  */\n int fsmonitor_is_trivial_response(const struct strbuf *query_result);\n \n+/*\n+ * Check if refresh_fsmonitor has been called at least once.\n+ * refresh_fsmonitor is idempotent. Returns true if fsmonitor is\n+ * not enabled (since the state will be \"fresh\" w/ CE_FSMONITOR_VALID unset)\n+ * This version is useful for assertions\n+ */\n+static inline int is_fsmonitor_refreshed(const struct index_state *istate)\n+{\n+\treturn !core_fsmonitor || istate->fsmonitor_has_run_once;\n+}\n+\n /*\n  * Set the given cache entries CE_FSMONITOR_VALID bit. This should be\n  * called any time the cache entry has been updated to reflect the\n-- \ngitgitgadget\n\n"},{"id":"419651","messageId":"xmqqo8fgry3g.fsf@gitster.g","threadId":"55316","inReplyTo":"75a3c46c405549d1f5127097729c556a7e297587.1616016143.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-18T20:44:19Z","receivedAt":"2021-03-18T20:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Nipunn Koorapati <nipunn@dropbox.com>\n>\n> Teach git to honor fsmonitor rather than issuing an lstat\n> when checking for dirty local deletes. Eliminates O(files)\n> lstats during `git diff HEAD`\n>\n> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n> ---\n>  diff-lib.c | 5 ++++-\n>  1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index b73cc1859a49..3fb538ad18e9 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -30,7 +30,7 @@\n>   */\n>  static int check_removed(const struct cache_entry *ce, struct stat *st)\n>  {\n> -\tif (lstat(ce->name, st) < 0) {\n> +\tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n\nSo when the cache entry is marked as VALID, we know it is there and\nunmodified without asking lstat().  Otherwise we ask lstat() as\nbefore.  OK.\n\n>  \t\tif (!is_missing_file_error(errno))\n>  \t\t\treturn -1;\n>  \t\treturn 1;\n> @@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n>  \tstruct object_id oid;\n>  \tconst char *name;\n>  \tchar merge_base_hex[GIT_MAX_HEXSZ + 1];\n> +\tstruct index_state *istate = revs->diffopt.repo->index;\n>  \n>  \tif (revs->pending.nr != 1)\n>  \t\tBUG(\"run_diff_index must be passed exactly one tree\");\n> @@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)\n>  \ttrace_performance_enter();\n>  \tent = revs->pending.objects;\n>  \n> +\trefresh_fsmonitor(istate);\n\nAnd the VALID bit is set only for the ones that are untouched?  When\ncore_fsmonitor is not set, or istate->fsmonitor_has_run_once is set,\nrefresh_fsmonitor() becomes no-op and does not even drop the VALID\nbit from the cache entries.  As run_diff_index() is rather\nlibrary-ish part of the system, are we sure no earlier attempts to\ninvoke fsmonitor have touched ce to set the VALID bit on at this\npoint?\n\nAssuming that we won't see stray VALID bit to confuse us, the patch\nlooks good to me, but I am not sure what to base confidence on that\nassumption.\n\nThanks.\n\n>  \tif (merge_base) {\n>  \t\tdiff_get_merge_base(revs, &oid);\n>  \t\tname = oid_to_hex_r(merge_base_hex, &oid);\n"},{"id":"419652","messageId":"xmqqk0q4rxxh.fsf@gitster.g","threadId":"55316","inReplyTo":"afd326c5011b09d89b6354817c1913d85142c335.1616016143.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-18T20:47:54Z","receivedAt":"2021-03-18T20:48:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Nipunn Koorapati <nipunn@dropbox.com>\n>\n> Validate that fsmonitor is valid to futureproof against bugs where\n> check_removed might be called from places that haven't refreshed.\n\nIsn't this the other way around, wrt to the previous step?\n\nAt least, \"pass around istate throughout the callchain in the\ndiff-lib.c file\" change should stand alone and come much earlier in\nthe series (perhaps as step #1).  Then \"call refresh_fsmonitor from\nrun_diff_index() and make sure in check_removed() that fsmonitor\ndoes not have bogus VALID bit\" assertion should come on top, as a\nsingle step, I would think.\n\n> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>\n> ---\n>  diff-lib.c  | 18 +++++++++++-------\n>  fsmonitor.h | 11 +++++++++++\n>  2 files changed, 22 insertions(+), 7 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 3fb538ad18e9..e5a58c9259cf 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -28,8 +28,9 @@\n>   * exists for ce that is a submodule -- it is a submodule that is not\n>   * checked out).  Return negative for an error.\n>   */\n> -static int check_removed(const struct cache_entry *ce, struct stat *st)\n> +static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)\n>  {\n> +\tassert(is_fsmonitor_refreshed(istate));\n>  \tif (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {\n>  \t\tif (!is_missing_file_error(errno))\n>  \t\t\treturn -1;\n> @@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\t\tmemset(&(dpath->parent[0]), 0,\n>  \t\t\t       sizeof(struct combine_diff_parent)*5);\n>  \n> -\t\t\tchanged = check_removed(ce, &st);\n> +\t\t\tchanged = check_removed(istate, ce, &st);\n>  \t\t\tif (!changed)\n>  \t\t\t\twt_mode = ce_mode_from_stat(ce, st.st_mode);\n>  \t\t\telse {\n> @@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n>  \t\t} else {\n>  \t\t\tstruct stat st;\n>  \n> -\t\t\tchanged = check_removed(ce, &st);\n> +\t\t\tchanged = check_removed(istate, ce, &st);\n>  \t\t\tif (changed) {\n>  \t\t\t\tif (changed < 0) {\n>  \t\t\t\t\tperror(ce->name);\n> @@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,\n>  \t\t       oid, oid_valid, ce->name, dirty_submodule);\n>  }\n>  \n> -static int get_stat_data(const struct cache_entry *ce,\n> +static int get_stat_data(const struct index_state *istate,\n> +\t\t\t const struct cache_entry *ce,\n>  \t\t\t const struct object_id **oidp,\n>  \t\t\t unsigned int *modep,\n>  \t\t\t int cached, int match_missing,\n> @@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,\n>  \tif (!cached && !ce_uptodate(ce)) {\n>  \t\tint changed;\n>  \t\tstruct stat st;\n> -\t\tchanged = check_removed(ce, &st);\n> +\t\tchanged = check_removed(istate, ce, &st);\n>  \t\tif (changed < 0)\n>  \t\t\treturn -1;\n>  \t\telse if (changed) {\n> @@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,\n>  \tconst struct object_id *oid;\n>  \tunsigned int mode;\n>  \tunsigned dirty_submodule = 0;\n> +\tstruct index_state *istate = revs->diffopt.repo->index;\n>  \n>  \t/*\n>  \t * New file in the index: it might actually be different in\n>  \t * the working tree.\n>  \t */\n> -\tif (get_stat_data(new_file, &oid, &mode, cached, match_missing,\n> +\tif (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,\n>  \t    &dirty_submodule, &revs->diffopt) < 0)\n>  \t\treturn;\n>  \n> @@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,\n>  \tunsigned int mode, oldmode;\n>  \tconst struct object_id *oid;\n>  \tunsigned dirty_submodule = 0;\n> +\tstruct index_state *istate = revs->diffopt.repo->index;\n>  \n> -\tif (get_stat_data(new_entry, &oid, &mode, cached, match_missing,\n> +\tif (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,\n>  \t\t\t  &dirty_submodule, &revs->diffopt) < 0) {\n>  \t\tif (report_missing)\n>  \t\t\tdiff_index_show_file(revs, \"-\", old_entry,\n> diff --git a/fsmonitor.h b/fsmonitor.h\n> index 7f1794b90b00..f20d72631d76 100644\n> --- a/fsmonitor.h\n> +++ b/fsmonitor.h\n> @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);\n>   */\n>  int fsmonitor_is_trivial_response(const struct strbuf *query_result);\n>  \n> +/*\n> + * Check if refresh_fsmonitor has been called at least once.\n> + * refresh_fsmonitor is idempotent. Returns true if fsmonitor is\n> + * not enabled (since the state will be \"fresh\" w/ CE_FSMONITOR_VALID unset)\n> + * This version is useful for assertions\n> + */\n> +static inline int is_fsmonitor_refreshed(const struct index_state *istate)\n> +{\n> +\treturn !core_fsmonitor || istate->fsmonitor_has_run_once;\n> +}\n> +\n>  /*\n>   * Set the given cache entries CE_FSMONITOR_VALID bit. This should be\n>   * called any time the cache entry has been updated to reflect the\n"},{"id":"419657","messageId":"CAN8Z4-VQHJVx6+-Lc4jF7NRwLxS6+9wW03FiZ2Ydqn0Mi30dKg@mail.gmail.com","threadId":"55316","inReplyTo":"xmqqo8fgry3g.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2021-03-18T21:36:43Z","receivedAt":"2021-03-18T21:37:41Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"On Thu, Mar 18, 2021 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:\n> >\n> > +     refresh_fsmonitor(istate);\n>\n> And the VALID bit is set only for the ones that are untouched?  When\n> core_fsmonitor is not set, or istate->fsmonitor_has_run_once is set,\n> refresh_fsmonitor() becomes no-op and does not even drop the VALID\n> bit from the cache entries.  As run_diff_index() is rather\n> library-ish part of the system, are we sure no earlier attempts to\n> invoke fsmonitor have touched ce to set the VALID bit on at this\n> point?\n>\n> Assuming that we won't see stray VALID bit to confuse us, the patch\n> looks good to me, but I am not sure what to base confidence on that\n> assumption.\n>\n> Thanks.\n\nMy understanding is that git's invariants around\nfsmonitor bit would ensure that the bit is set simultaneously w/ the\nrest of the index entry\nafter a stat (at the cursor/timestamp of the most recent refresh_fsmonitor).\n\nRegardless of whether earlier access to the VALID bit happened within\nthe command, the\nstate should be internally consistent at this point, meaning that if\nvalid is set, the rest of the\nentry is sensible.\n\nI did just manually confirm this here\n$ bin-wrappers/git diff HEAD\n$ rm zlib.c\n$ GIT_TRACE_FSMONITOR=1 bin-wrappers/git diff HEAD\n21:21:22.911316 fsmonitor.c:97          read fsmonitor extension\nsuccessful 'c:1615751750:32210:5:60'\n21:21:22.911417 fsmonitor.c:249         refresh fsmonitor\n21:21:22.937726 fsmonitor.c:301         fsmonitor process\n'.git/hooks/query-watchman' returned success\n21:21:22.937749 fsmonitor.c:228         fsmonitor_refresh_callback 'zlib.c'\n21:21:22.937755 fsmonitor.c:228         fsmonitor_refresh_callback '.git'\ndiff --git a/zlib.c b/zlib.c\ndeleted file mode 100644\n[rest of output omitted]\n\nThis should be confirmable in the testsuite via\nls t*.sh | grep diff | GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all xargs prove\n\nPerhaps dscho might be amenable to adding this variant to\nhttps://github.com/gitgitgadget/git/blob/master/.github/workflows/main.yml\nin the future - so fsmonitor tests run automatically on diffs there.\n\n--Nipunn\n"},{"id":"419668","messageId":"CAN8Z4-Ug4i-zBrz0xSRXZUCM5+50Cg46m6Ap5rsJWHu+=Nr6eQ@mail.gmail.com","threadId":"55316","inReplyTo":"xmqqk0q4rxxh.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2021-03-18T22:57:06Z","receivedAt":"2021-03-18T22:58:03Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"On Thu, Mar 18, 2021 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Isn't this the other way around, wrt to the previous step?\n>\n> At least, \"pass around istate throughout the callchain in the\n> diff-lib.c file\" change should stand alone and come much earlier in\n> the series (perhaps as step #1).  Then \"call refresh_fsmonitor from\n> run_diff_index() and make sure in check_removed() that fsmonitor\n> does not have bogus VALID bit\" assertion should come on top, as a\n> single step, I would think.\n\nJust to make sure I understand - it sounds like you're recommending I\nsplit this diff up into\ntwo parts - one which passes istate through the callstack, and a\nsecond which provides\nthis assertion. It also sounds like you're recommending reordering the series to\n\n1 - pass istate around callstack\n2 - add is_fsmonitor_refreshed and use it to assert fsmonitor is\nrefreshed in check_removed in prep for usage\n3 - use fsmonitor bit to save a call to lstat\n4 - Add perf benchmark test\n\nThis makes sense to me - and I can execute the factoring in the next\npatch series iteration\n\n\n--Nipunn\n"}]}