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

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

[Nipunn Koorapati via 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.

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

[PATCH 1/3] fsmonitor: skip lstat deletion check during git diff-index

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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

[PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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

Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Eric Sunshine]

On Sun, Mar 14, 2021 at 6:19 PM Nipunn Koorapati via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 <nipunn@dropbox.com>
> ---
> diff --git a/fsmonitor.h b/fsmonitor.h
> @@ -49,6 +49,17 @@ void refresh_fsmonitor(struct index_state *istate);
> +/*
> + * 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;
> +}

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

Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Nipunn Koorapati]

> 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?

Re: [PATCH 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Eric Sunshine]

On Mon, Mar 15, 2021 at 6:01 PM Nipunn Koorapati <nipunn1313@gmail.com> wrote:
> > 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

[PATCH 3/3] fsmonitor: add perf test for git diff HEAD

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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

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

[Nipunn Koorapati via 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

[PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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

Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index

[Junio C Hamano]

"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> 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 <nipunn@dropbox.com>
> ---
>  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) {

So when the cache entry is marked as VALID, we know it is there and
unmodified without asking lstat().  Otherwise we ask lstat() as
before.  OK.

>  		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);

And the VALID bit is set only for the ones that are untouched?  When
core_fsmonitor is not set, or istate->fsmonitor_has_run_once is set,
refresh_fsmonitor() becomes no-op and does not even drop the VALID
bit from the cache entries.  As run_diff_index() is rather
library-ish part of the system, are we sure no earlier attempts to
invoke fsmonitor have touched ce to set the VALID bit on at this
point?

Assuming that we won't see stray VALID bit to confuse us, the patch
looks good to me, but I am not sure what to base confidence on that
assumption.

Thanks.

>  	if (merge_base) {
>  		diff_get_merge_base(revs, &oid);
>  		name = oid_to_hex_r(merge_base_hex, &oid);

Re: [PATCH v2 1/3] fsmonitor: skip lstat deletion check during git diff-index

[Nipunn Koorapati]

On Thu, Mar 18, 2021 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > +     refresh_fsmonitor(istate);
>
> And the VALID bit is set only for the ones that are untouched?  When
> core_fsmonitor is not set, or istate->fsmonitor_has_run_once is set,
> refresh_fsmonitor() becomes no-op and does not even drop the VALID
> bit from the cache entries.  As run_diff_index() is rather
> library-ish part of the system, are we sure no earlier attempts to
> invoke fsmonitor have touched ce to set the VALID bit on at this
> point?
>
> Assuming that we won't see stray VALID bit to confuse us, the patch
> looks good to me, but I am not sure what to base confidence on that
> assumption.
>
> Thanks.

My understanding is that git's invariants around
fsmonitor bit would ensure that the bit is set simultaneously w/ the
rest of the index entry
after a stat (at the cursor/timestamp of the most recent refresh_fsmonitor).

Regardless of whether earlier access to the VALID bit happened within
the command, the
state should be internally consistent at this point, meaning that if
valid is set, the rest of the
entry is sensible.

I did just manually confirm this here
$ bin-wrappers/git diff HEAD
$ rm zlib.c
$ GIT_TRACE_FSMONITOR=1 bin-wrappers/git diff HEAD
21:21:22.911316 fsmonitor.c:97          read fsmonitor extension
successful 'c:1615751750:32210:5:60'
21:21:22.911417 fsmonitor.c:249         refresh fsmonitor
21:21:22.937726 fsmonitor.c:301         fsmonitor process
'.git/hooks/query-watchman' returned success
21:21:22.937749 fsmonitor.c:228         fsmonitor_refresh_callback 'zlib.c'
21:21:22.937755 fsmonitor.c:228         fsmonitor_refresh_callback '.git'
diff --git a/zlib.c b/zlib.c
deleted file mode 100644
[rest of output omitted]

This should be confirmable in the testsuite via
ls t*.sh | grep diff | GIT_TEST_FSMONITOR=$PWD/t7519/fsmonitor-all xargs prove

Perhaps dscho might be amenable to adding this variant to
https://github.com/gitgitgadget/git/blob/master/.github/workflows/main.yml
in the future - so fsmonitor tests run automatically on diffs there.

--Nipunn

[PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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

Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Junio C Hamano]

"Nipunn Koorapati via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nipunn Koorapati <nipunn@dropbox.com>
>
> Validate that fsmonitor is valid to futureproof against bugs where
> check_removed might be called from places that haven't refreshed.

Isn't this the other way around, wrt to the previous step?

At least, "pass around istate throughout the callchain in the
diff-lib.c file" change should stand alone and come much earlier in
the series (perhaps as step #1).  Then "call refresh_fsmonitor from
run_diff_index() and make sure in check_removed() that fsmonitor
does not have bogus VALID bit" assertion should come on top, as a
single step, I would think.

> Signed-off-by: Nipunn Koorapati <nipunn@dropbox.com>
> ---
>  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

Re: [PATCH v2 2/3] fsmonitor: add assertion that fsmonitor is valid to check_removed

[Nipunn Koorapati]

On Thu, Mar 18, 2021 at 1:47 PM Junio C Hamano <gitster@pobox.com> wrote:
> Isn't this the other way around, wrt to the previous step?
>
> At least, "pass around istate throughout the callchain in the
> diff-lib.c file" change should stand alone and come much earlier in
> the series (perhaps as step #1).  Then "call refresh_fsmonitor from
> run_diff_index() and make sure in check_removed() that fsmonitor
> does not have bogus VALID bit" assertion should come on top, as a
> single step, I would think.

Just to make sure I understand - it sounds like you're recommending I
split this diff up into
two parts - one which passes istate through the callstack, and a
second which provides
this assertion. It also sounds like you're recommending reordering the series to

1 - pass istate around callstack
2 - add is_fsmonitor_refreshed and use it to assert fsmonitor is
refreshed in check_removed in prep for usage
3 - use fsmonitor bit to save a call to lstat
4 - Add perf benchmark test

This makes sense to me - and I can execute the factoring in the next
patch series iteration


--Nipunn

[PATCH v2 3/3] fsmonitor: add perf test for git diff HEAD

[Nipunn Koorapati via GitGitGadget]

From: Nipunn Koorapati <nipunn@dropbox.com>

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 <nipunn@dropbox.com>
---
 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