Improve testability with GIT_TEST_FSMONITOR

The GIT_TEST_FSMONITOR environment variable allows run-time specification of the fsmonitor hook. Initially used by t7619-status-fsmonitor.sh, we can enable it across the test suite to see how it affects Git's behavior. In particular, we can specify the version of the hook that requests a result from Watchman to get actual updates to the files in the repo.

In many cases, our tests are simply not ready to handle this option. fsmonitor does not integrate well with features such as split index, bare repos, or submodules. Other times, we need to disable it because the test is being specific about what files Git inspects during a 'status' call.

The long-term vision is to be able to run CI builds using a file-system watcher like Watchman to get better coverage on this feature. These patches get us closer, but there are still some issues around overloading the Watchman interface when the tests are run in parallel. When using "prove -j8 t[0-8]*.sh" I see some failures that do not reproduce when running the test scripts in isolation (on Linux).

Thanks, -Stolee

Derrick Stolee (11):
fsmonitor: disable in a bare repo
fsmonitor: do not output to stderr for tests t1301-shared-repo.sh: disable FSMONITOR t1510-repo-setup.sh: disable fsmonitor if no .git dir
fsmonitor: disable fsmonitor with worktrees t3030-merge-recursive.sh: disable fsmonitor when tweaking
GIT_WORK_TREE
t3600-rm.sh: disable fsmonitor when deleting populated submodule
tests: disable fsmonitor in submodule tests
t7063: disable fsmonitor with status cache
t7519: disable external GIT_TEST_FSMONITOR variable
test-lib: clear watchman watches at test completion

 config.c                                     |  5 +++++
 t/t1301-shared-repo.sh                       |  1 +
 t/t1510-repo-setup.sh                        |  1 +
 t/t2400-worktree-add.sh                      |  2 ++
 t/t3030-merge-recursive.sh                   |  2 ++
 t/t3600-rm.sh                                |  1 +
 t/t4060-diff-submodule-option-diff-format.sh |  3 +++
 t/t5526-fetch-submodules.sh                  |  2 ++
 t/t7063-status-untracked-cache.sh            |  3 +++
 t/t7402-submodule-rebase.sh                  |  3 +++
 t/t7406-submodule-update.sh                  |  2 ++
 t/t7506-status-submodule.sh                  |  3 +++
 t/t7508-status.sh                            |  3 +++
 t/t7519-status-fsmonitor.sh                  |  3 +++
 t/t7519/fsmonitor-watchman                   |  1 -
 t/test-lib-functions.sh                      | 15 +++++++++++++++
 t/test-lib.sh                                |  2 ++
 17 files changed, 51 insertions(+), 1 deletion(-)

base-commit: dd0b61f577f041f1119bb3288451f8f9b7f9e3f2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-466%2Fderrickstolee%2Ftest-watchman-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-466/derrickstolee/test-watchman-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/466

gitgitgadget

From: Derrick Stolee <[email protected]>

t0003-attributes.sh

---
 config.c | 5 +++++
 1 file changed, 5 insertions(+)

