RE: [PATCH v2 1/1] unpack-trees: skip stat on fsmonitor-valid files

From: Kevin Willford <kevin.willford@microsoft.com>
Date: Nov 5, 2019, 21:40 UTC
Message-ID: <BN6PR21MB07869D880E1D0111F1A80E42917E0@BN6PR21MB0786.namprd21.prod.outlook.com>
In-Reply-To: <f76ba554ed25fb9877a223ef6481834f1831c8ca.1572967644.git.gitgitgadget@gmail.com>

Show 19 quoted lines

> Sent: Tuesday, November 5, 2019 8:27 AM
> From: Utsav Shah <utsav@dropbox.com>
> 
> diff --git a/t/t7113-post-index-change-hook.sh b/t/t7113-post-index-change-
> hook.sh
> index f011ad7eec..5ca2279d0d 100755
> --- a/t/t7113-post-index-change-hook.sh
> +++ b/t/t7113-post-index-change-hook.sh
> @@ -50,9 +50,6 @@ test_expect_success 'test status, add, commit, others
> trigger hook without flags
>  	git checkout -- dir1/file1.txt &&
>  	test_path_is_file testsuccess && rm -f testsuccess &&
>  	test_path_is_missing testfailure &&
> -	git update-index &&
> -	test_path_is_missing testsuccess &&
> -	test_path_is_missing testfailure &&
>  	git reset --soft &&
>  	test_path_is_missing testsuccess &&
>  	test_path_is_missing testfailure

Looking into this change and I wonder if instead we should be updating refresh_fsmonitor to only update istate->cache_changed if there was an entry where CE_FSMONITOR_VALID was turned off.

The reason I bring this up is because with this change, the post-index-change hook will behave differently depending on fsmonitor. It will pass if GIT_TEST_FSMONITOR is unset or set to fsmonitor-watchman. But when set to fsmonitor-all it will fail because it is going down the code path that invalidates all the entries and sets istate->cache_changed.

Show 38 quoted lines

> diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh index
> d8df990972..9cac3d3d8e 100755
> --- a/t/t7519-status-fsmonitor.sh
> +++ b/t/t7519-status-fsmonitor.sh
> @@ -106,6 +106,8 @@ EOF
> 
>  # test that "update-index --fsmonitor-valid" sets the fsmonitor valid bit
> test_expect_success 'update-index --fsmonitor-valid" sets the fsmonitor
> valid bit' '
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git update-index --fsmonitor &&
>  	git update-index --fsmonitor-valid dir1/modified &&
>  	git update-index --fsmonitor-valid dir2/modified && @@ -164,6
> +166,8 @@ EOF
> 
>  # test that newly added files are marked valid  test_expect_success 'newly
> added files are marked valid' '
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git add new &&
>  	git add dir1/new &&
>  	git add dir2/new &&
> @@ -218,11 +222,12 @@ test_expect_success '*only* files returned by the
> integration script get flagged  # Ensure commands that call refresh_index() to
> move the index back in time  # properly invalidate the fsmonitor cache
> test_expect_success 'refresh_index() invalidates fsmonitor cache' '
> -	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> -	EOF
>  	clean_repo &&
> +	write_integration_script &&
>  	dirty_repo &&
>  	git add . &&
> +	write_script .git/hooks/fsmonitor-test<<-\EOF &&
> +	EOF
>  	git commit -m "to reset" &&
>  	git reset HEAD~1 &&
>  	git status >actual &&

We need to take a close look at all the tests in t7519-status-fsmonitor.sh and see if we are doing the right thing with these changes because before most commands that read the index would only apply the bits from the fsmonitor bitmap to the cache entries. Whereas now, it does that but also applies what the fsmonitor hooks returns so the content of .git/hooks/fsmonitor-test is now affecting tests and commands where it was not before.

So if .git/hooks/fsmonitor-test has paths even git ls-files gets those paths marked dirty and that command is being used to validate the state of the CE_FSMONITOR_VALID. So I think in most cases for these tests we want the .git/hooks/fsmonitor-test to be empty before calling git ls-files so that doesn't change the index state.

Show 12 quoted lines

> 
>  	if (old && same(old, a)) {
>  		int update = 0;
> -		if (o->reset && o->update && !ce_uptodate(old) &&
> !ce_skip_worktree(old)) {
> +		if (o->reset && o->update && !ce_uptodate(old) &&
> !ce_skip_worktree(old) &&
> +			!(old->ce_flags & CE_FSMONITOR_VALID)) {
>  			struct stat st;
>  			if (lstat(old->name, &st) ||
>  			    ie_match_stat(o->src_index, old, &st,
> CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE))

FYI I have been testing with the ce_uptodate macro checking the CE_FSMONITOR_VALID flag instead and only have failures when using the fsmonitor-watchman script which I'm not sure if all the tests were ever passing using it.