On Mon, Oct 28, 2019 at 12:23 PM Kevin Willford
[off-list ref] wrote:

On Monday, October 28, 2019 12:40 AM Utsav Shah [off-list ref]
wrote:

quoted

quoted

I wonder if !ce_uptodate(old) should say "this one is up to date and
not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
codepaths that use ce_uptodate(ce) to decide to do X without paying
attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy in
the same way as you found this instance, or do they have legitimate
reason why they only check ce_uptodate(ce) and ignore fsmonitor?

Yes, there are other code paths as well. After reading the code some more, it
seems like there's no legitimate need to ignore fsmonitor.

quoted

If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
would make this fix unnecessary and fix other codepaths that check
only ce_uptodate() without checking fsmonitor.

I would need to go back and see if there was some reasoning why the
new flag was added but using CE_UPTODATE makes sense especially when
most calls to ce_mark_uptodate is followed directly by a call to
mark_fsmonitor_valid.

I've been playing around with the patch and trying to get the tests pass.

I've found that we set CE_UPTODATE when we try to skip worktrees to
stat in the case of sparse checkouts, and there are cases where we
mark cache entries up to date without consulting fsmonitor or stating
them.

It seems like making fsmonitor only modify CE_UPTODATE makes it hard
to verify and test correct fsmonitor behavior and debugging fsmonitor
with git ls-files -f. I think the patch also makes things overall
slightly more complicated.

There is a little more going on in the

mark_fsmonitor_X than just setting the bit though and the invalid
calls are not matched with code to clear the CE_UPTODATE flag.

Yeah. The patch to replace CE_FSMONITOR_VALID doesn't remove the need
for calling mark_fsmonitor_valid/mark_fsmonitor_invalid, since there's
special behavior like modifying the untracked cache which doesn't make
sense in a more general mark_ce_not_uptodate function.

The change to use CE_UPTODATE would have
more extensive effects and like you said we would need to make sure it
would not cause correctness issues in some corner case.

Did you run all the git tests with GIT_TEST_FSMONITOR set to
t/t7519/fsmonitor-all?  This will run the tests with fsmonitor on.  I was
getting multiple failures with this change and fsmonitor on.

I added the refresh_fsmonitor call to the tweak_fsmonitor after
using the bitmap to set the dirty entries.  This fixed most of the test
failures but there are still some failures that I haven't tracked down the
reason for.

I'm getting the same test failures with or without
GIT_TEST_FSMONITOR=t/t7519/fsmonitor-all and calling refresh_fsmonitor
in tweak_fsmonitor. Could you share your patch? I'm probably messing
something up, and I can try taking a look at fixing test cases as
well.

I will do some more digging and testing to see what other pitfalls there
might be with this change.

Thanks,
Kevin