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

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

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 will do some more digging and testing to see what other pitfalls there
might be with this change.

Thanks,
Kevin