{"thread":{"id":"52306","subject":"[PATCH 01/11] fsmonitor: disable in a bare repo","startedAt":"2019-11-21T22:20:31Z","lastAt":"2020-01-23T15:45:37Z","messageCount":41,"participants":["Derrick Stolee via GitGitGadget","Denton Liu","SZEDER Gábor","Derrick Stolee","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"386765","messageId":"9bf5a803e6d44ce1ff03225e57933c97c8841327.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 01/11] fsmonitor: disable in a bare repo","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:16Z","receivedAt":"2019-11-21T22:20:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nt0003-attributes.sh\n\nSigned-off-by: Derrick Stolee \n---\n config.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 3900e4947b..f6d4e2fae3 100644\n--- a/config.c\n+++ b/config.c\n@@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)\n \n int git_config_get_fsmonitor(void)\n {\n+\tif (!the_repository->worktree) {\n+\t\tcore_fsmonitor = 0;\n+\t\treturn 0;\n+\t}\n+\n \tif (git_config_get_pathname(\"core.fsmonitor\", &core_fsmonitor))\n \t\tcore_fsmonitor = getenv(\"GIT_TEST_FSMONITOR\");\n \n-- \ngitgitgadget\n\n"},{"id":"386766","messageId":"pull.466.git.1574374826.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":null,"subject":"[PATCH 00/11] Improve testability with GIT_TEST_FSMONITOR","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:15Z","receivedAt":"2019-11-21T22:20:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The GIT_TEST_FSMONITOR environment variable allows run-time specification of\nthe fsmonitor hook. Initially used by t7619-status-fsmonitor.sh, we can\nenable it across the test suite to see how it affects Git's behavior. In\nparticular, we can specify the version of the hook that requests a result\nfrom Watchman to get actual updates to the files in the repo.\n\nIn many cases, our tests are simply not ready to handle this option.\nfsmonitor does not integrate well with features such as split index, bare\nrepos, or submodules. Other times, we need to disable it because the test is\nbeing specific about what files Git inspects during a 'status' call.\n\nThe long-term vision is to be able to run CI builds using a file-system\nwatcher like Watchman to get better coverage on this feature. These patches\nget us closer, but there are still some issues around overloading the\nWatchman interface when the tests are run in parallel. When using \"prove -j8\nt[0-8]*.sh\" I see some failures that do not reproduce when running the test\nscripts in isolation (on Linux).\n\nThanks, -Stolee\n\nDerrick Stolee (11):\n fsmonitor: disable in a bare repo\n fsmonitor: do not output to stderr for tests\n t1301-shared-repo.sh: disable FSMONITOR\n t1510-repo-setup.sh: disable fsmonitor if no .git dir\n fsmonitor: disable fsmonitor with worktrees\n t3030-merge-recursive.sh: disable fsmonitor when tweaking\n GIT_WORK_TREE\n t3600-rm.sh: disable fsmonitor when deleting populated submodule\n tests: disable fsmonitor in submodule tests\n t7063: disable fsmonitor with status cache\n t7519: disable external GIT_TEST_FSMONITOR variable\n test-lib: clear watchman watches at test completion\n\n config.c | 5 +++++\n t/t1301-shared-repo.sh | 1 +\n t/t1510-repo-setup.sh | 1 +\n t/t2400-worktree-add.sh | 2 ++\n t/t3030-merge-recursive.sh | 2 ++\n t/t3600-rm.sh | 1 +\n t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n t/t5526-fetch-submodules.sh | 2 ++\n t/t7063-status-untracked-cache.sh | 3 +++\n t/t7402-submodule-rebase.sh | 3 +++\n t/t7406-submodule-update.sh | 2 ++\n t/t7506-status-submodule.sh | 3 +++\n t/t7508-status.sh | 3 +++\n t/t7519-status-fsmonitor.sh | 3 +++\n t/t7519/fsmonitor-watchman | 1 -\n t/test-lib-functions.sh | 15 +++++++++++++++\n t/test-lib.sh | 2 ++\n 17 files changed, 51 insertions(+), 1 deletion(-)\n\n\nbase-commit: dd0b61f577f041f1119bb3288451f8f9b7f9e3f2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-466%2Fderrickstolee%2Ftest-watchman-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-466/derrickstolee/test-watchman-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/466\n-- \ngitgitgadget\n"},{"id":"386767","messageId":"245cc1b2cc778de486b1ae5177baac3ae3fb93c5.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 02/11] fsmonitor: do not output to stderr for tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:17Z","receivedAt":"2019-11-21T22:20:32Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nt0003-attributes.sh\n\nSigned-off-by: Derrick Stolee \n---\n t/t7519/fsmonitor-watchman | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex d8e7a1e5ba..06312876aa 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -94,7 +94,6 @@ sub launch_watchman {\n \tmy $o = $json_pkg->new->utf8->decode($response);\n \n \tif ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n-\t\tprint STDERR \"Adding '$git_work_tree' to watchman's watch list.\\n\";\n \t\t$retry--;\n \t\tqx/watchman watch \"$git_work_tree\"/;\n \t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n-- \ngitgitgadget\n\n"},{"id":"386768","messageId":"ed40ce8e4cf877983ccfd909134f5ffe3a2b0c04.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 03/11] t1301-shared-repo.sh: disable FSMONITOR","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:18Z","receivedAt":"2019-11-21T22:20:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1301-shared-repo.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 2dc853d1be..665ade0cf2 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n '\n \n test_expect_success POSIXPERM 'forced modes' '\n+\tGIT_TEST_FSMONITOR=\"\" &&\n \tmkdir -p templates/hooks &&\n \techo update-server-info >templates/hooks/post-update &&\n \tchmod +x templates/hooks/post-update &&\n-- \ngitgitgadget\n\n"},{"id":"386769","messageId":"91797769825438318d243b4dfddd6784b733fe4d.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 04/11] t1510-repo-setup.sh: disable fsmonitor if no .git dir","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:19Z","receivedAt":"2019-11-21T22:20:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t1510-repo-setup.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 9974457f56..28dce0c26f 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -775,6 +775,7 @@ test_expect_success '#29: setup' '\n \tsetup_repo 29 non-existent gitfile true &&\n \tmkdir -p 29/sub/sub 29/wt/sub &&\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tcd 29 &&\n \t\tGIT_WORK_TREE=\"$here/29\" &&\n \t\texport GIT_WORK_TREE &&\n-- \ngitgitgadget\n\n"},{"id":"386770","messageId":"81c587651e31d157c37c813650788bd5b88a9433.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 05/11] fsmonitor: disable fsmonitor with worktrees","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:20Z","receivedAt":"2019-11-21T22:20:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t2400-worktree-add.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex e819ba741e..d4d3cbae0f 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -1,5 +1,7 @@\n #!/bin/sh\n \n+GIT_TEST_FSMONITOR=\"\"\n+\n test_description='test git worktree add'\n \n . ./test-lib.sh\n-- \ngitgitgadget\n\n"},{"id":"386771","messageId":"d0b212c9df86915e89bf6d101c84e8e137e3252a.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 07/11] t3600-rm.sh: disable fsmonitor when deleting populated submodule","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:22Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t3600-rm.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 66282a720e..64269bd89d 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' '\n '\n \n test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n+\tGIT_TEST_FSMONITOR=\"\" &&\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\n-- \ngitgitgadget\n\n"},{"id":"386772","messageId":"d5f5a3e2b9bed793f294f2e6eda6307a800f29d5.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 06/11] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:21Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/t3030-merge-recursive.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex ff641b348a..62f645d639 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' '\n \n test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tGIT_WORK_TREE=\"$PWD/ours-has-rename-work\" &&\n \t\texport GIT_WORK_TREE &&\n \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n@@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \n test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tGIT_WORK_TREE=\"$PWD/theirs-has-rename-work\" &&\n \t\texport GIT_WORK_TREE &&\n \t\tGIT_INDEX_FILE=\"$PWD/theirs-has-rename-index\" &&\n-- \ngitgitgadget\n\n"},{"id":"386773","messageId":"141a1909a44af1dc6a1bead323ebb0ce5842403f.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 09/11] t7063: disable fsmonitor with status cache","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:24Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe status cache tests use GIT_TRACE_UNTRACKED_STATS to check very\ndetailed statistics related to how much Git actually checked for\nuntracked files. The fsmonitor feature changes the expected behavior\nhere, so disable the GIT_TEST_FSMONITOR environment variable.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7063-status-untracked-cache.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 190ae149cf..c433738a3a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -4,6 +4,9 @@ test_description='test untracked cache'\n \n . ./test-lib.sh\n \n+# fsmonitor changes the expected behvaior of GIT_TRACE_UNTRACKED_STATS\n+GIT_TEST_FSMONITOR=\"\"\n+\n # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime\n # is updated lazily after contents in the directory changes, which\n # forces the untracked cache code to take the slow path. A test\n-- \ngitgitgadget\n\n"},{"id":"386774","messageId":"36f845cb7ee21443e7f20517968982222e775a6a.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 08/11] tests: disable fsmonitor in submodule tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:23Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature struggles with submodules. Disable the\nGIT_TEST_FSMONITOR environment variable before running tests with\na lot of submodule interactions.\n\nSigned-off-by: Derrick Stolee \n---\n t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n t/t5526-fetch-submodules.sh | 2 ++\n t/t7402-submodule-rebase.sh | 3 +++\n t/t7406-submodule-update.sh | 2 ++\n t/t7506-status-submodule.sh | 3 +++\n t/t7508-status.sh | 3 +++\n 6 files changed, 16 insertions(+)\n\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 9dcb69df5c..017417790e 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff.\n # Tested non-UTF-8 encoding\n test_encoding=\"ISO8859-1\"\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n # String \"added\" in German (translated with Google Translate), encoded in UTF-8,\n # used in sample commit log messages in add_file() function below.\n added=$(printf \"hinzugef\\303\\274gt\")\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 63205dfdf9..fb346bff05 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n # Copyright (c) 2010, Jens Lehmann\n \n+GIT_TEST_FSMONITOR=\"\"\n+\n test_description='Recursive \"git fetch\" for submodules'\n \n . ./test-lib.sh\ndiff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh\nindex 8e32f19007..c78e9009cf 100755\n--- a/t/t7402-submodule-rebase.sh\n+++ b/t/t7402-submodule-rebase.sh\n@@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules'\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_expect_success setup '\n \n \techo file > file &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c973278300..8d93aaef5f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -11,6 +11,8 @@ submodule and \"git submodule update --rebase/--merge\" does not detach the HEAD.\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n \n compare_head()\n {\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 08629a6e70..1a716f2c2a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -4,6 +4,9 @@ test_description='git status for submodule'\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_create_repo_with_commit () {\n \ttest_create_repo \"$1\" &&\n \t(\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 4e676cdce8..bf0487632d 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '\n \ttest_cmp expect output\n '\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_expect_success 'setup status submodule summary' '\n \ttest_create_repo sm && (\n \t\tcd sm &&\n-- \ngitgitgadget\n\n"},{"id":"386775","messageId":"47cecb4a83a3f726088ffba0b00679384c7349ae.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 11/11] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:26Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nSigned-off-by: Derrick Stolee \n---\n t/test-lib-functions.sh | 15 +++++++++++++++\n t/test-lib.sh | 2 ++\n 2 files changed, 17 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e0b3f28d3a..03573caf42 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1475,3 +1475,18 @@ test_set_port () {\n \tport=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))\n \teval $var=$port\n }\n+\n+test_clear_watchman () {\n+\tif test $GIT_TEST_FSMONITOR -ne \"\"\n+\tthen\n+\t\twatchman watch-list |\n+\t\t\tgrep \"$TRASH_DIRECTORY\" |\n+\t\t\tsed \"s/\\t\\\"//g\" |\n+\t\t\tsed \"s/\\\",//g\" >repo-list\n+\n+\t\tfor repo in $(cat repo-list)\n+\t\tdo\n+\t\t\twatchman watch-del \"$repo\"\n+\t\tdone\n+\tfi\n+}\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 30b07e310f..067a432ea5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1072,6 +1072,8 @@ test_atexit_handler () {\n \t# sure that the registered cleanup commands are run only once.\n \ttest : != \"$test_atexit_cleanup\" || return 0\n \n+\ttest_clear_watchman\n+\n \tsetup_malloc_check\n \ttest_eval_ \"$test_atexit_cleanup\"\n \ttest_atexit_cleanup=:\n-- \ngitgitgadget\n"},{"id":"386776","messageId":"cd717ef5de44c965ae82607baccd1557fed02825.1574374826.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH 10/11] t7519: disable external GIT_TEST_FSMONITOR variable","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-21T22:20:25Z","receivedAt":"2019-11-21T22:20:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe GIT_TEST_FSMONITOR variable was created specifically so the\nt7519-status-fsmonitor.sh test script could tweak the expected\nbehavior depending on its value. However, if we set it externally\nto use the Watchman integration, then it breaks the initial tests\nthat demonstrate behavior _without_ the fsmonitor feature.\n\nDisable this variable at the start of the script.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7519-status-fsmonitor.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 81a375fa0f..443d2e653b 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -8,6 +8,9 @@ test_description='git status with file system watcher'\n # \"git update-index --fsmonitor\" can be used to get the extension written\n # before testing the results.\n \n+# Disable an external value, as we will set it directly as needed.\n+GIT_TEST_FSMONITOR=\"\"\n+\n clean_repo () {\n \tgit reset --hard HEAD &&\n \tgit clean -fd\n-- \ngitgitgadget\n\n"},{"id":"386778","messageId":"20191121231818.GA22688@generichostname","threadId":"52306","inReplyTo":"9bf5a803e6d44ce1ff03225e57933c97c8841327.1574374826.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/11] fsmonitor: disable in a bare repo","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-11-21T23:18:18Z","receivedAt":"2019-11-21T23:18:23Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Stolee,\n\nOn Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> t0003-attributes.sh\n\nPatches 1 and 2 have this line in the commit message. I assume it's a\ntypo?\n\nAlso, for the patches with empty commit message bodies, it would be nice\nif they were filled in with detail for _why_ the change is being made.\nFor example, in this patch, a future reader might wonder why having\nfsmonitor enabled in a bare repo causes problems.\n\nThanks,\n\nDenton\n\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> config.c | 5 +++++\n> 1 file changed, 5 insertions(+)\n> \n> diff --git a/config.c b/config.c\n> index 3900e4947b..f6d4e2fae3 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)\n> \n> int git_config_get_fsmonitor(void)\n> {\n> +\tif (!the_repository->worktree) {\n> +\t\tcore_fsmonitor = 0;\n> +\t\treturn 0;\n> +\t}\n> +\n> \tif (git_config_get_pathname(\"core.fsmonitor\", &core_fsmonitor))\n> \t\tcore_fsmonitor = getenv(\"GIT_TEST_FSMONITOR\");\n> \n> -- \n> gitgitgadget\n> \n"},{"id":"386784","messageId":"20191122010645.GX23183@szeder.dev","threadId":"52306","inReplyTo":"47cecb4a83a3f726088ffba0b00679384c7349ae.1574374826.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] test-lib: clear watchman watches at test completion","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-22T01:06:45Z","receivedAt":"2019-11-22T01:06:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> Signed-off-by: Derrick Stolee \n> ---\n> t/test-lib-functions.sh | 15 +++++++++++++++\n> t/test-lib.sh | 2 ++\n> 2 files changed, 17 insertions(+)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index e0b3f28d3a..03573caf42 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -1475,3 +1475,18 @@ test_set_port () {\n> \tport=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))\n> \teval $var=$port\n> }\n> +\n> +test_clear_watchman () {\n> +\tif test $GIT_TEST_FSMONITOR -ne \"\"\n\nIn the rare cases when this function is invoked (see below) this\ncondition triggers an error from the shell running test script:\n\n - when the variable is not set, because of the lack of quotes around\n the variable name:\n\n $ ./t5570-git-daemon.sh \n [....]\n ok 21 - hostname interpolation works after LF-stripping\n ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator\n # passed all 21 test(s)\n 1..21\n\n - when the variable is set, because the '-ne' operator does integer\n comparison:\n\n $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh\n [...]\n ok 21 - hostname interpolation works after LF-stripping\n ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none\n # failed 1 among 21 test(s)\n 1..21\n\nPlease use 'if test -n \"$GIT_TEST_FSMONITOR\"' instead.\n\n> +\tthen\n> +\t\twatchman watch-list |\n\nThen with the above fixed, trying to run 'watchman' triggers another\nerror if it's not installed:\n\n $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh \n [...]\n ok 21 - hostname interpolation works after LF-stripping\n ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found\n # failed 1 among 21 test(s)\n\nI think we need an additional condition to run this only if\n't7519/fsmonitor-watchman' is used in the tests.\n\n> +\t\t\tgrep \"$TRASH_DIRECTORY\" |\n> +\t\t\tsed \"s/\\t\\\"//g\" |\n> +\t\t\tsed \"s/\\\",//g\" >repo-list\n> +\n> +\t\tfor repo in $(cat repo-list)\n> +\t\tdo\n> +\t\t\twatchman watch-del \"$repo\"\n> +\t\tdone\n> +\tfi\n> +}\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 30b07e310f..067a432ea5 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1072,6 +1072,8 @@ test_atexit_handler () {\n> \t# sure that the registered cleanup commands are run only once.\n> \ttest : != \"$test_atexit_cleanup\" || return 0\n> \n> +\ttest_clear_watchman\n\nI'm not sure where to put this call, but this is definitely not the\nright place for it. See that 'return 0' above in the context? That's\nwhere the test_atexit_handler function returns early when no atexit\nhandler commands are set, i.e. in all test scripts that don't involve\nsome kind of daemons, thus this call is not invoked in the majority of\ntest scripts.\n\nSimply moving this call before that early return is not good, because\nthen it would be invoked twice.\n\nAn option would be to register this call as an atexit command\nsomewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON\nis restored, perhaps). That way it would be invoked most of the time,\nand it would be invoked only once, but I'm not sure how it would work\nout with test scripts that unset GIT_TEST_FSMONITOR somewhere in the\nmiddle for the remainder of the test script. However, register the\natexit command only if GIT_TEST_FSMONITOR is set (to something\nwatchman-specific), so it won't be invoked at all if\nGIT_TEST_FSMONITOR is not set, and thus it won't generate additional\ntest output and trace.\n\nI don't have a better idea.\n\n> +\n> \tsetup_malloc_check\n> \ttest_eval_ \"$test_atexit_cleanup\"\n> \ttest_atexit_cleanup=:\n> -- \n> gitgitgadget\n"},{"id":"386786","messageId":"837dca23-1681-2111-efaf-e38d603ea3f6@gmail.com","threadId":"52306","inReplyTo":"20191121231818.GA22688@generichostname","subject":"Re: [PATCH 01/11] fsmonitor: disable in a bare repo","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-22T01:57:07Z","receivedAt":"2019-11-22T01:57:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/21/2019 6:18 PM, Denton Liu wrote:\n> Hi Stolee,\n> \n> On Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> t0003-attributes.sh\n> \n> Patches 1 and 2 have this line in the commit message. I assume it's a\n> typo?\n> \n> Also, for the patches with empty commit message bodies, it would be nice\n> if they were filled in with detail for _why_ the change is being made.\n> For example, in this patch, a future reader might wonder why having\n> fsmonitor enabled in a bare repo causes problems.\n\nThanks for pointing that out. I forgot to clean up these messages, and\nwhat I left in them right now is sometimes just the first test script that\nwas failing without the change.\n\nThanks,\n-Stolee\n"},{"id":"387771","messageId":"93325ecb-0bbe-e7fb-3b60-8fff81768f8f@gmail.com","threadId":"52306","inReplyTo":"20191122010645.GX23183@szeder.dev","subject":"Re: [PATCH 11/11] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-09T14:12:37Z","receivedAt":"2019-12-09T14:12:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/21/2019 8:06 PM, SZEDER Gábor wrote:\n\nThanks for this message. Sorry I'm so late getting back to it.\n\n> On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> t/test-lib-functions.sh | 15 +++++++++++++++\n>> t/test-lib.sh | 2 ++\n>> 2 files changed, 17 insertions(+)\n>>\n>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>> index e0b3f28d3a..03573caf42 100644\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -1475,3 +1475,18 @@ test_set_port () {\n>> \tport=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))\n>> \teval $var=$port\n>> }\n>> +\n>> +test_clear_watchman () {\n>> +\tif test $GIT_TEST_FSMONITOR -ne \"\"\n> \n> In the rare cases when this function is invoked (see below) this\n> condition triggers an error from the shell running test script:\n> \n> - when the variable is not set, because of the lack of quotes around\n> the variable name:\n> \n> $ ./t5570-git-daemon.sh \n> [....]\n> ok 21 - hostname interpolation works after LF-stripping\n> ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator\n> # passed all 21 test(s)\n> 1..21\n> \n> - when the variable is set, because the '-ne' operator does integer\n> comparison:\n> \n> $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh\n> [...]\n> ok 21 - hostname interpolation works after LF-stripping\n> ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none\n> # failed 1 among 21 test(s)\n> 1..21\n> \n> Please use 'if test -n \"$GIT_TEST_FSMONITOR\"' instead.\n\nThanks for the pointers.\n\n>> +\tthen\n>> +\t\twatchman watch-list |\n> \n> Then with the above fixed, trying to run 'watchman' triggers another\n> error if it's not installed:\n> \n> $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh \n> [...]\n> ok 21 - hostname interpolation works after LF-stripping\n> ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found\n> # failed 1 among 21 test(s)\n> \n> I think we need an additional condition to run this only if\n> 't7519/fsmonitor-watchman' is used in the tests.\n\nThe intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,\nand that can only use watchman (currently). Barring wanting to unset the\nvariable if it was set on purpose in a test script, the other options do\nnot actually return correct values to make use of the feature.\n\n>> +\t\t\tgrep \"$TRASH_DIRECTORY\" |\n>> +\t\t\tsed \"s/\\t\\\"//g\" |\n>> +\t\t\tsed \"s/\\\",//g\" >repo-list\n>> +\n>> +\t\tfor repo in $(cat repo-list)\n>> +\t\tdo\n>> +\t\t\twatchman watch-del \"$repo\"\n>> +\t\tdone\n>> +\tfi\n>> +}\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index 30b07e310f..067a432ea5 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -1072,6 +1072,8 @@ test_atexit_handler () {\n>> \t# sure that the registered cleanup commands are run only once.\n>> \ttest : != \"$test_atexit_cleanup\" || return 0\n>> \n>> +\ttest_clear_watchman\n> \n> I'm not sure where to put this call, but this is definitely not the\n> right place for it. See that 'return 0' above in the context? That's\n> where the test_atexit_handler function returns early when no atexit\n> handler commands are set, i.e. in all test scripts that don't involve\n> some kind of daemons, thus this call is not invoked in the majority of\n> test scripts.\n\nAh, I misunderstood the point of test_atexit_handler.\n\n> Simply moving this call before that early return is not good, because\n> then it would be invoked twice.\n> \n> An option would be to register this call as an atexit command\n> somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON\n> is restored, perhaps). That way it would be invoked most of the time,\n> and it would be invoked only once, but I'm not sure how it would work\n> out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the\n> middle for the remainder of the test script. However, register the\n> atexit command only if GIT_TEST_FSMONITOR is set (to something\n> watchman-specific), so it won't be invoked at all if\n> GIT_TEST_FSMONITOR is not set, and thus it won't generate additional\n> test output and trace.\n> \n> I don't have a better idea.\n\nShouldn't it be sufficient to add it into test_done? If the test fails,\nthen we could leave watches open, but that's no worse than we had without\nthis test_clear_watchman method.\n\nThanks,\n-Stolee\n\n"},{"id":"387795","messageId":"79bb4c8e7d74c9c85cd6e6616811fc9a2e7c0afa.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 1/8] fsmonitor: disable in a bare repo","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:09:57Z","receivedAt":"2019-12-09T16:10:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nIf the repository is bare, then there is no working directory to\nwatch. Disable the core_fsmonitor global in this case.\n\nBefore this change, the test t0003-attributes.sh would fail with\nGIT_TEST_FSMONITOR pointing to t/t7519/fsmonitor-watchman.\n\nSigned-off-by: Derrick Stolee \n---\n config.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex 3900e4947b..f6d4e2fae3 100644\n--- a/config.c\n+++ b/config.c\n@@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)\n \n int git_config_get_fsmonitor(void)\n {\n+\tif (!the_repository->worktree) {\n+\t\tcore_fsmonitor = 0;\n+\t\treturn 0;\n+\t}\n+\n \tif (git_config_get_pathname(\"core.fsmonitor\", &core_fsmonitor))\n \t\tcore_fsmonitor = getenv(\"GIT_TEST_FSMONITOR\");\n \n-- \ngitgitgadget\n\n"},{"id":"387796","messageId":"dd492091e329fbc75571dd5d64bfdfd845daec80.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 2/8] fsmonitor: do not output to stderr for tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:09:58Z","receivedAt":"2019-12-09T16:10:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nThe test t0003-attributes.sh and others would fail when\nGIT_TEST_FSMONITOR is pointing at t/t7519/fsmonitor-watchman because\nit sends a message over stderr when registering the repo with\nwatchman for the first time. Remove this stderr message for the\ntest script to avoid this noise.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7519/fsmonitor-watchman | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman\nindex d8e7a1e5ba..06312876aa 100755\n--- a/t/t7519/fsmonitor-watchman\n+++ b/t/t7519/fsmonitor-watchman\n@@ -94,7 +94,6 @@ sub launch_watchman {\n \tmy $o = $json_pkg->new->utf8->decode($response);\n \n \tif ($retry > 0 and $o->{error} and $o->{error} =~ m/unable to resolve root .* directory (.*) is not watched/) {\n-\t\tprint STDERR \"Adding '$git_work_tree' to watchman's watch list.\\n\";\n \t\t$retry--;\n \t\tqx/watchman watch \"$git_work_tree\"/;\n \t\tdie \"Failed to make watchman watch '$git_work_tree'.\\n\" .\n-- \ngitgitgadget\n\n"},{"id":"387797","messageId":"f9db0c3416ff50587bffe46bb00268d8094c1ad7.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 3/8] t1301-shared-repo.sh: disable FSMONITOR","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:09:59Z","receivedAt":"2019-12-09T16:10:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nThe test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR\nis set to t/t7519/fsmonitor-watchman because it changes permissions\nin an incompatible way.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1301-shared-repo.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 2dc853d1be..665ade0cf2 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n '\n \n test_expect_success POSIXPERM 'forced modes' '\n+\tGIT_TEST_FSMONITOR=\"\" &&\n \tmkdir -p templates/hooks &&\n \techo update-server-info >templates/hooks/post-update &&\n \tchmod +x templates/hooks/post-update &&\n-- \ngitgitgadget\n\n"},{"id":"387798","messageId":"efc16962ee2595db50bf051fc84632b8c70036b3.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:10:00Z","receivedAt":"2019-12-09T16:10:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nWorktrees use a \".git\" _file_ instead of a folder to point to\nthe base repo's .git directory and the proper worktree HEAD. The\nfsmonitor hook tries to create a JSON file inside the \".git\" folder\nwhich violates the expectation here. It would be better to properly\nfind a safe folder for storing this JSON file.\n\nThis is also a problem when a test script uses GIT_WORK_TREE.\n\nSigned-off-by: Derrick Stolee \n---\n t/t1510-repo-setup.sh | 1 +\n t/t2400-worktree-add.sh | 2 ++\n t/t3030-merge-recursive.sh | 2 ++\n 3 files changed, 5 insertions(+)\n\ndiff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\nindex 9974457f56..28dce0c26f 100755\n--- a/t/t1510-repo-setup.sh\n+++ b/t/t1510-repo-setup.sh\n@@ -775,6 +775,7 @@ test_expect_success '#29: setup' '\n \tsetup_repo 29 non-existent gitfile true &&\n \tmkdir -p 29/sub/sub 29/wt/sub &&\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tcd 29 &&\n \t\tGIT_WORK_TREE=\"$here/29\" &&\n \t\texport GIT_WORK_TREE &&\ndiff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\nindex e819ba741e..d4d3cbae0f 100755\n--- a/t/t2400-worktree-add.sh\n+++ b/t/t2400-worktree-add.sh\n@@ -1,5 +1,7 @@\n #!/bin/sh\n \n+GIT_TEST_FSMONITOR=\"\"\n+\n test_description='test git worktree add'\n \n . ./test-lib.sh\ndiff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\nindex ff641b348a..62f645d639 100755\n--- a/t/t3030-merge-recursive.sh\n+++ b/t/t3030-merge-recursive.sh\n@@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' '\n \n test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tGIT_WORK_TREE=\"$PWD/ours-has-rename-work\" &&\n \t\texport GIT_WORK_TREE &&\n \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n@@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n \n test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n \t(\n+\t\tGIT_TEST_FSMONITOR=\"\" &&\n \t\tGIT_WORK_TREE=\"$PWD/theirs-has-rename-work\" &&\n \t\texport GIT_WORK_TREE &&\n \t\tGIT_INDEX_FILE=\"$PWD/theirs-has-rename-index\" &&\n-- \ngitgitgadget\n\n"},{"id":"387799","messageId":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.git.1574374826.gitgitgadget@gmail.com","subject":"[PATCH v2 0/8] Improve testability with GIT_TEST_FSMONITOR","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:09:56Z","receivedAt":"2019-12-09T16:10:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"The GIT_TEST_FSMONITOR environment variable allows run-time specification of\nthe fsmonitor hook. Initially used by t7619-status-fsmonitor.sh, we can\nenable it across the test suite to see how it affects Git's behavior. In\nparticular, we can specify the version of the hook that requests a result\nfrom Watchman to get actual updates to the files in the repo.\n\nIn many cases, our tests are simply not ready to handle this option.\nfsmonitor does not integrate well with features such as split index, bare\nrepos, or submodules. Other times, we need to disable it because the test is\nbeing specific about what files Git inspects during a 'status' call.\n\nThe long-term vision is to be able to run CI builds using a file-system\nwatcher like Watchman to get better coverage on this feature. These patches\nget us closer, but there are still some issues around overloading the\nWatchman interface when the tests are run in parallel. When using \"prove -j8\nt[0-8]*.sh\" I see some failures that do not reproduce when running the test\nscripts in isolation (on Linux), but using \"-j5\" is enough to eliminate\nfailures.\n\nSome of these changes should be backed out in favor of a deeper fix, so in\nsome sense these settings of GIT_TEST_FSMONITOR=\"\" serve as TODOs for the\nfsmonitor feature.\n\nSome recent and upcoming work on fsmonitor by Utsav Shah and Kevin Willford\nmay help us reach the goal of running with watchman enabled in CI builds.\nI'll come back around with updates to the .azure-pipelines YAML files when\nwe feel the feature is ready for that.\n\nUpdates in V2:\n\n * Commit messages have been filled out completely. Most provide the same\n blurb of context before briefly describing the actual change.\n \n \n * Some commits were merged because they had similar causes (worktrees,\n submodules).\n \n \n * The test_clear_watchman function is updated and it is called by test_done\n instead of the atexit helper.\n \n \n\nThanks, -Stolee\n\nDerrick Stolee (8):\n fsmonitor: disable in a bare repo\n fsmonitor: do not output to stderr for tests\n t1301-shared-repo.sh: disable FSMONITOR\n t3030-merge-recursive.sh: disable fsmonitor when tweaking\n GIT_WORK_TREE\n tests: disable fsmonitor in submodule tests\n t7063: disable fsmonitor with status cache\n t7519: disable external GIT_TEST_FSMONITOR variable\n test-lib: clear watchman watches at test completion\n\n config.c | 5 +++++\n t/t1301-shared-repo.sh | 1 +\n t/t1510-repo-setup.sh | 1 +\n t/t2400-worktree-add.sh | 2 ++\n t/t3030-merge-recursive.sh | 2 ++\n t/t3404-rebase-interactive.sh | 1 +\n t/t3600-rm.sh | 1 +\n t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n t/t5526-fetch-submodules.sh | 2 ++\n t/t7063-status-untracked-cache.sh | 3 +++\n t/t7402-submodule-rebase.sh | 3 +++\n t/t7406-submodule-update.sh | 2 ++\n t/t7506-status-submodule.sh | 3 +++\n t/t7508-status.sh | 3 +++\n t/t7519-status-fsmonitor.sh | 3 +++\n t/t7519/fsmonitor-watchman | 1 -\n t/test-lib-functions.sh | 15 +++++++++++++++\n t/test-lib.sh | 4 ++++\n 18 files changed, 54 insertions(+), 1 deletion(-)\n\n\nbase-commit: dd0b61f577f041f1119bb3288451f8f9b7f9e3f2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-466%2Fderrickstolee%2Ftest-watchman-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-466/derrickstolee/test-watchman-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/466\n\nRange-diff vs v1:\n\n 1: 9bf5a803e6 ! 1: 79bb4c8e7d fsmonitor: disable in a bare repo\n @@ -2,7 +2,18 @@\n \n fsmonitor: disable in a bare repo\n \n - t0003-attributes.sh\n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n + If the repository is bare, then there is no working directory to\n + watch. Disable the core_fsmonitor global in this case.\n +\n + Before this change, the test t0003-attributes.sh would fail with\n + GIT_TEST_FSMONITOR pointing to t/t7519/fsmonitor-watchman.\n \n Signed-off-by: Derrick Stolee \n \n 2: 245cc1b2cc ! 2: dd492091e3 fsmonitor: do not output to stderr for tests\n @@ -2,7 +2,18 @@\n \n fsmonitor: do not output to stderr for tests\n \n - t0003-attributes.sh\n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n + The test t0003-attributes.sh and others would fail when\n + GIT_TEST_FSMONITOR is pointing at t/t7519/fsmonitor-watchman because\n + it sends a message over stderr when registering the repo with\n + watchman for the first time. Remove this stderr message for the\n + test script to avoid this noise.\n \n Signed-off-by: Derrick Stolee \n \n 3: ed40ce8e4c ! 3: f9db0c3416 t1301-shared-repo.sh: disable FSMONITOR\n @@ -2,6 +2,17 @@\n \n t1301-shared-repo.sh: disable FSMONITOR\n \n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n + The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR\n + is set to t/t7519/fsmonitor-watchman because it changes permissions\n + in an incompatible way.\n +\n Signed-off-by: Derrick Stolee \n \n diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n 4: 9179776982 < -: ---------- t1510-repo-setup.sh: disable fsmonitor if no .git dir\n 5: 81c587651e < -: ---------- fsmonitor: disable fsmonitor with worktrees\n 6: d5f5a3e2b9 ! 4: efc16962ee t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE\n @@ -2,8 +2,47 @@\n \n t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE\n \n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n + Worktrees use a \".git\" _file_ instead of a folder to point to\n + the base repo's .git directory and the proper worktree HEAD. The\n + fsmonitor hook tries to create a JSON file inside the \".git\" folder\n + which violates the expectation here. It would be better to properly\n + find a safe folder for storing this JSON file.\n +\n + This is also a problem when a test script uses GIT_WORK_TREE.\n +\n Signed-off-by: Derrick Stolee \n \n + diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\n + --- a/t/t1510-repo-setup.sh\n + +++ b/t/t1510-repo-setup.sh\n +@@\n + \tsetup_repo 29 non-existent gitfile true &&\n + \tmkdir -p 29/sub/sub 29/wt/sub &&\n + \t(\n ++\t\tGIT_TEST_FSMONITOR=\"\" &&\n + \t\tcd 29 &&\n + \t\tGIT_WORK_TREE=\"$here/29\" &&\n + \t\texport GIT_WORK_TREE &&\n +\n + diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n + --- a/t/t2400-worktree-add.sh\n + +++ b/t/t2400-worktree-add.sh\n +@@\n + #!/bin/sh\n + \n ++GIT_TEST_FSMONITOR=\"\"\n ++\n + test_description='test git worktree add'\n + \n + . ./test-lib.sh\n +\n diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n --- a/t/t3030-merge-recursive.sh\n +++ b/t/t3030-merge-recursive.sh\n 7: d0b212c9df < -: ---------- t3600-rm.sh: disable fsmonitor when deleting populated submodule\n 8: 36f845cb7e ! 5: a5b0bf6ac7 tests: disable fsmonitor in submodule tests\n @@ -2,12 +2,43 @@\n \n tests: disable fsmonitor in submodule tests\n \n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n The fsmonitor feature struggles with submodules. Disable the\n GIT_TEST_FSMONITOR environment variable before running tests with\n a lot of submodule interactions.\n \n Signed-off-by: Derrick Stolee \n \n + diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n + --- a/t/t3404-rebase-interactive.sh\n + +++ b/t/t3404-rebase-interactive.sh\n +@@\n + '\n + \n + test_expect_success 'submodule rebase setup' '\n ++\tGIT_TEST_FSMONITOR=\"\" &&\n + \tgit checkout A &&\n + \tmkdir sub &&\n + \t(\n +\n + diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n + --- a/t/t3600-rm.sh\n + +++ b/t/t3600-rm.sh\n +@@\n + '\n + \n + test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n ++\tGIT_TEST_FSMONITOR=\"\" &&\n + \tgit reset --hard &&\n + \tgit submodule update &&\n + \tgit -C submod checkout HEAD^ &&\n +\n diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\n --- a/t/t4060-diff-submodule-option-diff-format.sh\n +++ b/t/t4060-diff-submodule-option-diff-format.sh\n 9: 141a1909a4 ! 6: 9cd4a08d82 t7063: disable fsmonitor with status cache\n @@ -2,6 +2,13 @@\n \n t7063: disable fsmonitor with status cache\n \n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n The status cache tests use GIT_TRACE_UNTRACKED_STATS to check very\n detailed statistics related to how much Git actually checked for\n untracked files. The fsmonitor feature changes the expected behavior\n 10: cd717ef5de = 7: 215ec8688e t7519: disable external GIT_TEST_FSMONITOR variable\n 11: 47cecb4a83 ! 8: e51165f260 test-lib: clear watchman watches at test completion\n @@ -2,6 +2,29 @@\n \n test-lib: clear watchman watches at test completion\n \n + The fsmonitor feature allows an external tool such as watchman to\n + monitor the working directory. The direct test\n + t7619-status-fsmonitor.sh provides some coverage, but it would be\n + better to run the entire test suite with watchman enabled. This\n + would provide more confidence that the feature is working as\n + intended.\n +\n + When running the test suite in parallel with 'prove -j ', many\n + repos are created and deleted in parallel. When GIT_TEST_FSMONITOR\n + points to t/t7519/fsmonitor-watchman, this can lead to watchman\n + tracking many different folders, overloading its watch queue.\n +\n + As a test script completes, we can tell watchman to stop watching\n + the directories inside the TRASH_DIRECTORY.\n +\n + This is particularly important on Windows where watchman keeps an\n + open handle on the directories it watches, preventing them from\n + being deleted. There is currently a bug in watchman [1] where this\n + handle still is not closed, but if that is updated then these tests\n + can be run on Windows as well.\n +\n + [1] https://github.com/facebook/watchman/issues/764\n +\n Signed-off-by: Derrick Stolee \n \n diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n @@ -13,17 +36,17 @@\n }\n +\n +test_clear_watchman () {\n -+\tif test $GIT_TEST_FSMONITOR -ne \"\"\n ++\tif test -n \"$GIT_TEST_FSMONITOR\"\n +\tthen\n +\t\twatchman watch-list |\n +\t\t\tgrep \"$TRASH_DIRECTORY\" |\n -+\t\t\tsed \"s/\\t\\\"//g\" |\n -+\t\t\tsed \"s/\\\",//g\" >repo-list\n ++\t\t\tsed \"s/\\\",//g\" |\n ++\t\t\tsed \"s/\\\"//g\" >repo-list\n +\n -+\t\tfor repo in $(cat repo-list)\n ++\t\twhile read repo\n +\t\tdo\n +\t\t\twatchman watch-del \"$repo\"\n -+\t\tdone\n ++\t\tdone \n\nThe GIT_TEST_FSMONITOR variable was created specifically so the\nt7519-status-fsmonitor.sh test script could tweak the expected\nbehavior depending on its value. However, if we set it externally\nto use the Watchman integration, then it breaks the initial tests\nthat demonstrate behavior _without_ the fsmonitor feature.\n\nDisable this variable at the start of the script.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7519-status-fsmonitor.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh\nindex 81a375fa0f..443d2e653b 100755\n--- a/t/t7519-status-fsmonitor.sh\n+++ b/t/t7519-status-fsmonitor.sh\n@@ -8,6 +8,9 @@ test_description='git status with file system watcher'\n # \"git update-index --fsmonitor\" can be used to get the extension written\n # before testing the results.\n \n+# Disable an external value, as we will set it directly as needed.\n+GIT_TEST_FSMONITOR=\"\"\n+\n clean_repo () {\n \tgit reset --hard HEAD &&\n \tgit clean -fd\n-- \ngitgitgadget\n\n"},{"id":"387801","messageId":"e51165f260d564ccb7a9b8e696691eccb184c01a.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:10:04Z","receivedAt":"2019-12-09T16:10:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nWhen running the test suite in parallel with 'prove -j ', many\nrepos are created and deleted in parallel. When GIT_TEST_FSMONITOR\npoints to t/t7519/fsmonitor-watchman, this can lead to watchman\ntracking many different folders, overloading its watch queue.\n\nAs a test script completes, we can tell watchman to stop watching\nthe directories inside the TRASH_DIRECTORY.\n\nThis is particularly important on Windows where watchman keeps an\nopen handle on the directories it watches, preventing them from\nbeing deleted. There is currently a bug in watchman [1] where this\nhandle still is not closed, but if that is updated then these tests\ncan be run on Windows as well.\n\n[1] https://github.com/facebook/watchman/issues/764\n\nSigned-off-by: Derrick Stolee \n---\n t/test-lib-functions.sh | 15 +++++++++++++++\n t/test-lib.sh | 4 ++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e0b3f28d3a..ef840ce097 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -1475,3 +1475,18 @@ test_set_port () {\n \tport=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))\n \teval $var=$port\n }\n+\n+test_clear_watchman () {\n+\tif test -n \"$GIT_TEST_FSMONITOR\"\n+\tthen\n+\t\twatchman watch-list |\n+\t\t\tgrep \"$TRASH_DIRECTORY\" |\n+\t\t\tsed \"s/\\\",//g\" |\n+\t\t\tsed \"s/\\\"//g\" >repo-list\n+\n+\t\twhile read repo\n+\t\tdo\n+\t\t\twatchman watch-del \"$repo\"\n+\t\tdone \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nThe status cache tests use GIT_TRACE_UNTRACKED_STATS to check very\ndetailed statistics related to how much Git actually checked for\nuntracked files. The fsmonitor feature changes the expected behavior\nhere, so disable the GIT_TEST_FSMONITOR environment variable.\n\nSigned-off-by: Derrick Stolee \n---\n t/t7063-status-untracked-cache.sh | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/t/t7063-status-untracked-cache.sh b/t/t7063-status-untracked-cache.sh\nindex 190ae149cf..c433738a3a 100755\n--- a/t/t7063-status-untracked-cache.sh\n+++ b/t/t7063-status-untracked-cache.sh\n@@ -4,6 +4,9 @@ test_description='test untracked cache'\n \n . ./test-lib.sh\n \n+# fsmonitor changes the expected behvaior of GIT_TRACE_UNTRACKED_STATS\n+GIT_TEST_FSMONITOR=\"\"\n+\n # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime\n # is updated lazily after contents in the directory changes, which\n # forces the untracked cache code to take the slow path. A test\n-- \ngitgitgadget\n\n"},{"id":"387803","messageId":"a5b0bf6ac7cd28fa47e26ab481f781d74c656f6a.1575907804.git.gitgitgadget@gmail.com","threadId":"52306","inReplyTo":"pull.466.v2.git.1575907804.gitgitgadget@gmail.com","subject":"[PATCH v2 5/8] tests: disable fsmonitor in submodule tests","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-12-09T16:10:01Z","receivedAt":"2019-12-09T16:10:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe fsmonitor feature allows an external tool such as watchman to\nmonitor the working directory. The direct test\nt7619-status-fsmonitor.sh provides some coverage, but it would be\nbetter to run the entire test suite with watchman enabled. This\nwould provide more confidence that the feature is working as\nintended.\n\nThe fsmonitor feature struggles with submodules. Disable the\nGIT_TEST_FSMONITOR environment variable before running tests with\na lot of submodule interactions.\n\nSigned-off-by: Derrick Stolee \n---\n t/t3404-rebase-interactive.sh | 1 +\n t/t3600-rm.sh | 1 +\n t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n t/t5526-fetch-submodules.sh | 2 ++\n t/t7402-submodule-rebase.sh | 3 +++\n t/t7406-submodule-update.sh | 2 ++\n t/t7506-status-submodule.sh | 3 +++\n t/t7508-status.sh | 3 +++\n 8 files changed, 18 insertions(+)\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 461dd539ff..9dc7d1aefb 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -697,6 +697,7 @@ test_expect_success 'do \"noop\" when there is nothing to cherry-pick' '\n '\n \n test_expect_success 'submodule rebase setup' '\n+\tGIT_TEST_FSMONITOR=\"\" &&\n \tgit checkout A &&\n \tmkdir sub &&\n \t(\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex 66282a720e..64269bd89d 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' '\n '\n \n test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n+\tGIT_TEST_FSMONITOR=\"\" &&\n \tgit reset --hard &&\n \tgit submodule update &&\n \tgit -C submod checkout HEAD^ &&\ndiff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\nindex 9dcb69df5c..017417790e 100755\n--- a/t/t4060-diff-submodule-option-diff-format.sh\n+++ b/t/t4060-diff-submodule-option-diff-format.sh\n@@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff.\n # Tested non-UTF-8 encoding\n test_encoding=\"ISO8859-1\"\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n # String \"added\" in German (translated with Google Translate), encoded in UTF-8,\n # used in sample commit log messages in add_file() function below.\n added=$(printf \"hinzugef\\303\\274gt\")\ndiff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\nindex 63205dfdf9..fb346bff05 100755\n--- a/t/t5526-fetch-submodules.sh\n+++ b/t/t5526-fetch-submodules.sh\n@@ -1,6 +1,8 @@\n #!/bin/sh\n # Copyright (c) 2010, Jens Lehmann\n \n+GIT_TEST_FSMONITOR=\"\"\n+\n test_description='Recursive \"git fetch\" for submodules'\n \n . ./test-lib.sh\ndiff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh\nindex 8e32f19007..c78e9009cf 100755\n--- a/t/t7402-submodule-rebase.sh\n+++ b/t/t7402-submodule-rebase.sh\n@@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules'\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_expect_success setup '\n \n \techo file > file &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex c973278300..8d93aaef5f 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -11,6 +11,8 @@ submodule and \"git submodule update --rebase/--merge\" does not detach the HEAD.\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n \n compare_head()\n {\ndiff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\nindex 08629a6e70..1a716f2c2a 100755\n--- a/t/t7506-status-submodule.sh\n+++ b/t/t7506-status-submodule.sh\n@@ -4,6 +4,9 @@ test_description='git status for submodule'\n \n . ./test-lib.sh\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_create_repo_with_commit () {\n \ttest_create_repo \"$1\" &&\n \t(\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex 4e676cdce8..bf0487632d 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '\n \ttest_cmp expect output\n '\n \n+# fsmonitor does not work well with submodules\n+GIT_TEST_FSMONITOR=\"\"\n+\n test_expect_success 'setup status submodule summary' '\n \ttest_create_repo sm && (\n \t\tcd sm &&\n-- \ngitgitgadget\n\n"},{"id":"387858","messageId":"xmqqwob5ru27.fsf@gitster-ct.c.googlers.com","threadId":"52306","inReplyTo":"e51165f260d564ccb7a9b8e696691eccb184c01a.1575907804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T22:52:00Z","receivedAt":"2019-12-09T22:52:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> +test_clear_watchman () {\n> +\tif test -n \"$GIT_TEST_FSMONITOR\"\n> +\tthen\n> +\t\twatchman watch-list |\n> +\t\t\tgrep \"$TRASH_DIRECTORY\" |\n> +\t\t\tsed \"s/\\\",//g\" |\n> +\t\t\tsed \"s/\\\"//g\" >repo-list\n\nWhoa, this is scary. \"$TRASH_DIRECTORY\" comes from $(pwd) and the\nleading part of it can have arbitrary garbage like \"[a-z]*\" that may\nmatch paths \"watchman watch-list\" may emit that does not have\nanything to do with the temporary directory used by this test.\n\nWhat are these stripping of \", and \" about? Could you tell readers\nhow a typical output from the program we are reading from looks like\nperhaps in the log message or in-code comment around here?\n\nThanks.\n"},{"id":"387859","messageId":"20191209234016.GA6527@szeder.dev","threadId":"52306","inReplyTo":"93325ecb-0bbe-e7fb-3b60-8fff81768f8f@gmail.com","subject":"Re: [PATCH 11/11] test-lib: clear watchman watches at test completion","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-09T23:40:16Z","receivedAt":"2019-12-09T23:40:25Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote:\n> >> +\t\twatchman watch-list |\n> > \n> > Then with the above fixed, trying to run 'watchman' triggers another\n> > error if it's not installed:\n> > \n> > $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh \n> > [...]\n> > ok 21 - hostname interpolation works after LF-stripping\n> > ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found\n> > # failed 1 among 21 test(s)\n> > \n> > I think we need an additional condition to run this only if\n> > 't7519/fsmonitor-watchman' is used in the tests.\n> \n> The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,\n> and that can only use watchman (currently).\n\nI've just run 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all make',\nand it only failed one test in 't0090-cache-tree.sh', but the fix is\nalready in 'pu' in 61eea521fe (fsmonitor: do not compare bitmap size\nwith size of split index, 2019-11-13).\n\n\n> >> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> >> index 30b07e310f..067a432ea5 100644\n> >> --- a/t/test-lib.sh\n> >> +++ b/t/test-lib.sh\n> >> @@ -1072,6 +1072,8 @@ test_atexit_handler () {\n> >> \t# sure that the registered cleanup commands are run only once.\n> >> \ttest : != \"$test_atexit_cleanup\" || return 0\n> >> \n> >> +\ttest_clear_watchman\n> > \n> > I'm not sure where to put this call, but this is definitely not the\n> > right place for it. See that 'return 0' above in the context? That's\n> > where the test_atexit_handler function returns early when no atexit\n> > handler commands are set, i.e. in all test scripts that don't involve\n> > some kind of daemons, thus this call is not invoked in the majority of\n> > test scripts.\n> \n> Ah, I misunderstood the point of test_atexit_handler.\n> \n> > Simply moving this call before that early return is not good, because\n> > then it would be invoked twice.\n> > \n> > An option would be to register this call as an atexit command\n> > somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON\n> > is restored, perhaps). That way it would be invoked most of the time,\n> > and it would be invoked only once, but I'm not sure how it would work\n> > out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the\n> > middle for the remainder of the test script. However, register the\n> > atexit command only if GIT_TEST_FSMONITOR is set (to something\n> > watchman-specific), so it won't be invoked at all if\n> > GIT_TEST_FSMONITOR is not set, and thus it won't generate additional\n> > test output and trace.\n> > \n> > I don't have a better idea.\n> \n> Shouldn't it be sufficient to add it into test_done? If the test fails,\n> then we could leave watches open, but that's no worse than we had without\n> this test_clear_watchman method.\n\nI don't know enough about watchman to have an informed opinion.\n\nI think the answer mainly depends on what we want to achive and what\nhappens when a test script run with GIT_TEST_FSMONITOR exits without\ninvoking 'test_done' is re-executed (e.g. after a test case fails with\n'--immediate' or when the user hits ctrl-c or closes the terminal\nwindow mid-test).\n\nAs far as I understand the commit message of v2 of this patch [1], we\nmainly want two things:\n\n - Avoid overloading watchman's watch queue. For this it might\n indeed be sufficient to clear watches in 'test_done', because most\n test scripts tend to succeed most of the time.\n\n - Make GIT_TEST_FSMONITOR work reliably on Windows. For this, I'm\n afraid it's not enough in general, because a failure with\n '--immediate' or after a ctrl-c we won't run 'test_done', so we\n won't clear the watches, and watchman will keep the fd to the\n trash dir open, and, consequently, will interfere with subsequent\n executions of the same test script as it can't delete the still\n existing trash dir left over from the previous run.\n \n It could still be sufficient for fsmonitor-enabled CI builds,\n though, because there we don't re-run tests, don't hit ctrl-c, and\n (at least on Azure Pipelines) don't use '--immediate', and the\n whole VM/container/whatever is thrown away at end anyway.\n\n On Linux/Unix-y systems it probably doesn't matter much, because\n they can delete open directories, but I wonder what happens with a\n watch when the directory it is supposed observe gets deleted. If\n the watch is removed in this case, great; if it isn't, then...\n well, then what happens with it? Will it be overwritten with the\n next test run, or will there be duplicate watches for the same\n dir?\n\n[1] https://public-inbox.org/git/e51165f260d564ccb7a9b8e696691eccb184c01a.1575907804.git.gitgitgadget@gmail.com/\n\n"},{"id":"387860","messageId":"76b75c14-96ec-f8a5-a367-fb9b97a1c477@gmail.com","threadId":"52306","inReplyTo":"20191209234016.GA6527@szeder.dev","subject":"Re: [PATCH 11/11] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-10T01:43:37Z","receivedAt":"2019-12-10T01:43:42Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/9/2019 6:40 PM, SZEDER Gábor wrote:\n> On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote:\n>>>> +\t\twatchman watch-list |\n>>>\n>>> Then with the above fixed, trying to run 'watchman' triggers another\n>>> error if it's not installed:\n>>>\n>>> $ GIT_TEST_FSMONITOR=\"$PWD\"/t7519/fsmonitor-none ./t5570-git-daemon.sh \n>>> [...]\n>>> ok 21 - hostname interpolation works after LF-stripping\n>>> ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found\n>>> # failed 1 among 21 test(s)\n>>>\n>>> I think we need an additional condition to run this only if\n>>> 't7519/fsmonitor-watchman' is used in the tests.\n>>\n>> The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR,\n>> and that can only use watchman (currently).\n> \n> I've just run 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all make',\n> and it only failed one test in 't0090-cache-tree.sh', but the fix is\n> already in 'pu' in 61eea521fe (fsmonitor: do not compare bitmap size\n> with size of split index, 2019-11-13).\n> \n> \n>>>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>>>> index 30b07e310f..067a432ea5 100644\n>>>> --- a/t/test-lib.sh\n>>>> +++ b/t/test-lib.sh\n>>>> @@ -1072,6 +1072,8 @@ test_atexit_handler () {\n>>>> \t# sure that the registered cleanup commands are run only once.\n>>>> \ttest : != \"$test_atexit_cleanup\" || return 0\n>>>> \n>>>> +\ttest_clear_watchman\n>>>\n>>> I'm not sure where to put this call, but this is definitely not the\n>>> right place for it. See that 'return 0' above in the context? That's\n>>> where the test_atexit_handler function returns early when no atexit\n>>> handler commands are set, i.e. in all test scripts that don't involve\n>>> some kind of daemons, thus this call is not invoked in the majority of\n>>> test scripts.\n>>\n>> Ah, I misunderstood the point of test_atexit_handler.\n>>\n>>> Simply moving this call before that early return is not good, because\n>>> then it would be invoked twice.\n>>>\n>>> An option would be to register this call as an atexit command\n>>> somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON\n>>> is restored, perhaps). That way it would be invoked most of the time,\n>>> and it would be invoked only once, but I'm not sure how it would work\n>>> out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the\n>>> middle for the remainder of the test script. However, register the\n>>> atexit command only if GIT_TEST_FSMONITOR is set (to something\n>>> watchman-specific), so it won't be invoked at all if\n>>> GIT_TEST_FSMONITOR is not set, and thus it won't generate additional\n>>> test output and trace.\n>>>\n>>> I don't have a better idea.\n>>\n>> Shouldn't it be sufficient to add it into test_done? If the test fails,\n>> then we could leave watches open, but that's no worse than we had without\n>> this test_clear_watchman method.\n> \n> I don't know enough about watchman to have an informed opinion.\n> \n> I think the answer mainly depends on what we want to achive and what\n> happens when a test script run with GIT_TEST_FSMONITOR exits without\n> invoking 'test_done' is re-executed (e.g. after a test case fails with\n> '--immediate' or when the user hits ctrl-c or closes the terminal\n> window mid-test).\n> \n> As far as I understand the commit message of v2 of this patch [1], we\n> mainly want two things:\n> \n> - Avoid overloading watchman's watch queue. For this it might\n> indeed be sufficient to clear watches in 'test_done', because most\n> test scripts tend to succeed most of the time.\n> \n> - Make GIT_TEST_FSMONITOR work reliably on Windows. For this, I'm\n> afraid it's not enough in general, because a failure with\n> '--immediate' or after a ctrl-c we won't run 'test_done', so we\n> won't clear the watches, and watchman will keep the fd to the\n> trash dir open, and, consequently, will interfere with subsequent\n> executions of the same test script as it can't delete the still\n> existing trash dir left over from the previous run.\n\nYou are right. Running an individual test and ending it early would\nlead to these leaked handles. This assumes someone is aware of the\nGIT_TEST_FSMONITOR environment variable, so they are at least\ninteracting with the feature directly to some extent.\n\n> It could still be sufficient for fsmonitor-enabled CI builds,\n> though, because there we don't re-run tests, don't hit ctrl-c, and\n> (at least on Azure Pipelines) don't use '--immediate', and the\n> whole VM/container/whatever is thrown away at end anyway.\n\nThis is the hope. It would be nice to get to that point.\n\n> \n> On Linux/Unix-y systems it probably doesn't matter much, because\n> they can delete open directories, but I wonder what happens with a\n> watch when the directory it is supposed observe gets deleted. If\n> the watch is removed in this case, great; if it isn't, then...\n> well, then what happens with it? Will it be overwritten with the\n> next test run, or will there be duplicate watches for the same\n> dir?\n\nWhen a directory is deleted from under Watchman on Linux, the watch\nis removed...eventually. I'm not sure at exactly what point that happens.\nAt the very least, Watchman will receive and process the signals for all\nof the paths being removed inside the directory. Running 'watch-del'\nremoves that overhead.\n\nThanks,\n-Stolee\n"},{"id":"387862","messageId":"bfa73fab-ce2c-a05e-3568-cd406dd5c31f@gmail.com","threadId":"52306","inReplyTo":"xmqqwob5ru27.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-10T01:49:36Z","receivedAt":"2019-12-10T01:49:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/9/2019 5:52 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> +test_clear_watchman () {\n>> +\tif test -n \"$GIT_TEST_FSMONITOR\"\n>> +\tthen\n>> +\t\twatchman watch-list |\n>> +\t\t\tgrep \"$TRASH_DIRECTORY\" |\n>> +\t\t\tsed \"s/\\\",//g\" |\n>> +\t\t\tsed \"s/\\\"//g\" >repo-list\n> \n> Whoa, this is scary. \"$TRASH_DIRECTORY\" comes from $(pwd) and the\n> leading part of it can have arbitrary garbage like \"[a-z]*\" that may\n> match paths \"watchman watch-list\" may emit that does not have\n> anything to do with the temporary directory used by this test.\n\nHm. That is a good point. Can we assume that our version of grep has\na \"-F\" or \"--fixed-strings\" option? ([1] seems to say that \"-F\" would\nwork.)\n\n[1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs\n\n> What are these stripping of \", and \" about? Could you tell readers\n> how a typical output from the program we are reading from looks like\n> perhaps in the log message or in-code comment around here?\n\nWatchman outputs its list of paths in JSON format. Luckily, it formats\nthe output so the path lines are on separate lines, each quoted.\n\nFor example:\n\n{\n\t\"version\": \"4.9.0\",\n\t\"roots\": [\n\t\t\"\",\n\t\t\"\",\n\t\t\"\"\n\t]\n}\n\nThanks,\n-Stolee\n"},{"id":"387871","messageId":"xmqqo8wgsqnn.fsf@gitster-ct.c.googlers.com","threadId":"52306","inReplyTo":"bfa73fab-ce2c-a05e-3568-cd406dd5c31f@gmail.com","subject":"Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-10T05:20:12Z","receivedAt":"2019-12-10T05:20:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Hm. That is a good point. Can we assume that our version of grep has\n> a \"-F\" or \"--fixed-strings\" option? ([1] seems to say that \"-F\" would\n> work.)\n\n$ git grep \"grep -F\" -- \\*.sh\n\nis your friend ;-) \n\nAnd never use https://www.gnu.org/ manual as a yardstick---you will\nend up using GNUism that is not unavailable elsewhere pretty easily.\n\n> [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs\n>\n>> What are these stripping of \", and \" about? Could you tell readers\n>> how a typical output from the program we are reading from looks like\n>> perhaps in the log message or in-code comment around here?\n>\n> Watchman outputs its list of paths in JSON format. Luckily, it formats\n> the output so the path lines are on separate lines, each quoted.\n>\n> For example:\n>\n> {\n> \t\"version\": \"4.9.0\",\n> \t\"roots\": [\n> \t\t\"\",\n> \t\t\"\",\n> \t\t\"\"\n> \t]\n> }\n\nYeek; how is a dq in path represented? by doubling? by\nbackslash-quoting (if so how is a backslash in path represented)?\nBy something else?\n\nIt's OK at least for now to declare that our test repository does\nnot contain any funny paths, but in the longer run does the above\nmean that we somehow need to be able to grok JSON reliably in our\ntests? It may not be such a bad thing especially for longer term,\nas there are other parts of the system that may benefit from having\nJSON capable output readers in our tests (e.g. trace2 code can do\nJSON, right?)..\n\n"},{"id":"387872","messageId":"20191210094301.GB6527@szeder.dev","threadId":"52306","inReplyTo":"f9db0c3416ff50587bffe46bb00268d8094c1ad7.1575907804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/8] t1301-shared-repo.sh: disable FSMONITOR","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T09:43:01Z","receivedAt":"2019-12-10T09:43:10Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 09, 2019 at 04:09:59PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The fsmonitor feature allows an external tool such as watchman to\n> monitor the working directory. The direct test\n> t7619-status-fsmonitor.sh provides some coverage, but it would be\n> better to run the entire test suite with watchman enabled. This\n> would provide more confidence that the feature is working as\n> intended.\n> \n> The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR\n> is set to t/t7519/fsmonitor-watchman because it changes permissions\n> in an incompatible way.\n\nI don't understand this, and would like it to be more specific, i.e.\nwhich particular permission changes in which tests are incompatible\nwith watchman.\n\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t1301-shared-repo.sh | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\n> index 2dc853d1be..665ade0cf2 100755\n> --- a/t/t1301-shared-repo.sh\n> +++ b/t/t1301-shared-repo.sh\n> @@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n> '\n> \n> test_expect_success POSIXPERM 'forced modes' '\n> +\tGIT_TEST_FSMONITOR=\"\" &&\n\nThis is not in a subshell, so it disables GIT_TEST_FSMONITOR not only\nfor the test it is executed in but for the remainder of the test\nscript. From the commit message I can't decide whether that's\nintended.\n\n> \tmkdir -p templates/hooks &&\n> \techo update-server-info >templates/hooks/post-update &&\n> \tchmod +x templates/hooks/post-update &&\n> -- \n> gitgitgadget\n> \n"},{"id":"387873","messageId":"20191210094616.GC6527@szeder.dev","threadId":"52306","inReplyTo":"79bb4c8e7d74c9c85cd6e6616811fc9a2e7c0afa.1575907804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/8] fsmonitor: disable in a bare repo","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T09:46:16Z","receivedAt":"2019-12-10T09:46:22Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 09, 2019 at 04:09:57PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The fsmonitor feature allows an external tool such as watchman to\n> monitor the working directory. The direct test\n> t7619-status-fsmonitor.sh provides some coverage, but it would be\n\ns/t7619/t7519/\n\nThis typo is present in all but one other commit messages.\n\n"},{"id":"387876","messageId":"20191210100732.GD6527@szeder.dev","threadId":"52306","inReplyTo":"efc16962ee2595db50bf051fc84632b8c70036b3.1575907804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T10:07:32Z","receivedAt":"2019-12-10T10:07:39Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The fsmonitor feature allows an external tool such as watchman to\n> monitor the working directory. The direct test\n> t7619-status-fsmonitor.sh provides some coverage, but it would be\n> better to run the entire test suite with watchman enabled. This\n> would provide more confidence that the feature is working as\n> intended.\n> \n> Worktrees use a \".git\" _file_ instead of a folder to point to\n> the base repo's .git directory and the proper worktree HEAD. The\n> fsmonitor hook tries to create a JSON file inside the \".git\" folder\n> which violates the expectation here.\n\nYeah, there are a couple hardcoded paths in there, e.g.:\n\n open ($fh, \">\", \".git/watchman-response.json\");\n\nand, worse, not only in the test helper hook in\n't/t7519/fsmonitor-watchman' but in the sample hook template\n'templates/hooks--fsmonitor-watchman.sample' as well.\n\n> It would be better to properly\n> find a safe folder for storing this JSON file.\n\n git rev-parse --git-path ''\n\ngives us the right directory prefix to use and we could then append\nthe various filenames that must be accessed in there.\n\n> This is also a problem when a test script uses GIT_WORK_TREE.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> t/t1510-repo-setup.sh | 1 +\n> t/t2400-worktree-add.sh | 2 ++\n> t/t3030-merge-recursive.sh | 2 ++\n> 3 files changed, 5 insertions(+)\n> \n> diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh\n> index 9974457f56..28dce0c26f 100755\n> --- a/t/t1510-repo-setup.sh\n> +++ b/t/t1510-repo-setup.sh\n> @@ -775,6 +775,7 @@ test_expect_success '#29: setup' '\n> \tsetup_repo 29 non-existent gitfile true &&\n> \tmkdir -p 29/sub/sub 29/wt/sub &&\n> \t(\n> +\t\tGIT_TEST_FSMONITOR=\"\" &&\n> \t\tcd 29 &&\n> \t\tGIT_WORK_TREE=\"$here/29\" &&\n> \t\texport GIT_WORK_TREE &&\n> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh\n> index e819ba741e..d4d3cbae0f 100755\n> --- a/t/t2400-worktree-add.sh\n> +++ b/t/t2400-worktree-add.sh\n> @@ -1,5 +1,7 @@\n> #!/bin/sh\n> \n> +GIT_TEST_FSMONITOR=\"\"\n> +\n> test_description='test git worktree add'\n> \n> . ./test-lib.sh\n> diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh\n> index ff641b348a..62f645d639 100755\n> --- a/t/t3030-merge-recursive.sh\n> +++ b/t/t3030-merge-recursive.sh\n> @@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' '\n> \n> test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n> \t(\n> +\t\tGIT_TEST_FSMONITOR=\"\" &&\n> \t\tGIT_WORK_TREE=\"$PWD/ours-has-rename-work\" &&\n> \t\texport GIT_WORK_TREE &&\n> \t\tGIT_INDEX_FILE=\"$PWD/ours-has-rename-index\" &&\n> @@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '\n> \n> test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '\n> \t(\n> +\t\tGIT_TEST_FSMONITOR=\"\" &&\n> \t\tGIT_WORK_TREE=\"$PWD/theirs-has-rename-work\" &&\n> \t\texport GIT_WORK_TREE &&\n> \t\tGIT_INDEX_FILE=\"$PWD/theirs-has-rename-index\" &&\n> -- \n> gitgitgadget\n> \n"},{"id":"387877","messageId":"20191210101351.GE6527@szeder.dev","threadId":"52306","inReplyTo":"a5b0bf6ac7cd28fa47e26ab481f781d74c656f6a.1575907804.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/8] tests: disable fsmonitor in submodule tests","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T10:13:51Z","receivedAt":"2019-12-10T10:13:58Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n> \n> The fsmonitor feature allows an external tool such as watchman to\n> monitor the working directory. The direct test\n> t7619-status-fsmonitor.sh provides some coverage, but it would be\n> better to run the entire test suite with watchman enabled. This\n> would provide more confidence that the feature is working as\n> intended.\n> \n> The fsmonitor feature struggles with submodules. Disable the\n> GIT_TEST_FSMONITOR environment variable before running tests with\n> a lot of submodule interactions.\n> \n> Signed-off-by: Derrick Stolee \n> ---\n> t/t3404-rebase-interactive.sh | 1 +\n> t/t3600-rm.sh | 1 +\n> t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n> t/t5526-fetch-submodules.sh | 2 ++\n> t/t7402-submodule-rebase.sh | 3 +++\n> t/t7406-submodule-update.sh | 2 ++\n> t/t7506-status-submodule.sh | 3 +++\n> t/t7508-status.sh | 3 +++\n> 8 files changed, 18 insertions(+)\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 461dd539ff..9dc7d1aefb 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -697,6 +697,7 @@ test_expect_success 'do \"noop\" when there is nothing to cherry-pick' '\n> '\n> \n> test_expect_success 'submodule rebase setup' '\n> +\tGIT_TEST_FSMONITOR=\"\" &&\n\nThis disables GIT_TEST_FSMONITOR for the remainder of the test script,\nbut there are still a lot of non-submodule-specific tests to run.\n\n> \tgit checkout A &&\n> \tmkdir sub &&\n> \t(\n> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\n> index 66282a720e..64269bd89d 100755\n> --- a/t/t3600-rm.sh\n> +++ b/t/t3600-rm.sh\n> @@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' '\n> '\n> \n> test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' '\n> +\tGIT_TEST_FSMONITOR=\"\" &&\n\nLikewise.\n\n> \tgit reset --hard &&\n> \tgit submodule update &&\n> \tgit -C submod checkout HEAD^ &&\n> diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh\n> index 9dcb69df5c..017417790e 100755\n> --- a/t/t4060-diff-submodule-option-diff-format.sh\n> +++ b/t/t4060-diff-submodule-option-diff-format.sh\n> @@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff.\n> # Tested non-UTF-8 encoding\n> test_encoding=\"ISO8859-1\"\n> \n> +# fsmonitor does not work well with submodules\n> +GIT_TEST_FSMONITOR=\"\"\n> +\n> # String \"added\" in German (translated with Google Translate), encoded in UTF-8,\n> # used in sample commit log messages in add_file() function below.\n> added=$(printf \"hinzugef\\303\\274gt\")\n> diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh\n> index 63205dfdf9..fb346bff05 100755\n> --- a/t/t5526-fetch-submodules.sh\n> +++ b/t/t5526-fetch-submodules.sh\n> @@ -1,6 +1,8 @@\n> #!/bin/sh\n> # Copyright (c) 2010, Jens Lehmann\n> \n> +GIT_TEST_FSMONITOR=\"\"\n> +\n> test_description='Recursive \"git fetch\" for submodules'\n> \n> . ./test-lib.sh\n> diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh\n> index 8e32f19007..c78e9009cf 100755\n> --- a/t/t7402-submodule-rebase.sh\n> +++ b/t/t7402-submodule-rebase.sh\n> @@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules'\n> \n> . ./test-lib.sh\n> \n> +# fsmonitor does not work well with submodules\n> +GIT_TEST_FSMONITOR=\"\"\n> +\n> test_expect_success setup '\n> \n> \techo file > file &&\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index c973278300..8d93aaef5f 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -11,6 +11,8 @@ submodule and \"git submodule update --rebase/--merge\" does not detach the HEAD.\n> \n> . ./test-lib.sh\n> \n> +# fsmonitor does not work well with submodules\n> +GIT_TEST_FSMONITOR=\"\"\n> \n> compare_head()\n> {\n> diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh\n> index 08629a6e70..1a716f2c2a 100755\n> --- a/t/t7506-status-submodule.sh\n> +++ b/t/t7506-status-submodule.sh\n> @@ -4,6 +4,9 @@ test_description='git status for submodule'\n> \n> . ./test-lib.sh\n> \n> +# fsmonitor does not work well with submodules\n> +GIT_TEST_FSMONITOR=\"\"\n> +\n> test_create_repo_with_commit () {\n> \ttest_create_repo \"$1\" &&\n> \t(\n> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n> index 4e676cdce8..bf0487632d 100755\n> --- a/t/t7508-status.sh\n> +++ b/t/t7508-status.sh\n> @@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '\n> \ttest_cmp expect output\n> '\n> \n> +# fsmonitor does not work well with submodules\n> +GIT_TEST_FSMONITOR=\"\"\n> +\n\nLikewise.\n\n> test_expect_success 'setup status submodule summary' '\n> \ttest_create_repo sm && (\n> \t\tcd sm &&\n> -- \n> gitgitgadget\n> \n"},{"id":"387886","messageId":"b3e8ad07-b2cb-e024-405e-27d9f065f5fc@gmail.com","threadId":"52306","inReplyTo":"20191210100732.GD6527@szeder.dev","subject":"Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-10T13:45:27Z","receivedAt":"2019-12-10T13:45:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/10/2019 5:07 AM, SZEDER Gábor wrote:\n> On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The fsmonitor feature allows an external tool such as watchman to\n>> monitor the working directory. The direct test\n>> t7619-status-fsmonitor.sh provides some coverage, but it would be\n>> better to run the entire test suite with watchman enabled. This\n>> would provide more confidence that the feature is working as\n>> intended.\n>>\n>> Worktrees use a \".git\" _file_ instead of a folder to point to\n>> the base repo's .git directory and the proper worktree HEAD. The\n>> fsmonitor hook tries to create a JSON file inside the \".git\" folder\n>> which violates the expectation here.\n> \n> Yeah, there are a couple hardcoded paths in there, e.g.:\n> \n> open ($fh, \">\", \".git/watchman-response.json\");\n> \n> and, worse, not only in the test helper hook in\n> 't/t7519/fsmonitor-watchman' but in the sample hook template\n> 'templates/hooks--fsmonitor-watchman.sample' as well.\n> \n>> It would be better to properly\n>> find a safe folder for storing this JSON file.\n> \n> git rev-parse --git-path ''\n> \n> gives us the right directory prefix to use and we could then append\n> the various filenames that must be accessed in there.\n\nAdding another git process inside the hook is hopefully not\nthe only way to achieve something like this. The performance\nhit (mostly on Windows) would be a non-starter for me. (Yes,\nthe process creation to watchman is already a cost here, but\nlet's not make it worse.)\n\nPerhaps a better strategy would be to do something in-memory\ninstead of writing to a file. Not sure how much of that can\nbe done in the script.\n\n-Stolee\n"},{"id":"387887","messageId":"dd8c14d3-21b6-731c-bd50-650d063d686f@gmail.com","threadId":"52306","inReplyTo":"xmqqo8wgsqnn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-10T13:51:16Z","receivedAt":"2019-12-10T13:51:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"\n\nOn 12/10/2019 12:20 AM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> Hm. That is a good point. Can we assume that our version of grep has\n>> a \"-F\" or \"--fixed-strings\" option? ([1] seems to say that \"-F\" would\n>> work.)\n> \n> $ git grep \"grep -F\" -- \\*.sh\n> \n> is your friend ;-) \n\nYes, of course I should have just looked for examples.\n\n> And never use https://www.gnu.org/ manual as a yardstick---you will\n> end up using GNUism that is not unavailable elsewhere pretty easily.\n\nI tried to focus on the part that said \"this is part of POSIX\", but\nyou are right that may not be the best place to look.\n\n>> [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs\n>>\n>>> What are these stripping of \", and \" about? Could you tell readers\n>>> how a typical output from the program we are reading from looks like\n>>> perhaps in the log message or in-code comment around here?\n>>\n>> Watchman outputs its list of paths in JSON format. Luckily, it formats\n>> the output so the path lines are on separate lines, each quoted.\n>>\n>> For example:\n>>\n>> {\n>> \t\"version\": \"4.9.0\",\n>> \t\"roots\": [\n>> \t\t\"\",\n>> \t\t\"\",\n>> \t\t\"\"\n>> \t]\n>> }\n> \n> Yeek; how is a dq in path represented? by doubling? by\n> backslash-quoting (if so how is a backslash in path represented)?\n> By something else?\n> \n> It's OK at least for now to declare that our test repository does\n> not contain any funny paths, but in the longer run does the above\n> mean that we somehow need to be able to grok JSON reliably in our\n> tests? It may not be such a bad thing especially for longer term,\n> as there are other parts of the system that may benefit from having\n> JSON capable output readers in our tests (e.g. trace2 code can do\n> JSON, right?)..\n\ntrace2 can _write_ JSON, not parse it. However, we have some parsing\ncode (using a package) in the performance tests. I could try\nadapting that for this purpose. That package is not currently required\nby the test suite, so it causes some dependency issues when first\nrunning the perf suite. At least we wouldn't need the package\nunless running with GIT_TEST_FSMONITOR.\n\nMy guess is that this patch is going to be trouble, so I'll eject\nit in the next version and save the JSON parsing and everything\nfor its own series. We only really need it when we are getting\nclose to running watchman in CI on Windows.\n\nThanks,\n-Stolee\n"},{"id":"387888","messageId":"1fa28a9c-2a4d-fbcb-653b-99a567dbc7da@gmail.com","threadId":"52306","inReplyTo":"20191210101351.GE6527@szeder.dev","subject":"Re: [PATCH v2 5/8] tests: disable fsmonitor in submodule tests","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-12-10T13:57:25Z","receivedAt":"2019-12-10T13:57:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/10/2019 5:13 AM, SZEDER Gábor wrote:\n> On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote:\n>> From: Derrick Stolee \n>>\n>> The fsmonitor feature allows an external tool such as watchman to\n>> monitor the working directory. The direct test\n>> t7619-status-fsmonitor.sh provides some coverage, but it would be\n>> better to run the entire test suite with watchman enabled. This\n>> would provide more confidence that the feature is working as\n>> intended.\n>>\n>> The fsmonitor feature struggles with submodules. Disable the\n>> GIT_TEST_FSMONITOR environment variable before running tests with\n>> a lot of submodule interactions.\n>>\n>> Signed-off-by: Derrick Stolee \n>> ---\n>> t/t3404-rebase-interactive.sh | 1 +\n>> t/t3600-rm.sh | 1 +\n>> t/t4060-diff-submodule-option-diff-format.sh | 3 +++\n>> t/t5526-fetch-submodules.sh | 2 ++\n>> t/t7402-submodule-rebase.sh | 3 +++\n>> t/t7406-submodule-update.sh | 2 ++\n>> t/t7506-status-submodule.sh | 3 +++\n>> t/t7508-status.sh | 3 +++\n>> 8 files changed, 18 insertions(+)\n>>\n>> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n>> index 461dd539ff..9dc7d1aefb 100755\n>> --- a/t/t3404-rebase-interactive.sh\n>> +++ b/t/t3404-rebase-interactive.sh\n>> @@ -697,6 +697,7 @@ test_expect_success 'do \"noop\" when there is nothing to cherry-pick' '\n>> '\n>> \n>> test_expect_success 'submodule rebase setup' '\n>> +\tGIT_TEST_FSMONITOR=\"\" &&\n> \n> This disables GIT_TEST_FSMONITOR for the remainder of the test script,\n> but there are still a lot of non-submodule-specific tests to run.\n...\n>> diff --git a/t/t7508-status.sh b/t/t7508-status.sh\n>> index 4e676cdce8..bf0487632d 100755\n>> --- a/t/t7508-status.sh\n>> +++ b/t/t7508-status.sh\n>> @@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' '\n>> \ttest_cmp expect output\n>> '\n>> \n>> +# fsmonitor does not work well with submodules\n>> +GIT_TEST_FSMONITOR=\"\"\n>> +\n> \n> Likewise.\n\nWould it make sense to wrap the tests in a subshell without\nincreasing the tabbing inside the subshell? I can comment\nthe beginning of the subshell that we are disabling the\nvariable for a specific list of tests and the subshell can\nbe removed after the proper fixes make it work.\n\nThanks,\n-Stolee\n"},{"id":"387889","messageId":"nycvar.QRO.7.76.6.1912101457200.31080@tvgsbejvaqbjf.bet","threadId":"52306","inReplyTo":"xmqqo8wgsqnn.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 8/8] test-lib: clear watchman watches at test completion","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-12-10T14:09:02Z","receivedAt":"2019-12-10T14:09:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 9 Dec 2019, Junio C Hamano wrote:\n\n> Derrick Stolee writes:\n>\n> > Hm. That is a good point. Can we assume that our version of grep has\n> > a \"-F\" or \"--fixed-strings\" option? ([1] seems to say that \"-F\" would\n> > work.)\n>\n> $ git grep \"grep -F\" -- \\*.sh\n>\n> is your friend ;-)\n>\n> And never use https://www.gnu.org/ manual as a yardstick---you will\n> end up using GNUism that is not unavailable elsewhere pretty easily.\n>\n> > [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs\n\nI often look at GNU grep's man page first and then verify via\nhttps://man.openbsd.org/grep and\nhttps://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html\nthat the option can be considered portable.\n\n> >> What are these stripping of \", and \" about? Could you tell readers\n> >> how a typical output from the program we are reading from looks like\n> >> perhaps in the log message or in-code comment around here?\n> >\n> > Watchman outputs its list of paths in JSON format. Luckily, it formats\n> > the output so the path lines are on separate lines, each quoted.\n> >\n> > For example:\n> >\n> > {\n> > \t\"version\": \"4.9.0\",\n> > \t\"roots\": [\n> > \t\t\"\",\n> > \t\t\"\",\n> > \t\t\"\"\n> > \t]\n> > }\n>\n> Yeek; how is a dq in path represented? by doubling? by\n> backslash-quoting (if so how is a backslash in path represented)?\n> By something else?\n>\n> It's OK at least for now to declare that our test repository does\n> not contain any funny paths, but in the longer run does the above\n> mean that we somehow need to be able to grok JSON reliably in our\n> tests? It may not be such a bad thing especially for longer term,\n> as there are other parts of the system that may benefit from having\n> JSON capable output readers in our tests (e.g. trace2 code can do\n> JSON, right?)..\n\nFrom\nhttps://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf\n(section \"9 String\"):\n\nA string is a sequence of Unicode code points wrapped with quotation marks\n(U+0022). All code points may be placed within the quotation marks\nexcept for the code points that must be escaped: quotation mark\n(U+0022), reverse solidus (U+005C), and the control characters U+0000 to\nU+001F. There are two-character escape sequence representations of some\ncharacters.\n\n\t\\\"\trepresents the quotation mark character (U+0022).\n\t\\\\\trepresents the reverse solidus character(U+005C).\n\t\\/\trepresents the solidus character (U+002F).\n\t\\b\trepresents the backspace character(U+0008).\n\t\\f\trepresents the form feed character (U+000C).\n\t\\n\trepresents the line feed character (U+000A).\n\t\\r\trepresents the carriage return character (U+000D).\n\t\\t\trepresents the character tabulation character (U+0009).\n\n(Side note: It is amazing what things you learn unexpectedly, e.g. when\nresearching information about the JSON format, you learn that about the\nword \"solidus\", that it refers to the slash, and that it was once also\nknow as the \"shilling mark\"...)\n\nI am not sure why the forward slash needs to be escaped, but I guess that\nthis is voluntary rather than mandatory.\n\nCiao,\nDscho\n"},{"id":"387892","messageId":"20191210145743.GG6527@szeder.dev","threadId":"52306","inReplyTo":"b3e8ad07-b2cb-e024-405e-27d9f065f5fc@gmail.com","subject":"Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T14:57:43Z","receivedAt":"2019-12-10T14:57:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:\n> On 12/10/2019 5:07 AM, SZEDER Gábor wrote:\n> > On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:\n> >> From: Derrick Stolee \n> >>\n> >> The fsmonitor feature allows an external tool such as watchman to\n> >> monitor the working directory. The direct test\n> >> t7619-status-fsmonitor.sh provides some coverage, but it would be\n> >> better to run the entire test suite with watchman enabled. This\n> >> would provide more confidence that the feature is working as\n> >> intended.\n> >>\n> >> Worktrees use a \".git\" _file_ instead of a folder to point to\n> >> the base repo's .git directory and the proper worktree HEAD. The\n> >> fsmonitor hook tries to create a JSON file inside the \".git\" folder\n> >> which violates the expectation here.\n> > \n> > Yeah, there are a couple hardcoded paths in there, e.g.:\n> > \n> > open ($fh, \">\", \".git/watchman-response.json\");\n> > \n> > and, worse, not only in the test helper hook in\n> > 't/t7519/fsmonitor-watchman' but in the sample hook template\n> > 'templates/hooks--fsmonitor-watchman.sample' as well.\n> > \n> >> It would be better to properly\n> >> find a safe folder for storing this JSON file.\n> > \n> > git rev-parse --git-path ''\n> > \n> > gives us the right directory prefix to use and we could then append\n> > the various filenames that must be accessed in there.\n> \n> Adding another git process inside the hook is hopefully not\n> the only way to achieve something like this. The performance\n> hit (mostly on Windows) would be a non-starter for me. (Yes,\n> the process creation to watchman is already a cost here, but\n> let's not make it worse.)\n\nHrm, _when_ is the 'fsmonitor-watchman' hook invoked?! Every time a\ngit process tries to figure out what files have changed since e.g. the\nindex was written? For running an fsmonitor/watchman-enabled CI build\nit might be an acceptable compromise until we come up with something\nmore clever. 'man githooks' is not clear on this at all, it only says\nthat \"This hook is invoked when the configuration option\ncore.fsmonitor is set to .git/hooks/fsmonitor-watchman\".\n\n> Perhaps a better strategy would be to do something in-memory\n> instead of writing to a file. Not sure how much of that can\n> be done in the script.\n> \n> -Stolee\n"},{"id":"387893","messageId":"20191210150745.GH6527@szeder.dev","threadId":"52306","inReplyTo":"b3e8ad07-b2cb-e024-405e-27d9f065f5fc@gmail.com","subject":"Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-12-10T15:07:45Z","receivedAt":"2019-12-10T15:07:51Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:\n> >> Worktrees use a \".git\" _file_ instead of a folder to point to\n> >> the base repo's .git directory and the proper worktree HEAD. The\n> >> fsmonitor hook tries to create a JSON file inside the \".git\" folder\n> >> which violates the expectation here.\n> > \n> > Yeah, there are a couple hardcoded paths in there, e.g.:\n> > \n> > open ($fh, \">\", \".git/watchman-response.json\");\n> > \n> > and, worse, not only in the test helper hook in\n> > 't/t7519/fsmonitor-watchman' but in the sample hook template\n> > 'templates/hooks--fsmonitor-watchman.sample' as well.\n> > \n> >> It would be better to properly\n> >> find a safe folder for storing this JSON file.\n> > \n> > git rev-parse --git-path ''\n> > \n> > gives us the right directory prefix to use and we could then append\n> > the various filenames that must be accessed in there.\n> \n> Adding another git process inside the hook is hopefully not\n> the only way to achieve something like this. The performance\n> hit (mostly on Windows) would be a non-starter for me.\n\nOh, hang on, it seems that we could simply use $GIT_DIR.\n\nI added\n\n echo >&2 \"GIT_DIR in the fsmonitor hook: '$GIT_DIR'\"\n\nto 't/t7519/fsmonitor-all', and then run the test:\n\n test_expect_success 'test' '\n echo 1 >file &&\n git add file &&\n git commit -m first &&\n \n git worktree add --detach WT &&\n cd WT &&\n echo 2 >file &&\n git add -u\n '\n\nwith 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all', and in the\nverbose output got lines like:\n\n GIT_DIR in the fsmonitor hook: ''\n GIT_DIR in the fsmonitor hook: ''\n GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT'\n GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT'\n\nI'm not sure why $GIT_DIR is not exported to the hook script while in\nthe main working tree. Anyway, as it is now, if $GIT_DIR is\nunset/empty, then the hook should write to \".git/\", and if\nit is set, then to \"$GIT_DIR/\", so no git process is needed\nin the hook, only a getenv() and a condition.\n\n"},{"id":"390278","messageId":"73ca58e8-bc04-5607-c21e-e7cfacd8df37@gmail.com","threadId":"52306","inReplyTo":"20191210150745.GH6527@szeder.dev","subject":"Re: [PATCH v2 4/8] t3030-merge-recursive.sh: disable fsmonitor when tweaking GIT_WORK_TREE","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-01-23T15:45:32Z","receivedAt":"2020-01-23T15:45:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 12/10/2019 10:07 AM, SZEDER Gábor wrote:\n> On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote:\n>>>> Worktrees use a \".git\" _file_ instead of a folder to point to\n>>>> the base repo's .git directory and the proper worktree HEAD. The\n>>>> fsmonitor hook tries to create a JSON file inside the \".git\" folder\n>>>> which violates the expectation here.\n>>>\n>>> Yeah, there are a couple hardcoded paths in there, e.g.:\n>>>\n>>> open ($fh, \">\", \".git/watchman-response.json\");\n>>>\n>>> and, worse, not only in the test helper hook in\n>>> 't/t7519/fsmonitor-watchman' but in the sample hook template\n>>> 'templates/hooks--fsmonitor-watchman.sample' as well.\n>>>\n>>>> It would be better to properly\n>>>> find a safe folder for storing this JSON file.\n>>>\n>>> git rev-parse --git-path ''\n>>>\n>>> gives us the right directory prefix to use and we could then append\n>>> the various filenames that must be accessed in there.\n>>\n>> Adding another git process inside the hook is hopefully not\n>> the only way to achieve something like this. The performance\n>> hit (mostly on Windows) would be a non-starter for me.\n> \n> Oh, hang on, it seems that we could simply use $GIT_DIR.\n> \n> I added\n> \n> echo >&2 \"GIT_DIR in the fsmonitor hook: '$GIT_DIR'\"\n> \n> to 't/t7519/fsmonitor-all', and then run the test:\n> \n> test_expect_success 'test' '\n> echo 1 >file &&\n> git add file &&\n> git commit -m first &&\n> \n> git worktree add --detach WT &&\n> cd WT &&\n> echo 2 >file &&\n> git add -u\n> '\n> \n> with 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all', and in the\n> verbose output got lines like:\n> \n> GIT_DIR in the fsmonitor hook: ''\n> GIT_DIR in the fsmonitor hook: ''\n> GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT'\n> GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT'\n> \n> I'm not sure why $GIT_DIR is not exported to the hook script while in\n> the main working tree. Anyway, as it is now, if $GIT_DIR is\n> unset/empty, then the hook should write to \".git/\", and if\n> it is set, then to \"$GIT_DIR/\", so no git process is needed\n> in the hook, only a getenv() and a condition.\n\nThanks for this. It helps that also the test hooks were using the\n.git directory only for debug information, and that was commented-out\nin the v2 version of the hook.\n\nThanks,\n-Stolee\n\n"}]}