Improve testability with GIT_TEST_FSMONITOR #466

gitgitgadget · 2019-11-21T23:23:46Z

On the Git mailing list, Denton Liu wrote (reply to this):

Hi Stolee, On Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > t0003-attributes.sh Patches 1 and 2 have this line in the commit message. I assume it's a typo? Also, for the patches with empty commit message bodies, it would be nice if they were filled in with detail for _why_ the change is being made. For example, in this patch, a future reader might wonder why having fsmonitor enabled in a bare repo causes problems. Thanks, Denton > > Signed-off-by: Derrick Stolee <[email protected]> > --- > config.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/config.c b/config.c > index 3900e4947b..f6d4e2fae3 100644 > --- a/config.c > +++ b/config.c > @@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void) > > int git_config_get_fsmonitor(void) > { > + if (!the_repository->worktree) { > + core_fsmonitor = 0; > + return 0; > + } > + > if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor)) > core_fsmonitor = getenv("GIT_TEST_FSMONITOR"); > > -- > gitgitgadget >

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/2019 6:18 PM, Denton Liu wrote: > Hi Stolee, > > On Thu, Nov 21, 2019 at 10:20:16PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> t0003-attributes.sh > > Patches 1 and 2 have this line in the commit message. I assume it's a > typo? > > Also, for the patches with empty commit message bodies, it would be nice > if they were filled in with detail for _why_ the change is being made. > For example, in this patch, a future reader might wonder why having > fsmonitor enabled in a bare repo causes problems. Thanks for pointing that out. I forgot to clean up these messages, and what I left in them right now is sometimes just the first test script that was failing without the change. Thanks, -Stolee

gitgitgadget · 2019-12-10T09:47:05Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Dec 09, 2019 at 04:09:57PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fsmonitor feature allows an external tool such as watchman to > monitor the working directory. The direct test > t7619-status-fsmonitor.sh provides some coverage, but it would be s/t7619/t7519/ This typo is present in all but one other commit messages.

gitgitgadget · 2019-12-10T09:47:03Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Dec 09, 2019 at 04:09:59PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fsmonitor feature allows an external tool such as watchman to > monitor the working directory. The direct test > t7619-status-fsmonitor.sh provides some coverage, but it would be > better to run the entire test suite with watchman enabled. This > would provide more confidence that the feature is working as > intended. > > The test t1301-shared-repo.sh would fail when GIT_TEST_FSMONITOR > is set to t/t7519/fsmonitor-watchman because it changes permissions > in an incompatible way. I don't understand this, and would like it to be more specific, i.e. which particular permission changes in which tests are incompatible with watchman. > Signed-off-by: Derrick Stolee <[email protected]> > --- > t/t1301-shared-repo.sh | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh > index 2dc853d1be..665ade0cf2 100755 > --- a/t/t1301-shared-repo.sh > +++ b/t/t1301-shared-repo.sh > @@ -128,6 +128,7 @@ test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' ' > ' > > test_expect_success POSIXPERM 'forced modes' ' > + GIT_TEST_FSMONITOR="" && This is not in a subshell, so it disables GIT_TEST_FSMONITOR not only for the test it is executed in but for the remainder of the test script. From the commit message I can't decide whether that's intended. > mkdir -p templates/hooks && > echo update-server-info >templates/hooks/post-update && > chmod +x templates/hooks/post-update && > -- > gitgitgadget >