index 3900e4947b..f6d4e2fae3 100644
@@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)
 
 int git_config_get_fsmonitor(void)
 {
+	if (!the_repository->worktree) {
+		core_fsmonitor = 0;
+		return 0;
+	}
+
 	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
 		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 

gitgitgadget

From: Derrick Stolee <[email protected]>

t0003-attributes.sh

---
 t/t7519/fsmonitor-watchman | 1 -
 1 file changed, 1 deletion(-)

index d8e7a1e5ba..06312876aa 100755
@@ -94,7 +94,6 @@ sub launch_watchman {
 	my $o = $json_pkg->new->utf8->decode($response);
 
 	if ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {
-		print STDERR "Adding '$git_work_tree' to watchman's watch list.\n";
 		$retry--;
 		qx/watchman watch "$git_work_tree"/;
 		die "Failed to make watchman watch '$git_work_tree'.\n" .

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/t1301-shared-repo.sh | 1 +
 1 file changed, 1 insertion(+)

index 2dc853d1be..665ade0cf2 100755
@@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 '
 
 test_expect_success POSIXPERM 'forced modes' '
+	GIT_TEST_FSMONITOR="" &&
 	mkdir -p templates/hooks &&
 	echo update-server-info >templates/hooks/post-update &&
 	chmod +x templates/hooks/post-update &&

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/t1510-repo-setup.sh | 1 +
 1 file changed, 1 insertion(+)

index 9974457f56..28dce0c26f 100755
@@ -775,6 +775,7 @@ test_expect_success '#29: setup' '
 	setup_repo 29 non-existent gitfile true &&
 	mkdir -p 29/sub/sub 29/wt/sub &&
 	(
+		GIT_TEST_FSMONITOR="" &&
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
 		export GIT_WORK_TREE &&

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/t2400-worktree-add.sh | 2 ++
 1 file changed, 2 insertions(+)

index e819ba741e..d4d3cbae0f 100755
@@ -1,5 +1,7 @@
 #!/bin/sh
 
+GIT_TEST_FSMONITOR=""
+
 test_description='test git worktree add'
 
 . ./test-lib.sh

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/t3030-merge-recursive.sh | 2 ++
 1 file changed, 2 insertions(+)

index ff641b348a..62f645d639 100755
@@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' '
 
 test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 	(
+		GIT_TEST_FSMONITOR="" &&
 		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
 		export GIT_WORK_TREE &&
 		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&

@@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 
 test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
 	(
+		GIT_TEST_FSMONITOR="" &&
 		GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
 		export GIT_WORK_TREE &&
 		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/t3600-rm.sh | 1 +
 1 file changed, 1 insertion(+)

index 66282a720e..64269bd89d 100755
@@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' '
 '
 
 test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
+	GIT_TEST_FSMONITOR="" &&
 	git reset --hard &&
 	git submodule update &&
 	git -C submod checkout HEAD^ &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature struggles with submodules. Disable the GIT_TEST_FSMONITOR environment variable before running tests with a lot of submodule interactions.

---
 t/t4060-diff-submodule-option-diff-format.sh | 3 +++
 t/t5526-fetch-submodules.sh                  | 2 ++
 t/t7402-submodule-rebase.sh                  | 3 +++
 t/t7406-submodule-update.sh                  | 2 ++
 t/t7506-status-submodule.sh                  | 3 +++
 t/t7508-status.sh                            | 3 +++
 6 files changed, 16 insertions(+)

index 9dcb69df5c..017417790e 100755
@@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff.
 # Tested non-UTF-8 encoding
 test_encoding="ISO8859-1"
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 # String "added" in German (translated with Google Translate), encoded in UTF-8,
 # used in sample commit log messages in add_file() function below.
 added=$(printf "hinzugef\303\274gt")

index 63205dfdf9..fb346bff05 100755
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Copyright (c) 2010, Jens Lehmann
 
+GIT_TEST_FSMONITOR=""
+
 test_description='Recursive "git fetch" for submodules'
 
 . ./test-lib.sh

index 8e32f19007..c78e9009cf 100755
@@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules'
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_expect_success setup '
 
 	echo file > file &&

index c973278300..8d93aaef5f 100755
@@ -11,6 +11,8 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
 
 compare_head()
 {

index 08629a6e70..1a716f2c2a 100755
@@ -4,6 +4,9 @@ test_description='git status for submodule'
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_create_repo_with_commit () {
 	test_create_repo "$1" &&
 	(

index 4e676cdce8..bf0487632d 100755
@@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '
 	test_cmp expect output
 '
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_expect_success 'setup status submodule summary' '
 	test_create_repo sm && (
 		cd sm &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The status cache tests use GIT_TRACE_UNTRACKED_STATS to check very detailed statistics related to how much Git actually checked for untracked files. The fsmonitor feature changes the expected behavior here, so disable the GIT_TEST_FSMONITOR environment variable.

---
 t/t7063-status-untracked-cache.sh | 3 +++
 1 file changed, 3 insertions(+)

index 190ae149cf..c433738a3a 100755
@@ -4,6 +4,9 @@ test_description='test untracked cache'
 
 . ./test-lib.sh
 
+# fsmonitor changes the expected behvaior of GIT_TRACE_UNTRACKED_STATS
+GIT_TEST_FSMONITOR=""
+
 # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
 # is updated lazily after contents in the directory changes, which
 # forces the untracked cache code to take the slow path.  A test

gitgitgadget

From: Derrick Stolee <[email protected]>

The GIT_TEST_FSMONITOR variable was created specifically so the t7519-status-fsmonitor.sh test script could tweak the expected behavior depending on its value. However, if we set it externally to use the Watchman integration, then it breaks the initial tests that demonstrate behavior _without_ the fsmonitor feature.

Disable this variable at the start of the script.

---
 t/t7519-status-fsmonitor.sh | 3 +++
 1 file changed, 3 insertions(+)

index 81a375fa0f..443d2e653b 100755
@@ -8,6 +8,9 @@ test_description='git status with file system watcher'
 # "git update-index --fsmonitor" can be used to get the extension written
 # before testing the results.
 
+# Disable an external value, as we will set it directly as needed.
+GIT_TEST_FSMONITOR=""
+
 clean_repo () {
 	git reset --hard HEAD &&
 	git clean -fd

gitgitgadget

From: Derrick Stolee <[email protected]>

---
 t/test-lib-functions.sh | 15 +++++++++++++++
 t/test-lib.sh           |  2 ++
 2 files changed, 17 insertions(+)

index e0b3f28d3a..03573caf42 100644
@@ -1475,3 +1475,18 @@ test_set_port () {
 	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
 	eval $var=$port
 }
+
+test_clear_watchman () {
+	if test $GIT_TEST_FSMONITOR -ne ""
+	then
+		watchman watch-list |
+			grep "$TRASH_DIRECTORY" |
+			sed "s/\t\"//g" |
+			sed "s/\",//g" >repo-list
+
+		for repo in $(cat repo-list)
+		do
+			watchman watch-del "$repo"
+		done
+	fi
+}

index 30b07e310f..067a432ea5 100644
@@ -1072,6 +1072,8 @@ test_atexit_handler () {
 	# sure that the registered cleanup commands are run only once.
 	test : != "$test_atexit_cleanup" || return 0
 
+	test_clear_watchman
+
 	setup_malloc_check
 	test_eval_ "$test_atexit_cleanup"
 	test_atexit_cleanup=:

gitgitgadget

Hi Stolee,

Patches 1 and 2 have this line in the commit message. I assume it's a typo?

Also, for the patches with empty commit message bodies, it would be nice if they were filled in with detail for _why_ the change is being made. For example, in this patch, a future reader might wonder why having fsmonitor enabled in a bare repo causes problems.

Thanks,

Denton

Thanks for pointing that out. I forgot to clean up these messages, and what I left in them right now is sometimes just the first test script that was failing without the change.

Thanks,
-Stolee

On 11/21/2019 8:06 PM, SZEDER Gábor wrote:

Thanks for this message. Sorry I'm so late getting back to it.

Thanks for the pointers.

The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR, and that can only use watchman (currently). Barring wanting to unset the variable if it was set on purpose in a test script, the other options do not actually return correct values to make use of the feature.

Ah, I misunderstood the point of test_atexit_handler.

Shouldn't it be sufficient to add it into test_done? If the test fails, then we could leave watches open, but that's no worse than we had without this test_clear_watchman method.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

If the repository is bare, then there is no working directory to watch. Disable the core_fsmonitor global in this case.

Before this change, the test t0003-attributes.sh would fail with GIT_TEST_FSMONITOR pointing to t/t7519/fsmonitor-watchman.

---
 config.c | 5 +++++
 1 file changed, 5 insertions(+)

index 3900e4947b..f6d4e2fae3 100644
@@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)
 
 int git_config_get_fsmonitor(void)
 {
+	if (!the_repository->worktree) {
+		core_fsmonitor = 0;
+		return 0;
+	}
+
 	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
 		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
 

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

The test t0003-attributes.sh and others would fail when GIT_TEST_FSMONITOR is pointing at t/t7519/fsmonitor-watchman because it sends a message over stderr when registering the repo with watchman for the first time. Remove this stderr message for the test script to avoid this noise.

---
 t/t7519/fsmonitor-watchman | 1 -
 1 file changed, 1 deletion(-)

index d8e7a1e5ba..06312876aa 100755
@@ -94,7 +94,6 @@ sub launch_watchman {
 	my $o = $json_pkg->new->utf8->decode($response);
 
 	if ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {
-		print STDERR "Adding '$git_work_tree' to watchman's watch list.\n";
 		$retry--;
 		qx/watchman watch "$git_work_tree"/;
 		die "Failed to make watchman watch '$git_work_tree'.\n" .

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR is set to t/t7519/fsmonitor-watchman because it changes permissions in an incompatible way.

---
 t/t1301-shared-repo.sh | 1 +
 1 file changed, 1 insertion(+)

index 2dc853d1be..665ade0cf2 100755
@@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
 '
 
 test_expect_success POSIXPERM 'forced modes' '
+	GIT_TEST_FSMONITOR="" &&
 	mkdir -p templates/hooks &&
 	echo update-server-info >templates/hooks/post-update &&
 	chmod +x templates/hooks/post-update &&

gitgitgadget

The GIT_TEST_FSMONITOR environment variable allows run-time specification of the fsmonitor hook. Initially used by t7619-status-fsmonitor.sh, we can enable it across the test suite to see how it affects Git's behavior. In particular, we can specify the version of the hook that requests a result from Watchman to get actual updates to the files in the repo.

In many cases, our tests are simply not ready to handle this option. fsmonitor does not integrate well with features such as split index, bare repos, or submodules. Other times, we need to disable it because the test is being specific about what files Git inspects during a 'status' call.

The long-term vision is to be able to run CI builds using a file-system watcher like Watchman to get better coverage on this feature. These patches get us closer, but there are still some issues around overloading the Watchman interface when the tests are run in parallel. When using "prove -j8 t[0-8]*.sh" I see some failures that do not reproduce when running the test scripts in isolation (on Linux), but using "-j5" is enough to eliminate failures.

Some of these changes should be backed out in favor of a deeper fix, so in some sense these settings of GIT_TEST_FSMONITOR="" serve as TODOs for the fsmonitor feature.

Some recent and upcoming work on fsmonitor by Utsav Shah and Kevin Willford may help us reach the goal of running with watchman enabled in CI builds. I'll come back around with updates to the .azure-pipelines YAML files when we feel the feature is ready for that.

Updates in V2:

* Commit messages have been filled out completely. Most provide the same blurb of context before briefly describing the actual change.

* Some commits were merged because they had similar causes (worktrees, submodules).

* The test_clear_watchman function is updated and it is called by test_done instead of the atexit helper.

Thanks, -Stolee

Derrick Stolee (8):
fsmonitor: disable in a bare repo
fsmonitor: do not output to stderr for tests t1301-shared-repo.sh: disable FSMONITOR t3030-merge-recursive.sh: disable fsmonitor when tweaking
GIT_WORK_TREE
tests: disable fsmonitor in submodule tests
t7063: disable fsmonitor with status cache
t7519: disable external GIT_TEST_FSMONITOR variable
test-lib: clear watchman watches at test completion

 config.c                                     |  5 +++++
 t/t1301-shared-repo.sh                       |  1 +
 t/t1510-repo-setup.sh                        |  1 +
 t/t2400-worktree-add.sh                      |  2 ++
 t/t3030-merge-recursive.sh                   |  2 ++
 t/t3404-rebase-interactive.sh                |  1 +
 t/t3600-rm.sh                                |  1 +
 t/t4060-diff-submodule-option-diff-format.sh |  3 +++
 t/t5526-fetch-submodules.sh                  |  2 ++
 t/t7063-status-untracked-cache.sh            |  3 +++
 t/t7402-submodule-rebase.sh                  |  3 +++
 t/t7406-submodule-update.sh                  |  2 ++
 t/t7506-status-submodule.sh                  |  3 +++
 t/t7508-status.sh                            |  3 +++
 t/t7519-status-fsmonitor.sh                  |  3 +++
 t/t7519/fsmonitor-watchman                   |  1 -
 t/test-lib-functions.sh                      | 15 +++++++++++++++
 t/test-lib.sh                                |  4 ++++
 18 files changed, 54 insertions(+), 1 deletion(-)

base-commit: dd0b61f577f041f1119bb3288451f8f9b7f9e3f2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-466%2Fderrickstolee%2Ftest-watchman-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-466/derrickstolee/test-watchman-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/466

Range-diff vs v1:

1: 9bf5a803e6 ! 1: 79bb4c8e7d fsmonitor: disable in a bare repo
@@ -2,7 +2,18 @@

fsmonitor: disable in a bare repo

- t0003-attributes.sh
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ If the repository is bare, then there is no working directory to
+ watch. Disable the core_fsmonitor global in this case.
+
+ Before this change, the test t0003-attributes.sh would fail with
+ GIT_TEST_FSMONITOR pointing to t/t7519/fsmonitor-watchman.

2: 245cc1b2cc ! 2: dd492091e3 fsmonitor: do not output to stderr for tests
@@ -2,7 +2,18 @@

fsmonitor: do not output to stderr for tests

- t0003-attributes.sh
+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ The test t0003-attributes.sh and others would fail when
+ GIT_TEST_FSMONITOR is pointing at t/t7519/fsmonitor-watchman because
+ it sends a message over stderr when registering the repo with
+ watchman for the first time. Remove this stderr message for the
+ test script to avoid this noise.

3: ed40ce8e4c ! 3: f9db0c3416 t1301-shared-repo.sh: disable FSMONITOR
@@ -2,6 +2,17 @@

t1301-shared-repo.sh: disable FSMONITOR

+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR
+ is set to t/t7519/fsmonitor-watchman because it changes permissions
+ in an incompatible way.
+

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh

  4:  9179776982 <  -:  ---------- t1510-repo-setup.sh: disable fsmonitor if no .git dir
  5:  81c587651e <  -:  ---------- fsmonitor: disable fsmonitor with worktrees
  6:  d5f5a3e2b9 !  4:  efc16962ee t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

@@ -2,8 +2,47 @@

t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ Worktrees use a ".git" _file_ instead of a folder to point to
+ the base repo's .git directory and the proper worktree HEAD. The
+ fsmonitor hook tries to create a JSON file inside the ".git" folder
+ which violates the expectation here. It would be better to properly
+ find a safe folder for storing this JSON file.
+
+ This is also a problem when a test script uses GIT_WORK_TREE.
+

+ diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
+ --- a/t/t1510-repo-setup.sh
+ +++ b/t/t1510-repo-setup.sh
+@@
+ setup_repo 29 non-existent gitfile true &&
+ mkdir -p 29/sub/sub 29/wt/sub &&
+ (
++ GIT_TEST_FSMONITOR="" &&
+ cd 29 &&
+ GIT_WORK_TREE="$here/29" &&
+ export GIT_WORK_TREE &&
+
+ diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
+ --- a/t/t2400-worktree-add.sh
+ +++ b/t/t2400-worktree-add.sh
+@@
+ #!/bin/sh
+
++GIT_TEST_FSMONITOR=""
++
+ test_description='test git worktree add'
+
+ . ./test-lib.sh
+
diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh --- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh

  7:  d0b212c9df <  -:  ---------- t3600-rm.sh: disable fsmonitor when deleting populated submodule
  8:  36f845cb7e !  5:  a5b0bf6ac7 tests: disable fsmonitor in submodule tests

@@ -2,12 +2,43 @@

tests: disable fsmonitor in submodule tests

+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
The fsmonitor feature struggles with submodules. Disable the GIT_TEST_FSMONITOR environment variable before running tests with a lot of submodule interactions.

+ diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
+ --- a/t/t3404-rebase-interactive.sh
+ +++ b/t/t3404-rebase-interactive.sh
+@@
+ '
+
+ test_expect_success 'submodule rebase setup' '
++ GIT_TEST_FSMONITOR="" &&
+ git checkout A &&
+ mkdir sub &&
+ (
+
+ diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
+ --- a/t/t3600-rm.sh
+ +++ b/t/t3600-rm.sh
+@@
+ '
+
+ test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
++ GIT_TEST_FSMONITOR="" &&
+ git reset --hard &&
+ git submodule update &&
+ git -C submod checkout HEAD^ &&
+
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh --- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
9: 141a1909a4 ! 6: 9cd4a08d82 t7063: disable fsmonitor with status cache
@@ -2,6 +2,13 @@

t7063: disable fsmonitor with status cache

+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
The status cache tests use GIT_TRACE_UNTRACKED_STATS to check very detailed statistics related to how much Git actually checked for untracked files. The fsmonitor feature changes the expected behavior

 10:  cd717ef5de =  7:  215ec8688e t7519: disable external GIT_TEST_FSMONITOR variable
 11:  47cecb4a83 !  8:  e51165f260 test-lib: clear watchman watches at test completion

@@ -2,6 +2,29 @@

test-lib: clear watchman watches at test completion

+ The fsmonitor feature allows an external tool such as watchman to
+ monitor the working directory. The direct test
+ t7619-status-fsmonitor.sh provides some coverage, but it would be
+ better to run the entire test suite with watchman enabled. This
+ would provide more confidence that the feature is working as
+ intended.
+
+ When running the test suite in parallel with 'prove -j <N>', many
+ repos are created and deleted in parallel. When GIT_TEST_FSMONITOR
+ points to t/t7519/fsmonitor-watchman, this can lead to watchman
+ tracking many different folders, overloading its watch queue.
+
+ As a test script completes, we can tell watchman to stop watching
+ the directories inside the TRASH_DIRECTORY.
+
+ This is particularly important on Windows where watchman keeps an
+ open handle on the directories it watches, preventing them from
+ being deleted. There is currently a bug in watchman [1] where this
+ handle still is not closed, but if that is updated then these tests
+ can be run on Windows as well.
+
+ [1] https://github.com/facebook/watchman/issues/764
+

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
@@ -13,17 +36,17 @@
}
+
+test_clear_watchman () {
-+ if test $GIT_TEST_FSMONITOR -ne ""
++ if test -n "$GIT_TEST_FSMONITOR"
+ then
+ watchman watch-list |
+ grep "$TRASH_DIRECTORY" |
-+ sed "s/\t\"//g" |
-+ sed "s/\",//g" >repo-list
++ sed "s/\",//g" |
++ sed "s/\"//g" >repo-list
+
-+ for repo in $(cat repo-list)
++ while read repo
+ do
+ watchman watch-del "$repo"
-+ done
++ done <repo-list
+ fi
+}

@@ -31,11 +54,13 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- # sure that the registered cleanup commands are run only once.
- test : != "$test_atexit_cleanup" || return 0
+ test_done () {
+ GIT_EXIT_OK=t

++ # If watchman is being used with GIT_TEST_FSMONITOR, then
++ # clear all watches on directories inside the TRASH_DIRECTORY.
+ test_clear_watchman
+
- setup_malloc_check
- test_eval_ "$test_atexit_cleanup"
- test_atexit_cleanup=:
+ # Run the atexit commands _before_ the trash directory is
+ # removed, so the commands can access pidfiles and socket files.
+ test_atexit_handler

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

Worktrees use a ".git" _file_ instead of a folder to point to the base repo's .git directory and the proper worktree HEAD. The fsmonitor hook tries to create a JSON file inside the ".git" folder which violates the expectation here. It would be better to properly find a safe folder for storing this JSON file.

This is also a problem when a test script uses GIT_WORK_TREE.

---
 t/t1510-repo-setup.sh      | 1 +
 t/t2400-worktree-add.sh    | 2 ++
 t/t3030-merge-recursive.sh | 2 ++
 3 files changed, 5 insertions(+)

index 9974457f56..28dce0c26f 100755
@@ -775,6 +775,7 @@ test_expect_success '#29: setup' '
 	setup_repo 29 non-existent gitfile true &&
 	mkdir -p 29/sub/sub 29/wt/sub &&
 	(
+		GIT_TEST_FSMONITOR="" &&
 		cd 29 &&
 		GIT_WORK_TREE="$here/29" &&
 		export GIT_WORK_TREE &&

index e819ba741e..d4d3cbae0f 100755
@@ -1,5 +1,7 @@
 #!/bin/sh
 
+GIT_TEST_FSMONITOR=""
+
 test_description='test git worktree add'
 
 . ./test-lib.sh

index ff641b348a..62f645d639 100755
@@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' '
 
 test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 	(
+		GIT_TEST_FSMONITOR="" &&
 		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
 		export GIT_WORK_TREE &&
 		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&

@@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
 
 test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
 	(
+		GIT_TEST_FSMONITOR="" &&
 		GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
 		export GIT_WORK_TREE &&
 		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&

gitgitgadget

From: Derrick Stolee <[email protected]>

The GIT_TEST_FSMONITOR variable was created specifically so the t7519-status-fsmonitor.sh test script could tweak the expected behavior depending on its value. However, if we set it externally to use the Watchman integration, then it breaks the initial tests that demonstrate behavior _without_ the fsmonitor feature.

Disable this variable at the start of the script.

---
 t/t7519-status-fsmonitor.sh | 3 +++
 1 file changed, 3 insertions(+)

index 81a375fa0f..443d2e653b 100755
@@ -8,6 +8,9 @@ test_description='git status with file system watcher'
 # "git update-index --fsmonitor" can be used to get the extension written
 # before testing the results.
 
+# Disable an external value, as we will set it directly as needed.
+GIT_TEST_FSMONITOR=""
+
 clean_repo () {
 	git reset --hard HEAD &&
 	git clean -fd

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

When running the test suite in parallel with 'prove -j <N>', many repos are created and deleted in parallel. When GIT_TEST_FSMONITOR points to t/t7519/fsmonitor-watchman, this can lead to watchman tracking many different folders, overloading its watch queue.

As a test script completes, we can tell watchman to stop watching the directories inside the TRASH_DIRECTORY.

This is particularly important on Windows where watchman keeps an open handle on the directories it watches, preventing them from being deleted. There is currently a bug in watchman [1] where this handle still is not closed, but if that is updated then these tests can be run on Windows as well.

[1] https://github.com/facebook/watchman/issues/764

---
 t/test-lib-functions.sh | 15 +++++++++++++++
 t/test-lib.sh           |  4 ++++
 2 files changed, 19 insertions(+)

index e0b3f28d3a..ef840ce097 100644
@@ -1475,3 +1475,18 @@ test_set_port () {
 	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
 	eval $var=$port
 }
+
+test_clear_watchman () {
+	if test -n "$GIT_TEST_FSMONITOR"
+	then
+		watchman watch-list |
+			grep "$TRASH_DIRECTORY" |
+			sed "s/\",//g" |
+			sed "s/\"//g" >repo-list
+
+		while read repo
+		do
+			watchman watch-del "$repo"
+		done <repo-list
+	fi
+}

index 30b07e310f..4114953ebc 100644
@@ -1081,6 +1081,10 @@ test_atexit_handler () {
 test_done () {
 	GIT_EXIT_OK=t
 
+	# If watchman is being used with GIT_TEST_FSMONITOR, then
+	# clear all watches on directories inside the TRASH_DIRECTORY.
+	test_clear_watchman
+
 	# Run the atexit commands _before_ the trash directory is
 	# removed, so the commands can access pidfiles and socket files.
 	test_atexit_handler

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

The status cache tests use GIT_TRACE_UNTRACKED_STATS to check very detailed statistics related to how much Git actually checked for untracked files. The fsmonitor feature changes the expected behavior here, so disable the GIT_TEST_FSMONITOR environment variable.

---
 t/t7063-status-untracked-cache.sh | 3 +++
 1 file changed, 3 insertions(+)

index 190ae149cf..c433738a3a 100755
@@ -4,6 +4,9 @@ test_description='test untracked cache'
 
 . ./test-lib.sh
 
+# fsmonitor changes the expected behvaior of GIT_TRACE_UNTRACKED_STATS
+GIT_TEST_FSMONITOR=""
+
 # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
 # is updated lazily after contents in the directory changes, which
 # forces the untracked cache code to take the slow path.  A test

gitgitgadget

From: Derrick Stolee <[email protected]>

The fsmonitor feature allows an external tool such as watchman to monitor the working directory. The direct test t7619-status-fsmonitor.sh provides some coverage, but it would be better to run the entire test suite with watchman enabled. This would provide more confidence that the feature is working as intended.

The fsmonitor feature struggles with submodules. Disable the GIT_TEST_FSMONITOR environment variable before running tests with a lot of submodule interactions.

---
 t/t3404-rebase-interactive.sh                | 1 +
 t/t3600-rm.sh                                | 1 +
 t/t4060-diff-submodule-option-diff-format.sh | 3 +++
 t/t5526-fetch-submodules.sh                  | 2 ++
 t/t7402-submodule-rebase.sh                  | 3 +++
 t/t7406-submodule-update.sh                  | 2 ++
 t/t7506-status-submodule.sh                  | 3 +++
 t/t7508-status.sh                            | 3 +++
 8 files changed, 18 insertions(+)

index 461dd539ff..9dc7d1aefb 100755
@@ -697,6 +697,7 @@ test_expect_success 'do "noop" when there is nothing to cherry-pick' '
 '
 
 test_expect_success 'submodule rebase setup' '
+	GIT_TEST_FSMONITOR="" &&
 	git checkout A &&
 	mkdir sub &&
 	(

index 66282a720e..64269bd89d 100755
@@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' '
 '
 
 test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '
+	GIT_TEST_FSMONITOR="" &&
 	git reset --hard &&
 	git submodule update &&
 	git -C submod checkout HEAD^ &&

index 9dcb69df5c..017417790e 100755
@@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff.
 # Tested non-UTF-8 encoding
 test_encoding="ISO8859-1"
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 # String "added" in German (translated with Google Translate), encoded in UTF-8,
 # used in sample commit log messages in add_file() function below.
 added=$(printf "hinzugef\303\274gt")

index 63205dfdf9..fb346bff05 100755
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Copyright (c) 2010, Jens Lehmann
 
+GIT_TEST_FSMONITOR=""
+
 test_description='Recursive "git fetch" for submodules'
 
 . ./test-lib.sh

index 8e32f19007..c78e9009cf 100755
@@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules'
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_expect_success setup '
 
 	echo file > file &&

index c973278300..8d93aaef5f 100755
@@ -11,6 +11,8 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD.
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
 
 compare_head()
 {

index 08629a6e70..1a716f2c2a 100755
@@ -4,6 +4,9 @@ test_description='git status for submodule'
 
 . ./test-lib.sh
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_create_repo_with_commit () {
 	test_create_repo "$1" &&
 	(

index 4e676cdce8..bf0487632d 100755
@@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '
 	test_cmp expect output
 '
 
+# fsmonitor does not work well with submodules
+GIT_TEST_FSMONITOR=""
+
 test_expect_success 'setup status submodule summary' '
 	test_create_repo sm && (
 		cd sm &&

gitgitgadget

I've just run 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all make', and it only failed one test in 't0090-cache-tree.sh', but the fix is already in 'pu' in 61eea521fe (fsmonitor: do not compare bitmap size with size of split index, 2019-11-13).

I don't know enough about watchman to have an informed opinion.

I think the answer mainly depends on what we want to achive and what happens when a test script run with GIT_TEST_FSMONITOR exits without invoking 'test_done' is re-executed (e.g. after a test case fails with '--immediate' or when the user hits ctrl-c or closes the terminal window mid-test).

As far as I understand the commit message of v2 of this patch [1], we mainly want two things:

- Avoid overloading watchman's watch queue. For this it might indeed be sufficient to clear watches in 'test_done', because most test scripts tend to succeed most of the time.

- Make GIT_TEST_FSMONITOR work reliably on Windows. For this, I'm afraid it's not enough in general, because a failure with '--immediate' or after a ctrl-c we won't run 'test_done', so we won't clear the watches, and watchman will keep the fd to the trash dir open, and, consequently, will interfere with subsequent executions of the same test script as it can't delete the still existing trash dir left over from the previous run.

It could still be sufficient for fsmonitor-enabled CI builds, though, because there we don't re-run tests, don't hit ctrl-c, and (at least on Azure Pipelines) don't use '--immediate', and the whole VM/container/whatever is thrown away at end anyway.

On Linux/Unix-y systems it probably doesn't matter much, because they can delete open directories, but I wonder what happens with a watch when the directory it is supposed observe gets deleted. If the watch is removed in this case, great; if it isn't, then... well, then what happens with it? Will it be overwritten with the next test run, or will there be duplicate watches for the same dir?

[1] https://public-inbox.org/git/e51165f260d564ccb7a9b8e696691eccb184c01a.1575907804.git.gitgitgadget@gmail.com/

You are right. Running an individual test and ending it early would lead to these leaked handles. This assumes someone is aware of the GIT_TEST_FSMONITOR environment variable, so they are at least interacting with the feature directly to some extent.

This is the hope. It would be nice to get to that point.

When a directory is deleted from under Watchman on Linux, the watch is removed...eventually. I'm not sure at exactly what point that happens. At the very least, Watchman will receive and process the signals for all of the paths being removed inside the directory. Running 'watch-del' removes that overhead.

Thanks,
-Stolee

Hm. That is a good point. Can we assume that our version of grep has a "-F" or "--fixed-strings" option? ([1] seems to say that "-F" would work.)

[1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs

Watchman outputs its list of paths in JSON format. Luckily, it formats the output so the path lines are on separate lines, each quoted.

For example:

{
	"version": "4.9.0",
	"roots": [
		"<path1>",
		"<path2>",
		"<path3>"
	]
}

Thanks,
-Stolee

$ git grep "grep -F" -- \*.sh

is your friend ;-)

And never use https://www.gnu.org/ manual as a yardstick---you will end up using GNUism that is not unavailable elsewhere pretty easily.

Yeek; how is a dq in path represented? by doubling? by backslash-quoting (if so how is a backslash in path represented)? By something else?

It's OK at least for now to declare that our test repository does not contain any funny paths, but in the longer run does the above mean that we somehow need to be able to grok JSON reliably in our tests? It may not be such a bad thing especially for longer term, as there are other parts of the system that may benefit from having JSON capable output readers in our tests (e.g. trace2 code can do JSON, right?)..

I don't understand this, and would like it to be more specific, i.e. which particular permission changes in which tests are incompatible with watchman.

s/t7619/t7519/

This typo is present in all but one other commit messages.

Yeah, there are a couple hardcoded paths in there, e.g.:

  open ($fh, ">", ".git/watchman-response.json");

and, worse, not only in the test helper hook in 't/t7519/fsmonitor-watchman' but in the sample hook template 'templates/hooks--fsmonitor-watchman.sample' as well.

  git rev-parse --git-path ''

gives us the right directory prefix to use and we could then append the various filenames that must be accessed in there.

Adding another git process inside the hook is hopefully not the only way to achieve something like this. The performance hit (mostly on Windows) would be a non-starter for me. (Yes, the process creation to watchman is already a cost here, but let's not make it worse.)

Perhaps a better strategy would be to do something in-memory instead of writing to a file. Not sure how much of that can be done in the script.

-Stolee

Yes, of course I should have just looked for examples.

I tried to focus on the part that said "this is part of POSIX", but you are right that may not be the best place to look.

trace2 can _write_ JSON, not parse it. However, we have some parsing code (using a package) in the performance tests. I could try adapting that for this purpose. That package is not currently required by the test suite, so it causes some dependency issues when first running the perf suite. At least we wouldn't need the package unless running with GIT_TEST_FSMONITOR.

My guess is that this patch is going to be trouble, so I'll eject it in the next version and save the JSON parsing and everything for its own series. We only really need it when we are getting close to running watchman in CI on Windows.

Thanks,
-Stolee

...

Would it make sense to wrap the tests in a subshell without increasing the tabbing inside the subshell? I can comment the beginning of the subshell that we are disabling the variable for a specific list of tests and the subshell can be removed after the proper fixes make it work.

Thanks,
-Stolee

Hi,

I often look at GNU grep's man page first and then verify via
https://man.openbsd.org/grep and
https://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html
that the option can be considered portable.

From
https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf
(section "9 String"):

A string is a sequence of Unicode code points wrapped with quotation marks (U+0022). All code points may be placed within the quotation marks except for the code points that must be escaped: quotation mark (U+0022), reverse solidus (U+005C), and the control characters U+0000 to U+001F. There are two-character escape sequence representations of some characters.

\" represents the quotation mark character (U+0022).
\\ represents the reverse solidus character(U+005C).
\/ represents the solidus character (U+002F).
\b represents the backspace character(U+0008).
\f represents the form feed character (U+000C).
\n represents the line feed character (U+000A).
\r represents the carriage return character (U+000D).
\t represents the character tabulation character (U+0009).

(Side note: It is amazing what things you learn unexpectedly, e.g. when researching information about the JSON format, you learn that about the word "solidus", that it refers to the slash, and that it was once also know as the "shilling mark"...)

I am not sure why the forward slash needs to be escaped, but I guess that this is voluntary rather than mandatory.

Ciao,
Dscho

Hrm, _when_ is the 'fsmonitor-watchman' hook invoked?! Every time a git process tries to figure out what files have changed since e.g. the index was written? For running an fsmonitor/watchman-enabled CI build it might be an acceptable compromise until we come up with something more clever. 'man githooks' is not clear on this at all, it only says that "This hook is invoked when the configuration option core.fsmonitor is set to .git/hooks/fsmonitor-watchman".

Oh, hang on, it seems that we could simply use $GIT_DIR.

I added

echo >&2 "GIT_DIR in the fsmonitor hook: '$GIT_DIR'"

to 't/t7519/fsmonitor-all', and then run the test:

test_expect_success 'test' '

          echo 1 >file &&
          git add file &&
          git commit -m first &&
  
          git worktree add --detach WT &&
          cd WT &&
          echo 2 >file &&
          git add -u

'

with 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all', and in the verbose output got lines like:

GIT_DIR in the fsmonitor hook: ''
GIT_DIR in the fsmonitor hook: ''
GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT' GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT'

I'm not sure why $GIT_DIR is not exported to the hook script while in the main working tree. Anyway, as it is now, if $GIT_DIR is unset/empty, then the hook should write to ".git/<whatever>", and if it is set, then to "$GIT_DIR/<whatever>", so no git process is needed in the hook, only a getenv() and a condition.

Thanks for this. It helps that also the test hooks were using the .git directory only for debug information, and that was commented-out in the v2 version of the hook.

Thanks,
-Stolee