[PATCH 00/11] Improve testability with GIT_TEST_FSMONITOR

[PATCH 00/11] Improve testability with GIT_TEST_FSMONITOR

[Derrick Stolee via 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).

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

[PATCH 01/11] fsmonitor: disable in a bare repo

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

t0003-attributes.sh

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.c b/config.c
index 3900e4947b..f6d4e2fae3 100644
--- a/config.c
+++ b/config.c
@@ -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

[PATCH 02/11] fsmonitor: do not output to stderr for tests

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

t0003-attributes.sh

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7519/fsmonitor-watchman | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index d8e7a1e5ba..06312876aa 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -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

[PATCH 03/11] t1301-shared-repo.sh: disable FSMONITOR

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1301-shared-repo.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 2dc853d1be..665ade0cf2 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -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

[PATCH 04/11] t1510-repo-setup.sh: disable fsmonitor if no .git dir

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1510-repo-setup.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 9974457f56..28dce0c26f 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -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

[PATCH 05/11] fsmonitor: disable fsmonitor with worktrees

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t2400-worktree-add.sh | 2 ++
 1 file changed, 2 insertions(+)

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

[PATCH 06/11] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t3030-merge-recursive.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index ff641b348a..62f645d639 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -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

[PATCH 07/11] t3600-rm.sh: disable fsmonitor when deleting populated submodule

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t3600-rm.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 66282a720e..64269bd89d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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

[PATCH 08/11] tests: disable fsmonitor in submodule tests

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(+)

diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 9dcb69df5c..017417790e 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -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")
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 63205dfdf9..fb346bff05 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Copyright (c) 2010, Jens Lehmann
 
+GIT_TEST_FSMONITOR=""
+
 test_description='Recursive "git fetch" for submodules'
 
 . ./test-lib.sh
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index 8e32f19007..c78e9009cf 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -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 &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c973278300..8d93aaef5f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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()
 {
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 08629a6e70..1a716f2c2a 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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" &&
 	(
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 4e676cdce8..bf0487632d 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -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

[PATCH 09/11] t7063: disable fsmonitor with status cache

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7063-status-untracked-cache.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 190ae149cf..c433738a3a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -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

[PATCH 10/11] t7519: disable external GIT_TEST_FSMONITOR variable

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7519-status-fsmonitor.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 81a375fa0f..443d2e653b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

[PATCH 11/11] test-lib: clear watchman watches at test completion

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/test-lib-functions.sh | 15 +++++++++++++++
 t/test-lib.sh           |  2 ++
 2 files changed, 17 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e0b3f28d3a..03573caf42 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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
+}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30b07e310f..067a432ea5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Re: [PATCH 01/11] fsmonitor: disable in a bare repo

[Denton Liu]

Hi Stolee,

On Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> t0003-attributes.sh

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

> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  config.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/config.c b/config.c
> index 3900e4947b..f6d4e2fae3 100644
> --- a/config.c
> +++ b/config.c
> @@ -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
> 

Re: [PATCH 11/11] test-lib: clear watchman watches at test completion

[SZEDER Gábor]

On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/test-lib-functions.sh | 15 +++++++++++++++
>  t/test-lib.sh           |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e0b3f28d3a..03573caf42 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -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 ""

In the rare cases when this function is invoked (see below) this
condition triggers an error from the shell running test script:

  - when the variable is not set, because of the lack of quotes around
    the variable name:

      $ ./t5570-git-daemon.sh 
      [....]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator
      # passed all 21 test(s)
      1..21

  - when the variable is set, because the '-ne' operator does integer
    comparison:

      $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh
      [...]
      ok 21 - hostname interpolation works after LF-stripping
      ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none
      # failed 1 among 21 test(s)
      1..21

Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead.

> +	then
> +		watchman watch-list |

Then with the above fixed, trying to run 'watchman' triggers another
error if it's not installed:

  $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
  [...]
  ok 21 - hostname interpolation works after LF-stripping
  ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
  # failed 1 among 21 test(s)

I think we need an additional condition to run this only if
't7519/fsmonitor-watchman' is used in the tests.

> +			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
> +}
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 30b07e310f..067a432ea5 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -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