gitgitgadget · 2019-12-10T10:11:02Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fsmonitor feature allows an external tool such as watchman to > monitor the working directory. The direct test > t7619-status-fsmonitor.sh provides some coverage, but it would be > better to run the entire test suite with watchman enabled. This > would provide more confidence that the feature is working as > intended. > > Worktrees use a ".git" _file_ instead of a folder to point to > the base repo's .git directory and the proper worktree HEAD. The > fsmonitor hook tries to create a JSON file inside the ".git" folder > which violates the expectation here. Yeah, there are a couple hardcoded paths in there, e.g.: open ($fh, ">", ".git/watchman-response.json"); and, worse, not only in the test helper hook in 't/t7519/fsmonitor-watchman' but in the sample hook template 'templates/hooks--fsmonitor-watchman.sample' as well. > It would be better to properly > find a safe folder for storing this JSON file. git rev-parse --git-path '' gives us the right directory prefix to use and we could then append the various filenames that must be accessed in there. > This is also a problem when a test script uses GIT_WORK_TREE. > > Signed-off-by: Derrick Stolee <[email protected]> > --- > t/t1510-repo-setup.sh | 1 + > t/t2400-worktree-add.sh | 2 ++ > t/t3030-merge-recursive.sh | 2 ++ > 3 files changed, 5 insertions(+) > > diff --git a/t/t1510-repo-setup.sh b/t/t1510-repo-setup.sh > index 9974457f56..28dce0c26f 100755 > --- a/t/t1510-repo-setup.sh > +++ b/t/t1510-repo-setup.sh > @@ -775,6 +775,7 @@ test_expect_success '#29: setup' ' > setup_repo 29 non-existent gitfile true && > mkdir -p 29/sub/sub 29/wt/sub && > ( > + GIT_TEST_FSMONITOR="" && > cd 29 && > GIT_WORK_TREE="$here/29" && > export GIT_WORK_TREE && > diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh > index e819ba741e..d4d3cbae0f 100755 > --- a/t/t2400-worktree-add.sh > +++ b/t/t2400-worktree-add.sh > @@ -1,5 +1,7 @@ > #!/bin/sh > > +GIT_TEST_FSMONITOR="" > + > test_description='test git worktree add' > > . ./test-lib.sh > diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh > index ff641b348a..62f645d639 100755 > --- a/t/t3030-merge-recursive.sh > +++ b/t/t3030-merge-recursive.sh > @@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' ' > > test_expect_success 'merge-recursive w/ empty work tree - ours has rename' ' > ( > + GIT_TEST_FSMONITOR="" && > GIT_WORK_TREE="$PWD/ours-has-rename-work" && > export GIT_WORK_TREE && > GIT_INDEX_FILE="$PWD/ours-has-rename-index" && > @@ -545,6 +546,7 @@ test_expect_success 'merge-recursive w/ empty work tree - ours has rename' ' > > test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' ' > ( > + GIT_TEST_FSMONITOR="" && > GIT_WORK_TREE="$PWD/theirs-has-rename-work" && > export GIT_WORK_TREE && > GIT_INDEX_FILE="$PWD/theirs-has-rename-index" && > -- > gitgitgadget >

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/10/2019 5:07 AM, SZEDER Gábor wrote: > On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> The fsmonitor feature allows an external tool such as watchman to >> monitor the working directory. The direct test >> t7619-status-fsmonitor.sh provides some coverage, but it would be >> better to run the entire test suite with watchman enabled. This >> would provide more confidence that the feature is working as >> intended. >> >> Worktrees use a ".git" _file_ instead of a folder to point to >> the base repo's .git directory and the proper worktree HEAD. The >> fsmonitor hook tries to create a JSON file inside the ".git" folder >> which violates the expectation here. > > Yeah, there are a couple hardcoded paths in there, e.g.: > > open ($fh, ">", ".git/watchman-response.json"); > > and, worse, not only in the test helper hook in > 't/t7519/fsmonitor-watchman' but in the sample hook template > 'templates/hooks--fsmonitor-watchman.sample' as well. > >> It would be better to properly >> find a safe folder for storing this JSON file. > > git rev-parse --git-path '' > > gives us the right directory prefix to use and we could then append > the various filenames that must be accessed in there. Adding another git process inside the hook is hopefully not the only way to achieve something like this. The performance hit (mostly on Windows) would be a non-starter for me. (Yes, the process creation to watchman is already a cost here, but let's not make it worse.) Perhaps a better strategy would be to do something in-memory instead of writing to a file. Not sure how much of that can be done in the script. -Stolee

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote: > On 12/10/2019 5:07 AM, SZEDER Gábor wrote: > > On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <[email protected]> > >> > >> The fsmonitor feature allows an external tool such as watchman to > >> monitor the working directory. The direct test > >> t7619-status-fsmonitor.sh provides some coverage, but it would be > >> better to run the entire test suite with watchman enabled. This > >> would provide more confidence that the feature is working as > >> intended. > >> > >> Worktrees use a ".git" _file_ instead of a folder to point to > >> the base repo's .git directory and the proper worktree HEAD. The > >> fsmonitor hook tries to create a JSON file inside the ".git" folder > >> which violates the expectation here. > > > > Yeah, there are a couple hardcoded paths in there, e.g.: > > > > open ($fh, ">", ".git/watchman-response.json"); > > > > and, worse, not only in the test helper hook in > > 't/t7519/fsmonitor-watchman' but in the sample hook template > > 'templates/hooks--fsmonitor-watchman.sample' as well. > > > >> It would be better to properly > >> find a safe folder for storing this JSON file. > > > > git rev-parse --git-path '' > > > > gives us the right directory prefix to use and we could then append > > the various filenames that must be accessed in there. > > Adding another git process inside the hook is hopefully not > the only way to achieve something like this. The performance > hit (mostly on Windows) would be a non-starter for me. (Yes, > the process creation to watchman is already a cost here, but > let's not make it worse.) Hrm, _when_ is the 'fsmonitor-watchman' hook invoked?! Every time a git process tries to figure out what files have changed since e.g. the index was written? For running an fsmonitor/watchman-enabled CI build it might be an acceptable compromise until we come up with something more clever. 'man githooks' is not clear on this at all, it only says that "This hook is invoked when the configuration option core.fsmonitor is set to .git/hooks/fsmonitor-watchman". > Perhaps a better strategy would be to do something in-memory > instead of writing to a file. Not sure how much of that can > be done in the script. > > -Stolee

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote: > >> Worktrees use a ".git" _file_ instead of a folder to point to > >> the base repo's .git directory and the proper worktree HEAD. The > >> fsmonitor hook tries to create a JSON file inside the ".git" folder > >> which violates the expectation here. > > > > Yeah, there are a couple hardcoded paths in there, e.g.: > > > > open ($fh, ">", ".git/watchman-response.json"); > > > > and, worse, not only in the test helper hook in > > 't/t7519/fsmonitor-watchman' but in the sample hook template > > 'templates/hooks--fsmonitor-watchman.sample' as well. > > > >> It would be better to properly > >> find a safe folder for storing this JSON file. > > > > git rev-parse --git-path '' > > > > gives us the right directory prefix to use and we could then append > > the various filenames that must be accessed in there. > > Adding another git process inside the hook is hopefully not > the only way to achieve something like this. The performance > hit (mostly on Windows) would be a non-starter for me. Oh, hang on, it seems that we could simply use $GIT_DIR. I added echo >&2 "GIT_DIR in the fsmonitor hook: '$GIT_DIR'" to 't/t7519/fsmonitor-all', and then run the test: test_expect_success 'test' ' echo 1 >file && git add file && git commit -m first && git worktree add --detach WT && cd WT && echo 2 >file && git add -u ' with 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all', and in the verbose output got lines like: GIT_DIR in the fsmonitor hook: '' GIT_DIR in the fsmonitor hook: '' GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT' GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT' I'm not sure why $GIT_DIR is not exported to the hook script while in the main working tree. Anyway, as it is now, if $GIT_DIR is unset/empty, then the hook should write to ".git/<whatever>", and if it is set, then to "$GIT_DIR/<whatever>", so no git process is needed in the hook, only a getenv() and a condition.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/10/2019 10:07 AM, SZEDER Gábor wrote: > On Tue, Dec 10, 2019 at 08:45:27AM -0500, Derrick Stolee wrote: >>>> Worktrees use a ".git" _file_ instead of a folder to point to >>>> the base repo's .git directory and the proper worktree HEAD. The >>>> fsmonitor hook tries to create a JSON file inside the ".git" folder >>>> which violates the expectation here. >>> >>> Yeah, there are a couple hardcoded paths in there, e.g.: >>> >>> open ($fh, ">", ".git/watchman-response.json"); >>> >>> and, worse, not only in the test helper hook in >>> 't/t7519/fsmonitor-watchman' but in the sample hook template >>> 'templates/hooks--fsmonitor-watchman.sample' as well. >>> >>>> It would be better to properly >>>> find a safe folder for storing this JSON file. >>> >>> git rev-parse --git-path '' >>> >>> gives us the right directory prefix to use and we could then append >>> the various filenames that must be accessed in there. >> >> Adding another git process inside the hook is hopefully not >> the only way to achieve something like this. The performance >> hit (mostly on Windows) would be a non-starter for me. > > Oh, hang on, it seems that we could simply use $GIT_DIR. > > I added > > echo >&2 "GIT_DIR in the fsmonitor hook: '$GIT_DIR'" > > to 't/t7519/fsmonitor-all', and then run the test: > > test_expect_success 'test' ' > echo 1 >file && > git add file && > git commit -m first && > > git worktree add --detach WT && > cd WT && > echo 2 >file && > git add -u > ' > > with 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all', and in the > verbose output got lines like: > > GIT_DIR in the fsmonitor hook: '' > GIT_DIR in the fsmonitor hook: '' > GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT' > GIT_DIR in the fsmonitor hook: '/home/szeder/src/git/t/trash directory.t9999-test/.git/worktrees/WT' > > I'm not sure why $GIT_DIR is not exported to the hook script while in > the main working tree. Anyway, as it is now, if $GIT_DIR is > unset/empty, then the hook should write to ".git/<whatever>", and if > it is set, then to "$GIT_DIR/<whatever>", so no git process is needed > in the hook, only a getenv() and a condition. Thanks for this. It helps that also the test hooks were using the .git directory only for debug information, and that was commented-out in the v2 version of the hook. Thanks, -Stolee

