Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve testability with GIT_TEST_FSMONITOR #466

Conversation

derrickstolee
Copy link

@derrickstolee derrickstolee commented Nov 18, 2019

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

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

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

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

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

Updates in V2:

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

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

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

Thanks,
-Stolee

Cc: szeder.dev@gmail.com, ukshah2@illinois.edu, Kevin.Willford@microsoft.com

@derrickstolee
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 21, 2019

@@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <dstolee@microsoft.com>
> 
> t0003-attributes.sh

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

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

Thanks,

Denton

> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  config.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/config.c b/config.c
> index 3900e4947b..f6d4e2fae3 100644
> --- a/config.c
> +++ b/config.c
> @@ -2339,6 +2339,11 @@ int git_config_get_max_percent_split_change(void)
>  
>  int git_config_get_fsmonitor(void)
>  {
> +	if (!the_repository->worktree) {
> +		core_fsmonitor = 0;
> +		return 0;
> +	}
> +
>  	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
>  		core_fsmonitor = getenv("GIT_TEST_FSMONITOR");
>  
> -- 
> gitgitgadget
> 

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <dstolee@microsoft.com>
>>
>> t0003-attributes.sh
> 
> Patches 1 and 2 have this line in the commit message. I assume it's a
> typo?
> 
> Also, for the patches with empty commit message bodies, it would be nice
> if they were filled in with detail for _why_ the change is being made.
> For example, in this patch, a future reader might wonder why having
> fsmonitor enabled in a bare repo causes problems.

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

Thanks,
-Stolee

@@ -1475,3 +1475,18 @@ test_set_port () {
port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <dstolee@microsoft.com>
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  t/test-lib-functions.sh | 15 +++++++++++++++
>  t/test-lib.sh           |  2 ++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e0b3f28d3a..03573caf42 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -1475,3 +1475,18 @@ test_set_port () {
>  	port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
>  	eval $var=$port
>  }
> +
> +test_clear_watchman () {
> +	if test $GIT_TEST_FSMONITOR -ne ""

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

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

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

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

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

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

> +	then
> +		watchman watch-list |

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

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

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

> +			grep "$TRASH_DIRECTORY" |
> +			sed "s/\t\"//g" |
> +			sed "s/\",//g" >repo-list
> +
> +		for repo in $(cat repo-list)
> +		do
> +			watchman watch-del "$repo"
> +		done
> +	fi
> +}
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 30b07e310f..067a432ea5 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1072,6 +1072,8 @@ test_atexit_handler () {
>  	# sure that the registered cleanup commands are run only once.
>  	test : != "$test_atexit_cleanup" || return 0
>  
> +	test_clear_watchman

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

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

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

I don't have a better idea.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Thanks for the pointers.

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

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

>> +			grep "$TRASH_DIRECTORY" |
>> +			sed "s/\t\"//g" |
>> +			sed "s/\",//g" >repo-list
>> +
>> +		for repo in $(cat repo-list)
>> +		do
>> +			watchman watch-del "$repo"
>> +		done
>> +	fi
>> +}
>> diff --git a/t/test-lib.sh b/t/test-lib.sh
>> index 30b07e310f..067a432ea5 100644
>> --- a/t/test-lib.sh
>> +++ b/t/test-lib.sh
>> @@ -1072,6 +1072,8 @@ test_atexit_handler () {
>>  	# sure that the registered cleanup commands are run only once.
>>  	test : != "$test_atexit_cleanup" || return 0
>>  
>> +	test_clear_watchman
> 
> I'm not sure where to put this call, but this is definitely not the
> right place for it.  See that 'return 0' above in the context?  That's
> where the test_atexit_handler function returns early when no atexit
> handler commands are set, i.e. in all test scripts that don't involve
> some kind of daemons, thus this call is not invoked in the majority of
> test scripts.

Ah, I misunderstood the point of test_atexit_handler.

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

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

Thanks,
-Stolee

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

gitgitgadget bot commented Nov 22, 2019

This patch series was integrated into pu via git@4382d69.

@gitgitgadget gitgitgadget bot added the pu label Nov 22, 2019
@gitgitgadget
Copy link

gitgitgadget bot commented Nov 22, 2019

This branch is now known as ds/fsmonitor-testing.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 23, 2019

This patch series was integrated into pu via git@59a54b9.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 25, 2019

This patch series was integrated into pu via git@8f06d0a.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 25, 2019

This patch series was integrated into pu via git@0906a7b.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 27, 2019

This patch series was integrated into pu via git@307ef02.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 2, 2019

This patch series was integrated into pu via git@46768ea.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 3, 2019

This patch series was integrated into pu via git@26b6379.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 5, 2019

This patch series was integrated into pu via git@9460a10.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 6, 2019

This patch series was integrated into pu via git@452e34d.

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

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

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The GIT_TEST_FSMONITOR variable was created specifically so the
t7519-status-fsmonitor.sh test script could tweak the expected
behavior depending on its value. However, if we set it externally
to use the Watchman integration, then it breaks the initial tests
that demonstrate behavior _without_ the fsmonitor feature.

Disable this variable at the start of the script.

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
The fsmonitor feature allows an external tool such as watchman to
monitor the working directory. The direct test
t7619-status-fsmonitor.sh provides some coverage, but it would be
better to run the entire test suite with watchman enabled. This
would provide more confidence that the feature is working as
intended.

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

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

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

[1] facebook/watchman#764

Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
@derrickstolee
Copy link
Author

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 9, 2019

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 17, 2019

This patch series was integrated into pu via git@5bcd405.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 18, 2019

This patch series was integrated into pu via git@70b7c68.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 19, 2019

This patch series was integrated into pu via git@a1f9e6b.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 20, 2019

This patch series was integrated into pu via git@789c723.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 25, 2019

This patch series was integrated into pu via git@55cd2d8.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 26, 2019

This patch series was integrated into pu via git@06f1750.

@gitgitgadget
Copy link

gitgitgadget bot commented Dec 30, 2019

This patch series was integrated into pu via git@88624ce.

@dscho
Copy link
Member

dscho commented Dec 31, 2019

@derrickstolee this has been marked as "stalled" in the latest "What's cooking?" mail. Any plans on reviving this patch series?

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 2, 2020

This patch series was integrated into pu via git@06800c2.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 3, 2020

This patch series was integrated into pu via git@800f904.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 5, 2020

This patch series was integrated into pu via git@1001ac2.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 6, 2020

This patch series was integrated into pu via git@70b3477.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 7, 2020

This patch series was integrated into pu via git@dc77306.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 8, 2020

This patch series was integrated into pu via git@f6e3dda.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 9, 2020

This patch series was integrated into pu via git@4514a0c.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 9, 2020

This patch series was integrated into pu via git@ba01519.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 10, 2020

This patch series was integrated into pu via git@e5da6c2.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 13, 2020

This patch series was integrated into pu via git@9c61d79.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 13, 2020

This patch series was integrated into pu via git@3e02c00.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 15, 2020

This patch series was integrated into pu via git@c821497.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 17, 2020

This patch series was integrated into pu via git@c0ca6fd.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 24, 2020

This patch series was integrated into pu via git@531f563.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 27, 2020

This patch series was integrated into pu via git@1d89a6b.

@gitgitgadget
Copy link

gitgitgadget bot commented Jan 30, 2020

This patch series was integrated into pu via git@7a6cc5a.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 5, 2020

This patch series was integrated into pu via git@e4aa9b4.

@gitgitgadget
Copy link

gitgitgadget bot commented Feb 7, 2020

This patch series was integrated into pu via git@e1b6230.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants