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

Commit Message

Linus Arver via GitGitGadget Dec. 9, 2019, 4:10 p.m. UTC

From: Derrick Stolee <[e-mail]>

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

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

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

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

Comments

SZEDER Gábor Dec. 10, 2019, 10:07 a.m. UTC | #1

On Mon, Dec 09, 2019 at 04:10:00PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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
>

Derrick Stolee Dec. 10, 2019, 1:45 p.m. UTC | #2

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 <[e-mail]>
>>
>> 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

SZEDER Gábor Dec. 10, 2019, 2:57 p.m. UTC | #3

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 <[e-mail]>
> >>
> >> 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

SZEDER Gábor Dec. 10, 2019, 3:07 p.m. UTC | #4

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.

Derrick Stolee Jan. 23, 2020, 3:45 p.m. UTC | #5

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

Patch

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