I'm not sure where to put this call, but this is definitely not the
right place for it.  See that 'return 0' above in the context?  That's
where the test_atexit_handler function returns early when no atexit
handler commands are set, i.e. in all test scripts that don't involve
some kind of daemons, thus this call is not invoked in the majority of
test scripts.

Simply moving this call before that early return is not good, because
then it would be invoked twice.

An option would be to register this call as an atexit command
somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
is restored, perhaps).  That way it would be invoked most of the time,
and it would be invoked only once, but I'm not sure how it would work
out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
middle for the remainder of the test script.  However, register the
atexit command only if GIT_TEST_FSMONITOR is set (to something
watchman-specific), so it won't be invoked at all if
GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
test output and trace.

I don't have a better idea.

> +
>  	setup_malloc_check
>  	test_eval_ "$test_atexit_cleanup"
>  	test_atexit_cleanup=:
> -- 
> gitgitgadget

Re: [PATCH 01/11] fsmonitor: disable in a bare repo

[Derrick Stolee]

On 11/21/2019 6:18 PM, Denton Liu wrote:
> Hi Stolee,
> 
> On Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> t0003-attributes.sh
> 
> 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 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

Re: [PATCH 11/11] test-lib: clear watchman watches at test completion

[Derrick 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.

> On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  t/test-lib-functions.sh | 15 +++++++++++++++
>>  t/test-lib.sh           |  2 ++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index e0b3f28d3a..03573caf42 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -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 ""
> 
> In the rare cases when this function is invoked (see below) this
> condition triggers an error from the shell running test script:
> 
>   - when the variable is not set, because of the lack of quotes around
>     the variable name:
> 
>       $ ./t5570-git-daemon.sh 
>       [....]
>       ok 21 - hostname interpolation works after LF-stripping
>       ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator
>       # passed all 21 test(s)
>       1..21
> 
>   - when the variable is set, because the '-ne' operator does integer
>     comparison:
> 
>       $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh
>       [...]
>       ok 21 - hostname interpolation works after LF-stripping
>       ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none
>       # failed 1 among 21 test(s)
>       1..21
> 
> Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead.

Thanks for the pointers.

>> +	then
>> +		watchman watch-list |
> 
> Then with the above fixed, trying to run 'watchman' triggers another
> error if it's not installed:
> 
>   $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
>   [...]
>   ok 21 - hostname interpolation works after LF-stripping
>   ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
>   # failed 1 among 21 test(s)
> 
> I think we need an additional condition to run this only if
> 't7519/fsmonitor-watchman' is used in the tests.

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.

>> +			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
>> +}
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 30b07e310f..067a432ea5 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -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
> 
> I'm not sure where to put this call, but this is definitely not the
> right place for it.  See that 'return 0' above in the context?  That's
> where the test_atexit_handler function returns early when no atexit
> handler commands are set, i.e. in all test scripts that don't involve
> some kind of daemons, thus this call is not invoked in the majority of
> test scripts.

Ah, I misunderstood the point of test_atexit_handler.

> Simply moving this call before that early return is not good, because
> then it would be invoked twice.
> 
> An option would be to register this call as an atexit command
> somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
> is restored, perhaps).  That way it would be invoked most of the time,
> and it would be invoked only once, but I'm not sure how it would work
> out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
> middle for the remainder of the test script.  However, register the
> atexit command only if GIT_TEST_FSMONITOR is set (to something
> watchman-specific), so it won't be invoked at all if
> GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
> test output and trace.
> 
> I don't have a better idea.

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

[PATCH v2 0/8] Improve testability with GIT_TEST_FSMONITOR