gitgitgadget · 2019-12-10T10:17:05Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > The fsmonitor feature allows an external tool such as watchman to > monitor the working directory. The direct test > t7619-status-fsmonitor.sh provides some coverage, but it would be > better to run the entire test suite with watchman enabled. This > would provide more confidence that the feature is working as > intended. > > The fsmonitor feature struggles with submodules. Disable the > GIT_TEST_FSMONITOR environment variable before running tests with > a lot of submodule interactions. > > Signed-off-by: Derrick Stolee <[email protected]> > --- > t/t3404-rebase-interactive.sh | 1 + > t/t3600-rm.sh | 1 + > t/t4060-diff-submodule-option-diff-format.sh | 3 +++ > t/t5526-fetch-submodules.sh | 2 ++ > t/t7402-submodule-rebase.sh | 3 +++ > t/t7406-submodule-update.sh | 2 ++ > t/t7506-status-submodule.sh | 3 +++ > t/t7508-status.sh | 3 +++ > 8 files changed, 18 insertions(+) > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 461dd539ff..9dc7d1aefb 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -697,6 +697,7 @@ test_expect_success 'do "noop" when there is nothing to cherry-pick' ' > ' > > test_expect_success 'submodule rebase setup' ' > + GIT_TEST_FSMONITOR="" && This disables GIT_TEST_FSMONITOR for the remainder of the test script, but there are still a lot of non-submodule-specific tests to run. > git checkout A && > mkdir sub && > ( > diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh > index 66282a720e..64269bd89d 100755 > --- a/t/t3600-rm.sh > +++ b/t/t3600-rm.sh > @@ -355,6 +355,7 @@ test_expect_success 'rm succeeds when given a directory with a trailing /' ' > ' > > test_expect_success 'rm of a populated submodule with different HEAD fails unless forced' ' > + GIT_TEST_FSMONITOR="" && Likewise. > git reset --hard && > git submodule update && > git -C submod checkout HEAD^ && > diff --git a/t/t4060-diff-submodule-option-diff-format.sh b/t/t4060-diff-submodule-option-diff-format.sh > index 9dcb69df5c..017417790e 100755 > --- a/t/t4060-diff-submodule-option-diff-format.sh > +++ b/t/t4060-diff-submodule-option-diff-format.sh > @@ -15,6 +15,9 @@ This test tries to verify the sanity of --submodule=diff option of git diff. > # Tested non-UTF-8 encoding > test_encoding="ISO8859-1" > > +# fsmonitor does not work well with submodules > +GIT_TEST_FSMONITOR="" > + > # String "added" in German (translated with Google Translate), encoded in UTF-8, > # used in sample commit log messages in add_file() function below. > added=$(printf "hinzugef\303\274gt") > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index 63205dfdf9..fb346bff05 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -1,6 +1,8 @@ > #!/bin/sh > # Copyright (c) 2010, Jens Lehmann > > +GIT_TEST_FSMONITOR="" > + > test_description='Recursive "git fetch" for submodules' > > . ./test-lib.sh > diff --git a/t/t7402-submodule-rebase.sh b/t/t7402-submodule-rebase.sh > index 8e32f19007..c78e9009cf 100755 > --- a/t/t7402-submodule-rebase.sh > +++ b/t/t7402-submodule-rebase.sh > @@ -7,6 +7,9 @@ test_description='Test rebasing, stashing, etc. with submodules' > > . ./test-lib.sh > > +# fsmonitor does not work well with submodules > +GIT_TEST_FSMONITOR="" > + > test_expect_success setup ' > > echo file > file && > diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh > index c973278300..8d93aaef5f 100755 > --- a/t/t7406-submodule-update.sh > +++ b/t/t7406-submodule-update.sh > @@ -11,6 +11,8 @@ submodule and "git submodule update --rebase/--merge" does not detach the HEAD. > > . ./test-lib.sh > > +# fsmonitor does not work well with submodules > +GIT_TEST_FSMONITOR="" > > compare_head() > { > diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh > index 08629a6e70..1a716f2c2a 100755 > --- a/t/t7506-status-submodule.sh > +++ b/t/t7506-status-submodule.sh > @@ -4,6 +4,9 @@ test_description='git status for submodule' > > . ./test-lib.sh > > +# fsmonitor does not work well with submodules > +GIT_TEST_FSMONITOR="" > + > test_create_repo_with_commit () { > test_create_repo "$1" && > ( > diff --git a/t/t7508-status.sh b/t/t7508-status.sh > index 4e676cdce8..bf0487632d 100755 > --- a/t/t7508-status.sh > +++ b/t/t7508-status.sh > @@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' ' > test_cmp expect output > ' > > +# fsmonitor does not work well with submodules > +GIT_TEST_FSMONITOR="" > + Likewise. > test_expect_success 'setup status submodule summary' ' > test_create_repo sm && ( > cd sm && > -- > gitgitgadget >

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/10/2019 5:13 AM, SZEDER Gábor wrote: > On Mon, Dec 09, 2019 at 04:10:01PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> The fsmonitor feature allows an external tool such as watchman to >> monitor the working directory. The direct test >> t7619-status-fsmonitor.sh provides some coverage, but it would be >> better to run the entire test suite with watchman enabled. This >> would provide more confidence that the feature is working as >> intended. >> >> The fsmonitor feature struggles with submodules. Disable the >> GIT_TEST_FSMONITOR environment variable before running tests with >> a lot of submodule interactions. >> >> Signed-off-by: Derrick Stolee <[email protected]> >> --- >> t/t3404-rebase-interactive.sh | 1 + >> t/t3600-rm.sh | 1 + >> t/t4060-diff-submodule-option-diff-format.sh | 3 +++ >> t/t5526-fetch-submodules.sh | 2 ++ >> t/t7402-submodule-rebase.sh | 3 +++ >> t/t7406-submodule-update.sh | 2 ++ >> t/t7506-status-submodule.sh | 3 +++ >> t/t7508-status.sh | 3 +++ >> 8 files changed, 18 insertions(+) >> >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 461dd539ff..9dc7d1aefb 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -697,6 +697,7 @@ test_expect_success 'do "noop" when there is nothing to cherry-pick' ' >> ' >> >> test_expect_success 'submodule rebase setup' ' >> + GIT_TEST_FSMONITOR="" && > > This disables GIT_TEST_FSMONITOR for the remainder of the test script, > but there are still a lot of non-submodule-specific tests to run. ... >> diff --git a/t/t7508-status.sh b/t/t7508-status.sh >> index 4e676cdce8..bf0487632d 100755 >> --- a/t/t7508-status.sh >> +++ b/t/t7508-status.sh >> @@ -846,6 +846,9 @@ test_expect_success 'status refreshes the index' ' >> test_cmp expect output >> ' >> >> +# fsmonitor does not work well with submodules >> +GIT_TEST_FSMONITOR="" >> + > > Likewise. Would it make sense to wrap the tests in a subshell without increasing the tabbing inside the subshell? I can comment the beginning of the subshell that we are disabling the variable for a specific list of tests and the subshell can be removed after the proper fixes make it work. Thanks, -Stolee

