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

From: Kevin Willford <kevin.willford@microsoft.com>
Date: Oct 28, 2019, 19:23 UTC
Message-ID: <BN6PR21MB078676C4FA54391B1954D5F791660@BN6PR21MB0786.namprd21.prod.outlook.com>
In-Reply-To: <CAPYzU3N9mDfHVogfq=mhJFj6VOjS2z4ui4msnDdK6pOtVBa_QA@mail.gmail.com>

On Monday, October 28, 2019 12:40 AM Utsav Shah <utsav@dropbox.com> wrote:

Show 17 quoted lines

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