[Derrick Stolee via 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.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
  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.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
  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.
     +
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
       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.
     +
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     + 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.
      
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     + 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
     +
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
       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

[PATCH v2 1/8] fsmonitor: disable in a bare repo

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 config.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.c b/config.c
index 3900e4947b..f6d4e2fae3 100644
--- a/config.c
+++ b/config.c
@@ -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

[PATCH v2 2/8] fsmonitor: do not output to stderr for tests

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7519/fsmonitor-watchman | 1 -
 1 file changed, 1 deletion(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index d8e7a1e5ba..06312876aa 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -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

[PATCH v2 3/8] t1301-shared-repo.sh: disable FSMONITOR

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1301-shared-repo.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
index 2dc853d1be..665ade0cf2 100755
--- a/t/t1301-shared-repo.sh
+++ b/t/t1301-shared-repo.sh
@@ -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

[PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t1510-repo-setup.sh      | 1 +
 t/t2400-worktree-add.sh    | 2 ++
 t/t3030-merge-recursive.sh | 2 ++
 3 files changed, 5 insertions(+)

diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
index 9974457f56..28dce0c26f 100755
--- a/t/t1510-repo-setup.sh
+++ b/t/t1510-repo-setup.sh
@@ -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 &&
diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index e819ba741e..d4d3cbae0f 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -1,5 +1,7 @@
 #!/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
index ff641b348a..62f645d639 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -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

[PATCH v2 5/8] tests: disable fsmonitor in submodule tests

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 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(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 461dd539ff..9dc7d1aefb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -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 &&
 	(
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 66282a720e..64269bd89d 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -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^ &&
diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh
index 9dcb69df5c..017417790e 100755
--- a/t/t4060-diff-submodule-option-diff-format.sh
+++ b/t/t4060-diff-submodule-option-diff-format.sh
@@ -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")
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 63205dfdf9..fb346bff05 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -1,6 +1,8 @@
 #!/bin/sh
 # Copyright (c) 2010, Jens Lehmann
 
+GIT_TEST_FSMONITOR=""
+
 test_description='Recursive "git fetch" for submodules'
 
 . ./test-lib.sh
diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
index 8e32f19007..c78e9009cf 100755
--- a/t/t7402-submodule-rebase.sh
+++ b/t/t7402-submodule-rebase.sh
@@ -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 &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index c973278300..8d93aaef5f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -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()
 {
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 08629a6e70..1a716f2c2a 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -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" &&
 	(
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 4e676cdce8..bf0487632d 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -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

[PATCH v2 6/8] t7063: disable fsmonitor with status cache

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7063-status-untracked-cache.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh
index 190ae149cf..c433738a3a 100755
--- a/t/t7063-status-untracked-cache.sh
+++ b/t/t7063-status-untracked-cache.sh
@@ -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

[PATCH v2 7/8] t7519: disable external GIT_TEST_FSMONITOR variable

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/t7519-status-fsmonitor.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index 81a375fa0f..443d2e653b 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

[PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Derrick Stolee via GitGitGadget]

From: Derrick Stolee <dstolee@microsoft.com>

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 t/test-lib-functions.sh | 15 +++++++++++++++
 t/test-lib.sh           |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e0b3f28d3a..ef840ce097 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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
+}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 30b07e310f..4114953ebc 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -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

Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Junio C Hamano]

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +test_clear_watchman () {
> +	if test -n "$GIT_TEST_FSMONITOR"
> +	then
> +		watchman watch-list |
> +			grep "$TRASH_DIRECTORY" |
> +			sed "s/\",//g" |
> +			sed "s/\"//g" >repo-list

Whoa, this is scary.  "$TRASH_DIRECTORY" comes from $(pwd) and the
leading part of it can have arbitrary garbage like "[a-z]*" that may
match paths "watchman watch-list" may emit that does not have
anything to do with the temporary directory used by this test.

What are these stripping of ", and " about?  Could you tell readers
how a typical output from the program we are reading from looks like
perhaps in the log message or in-code comment around here?

Thanks.

Re: [PATCH 11/11] test-lib: clear watchman watches at test completion

[SZEDER Gábor]

On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote:
> >> +		watchman watch-list |
> > 
> > Then with the above fixed, trying to run 'watchman' triggers another
> > error if it's not installed:
> > 
> >   $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
> >   [...]
> >   ok 21 - hostname interpolation works after LF-stripping
> >   ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
> >   # failed 1 among 21 test(s)
> > 
> > I think we need an additional condition to run this only if
> > 't7519/fsmonitor-watchman' is used in the tests.
> 
> The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,
> and that can only use watchman (currently).

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


> >> diff --git a/t/test-lib.sh b/t/test-lib.sh
> >> index 30b07e310f..067a432ea5 100644
> >> --- a/t/test-lib.sh
> >> +++ b/t/test-lib.sh
> >> @@ -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
> > 
> > I'm not sure where to put this call, but this is definitely not the
> > right place for it.  See that 'return 0' above in the context?  That's
> > where the test_atexit_handler function returns early when no atexit
> > handler commands are set, i.e. in all test scripts that don't involve
> > some kind of daemons, thus this call is not invoked in the majority of
> > test scripts.
> 
> Ah, I misunderstood the point of test_atexit_handler.
> 
> > Simply moving this call before that early return is not good, because
> > then it would be invoked twice.
> > 
> > An option would be to register this call as an atexit command
> > somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
> > is restored, perhaps).  That way it would be invoked most of the time,
> > and it would be invoked only once, but I'm not sure how it would work
> > out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
> > middle for the remainder of the test script.  However, register the
> > atexit command only if GIT_TEST_FSMONITOR is set (to something
> > watchman-specific), so it won't be invoked at all if
> > GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
> > test output and trace.
> > 
> > I don't have a better idea.
> 
> 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.

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/

Re: [PATCH 11/11] test-lib: clear watchman watches at test completion

[Derrick Stolee]

On 12/9/2019 6:40 PM, SZEDER Gábor wrote:
> On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote:
>>>> +		watchman watch-list |
>>>
>>> Then with the above fixed, trying to run 'watchman' triggers another
>>> error if it's not installed:
>>>
>>>   $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh 
>>>   [...]
>>>   ok 21 - hostname interpolation works after LF-stripping
>>>   ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found
>>>   # failed 1 among 21 test(s)
>>>
>>> I think we need an additional condition to run this only if
>>> 't7519/fsmonitor-watchman' is used in the tests.
>>
>> The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,
>> and that can only use watchman (currently).
> 
> 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).
> 
> 
>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>>>> index 30b07e310f..067a432ea5 100644
>>>> --- a/t/test-lib.sh
>>>> +++ b/t/test-lib.sh
>>>> @@ -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
>>>
>>> I'm not sure where to put this call, but this is definitely not the
>>> right place for it.  See that 'return 0' above in the context?  That's
>>> where the test_atexit_handler function returns early when no atexit
>>> handler commands are set, i.e. in all test scripts that don't involve
>>> some kind of daemons, thus this call is not invoked in the majority of
>>> test scripts.
>>
>> Ah, I misunderstood the point of test_atexit_handler.
>>
>>> Simply moving this call before that early return is not good, because
>>> then it would be invoked twice.
>>>
>>> An option would be to register this call as an atexit command
>>> somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON
>>> is restored, perhaps).  That way it would be invoked most of the time,
>>> and it would be invoked only once, but I'm not sure how it would work
>>> out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the
>>> middle for the remainder of the test script.  However, register the
>>> atexit command only if GIT_TEST_FSMONITOR is set (to something
>>> watchman-specific), so it won't be invoked at all if
>>> GIT_TEST_FSMONITOR is not set, and thus it won't generate additional
>>> test output and trace.
>>>
>>> I don't have a better idea.
>>
>> 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.
> 
> 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.

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.

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

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

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

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

Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Derrick Stolee]

On 12/9/2019 5:52 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> +test_clear_watchman () {
>> +	if test -n "$GIT_TEST_FSMONITOR"
>> +	then
>> +		watchman watch-list |
>> +			grep "$TRASH_DIRECTORY" |
>> +			sed "s/\",//g" |
>> +			sed "s/\"//g" >repo-list
> 
> Whoa, this is scary.  "$TRASH_DIRECTORY" comes from $(pwd) and the
> leading part of it can have arbitrary garbage like "[a-z]*" that may
> match paths "watchman watch-list" may emit that does not have
> anything to do with the temporary directory used by this test.

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

> What are these stripping of ", and " about?  Could you tell readers
> how a typical output from the program we are reading from looks like
> perhaps in the log message or in-code comment around here?

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

Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

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

$ 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.

> [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs
>
>> What are these stripping of ", and " about?  Could you tell readers
>> how a typical output from the program we are reading from looks like
>> perhaps in the log message or in-code comment around here?
>
> 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>"
> 	]
> }

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

Re: [PATCH v2 3/8] t1301-shared-repo.sh: disable FSMONITOR

[SZEDER Gábor]

On Mon, Dec 09, 2019 at 04:09:59PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

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.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1301-shared-repo.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh
> index 2dc853d1be..665ade0cf2 100755
> --- a/t/t1301-shared-repo.sh
> +++ b/t/t1301-shared-repo.sh
> @@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '
>  '
>  
>  test_expect_success POSIXPERM 'forced modes' '
> +	GIT_TEST_FSMONITOR="" &&

This is not in a subshell, so it disables GIT_TEST_FSMONITOR not only
for the test it is executed in but for the remainder of the test
script.  From the commit message I can't decide whether that's
intended.

>  	mkdir -p templates/hooks &&
>  	echo update-server-info >templates/hooks/post-update &&
>  	chmod +x templates/hooks/post-update &&
> -- 
> gitgitgadget
> 

Re: [PATCH v2 1/8] fsmonitor: disable in a bare repo

[SZEDER Gábor]

On Mon, Dec 09, 2019 at 04:09:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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

s/t7619/t7519/

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

Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[SZEDER Gábor]

On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.

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.

> It would be better to properly
> find a safe folder for storing this JSON file.

  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.

> This is also a problem when a test script uses GIT_WORK_TREE.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/t1510-repo-setup.sh      | 1 +
>  t/t2400-worktree-add.sh    | 2 ++
>  t/t3030-merge-recursive.sh | 2 ++
>  3 files changed, 5 insertions(+)
> 
> diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh
> index 9974457f56..28dce0c26f 100755
> --- a/t/t1510-repo-setup.sh
> +++ b/t/t1510-repo-setup.sh
> @@ -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 &&
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index e819ba741e..d4d3cbae0f 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -1,5 +1,7 @@
>  #!/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
> index ff641b348a..62f645d639 100755
> --- a/t/t3030-merge-recursive.sh
> +++ b/t/t3030-merge-recursive.sh
> @@ -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
> 

Re: [PATCH v2 5/8] tests: disable fsmonitor in submodule tests

[SZEDER Gábor]

On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
> 
> 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.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  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(+)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 461dd539ff..9dc7d1aefb 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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="" &&

This disables GIT_TEST_FSMONITOR for the remainder of the test script,
but there are still a lot of non-submodule-specific tests to run.

>  	git checkout A &&
>  	mkdir sub &&
>  	(
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index 66282a720e..64269bd89d 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -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="" &&

Likewise.

>  	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
> index 9dcb69df5c..017417790e 100755
> --- a/t/t4060-diff-submodule-option-diff-format.sh
> +++ b/t/t4060-diff-submodule-option-diff-format.sh
> @@ -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")
> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> index 63205dfdf9..fb346bff05 100755
> --- a/t/t5526-fetch-submodules.sh
> +++ b/t/t5526-fetch-submodules.sh
> @@ -1,6 +1,8 @@
>  #!/bin/sh
>  # Copyright (c) 2010, Jens Lehmann
>  
> +GIT_TEST_FSMONITOR=""
> +
>  test_description='Recursive "git fetch" for submodules'
>  
>  . ./test-lib.sh
> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh
> index 8e32f19007..c78e9009cf 100755
> --- a/t/t7402-submodule-rebase.sh
> +++ b/t/t7402-submodule-rebase.sh
> @@ -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 &&
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c973278300..8d93aaef5f 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -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()
>  {
> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
> index 08629a6e70..1a716f2c2a 100755
> --- a/t/t7506-status-submodule.sh
> +++ b/t/t7506-status-submodule.sh
> @@ -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" &&
>  	(
> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
> index 4e676cdce8..bf0487632d 100755
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -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=""
> +

Likewise.

>  test_expect_success 'setup status submodule summary' '
>  	test_create_repo sm && (
>  		cd sm &&
> -- 
> gitgitgadget
> 

Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[Derrick Stolee]

On 12/10/2019 5:07 AM, SZEDER Gábor wrote:
> On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
> 
> 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.
> 
>> It would be better to properly
>> find a safe folder for storing this JSON file.
> 
>   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

Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Derrick Stolee]

On 12/10/2019 12:20 AM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>> 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.)
> 
> $ git grep "grep -F" -- \*.sh
> 
> is your friend ;-) 

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

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

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.

>> [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs
>>
>>> What are these stripping of ", and " about?  Could you tell readers
>>> how a typical output from the program we are reading from looks like
>>> perhaps in the log message or in-code comment around here?
>>
>> 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>"
>> 	]
>> }
> 
> 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?)..

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

Re: [PATCH v2 5/8] tests: disable fsmonitor in submodule tests

[Derrick Stolee]

On 12/10/2019 5:13 AM, SZEDER Gábor wrote:
> On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote:
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> 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.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  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(+)
>>
>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
>> index 461dd539ff..9dc7d1aefb 100755
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -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="" &&
> 
> This disables GIT_TEST_FSMONITOR for the remainder of the test script,
> but there are still a lot of non-submodule-specific tests to run.
...
>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh
>> index 4e676cdce8..bf0487632d 100755
>> --- a/t/t7508-status.sh
>> +++ b/t/t7508-status.sh
>> @@ -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=""
>> +
> 
> Likewise.

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

Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion

[Johannes Schindelin]

Hi,

On Mon, 9 Dec 2019, Junio C Hamano wrote:

> Derrick Stolee <stolee@gmail.com> writes:
>
> > 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.)
>
> $ 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.
>
> > [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs

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.

> >> What are these stripping of ", and " about?  Could you tell readers
> >> how a typical output from the program we are reading from looks like
> >> perhaps in the log message or in-code comment around here?
> >
> > 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>"
> > 	]
> > }
>
> 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?)..

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

Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[SZEDER Gábor]

On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:
> On 12/10/2019 5:07 AM, SZEDER Gábor wrote:
> > On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:
> >> From: Derrick Stolee <dstolee@microsoft.com>
> >>
> >> 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.
> > 
> > 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.
> > 
> >> It would be better to properly
> >> find a safe folder for storing this JSON file.
> > 
> >   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.)

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".

> 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

Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[SZEDER Gábor]

On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:
> >> 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.
> > 
> > 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.
> > 
> >> It would be better to properly
> >> find a safe folder for storing this JSON file.
> > 
> >   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.

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.

Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE

[Derrick Stolee]

On 12/10/2019 10:07 AM, SZEDER Gábor wrote:
> On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:
>>>> 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.
>>>
>>> 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.
>>>
>>>> It would be better to properly
>>>> find a safe folder for storing this JSON file.
>>>
>>>   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.
> 
> 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