Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor

[Utsav Shah <utsav@dropbox.com>]

On Mon, Oct 28, 2019 at 12:23 PM Kevin Willford
<Kevin.Willford@microsoft.com> wrote:
>
> On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com>
> wrote:
>
> > > 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.
> >
> > > 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