gitgitgadget · 2019-11-22T01:11:29Z

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <[email protected]> > > Signed-off-by: Derrick Stolee <[email protected]> > --- > t/test-lib-functions.sh | 15 +++++++++++++++ > t/test-lib.sh | 2 ++ > 2 files changed, 17 insertions(+) > > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh > index e0b3f28d3a..03573caf42 100644 > --- a/t/test-lib-functions.sh > +++ b/t/test-lib-functions.sh > @@ -1475,3 +1475,18 @@ test_set_port () { > port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0})) > eval $var=$port > } > + > +test_clear_watchman () { > + if test $GIT_TEST_FSMONITOR -ne "" In the rare cases when this function is invoked (see below) this condition triggers an error from the shell running test script: - when the variable is not set, because of the lack of quotes around the variable name: $ ./t5570-git-daemon.sh [....] ok 21 - hostname interpolation works after LF-stripping ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator # passed all 21 test(s) 1..21 - when the variable is set, because the '-ne' operator does integer comparison: $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh [...] ok 21 - hostname interpolation works after LF-stripping ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none # failed 1 among 21 test(s) 1..21 Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead. > + then > + watchman watch-list | Then with the above fixed, trying to run 'watchman' triggers another error if it's not installed: $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh [...] ok 21 - hostname interpolation works after LF-stripping ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found # failed 1 among 21 test(s) I think we need an additional condition to run this only if 't7519/fsmonitor-watchman' is used in the tests. > + grep "$TRASH_DIRECTORY" | > + sed "s/\t\"//g" | > + sed "s/\",//g" >repo-list > + > + for repo in $(cat repo-list) > + do > + watchman watch-del "$repo" > + done > + fi > +} > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 30b07e310f..067a432ea5 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -1072,6 +1072,8 @@ test_atexit_handler () { > # sure that the registered cleanup commands are run only once. > test : != "$test_atexit_cleanup" || return 0 > > + test_clear_watchman I'm not sure where to put this call, but this is definitely not the right place for it. See that 'return 0' above in the context? That's where the test_atexit_handler function returns early when no atexit handler commands are set, i.e. in all test scripts that don't involve some kind of daemons, thus this call is not invoked in the majority of test scripts. Simply moving this call before that early return is not good, because then it would be invoked twice. An option would be to register this call as an atexit command somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON is restored, perhaps). That way it would be invoked most of the time, and it would be invoked only once, but I'm not sure how it would work out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the middle for the remainder of the test script. However, register the atexit command only if GIT_TEST_FSMONITOR is set (to something watchman-specific), so it won't be invoked at all if GIT_TEST_FSMONITOR is not set, and thus it won't generate additional test output and trace. I don't have a better idea. > + > setup_malloc_check > test_eval_ "$test_atexit_cleanup" > test_atexit_cleanup=: > -- > gitgitgadget

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 11/21/2019 8:06 PM, SZEDER Gábor wrote: Thanks for this message. Sorry I'm so late getting back to it. > On Thu, Nov 21, 2019 at 10:20:26PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <[email protected]> >> >> Signed-off-by: Derrick Stolee <[email protected]> >> --- >> t/test-lib-functions.sh | 15 +++++++++++++++ >> t/test-lib.sh | 2 ++ >> 2 files changed, 17 insertions(+) >> >> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh >> index e0b3f28d3a..03573caf42 100644 >> --- a/t/test-lib-functions.sh >> +++ b/t/test-lib-functions.sh >> @@ -1475,3 +1475,18 @@ test_set_port () { >> port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0})) >> eval $var=$port >> } >> + >> +test_clear_watchman () { >> + if test $GIT_TEST_FSMONITOR -ne "" > > In the rare cases when this function is invoked (see below) this > condition triggers an error from the shell running test script: > > - when the variable is not set, because of the lack of quotes around > the variable name: > > $ ./t5570-git-daemon.sh > [....] > ok 21 - hostname interpolation works after LF-stripping > ./t5570-git-daemon.sh: 1482: test: -ne: unexpected operator > # passed all 21 test(s) > 1..21 > > - when the variable is set, because the '-ne' operator does integer > comparison: > > $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh > [...] > ok 21 - hostname interpolation works after LF-stripping > ./t5570-git-daemon.sh: 1482: test: Illegal number: /home/szeder/src/git/t/t7519/fsmonitor-none > # failed 1 among 21 test(s) > 1..21 > > Please use 'if test -n "$GIT_TEST_FSMONITOR"' instead. Thanks for the pointers. >> + then >> + watchman watch-list | > > Then with the above fixed, trying to run 'watchman' triggers another > error if it's not installed: > > $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh > [...] > ok 21 - hostname interpolation works after LF-stripping > ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found > # failed 1 among 21 test(s) > > I think we need an additional condition to run this only if > 't7519/fsmonitor-watchman' is used in the tests. The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR, and that can only use watchman (currently). Barring wanting to unset the variable if it was set on purpose in a test script, the other options do not actually return correct values to make use of the feature. >> + grep "$TRASH_DIRECTORY" | >> + sed "s/\t\"//g" | >> + sed "s/\",//g" >repo-list >> + >> + for repo in $(cat repo-list) >> + do >> + watchman watch-del "$repo" >> + done >> + fi >> +} >> diff --git a/t/test-lib.sh b/t/test-lib.sh >> index 30b07e310f..067a432ea5 100644 >> --- a/t/test-lib.sh >> +++ b/t/test-lib.sh >> @@ -1072,6 +1072,8 @@ test_atexit_handler () { >> # sure that the registered cleanup commands are run only once. >> test : != "$test_atexit_cleanup" || return 0 >> >> + test_clear_watchman > > I'm not sure where to put this call, but this is definitely not the > right place for it. See that 'return 0' above in the context? That's > where the test_atexit_handler function returns early when no atexit > handler commands are set, i.e. in all test scripts that don't involve > some kind of daemons, thus this call is not invoked in the majority of > test scripts. Ah, I misunderstood the point of test_atexit_handler. > Simply moving this call before that early return is not good, because > then it would be invoked twice. > > An option would be to register this call as an atexit command > somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON > is restored, perhaps). That way it would be invoked most of the time, > and it would be invoked only once, but I'm not sure how it would work > out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the > middle for the remainder of the test script. However, register the > atexit command only if GIT_TEST_FSMONITOR is set (to something > watchman-specific), so it won't be invoked at all if > GIT_TEST_FSMONITOR is not set, and thus it won't generate additional > test output and trace. > > I don't have a better idea. Shouldn't it be sufficient to add it into test_done? If the test fails, then we could leave watches open, but that's no worse than we had without this test_clear_watchman method. Thanks, -Stolee

On the Git mailing list, SZEDER Gábor wrote (reply to this):

On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote: > >> + watchman watch-list | > > > > Then with the above fixed, trying to run 'watchman' triggers another > > error if it's not installed: > > > > $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh > > [...] > > ok 21 - hostname interpolation works after LF-stripping > > ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found > > # failed 1 among 21 test(s) > > > > I think we need an additional condition to run this only if > > 't7519/fsmonitor-watchman' is used in the tests. > > The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR, > and that can only use watchman (currently). I've just run 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all make', and it only failed one test in 't0090-cache-tree.sh', but the fix is already in 'pu' in 61eea521fe (fsmonitor: do not compare bitmap size with size of split index, 2019-11-13). > >> diff --git a/t/test-lib.sh b/t/test-lib.sh > >> index 30b07e310f..067a432ea5 100644 > >> --- a/t/test-lib.sh > >> +++ b/t/test-lib.sh > >> @@ -1072,6 +1072,8 @@ test_atexit_handler () { > >> # sure that the registered cleanup commands are run only once. > >> test : != "$test_atexit_cleanup" || return 0 > >> > >> + test_clear_watchman > > > > I'm not sure where to put this call, but this is definitely not the > > right place for it. See that 'return 0' above in the context? That's > > where the test_atexit_handler function returns early when no atexit > > handler commands are set, i.e. in all test scripts that don't involve > > some kind of daemons, thus this call is not invoked in the majority of > > test scripts. > > Ah, I misunderstood the point of test_atexit_handler. > > > Simply moving this call before that early return is not good, because > > then it would be invoked twice. > > > > An option would be to register this call as an atexit command > > somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON > > is restored, perhaps). That way it would be invoked most of the time, > > and it would be invoked only once, but I'm not sure how it would work > > out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the > > middle for the remainder of the test script. However, register the > > atexit command only if GIT_TEST_FSMONITOR is set (to something > > watchman-specific), so it won't be invoked at all if > > GIT_TEST_FSMONITOR is not set, and thus it won't generate additional > > test output and trace. > > > > I don't have a better idea. > > Shouldn't it be sufficient to add it into test_done? If the test fails, > then we could leave watches open, but that's no worse than we had without > this test_clear_watchman method. I don't know enough about watchman to have an informed opinion. I think the answer mainly depends on what we want to achive and what happens when a test script run with GIT_TEST_FSMONITOR exits without invoking 'test_done' is re-executed (e.g. after a test case fails with '--immediate' or when the user hits ctrl-c or closes the terminal window mid-test). As far as I understand the commit message of v2 of this patch [1], we mainly want two things: - Avoid overloading watchman's watch queue. For this it might indeed be sufficient to clear watches in 'test_done', because most test scripts tend to succeed most of the time. - Make GIT_TEST_FSMONITOR work reliably on Windows. For this, I'm afraid it's not enough in general, because a failure with '--immediate' or after a ctrl-c we won't run 'test_done', so we won't clear the watches, and watchman will keep the fd to the trash dir open, and, consequently, will interfere with subsequent executions of the same test script as it can't delete the still existing trash dir left over from the previous run. It could still be sufficient for fsmonitor-enabled CI builds, though, because there we don't re-run tests, don't hit ctrl-c, and (at least on Azure Pipelines) don't use '--immediate', and the whole VM/container/whatever is thrown away at end anyway. On Linux/Unix-y systems it probably doesn't matter much, because they can delete open directories, but I wonder what happens with a watch when the directory it is supposed observe gets deleted. If the watch is removed in this case, great; if it isn't, then... well, then what happens with it? Will it be overwritten with the next test run, or will there be duplicate watches for the same dir? [1] https://public-inbox.org/git/e51165f260d564ccb7a9b8e696691eccb184c01a.1575907804.git.gitgitgadget@gmail.com/

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/9/2019 6:40 PM, SZEDER Gábor wrote: > On Mon, Dec 09, 2019 at 09:12:37AM -0500, Derrick Stolee wrote: >>>> + watchman watch-list | >>> >>> Then with the above fixed, trying to run 'watchman' triggers another >>> error if it's not installed: >>> >>> $ GIT_TEST_FSMONITOR="$PWD"/t7519/fsmonitor-none ./t5570-git-daemon.sh >>> [...] >>> ok 21 - hostname interpolation works after LF-stripping >>> ./t5570-git-daemon.sh: 1484: ./t5570-git-daemon.sh: watchman: not found >>> # failed 1 among 21 test(s) >>> >>> I think we need an additional condition to run this only if >>> 't7519/fsmonitor-watchman' is used in the tests. >> >> The intention is to enable a test-suite-wide run using GIT_TEST_FSMONITOR, >> and that can only use watchman (currently). > > I've just run 'GIT_TEST_FSMONITOR=$(pwd)/t7519/fsmonitor-all make', > and it only failed one test in 't0090-cache-tree.sh', but the fix is > already in 'pu' in 61eea521fe (fsmonitor: do not compare bitmap size > with size of split index, 2019-11-13). > > >>>> diff --git a/t/test-lib.sh b/t/test-lib.sh >>>> index 30b07e310f..067a432ea5 100644 >>>> --- a/t/test-lib.sh >>>> +++ b/t/test-lib.sh >>>> @@ -1072,6 +1072,8 @@ test_atexit_handler () { >>>> # sure that the registered cleanup commands are run only once. >>>> test : != "$test_atexit_cleanup" || return 0 >>>> >>>> + test_clear_watchman >>> >>> I'm not sure where to put this call, but this is definitely not the >>> right place for it. See that 'return 0' above in the context? That's >>> where the test_atexit_handler function returns early when no atexit >>> handler commands are set, i.e. in all test scripts that don't involve >>> some kind of daemons, thus this call is not invoked in the majority of >>> test scripts. >> >> Ah, I misunderstood the point of test_atexit_handler. >> >>> Simply moving this call before that early return is not good, because >>> then it would be invoked twice. >>> >>> An option would be to register this call as an atexit command >>> somewhere late in 'test-lib.sh' (around where GIT_TEST_GETTEXT_POISON >>> is restored, perhaps). That way it would be invoked most of the time, >>> and it would be invoked only once, but I'm not sure how it would work >>> out with test scripts that unset GIT_TEST_FSMONITOR somewhere in the >>> middle for the remainder of the test script. However, register the >>> atexit command only if GIT_TEST_FSMONITOR is set (to something >>> watchman-specific), so it won't be invoked at all if >>> GIT_TEST_FSMONITOR is not set, and thus it won't generate additional >>> test output and trace. >>> >>> I don't have a better idea. >> >> Shouldn't it be sufficient to add it into test_done? If the test fails, >> then we could leave watches open, but that's no worse than we had without >> this test_clear_watchman method. > > I don't know enough about watchman to have an informed opinion. > > I think the answer mainly depends on what we want to achive and what > happens when a test script run with GIT_TEST_FSMONITOR exits without > invoking 'test_done' is re-executed (e.g. after a test case fails with > '--immediate' or when the user hits ctrl-c or closes the terminal > window mid-test). > > As far as I understand the commit message of v2 of this patch [1], we > mainly want two things: > > - Avoid overloading watchman's watch queue. For this it might > indeed be sufficient to clear watches in 'test_done', because most > test scripts tend to succeed most of the time. > > - Make GIT_TEST_FSMONITOR work reliably on Windows. For this, I'm > afraid it's not enough in general, because a failure with > '--immediate' or after a ctrl-c we won't run 'test_done', so we > won't clear the watches, and watchman will keep the fd to the > trash dir open, and, consequently, will interfere with subsequent > executions of the same test script as it can't delete the still > existing trash dir left over from the previous run. You are right. Running an individual test and ending it early would lead to these leaked handles. This assumes someone is aware of the GIT_TEST_FSMONITOR environment variable, so they are at least interacting with the feature directly to some extent. > It could still be sufficient for fsmonitor-enabled CI builds, > though, because there we don't re-run tests, don't hit ctrl-c, and > (at least on Azure Pipelines) don't use '--immediate', and the > whole VM/container/whatever is thrown away at end anyway. This is the hope. It would be nice to get to that point. > > On Linux/Unix-y systems it probably doesn't matter much, because > they can delete open directories, but I wonder what happens with a > watch when the directory it is supposed observe gets deleted. If > the watch is removed in this case, great; if it isn't, then... > well, then what happens with it? Will it be overwritten with the > next test run, or will there be duplicate watches for the same > dir? When a directory is deleted from under Watchman on Linux, the watch is removed...eventually. I'm not sure at exactly what point that happens. At the very least, Watchman will receive and process the signals for all of the paths being removed inside the directory. Running 'watch-del' removes that overhead. Thanks, -Stolee

gitgitgadget · 2019-12-09T22:52:55Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <[email protected]> writes: > +test_clear_watchman () { > + if test -n "$GIT_TEST_FSMONITOR" > + then > + watchman watch-list | > + grep "$TRASH_DIRECTORY" | > + sed "s/\",//g" | > + sed "s/\"//g" >repo-list Whoa, this is scary. "$TRASH_DIRECTORY" comes from $(pwd) and the leading part of it can have arbitrary garbage like "[a-z]*" that may match paths "watchman watch-list" may emit that does not have anything to do with the temporary directory used by this test. What are these stripping of ", and " about? Could you tell readers how a typical output from the program we are reading from looks like perhaps in the log message or in-code comment around here? Thanks.

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/9/2019 5:52 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <[email protected]> writes: > >> +test_clear_watchman () { >> + if test -n "$GIT_TEST_FSMONITOR" >> + then >> + watchman watch-list | >> + grep "$TRASH_DIRECTORY" | >> + sed "s/\",//g" | >> + sed "s/\"//g" >repo-list > > Whoa, this is scary. "$TRASH_DIRECTORY" comes from $(pwd) and the > leading part of it can have arbitrary garbage like "[a-z]*" that may > match paths "watchman watch-list" may emit that does not have > anything to do with the temporary directory used by this test. Hm. That is a good point. Can we assume that our version of grep has a "-F" or "--fixed-strings" option? ([1] seems to say that "-F" would work.) [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs > What are these stripping of ", and " about? Could you tell readers > how a typical output from the program we are reading from looks like > perhaps in the log message or in-code comment around here? Watchman outputs its list of paths in JSON format. Luckily, it formats the output so the path lines are on separate lines, each quoted. For example: { "version": "4.9.0", "roots": [ "<path1>", "<path2>", "<path3>" ] } Thanks, -Stolee

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <[email protected]> writes: > Hm. That is a good point. Can we assume that our version of grep has > a "-F" or "--fixed-strings" option? ([1] seems to say that "-F" would > work.) $ git grep "grep -F" -- \*.sh is your friend ;-) And never use https://www.gnu.org/ manual as a yardstick---you will end up using GNUism that is not unavailable elsewhere pretty easily. > [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs > >> What are these stripping of ", and " about? Could you tell readers >> how a typical output from the program we are reading from looks like >> perhaps in the log message or in-code comment around here? > > Watchman outputs its list of paths in JSON format. Luckily, it formats > the output so the path lines are on separate lines, each quoted. > > For example: > > { > "version": "4.9.0", > "roots": [ > "<path1>", > "<path2>", > "<path3>" > ] > } Yeek; how is a dq in path represented? by doubling? by backslash-quoting (if so how is a backslash in path represented)? By something else? It's OK at least for now to declare that our test repository does not contain any funny paths, but in the longer run does the above mean that we somehow need to be able to grok JSON reliably in our tests? It may not be such a bad thing especially for longer term, as there are other parts of the system that may benefit from having JSON capable output readers in our tests (e.g. trace2 code can do JSON, right?)..

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 12/10/2019 12:20 AM, Junio C Hamano wrote: > Derrick Stolee <[email protected]> writes: > >> Hm. That is a good point. Can we assume that our version of grep has >> a "-F" or "--fixed-strings" option? ([1] seems to say that "-F" would >> work.) > > $ git grep "grep -F" -- \*.sh > > is your friend ;-) Yes, of course I should have just looked for examples. > And never use https://www.gnu.org/ manual as a yardstick---you will > end up using GNUism that is not unavailable elsewhere pretty easily. I tried to focus on the part that said "this is part of POSIX", but you are right that may not be the best place to look. >> [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs >> >>> What are these stripping of ", and " about? Could you tell readers >>> how a typical output from the program we are reading from looks like >>> perhaps in the log message or in-code comment around here? >> >> Watchman outputs its list of paths in JSON format. Luckily, it formats >> the output so the path lines are on separate lines, each quoted. >> >> For example: >> >> { >> "version": "4.9.0", >> "roots": [ >> "<path1>", >> "<path2>", >> "<path3>" >> ] >> } > > Yeek; how is a dq in path represented? by doubling? by > backslash-quoting (if so how is a backslash in path represented)? > By something else? > > It's OK at least for now to declare that our test repository does > not contain any funny paths, but in the longer run does the above > mean that we somehow need to be able to grok JSON reliably in our > tests? It may not be such a bad thing especially for longer term, > as there are other parts of the system that may benefit from having > JSON capable output readers in our tests (e.g. trace2 code can do > JSON, right?).. trace2 can _write_ JSON, not parse it. However, we have some parsing code (using a package) in the performance tests. I could try adapting that for this purpose. That package is not currently required by the test suite, so it causes some dependency issues when first running the perf suite. At least we wouldn't need the package unless running with GIT_TEST_FSMONITOR. My guess is that this patch is going to be trouble, so I'll eject it in the next version and save the JSON parsing and everything for its own series. We only really need it when we are getting close to running watchman in CI on Windows. Thanks, -Stolee

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi, On Mon, 9 Dec 2019, Junio C Hamano wrote: > Derrick Stolee <[email protected]> writes: > > > Hm. That is a good point. Can we assume that our version of grep has > > a "-F" or "--fixed-strings" option? ([1] seems to say that "-F" would > > work.) > > $ git grep "grep -F" -- \*.sh > > is your friend ;-) > > And never use https://www.gnu.org/ manual as a yardstick---you will > end up using GNUism that is not unavailable elsewhere pretty easily. > > > [1] https://www.gnu.org/savannah-checkouts/gnu/grep/manual/grep.html#index-grep-programs I often look at GNU grep's man page first and then verify via https://man.openbsd.org/grep and https://pubs.opengroup.org/onlinepubs/009695399/utilities/grep.html that the option can be considered portable. > >> What are these stripping of ", and " about? Could you tell readers > >> how a typical output from the program we are reading from looks like > >> perhaps in the log message or in-code comment around here? > > > > Watchman outputs its list of paths in JSON format. Luckily, it formats > > the output so the path lines are on separate lines, each quoted. > > > > For example: > > > > { > > "version": "4.9.0", > > "roots": [ > > "<path1>", > > "<path2>", > > "<path3>" > > ] > > } > > Yeek; how is a dq in path represented? by doubling? by > backslash-quoting (if so how is a backslash in path represented)? > By something else? > > It's OK at least for now to declare that our test repository does > not contain any funny paths, but in the longer run does the above > mean that we somehow need to be able to grok JSON reliably in our > tests? It may not be such a bad thing especially for longer term, > as there are other parts of the system that may benefit from having > JSON capable output readers in our tests (e.g. trace2 code can do > JSON, right?).. From https://www.ecma-international.org/publications/files/ECMA-ST/ECMA-404.pdf (section "9 String"): A string is a sequence of Unicode code points wrapped with quotation marks (U+0022). All code points may be placed within the quotation marks except for the code points that must be escaped: quotation mark (U+0022), reverse solidus (U+005C), and the control characters U+0000 to U+001F. There are two-character escape sequence representations of some characters. \" represents the quotation mark character (U+0022). \\ represents the reverse solidus character(U+005C). \/ represents the solidus character (U+002F). \b represents the backspace character(U+0008). \f represents the form feed character (U+000C). \n represents the line feed character (U+000A). \r represents the carriage return character (U+000D). \t represents the character tabulation character (U+0009). (Side note: It is amazing what things you learn unexpectedly, e.g. when researching information about the JSON format, you learn that about the word "solidus", that it refers to the slash, and that it was once also know as the "shilling mark"...) I am not sure why the forward slash needs to be escaped, but I guess that this is voluntary rather than mandatory. Ciao, Dscho

-Original file line number
+Diff line change
@@ Expand Up / @@ -4,6 +4,9 @@ test_description='test untracked cache' @@
     . ./test-lib.sh
+    # fsmonitor changes the expected behvaior of GIT_TRACE_UNTRACKED_STATS
+    GIT_TEST_FSMONITOR=""
     # On some filesystems (e.g. FreeBSD's ext2 and ufs) directory mtime
     # is updated lazily after contents in the directory changes, which
     # forces the untracked cache code to take the slow path.  A test
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -8,6 +8,9 @@ test_description='git status with file system watcher' @@
     # "git update-index --fsmonitor" can be used to get the extension written
     # before testing the results.
+    # Disable an external value, as we will set it directly as needed.
+    GIT_TEST_FSMONITOR=""
     clean_repo () {
     	git reset --hard HEAD &&
     	git clean -fd
@@ Expand Down @@

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

-Original file line number
+Diff line change
@@ Expand Up / @@ -1081,6 +1081,10 @@ test_atexit_handler () { @@
     test_done () {
     	GIT_EXIT_OK=t
+    	# If watchman is being used with GIT_TEST_FSMONITOR, then
+    	# clear all watches on directories inside the TRASH_DIRECTORY.
+    	test_clear_watchman
     	# Run the atexit commands _before_ the trash directory is
     	# removed, so the commands can access pidfiles and socket files.
     	test_atexit_handler
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void) @@
     int git_config_get_fsmonitor(void)
     {
+    	if (!the_repository->worktree) {
+    		core_fsmonitor = 0;
+    		return 0;
+    	}
     	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
     		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     '
     test_expect_success POSIXPERM 'forced modes' '
+    	GIT_TEST_FSMONITOR="" &&
     	mkdir -p templates/hooks &&
     	echo update-server-info >templates/hooks/post-update &&
     	chmod +x templates/hooks/post-update &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -775,6 +775,7 @@ test_expect_success '#29: setup' ' @@
     	setup_repo 29 non-existent gitfile true &&
     	mkdir -p 29/sub/sub 29/wt/sub &&
     	(
+    		GIT_TEST_FSMONITOR="" &&
     		cd 29 &&
     		GIT_WORK_TREE="$here/29" &&
     		export GIT_WORK_TREE &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -520,6 +520,7 @@ test_expect_success 'reset and bind merge' ' @@
     test_expect_success 'merge-recursive w/ empty work tree - ours has rename' '
     	(
+    		GIT_TEST_FSMONITOR="" &&
     		GIT_WORK_TREE="$PWD/ours-has-rename-work" &&
     		export GIT_WORK_TREE &&
     		GIT_INDEX_FILE="$PWD/ours-has-rename-index" &&
@@ Expand All @@
     test_expect_success 'merge-recursive w/ empty work tree - theirs has rename' '
     	(
+    		GIT_TEST_FSMONITOR="" &&
     		GIT_WORK_TREE="$PWD/theirs-has-rename-work" &&
     		export GIT_WORK_TREE &&
     		GIT_INDEX_FILE="$PWD/theirs-has-rename-index" &&
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up @@
     # Tested non-UTF-8 encoding
     test_encoding="ISO8859-1"
+    # fsmonitor does not work well with submodules
+    GIT_TEST_FSMONITOR=""
     # String "added" in German (translated with Google Translate), encoded in UTF-8,
     # used in sample commit log messages in add_file() function below.
     added=$(printf "hinzugef\303\274gt")
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -1475,3 +1475,18 @@ test_set_port () { @@
     	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
     	eval $var=$port
     }
+    test_clear_watchman () {
+    	if test -n "$GIT_TEST_FSMONITOR"
+    	then
+    		watchman watch-list |
+    			grep "$TRASH_DIRECTORY" |
+    			sed "s/\",//g" |
+    			sed "s/\"//g" >repo-list
+    		while read repo
+    		do
+    			watchman watch-del "$repo"
+    		done <repo-list
+    	fi
+    }

Improve testability with GIT_TEST_FSMONITOR #466

Uh oh!

Improve testability with GIT_TEST_FSMONITOR #466

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

gitgitgadget bot Nov 21, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Nov 22, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

gitgitgadget bot Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!