git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Utsav Shah <utsav@dropbox.com>
Date
Nov 6, 2019, 04:36 UTC
Message-ID
<CAPYzU3PLVhGWcautao1oenbKbgcOB_99PoyVEcK1hnUk5XErKg@mail.gmail.com>
In-Reply-To
<BN6PR21MB07869D880E1D0111F1A80E42917E0@BN6PR21MB0786.namprd21.prod.outlook.com>

On Tue, Nov 5, 2019 at 1:40 PM Kevin Willford <Kevin.Willford@microsoft.com> wrote:

Show 30 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.

Thanks, this observation was correct. v3 of this patch will check if the index actually needs to mark its cache as changed, and this test passes without modification.

Show 53 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.

I audited these tests very closely, and to the best of my knowledge, the modifications made are valid.

For test failures of

test_expect_success 'update-index --fsmonitor-valid sets the fsmonitor valid bit' test_expect_success 'newly added files are marked valid'

It's relatively straightforward that our patch now runs the fsmonitor hook so we need to make sure the hook doesn't return anything.

The trickiest case was "refresh_index()" test, and I've made a slight change to make it clearer why that test was failing.

@@ -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 &&
        dirty_repo &&
+       write_integration_script &&
        git add . &&
+       write_script .git/hooks/fsmonitor-test<<-\EOF &&
+       EOF
        git commit -m "to reset" &&
        git reset HEAD~1 &&
        git status >actual &&

With patch v2, git add was failing to add all files, since it now
consults the fsmonitor hook which wrongly implied that no files were
modified. This was rectified by the write_integration_script. After
that, we immediately ensure that the test fsmonitor doesn't return any
files, and the test passes.


>
> >
> >       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.
>

Yeah, I see the same results.

The one part that I don't fully understand if safe is copying over the
o->src_index->fsmonitor_last_update to the result index in
unpack-trees. I don't understand the implications of that, and if
that's the only field worth copying over, or if we should be copying
over other fields like the bitmap as well.
Previous: Kevin WillfordNext: Kevin Willford
Message 15 of 26 in “unpack-trees: skip lstat on files based on fsmonitor”
  1. 0/1 unpack-trees: skip lstat on files based on fsmonitorUtsav Shah via GitGitGadget, Oct 25, 2019
  2. 1/1 unpack-trees: skip lstat based on fsmonitorUtsav Shah via GitGitGadget, Oct 25, 2019
  3. Junio C HamanoOct 28, 2019
  4. Utsav ShahOct 28, 2019
  5. Kevin WillfordOct 28, 2019
  6. Utsav ShahOct 29, 2019
  7. Kevin WillfordOct 29, 2019
  8. Utsav ShahOct 29, 2019
  9. Junio C HamanoOct 30, 2019
  10. Utsav ShahOct 30, 2019
  11. Junio C HamanoNov 4, 2019
  12. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 5, 2019
  13. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 5, 2019
  14. Kevin WillfordNov 5, 2019
  15. Utsav ShahNov 6, 2019
  16. Kevin WillfordNov 6, 2019
  17. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 6, 2019
  18. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 6, 2019
  19. Junio C HamanoNov 6, 2019
  20. Utsav ShahNov 6, 2019
  21. Utsav ShahNov 8, 2019
  22. Junio C HamanoNov 8, 2019
  23. Junio C HamanoNov 6, 2019
  24. 0/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 20, 2019
  25. 1/1 unpack-trees: skip stat on fsmonitor-valid filesUtsav Shah via GitGitGadget, Nov 20, 2019
  26. Junio C HamanoNov 21, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.