[PATCH 0/3] teach git to respect fsmonitor in diff-index

Skip lstat deletion check during git diff-index (similar to how it already
does so in git diff-files). Add perf benchmark for this case. Add assert for
guaranteeing that fsmonitor is refreshed in this case.

Nipunn Koorapati (3):
  fsmonitor: skip lstat deletion check during git diff-index
  fsmonitor: add assertion that fsmonitor is valid to check_removed
  fsmonitor: add perf test for git diff HEAD

 diff-lib.c                | 23 +++++++++++++++--------
 fsmonitor.h               | 11 +++++++++++
 t/helper/test-chmtime.c   |  3 ++-
 t/perf/p7519-fsmonitor.sh |  4 ++++
 4 files changed, 32 insertions(+), 9 deletions(-)


base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-903%2Fnipunn1313%2Fnk%2Ffsmonitor-in-diff-index-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-903/nipunn1313/nk/fsmonitor-in-diff-index-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/903
-- 
gitgitgadget

From: Nipunn Koorapati <redacted>

Teach git to honor fsmonitor rather than issuing an lstat
when checking for dirty local deletes. Eliminates O(files)
lstats during `git diff HEAD`

Signed-off-by: Nipunn Koorapati <redacted>
---
 diff-lib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index b73cc1859a49..3fb538ad18e9 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -30,7 +30,7 @@
  */
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
-	if (lstat(ce->name, st) < 0) {
+	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
 		if (!is_missing_file_error(errno))
 			return -1;
 		return 1;

@@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 	struct object_id oid;
 	const char *name;
 	char merge_base_hex[GIT_MAX_HEXSZ + 1];
+	struct index_state *istate = revs->diffopt.repo->index;
 
 	if (revs->pending.nr != 1)
 		BUG("run_diff_index must be passed exactly one tree");

@@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 	trace_performance_enter();
 	ent = revs->pending.objects;
 
+	refresh_fsmonitor(istate);
+
 	if (merge_base) {
 		diff_get_merge_base(revs, &oid);
 		name = oid_to_hex_r(merge_base_hex, &oid);

-- 
gitgitgadget

From: Nipunn Koorapati <redacted>

Update the xargs call so that if your large repo contains
symlinks, test-tool chmtime failure does not end the script.

On Linux
Test                                                          this tree           upstream/master
---------------------------------------------------------------------------------------------------------
7519.4: status (fsmonitor=fsmonitor-watchman)                 0.52(0.43+0.10)     0.53(0.49+0.05) +1.9%
7519.5: status -uno (fsmonitor=fsmonitor-watchman)            0.21(0.15+0.07)     0.22(0.13+0.09) +4.8%
7519.6: status -uall (fsmonitor=fsmonitor-watchman)           1.65(0.93+0.71)     1.69(1.03+0.65) +2.4%
7519.7: status (dirty) (fsmonitor=fsmonitor-watchman)         11.99(11.34+1.58)   11.95(11.02+1.79) -0.3%
7519.8: diff (fsmonitor=fsmonitor-watchman)                   0.25(0.17+0.26)     0.25(0.18+0.26) +0.0%
7519.9: diff HEAD (fsmonitor=fsmonitor-watchman)              0.39(0.25+0.34)     0.89(0.35+0.74) +128.2%
7519.10: diff -- 0_files (fsmonitor=fsmonitor-watchman)       0.16(0.13+0.04)     0.16(0.12+0.05) +0.0%
7519.11: diff -- 10_files (fsmonitor=fsmonitor-watchman)      0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%
7519.12: diff -- 100_files (fsmonitor=fsmonitor-watchman)     0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%
7519.13: diff -- 1000_files (fsmonitor=fsmonitor-watchman)    0.16(0.11+0.06)     0.16(0.12+0.05) +0.0%
7519.14: diff -- 10000_files (fsmonitor=fsmonitor-watchman)   0.18(0.13+0.06)     0.17(0.10+0.08) -5.6%
7519.15: add (fsmonitor=fsmonitor-watchman)                   2.25(1.53+0.68)     2.25(1.47+0.74) +0.0%
7519.18: status (fsmonitor=disabled)                          0.88(0.73+1.03)     0.89(0.67+1.08) +1.1%
7519.19: status -uno (fsmonitor=disabled)                     0.45(0.43+0.89)     0.45(0.34+0.98) +0.0%
7519.20: status -uall (fsmonitor=disabled)                    1.88(1.16+1.58)     1.88(1.22+1.51) +0.0%
7519.21: status (dirty) (fsmonitor=disabled)                  7.53(7.05+2.11)     7.53(6.98+2.04) +0.0%
7519.22: diff (fsmonitor=disabled)                            0.42(0.37+0.92)     0.42(0.38+0.91) +0.0%
7519.23: diff HEAD (fsmonitor=disabled)                       0.44(0.41+0.90)     0.44(0.40+0.91) +0.0%
7519.24: diff -- 0_files (fsmonitor=disabled)                 0.13(0.09+0.05)     0.13(0.09+0.05) +0.0%
7519.25: diff -- 10_files (fsmonitor=disabled)                0.13(0.10+0.04)     0.13(0.10+0.04) +0.0%
7519.26: diff -- 100_files (fsmonitor=disabled)               0.13(0.09+0.05)     0.13(0.10+0.04) +0.0%
7519.27: diff -- 1000_files (fsmonitor=disabled)              0.13(0.09+0.06)     0.13(0.09+0.05) +0.0%
7519.28: diff -- 10000_files (fsmonitor=disabled)             0.14(0.11+0.05)     0.14(0.10+0.05) +0.0%
7519.29: add (fsmonitor=disabled)                             2.43(1.61+1.64)     2.43(1.69+1.57) +0.0%

On linux (2.29.2 vs w/ this patch):
nipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff 2>&1 | grep lstat
  0.04    0.000063           3        20         6 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff HEAD 2>&1 | grep lstat
 94.98    5.242262          10    523783        13 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff 2>&1 | grep lstat
  0.38    0.000032           5         7         3 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep lstat
 99.44    0.741892           9     81634        10 lstat

On mac (2.29.2 vs w/ this patch):
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff 2>&1 | grep "^lstat64 "
lstat64                                         8
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff HEAD 2>&1 | grep "^lstat64 "
lstat64                                    120242
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff 2>&1 | grep "^lstat64 "
lstat64                                         4
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep "^lstat64 "
lstat64                                      4497

There are still a bunch of lstats - on directories, but not every file. Progress!

Signed-off-by: Nipunn Koorapati <redacted>
---
 t/helper/test-chmtime.c   | 3 ++-
 t/perf/p7519-fsmonitor.sh | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index aa22af48c2a6..a8b143d11ab7 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c

@@ -111,7 +111,8 @@ int cmd__chmtime(int argc, const char **argv)
 		if (stat(argv[i], &sb) < 0) {
 			fprintf(stderr, "Failed to stat %s: %s\n",
 			        argv[i], strerror(errno));
-			return 1;
+			// Skip and move on - eg if it's a broken symlink
+			continue;
 		}
 
 #ifdef GIT_WINDOWS_NATIVE

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index b657564aed60..5eb5044a103c 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh

@@ -216,6 +216,10 @@ test_fsmonitor_suite() {
 		git diff
 	'
 
+	test_perf_w_drop_caches "diff HEAD ($DESC)" '
+		git diff HEAD
+	'
+
 	test_perf_w_drop_caches "diff -- 0_files ($DESC)" '
 		git diff -- 1_file
 	'

-- 
gitgitgadget

From: Nipunn Koorapati <redacted>

Validate that fsmonitor is valid to futureproof against bugs where
check_removed might be called from places that haven't refreshed.

Signed-off-by: Nipunn Koorapati <redacted>
---
 diff-lib.c  | 18 +++++++++++-------
 fsmonitor.h | 11 +++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 3fb538ad18e9..e5a58c9259cf 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -28,8 +28,9 @@
  * exists for ce that is a submodule -- it is a submodule that is not
  * checked out).  Return negative for an error.
  */
-static int check_removed(const struct cache_entry *ce, struct stat *st)
+static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
 {
+	assert(is_fsmonitor_refreshed(istate));
 	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
 		if (!is_missing_file_error(errno))
 			return -1;

@@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			memset(&(dpath->parent[0]), 0,
 			       sizeof(struct combine_diff_parent)*5);
 
-			changed = check_removed(ce, &st);
+			changed = check_removed(istate, ce, &st);
 			if (!changed)
 				wt_mode = ce_mode_from_stat(ce, st.st_mode);
 			else {

@@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		} else {
 			struct stat st;
 
-			changed = check_removed(ce, &st);
+			changed = check_removed(istate, ce, &st);
 			if (changed) {
 				if (changed < 0) {
 					perror(ce->name);

@@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,
 		       oid, oid_valid, ce->name, dirty_submodule);
 }
 
-static int get_stat_data(const struct cache_entry *ce,
+static int get_stat_data(const struct index_state *istate,
+			 const struct cache_entry *ce,
 			 const struct object_id **oidp,
 			 unsigned int *modep,
 			 int cached, int match_missing,

@@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,
 	if (!cached && !ce_uptodate(ce)) {
 		int changed;
 		struct stat st;
-		changed = check_removed(ce, &st);
+		changed = check_removed(istate, ce, &st);
 		if (changed < 0)
 			return -1;
 		else if (changed) {

@@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,
 	const struct object_id *oid;
 	unsigned int mode;
 	unsigned dirty_submodule = 0;
+	struct index_state *istate = revs->diffopt.repo->index;
 
 	/*
 	 * New file in the index: it might actually be different in
 	 * the working tree.
 	 */
-	if (get_stat_data(new_file, &oid, &mode, cached, match_missing,
+	if (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,
 	    &dirty_submodule, &revs->diffopt) < 0)
 		return;
 

@@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,
 	unsigned int mode, oldmode;
 	const struct object_id *oid;
 	unsigned dirty_submodule = 0;
+	struct index_state *istate = revs->diffopt.repo->index;
 
-	if (get_stat_data(new_entry, &oid, &mode, cached, match_missing,
+	if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,
 			  &dirty_submodule, &revs->diffopt) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old_entry,

diff --git a/fsmonitor.h b/fsmonitor.h
index 7f1794b90b00..c12f10117544 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h

@@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);
  */
 int fsmonitor_is_trivial_response(const struct strbuf *query_result);
 
+/*
+ * Check if refresh_fsmonitor has been called at least once.
+ * refresh_fsmonitor is idempotent. Returns true if fsmonitor is
+ * not enabled (since the state will be "fresh" w/ CE_FSMONITOR_VALID unset)
+ * This version is useful for assertions
+ */
+static inline int is_fsmonitor_refreshed(const struct index_state *istate)
+{
+    return !core_fsmonitor || istate->fsmonitor_has_run_once;
+}
+
 /*
  * Set the given cache entries CE_FSMONITOR_VALID bit. This should be
  * called any time the cache entry has been updated to reflect the

-- 
gitgitgadget

On Sun, Mar 14, 2021 at 6:19 PM Nipunn Koorapati via GitGitGadget
[off-list ref] wrote:

Unusual 4-space indentation rather than typical 1-tab.

Unusual 4-space indentation rather than typical 1-tab.

Thanks for identifying - will fix in the next patch series. Perhaps in
a separate patch we could add a unit test that validates the codebase
for such style?

On Mon, Mar 15, 2021 at 6:01 PM Nipunn Koorapati [off-list ref] wrote:

quoted

Unusual 4-space indentation rather than typical 1-tab.

Thanks for identifying - will fix in the next patch series. Perhaps in
a separate patch we could add a unit test that validates the codebase
for such style?

My guess is that a test which complains about existing style
violations would not be particularly helpful since it's output likely
would be noisy due to existing style violations. (For the same reason,
we wouldn't want to complain about style violations in tests either
since existing test scripts are full of violations.) What is more
interesting than identifying existing violations is identifying
violations before they make it into the codebase. For instance, you
could run your patches through `checkpatch.pl`[1] before submitting
them.

[1]: https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl

From: Nipunn Koorapati <redacted>

Teach git to honor fsmonitor rather than issuing an lstat
when checking for dirty local deletes. Eliminates O(files)
lstats during `git diff HEAD`

Signed-off-by: Nipunn Koorapati <redacted>
---
 diff-lib.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/diff-lib.c b/diff-lib.c
index b73cc1859a49..3fb538ad18e9 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -30,7 +30,7 @@
  */
 static int check_removed(const struct cache_entry *ce, struct stat *st)
 {
-	if (lstat(ce->name, st) < 0) {
+	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
 		if (!is_missing_file_error(errno))
 			return -1;
 		return 1;

@@ -574,6 +574,7 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 	struct object_id oid;
 	const char *name;
 	char merge_base_hex[GIT_MAX_HEXSZ + 1];
+	struct index_state *istate = revs->diffopt.repo->index;
 
 	if (revs->pending.nr != 1)
 		BUG("run_diff_index must be passed exactly one tree");

@@ -581,6 +582,8 @@ int run_diff_index(struct rev_info *revs, unsigned int option)
 	trace_performance_enter();
 	ent = revs->pending.objects;
 
+	refresh_fsmonitor(istate);
+
 	if (merge_base) {
 		diff_get_merge_base(revs, &oid);
 		name = oid_to_hex_r(merge_base_hex, &oid);

-- 
gitgitgadget

Skip lstat deletion check during git diff-index (similar to how it already
does so in git diff-files). Add perf benchmark for this case. Add assert for
guaranteeing that fsmonitor is refreshed in this case.

Update since Patch Series V1:

 * Fix spaces->tabs issue in fsmonitor.h
 * Remove comment in test-chmtime.c - to avoid it going stale

cc: Eric Sunshine sunshine@sunshineco.com

Nipunn Koorapati (3):
  fsmonitor: skip lstat deletion check during git diff-index
  fsmonitor: add assertion that fsmonitor is valid to check_removed
  fsmonitor: add perf test for git diff HEAD

 diff-lib.c                | 23 +++++++++++++++--------
 fsmonitor.h               | 11 +++++++++++
 t/helper/test-chmtime.c   |  4 ++--
 t/perf/p7519-fsmonitor.sh |  4 ++++
 4 files changed, 32 insertions(+), 10 deletions(-)


base-commit: 13d7ab6b5d7929825b626f050b62a11241ea4945
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-903%2Fnipunn1313%2Fnk%2Ffsmonitor-in-diff-index-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-903/nipunn1313/nk/fsmonitor-in-diff-index-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/903

Range-diff vs v1:

 1:  75a3c46c4055 = 1:  75a3c46c4055 fsmonitor: skip lstat deletion check during git diff-index
 2:  dda5b537a3f0 ! 2:  afd326c5011b fsmonitor: add assertion that fsmonitor is valid to check_removed
     @@ fsmonitor.h: void refresh_fsmonitor(struct index_state *istate);
      + */
      +static inline int is_fsmonitor_refreshed(const struct index_state *istate)
      +{
     -+    return !core_fsmonitor || istate->fsmonitor_has_run_once;
     ++	return !core_fsmonitor || istate->fsmonitor_has_run_once;
      +}
      +
       /*
 3:  740302586dd8 ! 3:  f9d0fd594fdb fsmonitor: add perf test for git diff HEAD
     @@ Commit message
      
       ## t/helper/test-chmtime.c ##
      @@ t/helper/test-chmtime.c: int cmd__chmtime(int argc, const char **argv)
     + 		uintmax_t mtime;
     + 
       		if (stat(argv[i], &sb) < 0) {
     - 			fprintf(stderr, "Failed to stat %s: %s\n",
     +-			fprintf(stderr, "Failed to stat %s: %s\n",
     ++			fprintf(stderr, "Failed to stat %s: %s. Skipping\n",
       			        argv[i], strerror(errno));
      -			return 1;
     -+			// Skip and move on - eg if it's a broken symlink
      +			continue;
       		}
       

-- 
gitgitgadget

From: Nipunn Koorapati <redacted>

Update the xargs call so that if your large repo contains
symlinks, test-tool chmtime failure does not end the script.

On Linux
Test                                                          this tree           upstream/master
---------------------------------------------------------------------------------------------------------
7519.4: status (fsmonitor=fsmonitor-watchman)                 0.52(0.43+0.10)     0.53(0.49+0.05) +1.9%
7519.5: status -uno (fsmonitor=fsmonitor-watchman)            0.21(0.15+0.07)     0.22(0.13+0.09) +4.8%
7519.6: status -uall (fsmonitor=fsmonitor-watchman)           1.65(0.93+0.71)     1.69(1.03+0.65) +2.4%
7519.7: status (dirty) (fsmonitor=fsmonitor-watchman)         11.99(11.34+1.58)   11.95(11.02+1.79) -0.3%
7519.8: diff (fsmonitor=fsmonitor-watchman)                   0.25(0.17+0.26)     0.25(0.18+0.26) +0.0%
7519.9: diff HEAD (fsmonitor=fsmonitor-watchman)              0.39(0.25+0.34)     0.89(0.35+0.74) +128.2%
7519.10: diff -- 0_files (fsmonitor=fsmonitor-watchman)       0.16(0.13+0.04)     0.16(0.12+0.05) +0.0%
7519.11: diff -- 10_files (fsmonitor=fsmonitor-watchman)      0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%
7519.12: diff -- 100_files (fsmonitor=fsmonitor-watchman)     0.16(0.12+0.05)     0.16(0.12+0.05) +0.0%
7519.13: diff -- 1000_files (fsmonitor=fsmonitor-watchman)    0.16(0.11+0.06)     0.16(0.12+0.05) +0.0%
7519.14: diff -- 10000_files (fsmonitor=fsmonitor-watchman)   0.18(0.13+0.06)     0.17(0.10+0.08) -5.6%
7519.15: add (fsmonitor=fsmonitor-watchman)                   2.25(1.53+0.68)     2.25(1.47+0.74) +0.0%
7519.18: status (fsmonitor=disabled)                          0.88(0.73+1.03)     0.89(0.67+1.08) +1.1%
7519.19: status -uno (fsmonitor=disabled)                     0.45(0.43+0.89)     0.45(0.34+0.98) +0.0%
7519.20: status -uall (fsmonitor=disabled)                    1.88(1.16+1.58)     1.88(1.22+1.51) +0.0%
7519.21: status (dirty) (fsmonitor=disabled)                  7.53(7.05+2.11)     7.53(6.98+2.04) +0.0%
7519.22: diff (fsmonitor=disabled)                            0.42(0.37+0.92)     0.42(0.38+0.91) +0.0%
7519.23: diff HEAD (fsmonitor=disabled)                       0.44(0.41+0.90)     0.44(0.40+0.91) +0.0%
7519.24: diff -- 0_files (fsmonitor=disabled)                 0.13(0.09+0.05)     0.13(0.09+0.05) +0.0%
7519.25: diff -- 10_files (fsmonitor=disabled)                0.13(0.10+0.04)     0.13(0.10+0.04) +0.0%
7519.26: diff -- 100_files (fsmonitor=disabled)               0.13(0.09+0.05)     0.13(0.10+0.04) +0.0%
7519.27: diff -- 1000_files (fsmonitor=disabled)              0.13(0.09+0.06)     0.13(0.09+0.05) +0.0%
7519.28: diff -- 10000_files (fsmonitor=disabled)             0.14(0.11+0.05)     0.14(0.10+0.05) +0.0%
7519.29: add (fsmonitor=disabled)                             2.43(1.61+1.64)     2.43(1.69+1.57) +0.0%

On linux (2.29.2 vs w/ this patch):
nipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff 2>&1 | grep lstat
  0.04    0.000063           3        20         6 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c git diff HEAD 2>&1 | grep lstat
 94.98    5.242262          10    523783        13 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff 2>&1 | grep lstat
  0.38    0.000032           5         7         3 lstat
nipunn@nipunn-dbx:~/src/server3$ strace -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep lstat
 99.44    0.741892           9     81634        10 lstat

On mac (2.29.2 vs w/ this patch):
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff 2>&1 | grep "^lstat64 "
lstat64                                         8
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c git diff HEAD 2>&1 | grep "^lstat64 "
lstat64                                    120242
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff 2>&1 | grep "^lstat64 "
lstat64                                         4
nipunn-mbp:server nipunn$ sudo dtruss -L -f -c ../git/bin-wrappers/git diff HEAD 2>&1 | grep "^lstat64 "
lstat64                                      4497

There are still a bunch of lstats - on directories, but not every file. Progress!

Signed-off-by: Nipunn Koorapati <redacted>
---
 t/helper/test-chmtime.c   | 4 ++--
 t/perf/p7519-fsmonitor.sh | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index aa22af48c2a6..524b55ca496c 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c

@@ -109,9 +109,9 @@ int cmd__chmtime(int argc, const char **argv)
 		uintmax_t mtime;
 
 		if (stat(argv[i], &sb) < 0) {
-			fprintf(stderr, "Failed to stat %s: %s\n",
+			fprintf(stderr, "Failed to stat %s: %s. Skipping\n",
 			        argv[i], strerror(errno));
-			return 1;
+			continue;
 		}
 
 #ifdef GIT_WINDOWS_NATIVE

diff --git a/t/perf/p7519-fsmonitor.sh b/t/perf/p7519-fsmonitor.sh
index b657564aed60..5eb5044a103c 100755
--- a/t/perf/p7519-fsmonitor.sh
+++ b/t/perf/p7519-fsmonitor.sh

@@ -216,6 +216,10 @@ test_fsmonitor_suite() {
 		git diff
 	'
 
+	test_perf_w_drop_caches "diff HEAD ($DESC)" '
+		git diff HEAD
+	'
+
 	test_perf_w_drop_caches "diff -- 0_files ($DESC)" '
 		git diff -- 1_file
 	'

-- 
gitgitgadget

From: Nipunn Koorapati <redacted>

Validate that fsmonitor is valid to futureproof against bugs where
check_removed might be called from places that haven't refreshed.

Signed-off-by: Nipunn Koorapati <redacted>
---
 diff-lib.c  | 18 +++++++++++-------
 fsmonitor.h | 11 +++++++++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index 3fb538ad18e9..e5a58c9259cf 100644
--- a/diff-lib.c
+++ b/diff-lib.c

@@ -28,8 +28,9 @@
  * exists for ce that is a submodule -- it is a submodule that is not
  * checked out).  Return negative for an error.
  */
-static int check_removed(const struct cache_entry *ce, struct stat *st)
+static int check_removed(const struct index_state *istate, const struct cache_entry *ce, struct stat *st)
 {
+	assert(is_fsmonitor_refreshed(istate));
 	if (!(ce->ce_flags & CE_FSMONITOR_VALID) && lstat(ce->name, st) < 0) {
 		if (!is_missing_file_error(errno))
 			return -1;

@@ -136,7 +137,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 			memset(&(dpath->parent[0]), 0,
 			       sizeof(struct combine_diff_parent)*5);
 
-			changed = check_removed(ce, &st);
+			changed = check_removed(istate, ce, &st);
 			if (!changed)
 				wt_mode = ce_mode_from_stat(ce, st.st_mode);
 			else {

@@ -216,7 +217,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
 		} else {
 			struct stat st;
 
-			changed = check_removed(ce, &st);
+			changed = check_removed(istate, ce, &st);
 			if (changed) {
 				if (changed < 0) {
 					perror(ce->name);

@@ -278,7 +279,8 @@ static void diff_index_show_file(struct rev_info *revs,
 		       oid, oid_valid, ce->name, dirty_submodule);
 }
 
-static int get_stat_data(const struct cache_entry *ce,
+static int get_stat_data(const struct index_state *istate,
+			 const struct cache_entry *ce,
 			 const struct object_id **oidp,
 			 unsigned int *modep,
 			 int cached, int match_missing,

@@ -290,7 +292,7 @@ static int get_stat_data(const struct cache_entry *ce,
 	if (!cached && !ce_uptodate(ce)) {
 		int changed;
 		struct stat st;
-		changed = check_removed(ce, &st);
+		changed = check_removed(istate, ce, &st);
 		if (changed < 0)
 			return -1;
 		else if (changed) {

@@ -321,12 +323,13 @@ static void show_new_file(struct rev_info *revs,
 	const struct object_id *oid;
 	unsigned int mode;
 	unsigned dirty_submodule = 0;
+	struct index_state *istate = revs->diffopt.repo->index;
 
 	/*
 	 * New file in the index: it might actually be different in
 	 * the working tree.
 	 */
-	if (get_stat_data(new_file, &oid, &mode, cached, match_missing,
+	if (get_stat_data(istate, new_file, &oid, &mode, cached, match_missing,
 	    &dirty_submodule, &revs->diffopt) < 0)
 		return;
 

@@ -342,8 +345,9 @@ static int show_modified(struct rev_info *revs,
 	unsigned int mode, oldmode;
 	const struct object_id *oid;
 	unsigned dirty_submodule = 0;
+	struct index_state *istate = revs->diffopt.repo->index;
 
-	if (get_stat_data(new_entry, &oid, &mode, cached, match_missing,
+	if (get_stat_data(istate, new_entry, &oid, &mode, cached, match_missing,
 			  &dirty_submodule, &revs->diffopt) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old_entry,

diff --git a/fsmonitor.h b/fsmonitor.h
index 7f1794b90b00..f20d72631d76 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h

@@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);
  */
 int fsmonitor_is_trivial_response(const struct strbuf *query_result);
 
+/*
+ * Check if refresh_fsmonitor has been called at least once.
+ * refresh_fsmonitor is idempotent. Returns true if fsmonitor is
+ * not enabled (since the state will be "fresh" w/ CE_FSMONITOR_VALID unset)
+ * This version is useful for assertions
+ */
+static inline int is_fsmonitor_refreshed(const struct index_state *istate)
+{
+	return !core_fsmonitor || istate->fsmonitor_has_run_once;
+}
+
 /*
  * Set the given cache entries CE_FSMONITOR_VALID bit. This should be
  * called any time the cache entry has been updated to reflect the

-- 
gitgitgadget